Skip to content

Add support for modifying iov settings in ncproxy#964

Merged
katiewasnothere merged 1 commit intomicrosoft:masterfrom
katiewasnothere:ncproxy-iov
Apr 1, 2021
Merged

Add support for modifying iov settings in ncproxy#964
katiewasnothere merged 1 commit intomicrosoft:masterfrom
katiewasnothere:ncproxy-iov

Conversation

@katiewasnothere
Copy link

@katiewasnothere katiewasnothere commented Mar 10, 2021

This PR adds the ability to modify a network through ncproxy, particularly for disabling/enabling IOV on an endpoint. Thanks to @dcantah for writing a good chunk of the ModifyNIC flow.

Currently we require that the caller passes in all values related to IOV settings. This is because there is a discrepancy between how HCS and HCN treats unset values, see comment in discussion.

Signed-off-by: Kathryn Baldauf kabaldau@microsoft.com

@katiewasnothere katiewasnothere requested a review from a team as a code owner March 10, 2021 21:20
@katiewasnothere katiewasnothere force-pushed the ncproxy-iov branch 3 times, most recently from 54bdbf5 to c176d66 Compare March 16, 2021 19:05
@katiewasnothere
Copy link
Author

@microsoft/containerplat I was able to validate this again :)

@dcantah
Copy link
Contributor

dcantah commented Mar 29, 2021

This needs a rebase I believe, it's still showing the appveyor ci

@dcantah
Copy link
Contributor

dcantah commented Mar 29, 2021

Changes lgtm besides the logging/nits, but don't want to lgtm as I forget if it resets on push now with our new settings

@dcantah
Copy link
Contributor

dcantah commented Mar 29, 2021

Can take the TODO out of the PR description now also as you re-validated

@katiewasnothere katiewasnothere force-pushed the ncproxy-iov branch 2 times, most recently from de5a934 to 463f292 Compare March 29, 2021 20:47
Signed-off-by: Kathryn Baldauf <kabaldau@microsoft.com>
Copy link
Contributor

@dcantah dcantah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants