Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue preventing lowering of PF's MTU #700

Merged
merged 1 commit into from
Jun 6, 2024
Merged

Fix issue preventing lowering of PF's MTU #700

merged 1 commit into from
Jun 6, 2024

Conversation

almaslennikov
Copy link
Contributor

@almaslennikov almaslennikov commented May 17, 2024 •
edited
Loading

When the MTU set in the SRIOV Network Node Policy is lower than the actual MTU of the PF, it cannot be changed due to a condition in question. This triggers the reconcile loop for the Node state indefinitely, preventing the configuration from completing.

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@coveralls
Copy link

coveralls commented May 17, 2024 •
edited
Loading

Pull Request Test Coverage Report for Build 9363135127

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.05%) to 39.701%
Totals Coverage Status
Change from base Build 9212771639: 0.05%
Covered Lines: 5181
Relevant Lines: 13050
💛 - Coveralls

@almaslennikov
Copy link
Contributor Author

almaslennikov commented May 17, 2024 •
edited
Loading

I see that there was a reason for never allowing to lower the PF's MTU: #489

@SchSeba @adrianchiris @bn222 What's your take on this?

I see two options here: either allow to lower the MTU of the PF or also update the condition here with regards to how we decide if sriov device should get updated

@mlguerrero12
Copy link
Contributor

Thanks @almaslennikov. Another way to hit this issue is when the user manually increases the mtu after having configured a policy with a lower mtu. We have a test (should reconcile managed VF if status changes) in test_sriov_operator.go for the opposite case (manually decreasing mtu).

I think we should go for the second option, aligning NeedToUpdateSriov with which is actually being applied in order to avoid the loop. Please consider adding a test for this scenario as well.

@adrianchiris
Copy link
Collaborator

IMO we should update NeedToUpdateSriov so we support the use-case of a PF having a different (higher) MTU than VF as requested in spec.

@SchSeba
Copy link
Collaborator

SchSeba commented May 19, 2024 •
edited
Loading

agree if the MTU on the PF is higher the NeedToUpdateSriov should return false

@almaslennikov please implement a functional test to cover this case I will review this PR.
another test as requested by @mlguerrero12 is to apply a policy lower the MTU on the host and see that we return the MTU back to be the policy MTU

@github-actions GitHub Actions
Copy link

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@almaslennikov
Copy link
Contributor Author

/test-e2e-all

@SchSeba
Copy link
Collaborator

SchSeba commented May 27, 2024

Hi @almaslennikov please check the unit-tests

@SchSeba SchSeba mentioned this pull request May 27, 2024
Merged
@almaslennikov
Fix issue with infinite reconciling higher MTU
c2c7817 
When the MTU set in the SRIOV Network Node Policy is lower than the
actual MTU of the PF, it triggers the reconcile loop for the Node state
indefinitely, preventing the configuration from completing.

Signed-off-by: amaslennikov <[email protected]>
@github-actions GitHub Actions
Copy link

github-actions bot commented Jun 4, 2024

Thanks for your PR,
To run vendors CIs use one of:

  • /test-all: To run all tests for all vendors.
  • /test-e2e-all: To run all E2E tests for all vendors.
  • /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

  • /skip-all: To skip all tests for all vendors.
  • /skip-e2e-all: To skip all E2E tests for all vendors.
  • /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
    Best regards.

@almaslennikov
Copy link
Contributor Author

/test-e2e-all

1 similar comment
@e0ne
Copy link
Collaborator

e0ne commented Jun 5, 2024

/test-e2e-all

adrianchiris
adrianchiris reviewed Jun 5, 2024
View reviewed changes
test/conformance/tests/test_sriov_operator.go
nodeState, err := clients.SriovNetworkNodeStates(operatorNamespace).Get(context.Background(), node, metav1.GetOptions{})
Expect(err).ToNot(HaveOccurred())
return nodeState.Status.Interfaces
}, 3*time.Minute, 15*time.Second).Should(ContainElement(MatchFields(
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole flow extends the this test by quite a bit.

@zeeke ? thoughts ? do we want this back and forth of the MTU in e2e tests

Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with it for now if we see that it's unstable for some reason we can revisit this

adrianchiris
adrianchiris approved these changes Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

LGTM, thx @almaslennikov

e0ne
e0ne approved these changes Jun 5, 2024
View reviewed changes
Copy link
Collaborator

@e0ne e0ne left a comment

Choose a reason for hiding this comment

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

LGTM.

Nvidia e2e test CI seems to be broken now

@mlguerrero12
Copy link
Contributor

LGTM

SchSeba
SchSeba approved these changes Jun 6, 2024
View reviewed changes
test/conformance/tests/test_sriov_operator.go
nodeState, err := clients.SriovNetworkNodeStates(operatorNamespace).Get(context.Background(), node, metav1.GetOptions{})
Expect(err).ToNot(HaveOccurred())
return nodeState.Status.Interfaces
}, 3*time.Minute, 15*time.Second).Should(ContainElement(MatchFields(
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am fine with it for now if we see that it's unstable for some reason we can revisit this

@SchSeba SchSeba merged commit a767d33 into k8snetworkplumbingwg:master Jun 6, 2024
12 of 13 checks passed
This was referenced Jul 5, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@e0ne e0ne e0ne approved these changes

@SchSeba SchSeba SchSeba approved these changes

@adrianchiris adrianchiris adrianchiris approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@almaslennikov @coveralls @mlguerrero12 @adrianchiris @SchSeba @e0ne