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.

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

Start post-network-service after sriov-config #845

Merged
merged 1 commit into from
Feb 18, 2025

Conversation

e0ne
Copy link
Collaborator

@e0ne e0ne commented Feb 14, 2025

sriov-config-post-network-service sould be executed after sriov-config.

Copy link

Thanks for your PR,
To run vendors CIs, Maintainers can 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, Maintainers can 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 Feb 14, 2025

Pull Request Test Coverage Report for Build 13370487627

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.07%) to 47.363%

Totals Coverage Status
Change from base Build 13109399258: 0.07%
Covered Lines: 7265
Relevant Lines: 15339

💛 - Coveralls

sriov-config-post-network-service sould be executed after
sriov-config.

Signed-off-by: Ivan Kolodiazhnyi <[email protected]>
Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

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

LGTM

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.

nice catch! no idea why it didnt fail until now 😟

@adrianchiris adrianchiris merged commit 0ead8c5 into k8snetworkplumbingwg:master Feb 18, 2025
14 checks passed
@e0ne
Copy link
Collaborator Author

e0ne commented Feb 18, 2025

nice catch! no idea why it didnt fail until now 😟

maybe it's related to kernel changes and/or system performance

Copy link
Collaborator

@ykulazhenkov ykulazhenkov left a comment

Choose a reason for hiding this comment

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

I'm not against adding an explicit dependency between the services, but the logic should work without it on systems with NetworkManager or systemd-networkd if everything is working as was designed.

How it should work on a system with NetworkManager:

  • sriov-config.service has type=oneshot and Before=NetworkManager.service – meaning the network manager should start after the sriov-config.service process completes

  • sriov-config-post-network.service has After=NetworkManager-wait-online.service

  • NetworkManager-wait-online.service depends on NetworkManager.service.

So, the order should be sriov-config.service > NetworkManager > sriov-config-post-network.service.

For systems with systemd-networkd the logic is the same, but networkd services are used as dependencies.

The explicit dependency should be needed only in case if NetworkManager, system-networkd and ovs are not available on the system.

If the explicit dependency is really needed that probably means that the dependency chain doesn’t work as expected and “with bond scenarios” will not work.

Bond scenario:

  1. sriov-config.service configure NIC mode and create VFs, unbind VFs from the driver at the end of execution
  2. NetworkManager configures PFs to assemble a bond
  3. sriov-config-post-network.service bind VFs to the driver

@e0ne Do you have a full understanding why explicit dependency is needed?

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.

6 participants