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

datapath/linux/ethtool: deflake TestIsVirtualDriver #26027

Merged
merged 1 commit into from Jun 8, 2023

Conversation

tklauser
Copy link
Member

@tklauser tklauser commented Jun 8, 2023

Currently, TestIsVirtualDriver is getting a list of all currently present network interfaces and then iterates them to call IsVirtualDriver. This is brittle, given a network interface can disappear between the call to netlink.LinkList and the call to IsVirtualDriver, as happens e.g. when creating dummyN interfaces in tests that run parallel to TestIsVirtualDriver. This then leads to a flake in the test.

The proper fix is to change TestIsVirtualDriver to create a veth pair inside a new netns (so the interface names don't clash existing ones) and then call IsVirtualDriver only on that one and expect it to return true, nil. That way we also better test its actual behavior.

Fixes #25888

Currently, TestIsVirtualDriver is getting a list of all currently
present network interfaces and then iterates them to call
IsVirtualDriver. This is brittle, given a network interface can
disappear between the call to netlink.LinkList and the call to
IsVirtualDriver, as happens e.g. when creating dummyN interfaces in
tests that run parallel to TestIsVirtualDriver. This then leads to a
flake in the test.

The proper fix is to change TestIsVirtualDriver to create a veth pair
inside a new netns (so the interface names don't clash existing ones)
and then call IsVirtualDriver only on that one and expect it to return
true, nil. That way we also better test its actual behavior.

Fixes #25888

Signed-off-by: Tobias Klauser <tobias@cilium.io>
@tklauser tklauser added sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/ci This PR makes changes to the CI. ci/flake This is a known failure that occurs in the tree. Please investigate me! labels Jun 8, 2023
@tklauser tklauser requested a review from a team as a code owner June 8, 2023 11:21
@tklauser tklauser requested a review from aspsk June 8, 2023 11:21
@tklauser
Copy link
Member Author

tklauser commented Jun 8, 2023

/test

@aspsk
Copy link
Contributor

aspsk commented Jun 8, 2023

Copy link
Contributor

@aspsk aspsk left a comment

Choose a reason for hiding this comment

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

Looks great! (This PR also make me to recall if Go orders defers as lifo :-))

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 8, 2023
@tklauser tklauser merged commit a3c1e35 into main Jun 8, 2023
64 of 65 checks passed
@tklauser tklauser deleted the pr/tklauser/test-isvirtualdriver-flake-fix branch June 8, 2023 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/flake This is a known failure that occurs in the tree. Please investigate me! ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CI: ethtool.TestIsVirtualDriver fails in RuntimeDatapathPrivilegedUnitTests
2 participants