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

node-neigh: Wait instead of sleeping in unit tests #17035

Merged
merged 1 commit into from Aug 4, 2021

Conversation

aanm
Copy link
Member

@aanm aanm commented Aug 3, 2021

We can inspect the neighLastPingByNextHop map to check
when insertNeighbor() or deleteNeighbor() was called.

Fixes: e68848b ("remove ARP entries left from previous Cilium run")
Signed-off-by: André Martins andre@cilium.io

Fixes #17034

@aanm aanm added the release-note/ci This PR makes changes to the CI. label Aug 3, 2021
@aanm aanm requested review from brb and a team August 3, 2021 00:11
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.10.4 Aug 3, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.8.12 Aug 3, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.10 Aug 3, 2021
@aanm
Copy link
Member Author

aanm commented Aug 3, 2021

test-runtime

@maintainer-s-little-helper
Copy link

Job 'Cilium-PR-Runtime-4.9' hit: #17034 (94.18% similarity)

@@ -1405,7 +1405,7 @@ func (s *linuxPrivilegedIPv4OnlyTestSuite) TestArpPingHandling(c *check.C) {
c.Assert(found, check.Equals, false)

c.Assert(linuxNodeHandler.NodeAdd(nodev3), check.IsNil)
time.Sleep(100 * time.Millisecond) // insertNeighbor is invoked async
wait(nodev3.Identity(), nil, false)
Copy link
Member

Choose a reason for hiding this comment

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

Either you need to pass the now value or make the function wait to accept before = nil.

@brb
Copy link
Member

brb commented Aug 3, 2021

I'm wondering why all of sudden we started to hit the TestArp flakes again. Another issue: #17020.

@aanm
Copy link
Member Author

aanm commented Aug 3, 2021

/mlh new-flake Cilium-PR-Runtime-4.9

👍 created #17038

@aanm aanm force-pushed the pr/fix-privileged-unit-arp branch from 15b8060 to c5d404a Compare August 3, 2021 13:31
@aanm aanm requested a review from brb August 3, 2021 13:31
linuxNodeHandler.NodeCleanNeighbors()
wait(nodev3.Identity(), &now, true)
Copy link
Member

Choose a reason for hiding this comment

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

now is not needed for the deletion.

@aanm aanm force-pushed the pr/fix-privileged-unit-arp branch from c5d404a to 914c6a9 Compare August 3, 2021 13:32
@aanm aanm requested a review from brb August 3, 2021 13:33
@aanm
Copy link
Member Author

aanm commented Aug 3, 2021

test-runtime

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

🚀 🌔

@maintainer-s-little-helper
Copy link

Job 'Cilium-PR-Runtime-4.9' hit: #17034 (94.02% similarity)

@aanm aanm force-pushed the pr/fix-privileged-unit-arp branch from 914c6a9 to 2c9553b Compare August 3, 2021 22:36
@aanm
Copy link
Member Author

aanm commented Aug 3, 2021

test-runtime

@maintainer-s-little-helper
Copy link

Job 'Cilium-PR-Runtime-4.9' hit: #17034 (94.23% similarity)

We can inspect the neighLastPingByNextHop map to check
when insertNeighbor() or deleteNeighbor() was called.

Fixes: e68848b ("remove ARP entries left from previous Cilium run")
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/fix-privileged-unit-arp branch from 2c9553b to f81fa2a Compare August 4, 2021 11:41
@aanm aanm merged commit 2017e04 into cilium:master Aug 4, 2021
@aanm aanm deleted the pr/fix-privileged-unit-arp branch August 4, 2021 15:08
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.10 in 1.10.4 Aug 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.10 Aug 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.12 Aug 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.12 Aug 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.12 Aug 10, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.10 to Backport done to v1.10 in 1.10.4 Aug 13, 2021
@joestringer joestringer added this to Backport pending to v1.9 in 1.9.11 Sep 1, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.10 Sep 1, 2021
@joestringer joestringer removed this from Backport pending to v1.9 in 1.9.11 Sep 1, 2021
@joestringer joestringer added this to Backport done to v1.9 in 1.9.10 Sep 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.10.4
Backport done to v1.10
1.8.12
Backport done to v1.8
1.9.10
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

CI: RuntimePrivilegedUnitTests Run Tests
5 participants