E2E improvements for claim server side apply#6182
Conversation
|
All 5 E2E suites failed, but none of them were 4/5 of the failures were for I'll try to dig a bit, but let me know if you see any immediate obvious clues 🤔 |
|
Ah, I think I see what's going on here @turkenh, this is probably related to the changes in this PR to bump the In the But it looks like now the e2e framework appends another error message ( edit: hehe, this new error message was definitely added in the |
negz
left a comment
negz
left a comment
There was a problem hiding this comment.
Nice! Just some style nits - feel free to take them or leave them.
| if deadlineExceed(err) { | ||
| t.Logf("Deployment %s/%s had running pod that did not change after %s", dp.GetNamespace(), dp.GetName(), since(start)) | ||
| } else { | ||
| t.Errorf("Error while observing pod for deployment %s/%s", dp.GetNamespace(), dp.GetName()) | ||
| } |
There was a problem hiding this comment.
| if deadlineExceed(err) { | |
| t.Logf("Deployment %s/%s had running pod that did not change after %s", dp.GetNamespace(), dp.GetName(), since(start)) | |
| } else { | |
| t.Errorf("Error while observing pod for deployment %s/%s", dp.GetNamespace(), dp.GetName()) | |
| } | |
| if deadlineExceed(err) { | |
| t.Logf("Deployment %s/%s had running pod that did not change after %s", dp.GetNamespace(), dp.GetName(), since(start)) | |
| return ctx | |
| t.Errorf("Error while observing pod for deployment %s/%s", dp.GetNamespace(), dp.GetName()) |
Style nit: I prefer to return early to reduce nesting where possible. This captures that the "Error while observing..." is kinda the default/catchall case for the block it's in.
There was a problem hiding this comment.
yep that's a good pattern and the intent of this block is preserved with an earlier return.
this same type of check was in 3 places, so I updated all 3 in 93f96e6.
| if wantSet && !found { | ||
| t.Logf("did not find arg %s within %s", arg, c.Args) | ||
| return false, nil | ||
| } else if !wantSet && found { | ||
| t.Logf("unexpectedly found arg %s within %s", arg, c.Args) | ||
| return false, nil | ||
| } | ||
|
|
||
| return true, nil |
There was a problem hiding this comment.
Style nit: I think this all could be a switch statement.
There was a problem hiding this comment.
made this style change to a switch statement in 93f96e6
This picks up kubernetes-sigs/e2e-framework#421 from @negz that improves e2e test logging to include more accurate call points. Signed-off-by: Jared Watts <[email protected]>
Signed-off-by: Jared Watts <[email protected]>
Signed-off-by: Jared Watts <[email protected]>
Signed-off-by: Jared Watts <[email protected]>
Signed-off-by: Jared Watts <[email protected]>
2 participants
Description of your changes
This PR focuses on improving the reliability of the
TestPropagateFieldsRemovalToXRAfterUpgradeE2E test. This test has been consistently flaky according to buildpulse and as we mature claim SSA to beta, we want more reliability to be confident in the promotion.From brainstorming with @negz, the theory is that the claim is being updated before the upgrade to enable SSA is fully completed as described in #6090 (comment). Therefore, we now run some more thorough checks to ensure the upgrade is complete, e.g. the
--enable-ssa-claimsflag is set and the pod has stayed running for at least 10 seconds.With these changes, I was able to run the
TestPropagateFieldsRemovalToXRAfterUpgradetest overnight in a loop 30 times with 0 failures 🎉There are also some E2E test README additions from my lessons on running and debugging the tests more easily.
Related to #5656
I have:
earthly +reviewableto ensure this PR is ready for review.Added or updated unit tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.