-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
test: Fix pod cleanup after various tests #18448
test: Fix pod cleanup after various tests #18448
Conversation
/test Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR: Click to show.Test Name
Failure Output
If it is a flake, comment |
😂 of course this PR hits the exact same problem, but with leftover state from a different test:
Am I approaching this the wrong way? Should we be making sure all of the tests clean up after themselves properly or should we just make sure that the first test in each test group gets the environment back into a good state? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well spotted!
(Thanks GH for not refreshing the PR comments when I'm reviewing) To be honest I kinda assumed that every test was already supposed to clean up after itself, unless a test suite is specifically designed with environment being kept around for several tests in a row (in which case the suite should clean up). So I was convinced your approach would be right... |
I think this is basically a decision we have to make :-) Note that this approach can be right, but the same class of bug is present elsewhere in the testsuite. The failure that happened here is not the exact same test or conditions as the one I reported in #18447, since the previous test in the failure on this PR was in The likelihood of this failure is based on (1) removing and redeploying Cilium, (2) race conditions between deleting app pods and starting the next test, and (3) how many different test files there are[1] Maybe we just need to apply the same fix to K8sDatapathConfig and then we're done. I guess I'll just look over the other test files and look for a similar pattern. [[1]] If a bug like this is caused by incorrect ordering between |
d595a65
to
f32d94c
Compare
1b12a56
to
cbd789d
Compare
I'm hoping that commit d4739ad will fix the above issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed d4739ad. The change looks correct to me.
Thanks! 🙏
While at it, we should remove this duplicate code that deploys the yaml - https://github.com/cilium/cilium/blob/master/test/k8sT/Services.go#L1639-L1641. I can fix this in a separate PR if you don't want to do it in this PR.
The next commit will then shift the cleanup logic
out of a Describe level AfterEach and into the individual Contexts.
Can you confirm the commit? Might as well double check that too.
Please do, I'm hoping that this PR is now in a good state to merge & stabilize the tree.
The commit is 30a6d90 . I can't confirm it in the commit message unfortunately because github will rewrite the commit shas when we merge the PR. Hence I just rearranged the commits to be in order so that this statement would be correct for the next commit. (The only reason for these gymnastics is to prevent introducing the failure into the tree then removing it again, so I moved the commit earlier in the ordering in this branch). |
CodeQL analysis seems like it's maybe a new checker that has been introduced which is triggering false positives on regexes in code that this PR doesn't touch. Ignoring. |
/test |
Travis hit pull ratelimit issue, ignoring (prior versions passed successfully and this PR is focused on ginkgo test code which isn't tested by Travis). Everything else passed, the PR has been reviewed, and this fixes a common CI flake on master. Merging. |
See individual commits for more details.
According to the Cilium Ginkgo e2e test docs, All
AfterEach
statements are executed before allAfterAll
statements.Previously, some of this code was assuming that
AfterAll
inside a context would run first (to delete pods) and then theAfterEach
would run outside the context (to wait for the pods to terminate), thenAfterAll
outside the context to finally clean up the Cilium pods. The result was that in issue #18447, the next test to run could hit a race condition where pods were deleted but did not fully terminate before Cilium was removed. Cilium ends up getting deleted before all the pods, which means that there is no longer any way to execute theCNI DEL
operation for those pods, so they get stuck inTerminating
state.Other parts of the code seemed to just omit the check for whether the pods were correctly terminated or not.
Fix these isseus by moving checks that the test pods are fully terminated into the statements where the pods are deleted.
Fixes: 412e299 ("test: Move LRP tests to a separate suite")
Fixes: #18447
Fixes: #18566
Note, I initially marked this for backport to v1.10 based on the first LRP test refactor commit, but left it like this since it seems like it should make the testsuite a bit more robust in general. I don't know if each of the changes will successfully backport to v1.10 or not. If not, we can maybe just drop those changes from the commit. Or we could just decide to avoid backporting the PR to v1.10 altogether.