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

service/test: Fix waiting in testSessionAffinity and regroup affinity match map updates #11519

Merged
merged 2 commits into from May 14, 2020

Conversation

brb
Copy link
Member

@brb brb commented May 13, 2020

See commit msgs.

Fix #11296
Partially_fix #11442

@brb brb added kind/bug This is a bug in the Cilium logic. area/CI Continuous Integration testing issue or flake labels May 13, 2020
@brb brb requested a review from a team as a code owner May 13, 2020 12:19
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

2 similar comments
@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper
Copy link

Please set the appropriate release note label.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 13, 2020
@brb brb added the release-note/ci This PR makes changes to the CI. label May 13, 2020
@brb brb added release-note/misc This PR makes changes that have no direct user impact. and removed release-note/ci This PR makes changes to the CI. labels May 13, 2020
@brb
Copy link
Member Author

brb commented May 13, 2020

test-me-please

@coveralls
Copy link

coveralls commented May 13, 2020

Coverage Status

Coverage increased (+0.01%) to 37.036% when pulling d5520c2 on pr/brb/fix-session-affinity-wait into ad6ef2a on master.

@brb brb force-pushed the pr/brb/fix-session-affinity-wait branch from 5a5a9ad to bf44f71 Compare May 13, 2020 16:43
@brb
Copy link
Member Author

brb commented May 13, 2020

test-me-please

test/k8sT/Services.go Outdated Show resolved Hide resolved
@brb
Copy link
Member Author

brb commented May 13, 2020

test-focus K8sService*

@brb brb force-pushed the pr/brb/fix-session-affinity-wait branch from bf44f71 to 22a69a5 Compare May 14, 2020 08:09
@brb
Copy link
Member Author

brb commented May 14, 2020

test-focus .sessionAffinity.

@brb
Copy link
Member Author

brb commented May 14, 2020

test-focus K8sService*

@brb brb added the ci/flake This is a known failure that occurs in the tree. Please investigate me! label May 14, 2020
@brb brb added this to In Progress (Cilium) in CI Force via automation May 14, 2020
@brb brb force-pushed the pr/brb/fix-session-affinity-wait branch from 22a69a5 to b0d0838 Compare May 14, 2020 10:13
@brb brb requested a review from a team May 14, 2020 10:13
@brb
Copy link
Member Author

brb commented May 14, 2020

test-focus K8sService*

@brb brb changed the title test: Fix waiting in testSessionAffinity service/test: Fix waiting in testSessionAffinity and regroup affinity match map updates May 14, 2020
@brb
Copy link
Member Author

brb commented May 14, 2020

test-me-please

@brb brb requested a review from joestringer May 14, 2020 13:26
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

[Ignore this comment - I forgot to add inline comment before submitting the review, sorry for the noise]

Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Looks good, and nice catch! One question.

test/k8sT/Services.go Outdated Show resolved Hide resolved
@brb brb force-pushed the pr/brb/fix-session-affinity-wait branch from b0d0838 to d5520c2 Compare May 14, 2020 15:11
@brb brb requested a review from qmonnet May 14, 2020 15:11
@brb
Copy link
Member Author

brb commented May 14, 2020

test-focus K8sService*

brb added 2 commits May 14, 2020 17:11
Previously, the waiting for the cilium endpoint was broken in
testSessionAffinity helper: jq didn't return error after the endpoint
has been removed, which lead to the long duration times.

Fixing the jq issue revealed, that it's not enough to wait until
the endpoint has disappeared from "cilium endpoints list".

Further debugging has shown, that the first request from outside after
the pod removal was failing because the IPCache entry was missing
for the new pod at the time the request was sent which lead to
the packet drop (see the comment in the test helper for more details).

Signed-off-by: Martynas Pumputis <m@lambda.lt>
Add matches for new backends after the backends have been inserted into
the BPF maps. Otherwise, if an affinity for the svc existed before and
its backend ID == the new backend ID, then the new backend will be
selected which is missing for a short period of time in the map (until
it's inserted).

Signed-off-by: Martynas Pumputis <m@lambda.lt>
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

Thanks for the change, looks all good to me now.

@aanm aanm merged commit 1476f64 into master May 14, 2020
1.8.0 automation moved this from In progress to Merged May 14, 2020
CI Force automation moved this from In Progress (Cilium) to Fixed / Done May 14, 2020
@aanm aanm deleted the pr/brb/fix-session-affinity-wait branch May 14, 2020 18:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/CI Continuous Integration testing issue or flake ci/flake This is a known failure that occurs in the tree. Please investigate me! kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
CI Force
  
Fixed / Done
Development

Successfully merging this pull request may close these issues.

CI: SessionAffinity tests take 8 minutes to run
5 participants