chore: Parallelize EnsureCleanState for e2e tests, adding timing information #20998
Conversation
…rmation Closes argoproj#20968 Closes argoproj#20967 A number of cleanup and create steps can be done in parallel, reducing the total runtime. Gather timing information as well and log it. Signed-off-by: Andrii Korotkov <[email protected]>
❌ Preview Environment deleted from BunnyshellAvailable commands (reply to this comment):
|
Signed-off-by: Andrii Korotkov <[email protected]>
…n't fit in Github logs and gets truncated Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
…emoving namespaces Signed-off-by: Andrii Korotkov <[email protected]>
…king based on timings Signed-off-by: Andrii Korotkov <[email protected]>
… no wait commands Signed-off-by: Andrii Korotkov <[email protected]>
…o server watcher seems to fail Signed-off-by: Andrii Korotkov <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20998 +/- ##
==========================================
- Coverage 55.07% 55.02% -0.05%
==========================================
Files 324 324
Lines 55472 55472
==========================================
- Hits 30550 30524 -26
- Misses 22308 22327 +19
- Partials 2614 2621 +7 ☔ View full report in Codecov by Sentry. |
|
Seems to save a couple of minutes. The savings aren't as big as expected, since login as admin and config maps updates are taking some time, and the later doesn't parallelize well (sequential time is similar to parallel time).
|
|
Can we remove the timing info code to simplify things now that you've done the initial speedup/analysis? |
Signed-off-by: Andrii Korotkov <[email protected]>
I'm not done yet :) I'm trying another commit to only do updates when necessary for slowest parts. |
Signed-off-by: Andrii Korotkov <[email protected]>
|
I'd only leave it in place if there's also documentation on how to use the logging for further optimization and if there's a strong likelihood that someone will actually use it to significantly improve performance. If the upper bound to improvement is ~30s, I say kill the code. |
|
Okay, I'll clean it up when done with optimizations here. |
Signed-off-by: Andrii Korotkov <[email protected]>
|
Yay, best case time for login as admin was reduced to ~2ms instead of ~500-600ms due to skipping re-login if not necessary. Config map reset was reduced to ~20-200ms instead of ~800-1000ms due to skipping update if equivalent. |
|
That's another ~4 min of savings, bringing total savings to ~6 min.
|
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
|
About the same times |
|
@crenshaw-dev, I think this is ready to go now. |
andrii-korotkov-verkada
requested review from
leoluz,
crenshaw-dev and
ishitasequeira
test/e2e/fixture/fixture.go
Outdated
| wg.Add(1) | ||
| go func() { | ||
| defer wg.Done() | ||
| function() |
There was a problem hiding this comment.
Are we relying on os.Exits in the parallel functions for error handling?
There was a problem hiding this comment.
Hm, that's a good point, probably need to return the errors to the main function.
There was a problem hiding this comment.
It's not that easy. I've checked and it's relying on os.Exit after logging Fatal. Is that okay?
There was a problem hiding this comment.
I've figured out a way.
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
|
With the latest update, it seems to be only a tiny bit faster, maybe winning like 20 seconds. Before: After: |
| "delete_applications_resources_namespace": func() error { | ||
| // Delete the applicationset-e2e namespace, if it exists | ||
| err := fixtureClient.KubeClientset.CoreV1().Namespaces().Delete(context.Background(), ApplicationsResourcesNamespace, v1.DeleteOptions{PropagationPolicy: &policy}) | ||
| if err != nil && !strings.Contains(err.Error(), "not found") { // 'not found' error is expected |
There was a problem hiding this comment.
I believe there's a k8s library function that checks whether an error is a "not found" error.
There was a problem hiding this comment.
Updated to use that
|
So total savings here is around 5min? |
|
About 6-7 minutes. |
…2e-tests Signed-off-by: Andrii Korotkov <[email protected]>
|
I've merged master in and let's wait for the test run. |
Signed-off-by: Andrii Korotkov <[email protected]>
|
@crenshaw-dev, new results are in, with this PR we are down to ~30 min to run e2e tests.
|
|
For some reason the tests for v1.29 and v1.28 were slower. |
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
Signed-off-by: Andrii Korotkov <[email protected]>
andrii-korotkov-verkada
deleted the
20968-parallelize-ensure-clean-state-for-e2e-tests
branch
…rmation (argoproj#20998) * chore: Parallelize EnsureCleanState for e2e tests, adding timing information Closes argoproj#20968 Closes argoproj#20967 A number of cleanup and create steps can be done in parallel, reducing the total runtime. Gather timing information as well and log it. Signed-off-by: Andrii Korotkov <[email protected]> * Add underscores to function map keys Signed-off-by: Andrii Korotkov <[email protected]> * Have separate log statements for timing fields, since everything doesn't fit in Github logs and gets truncated Signed-off-by: Andrii Korotkov <[email protected]> * Re-run tests Signed-off-by: Andrii Korotkov <[email protected]> * Fix duplicate removal attempts, cleanup things in namespaces before removing namespaces Signed-off-by: Andrii Korotkov <[email protected]> * Combine config map cleanups, since they seem to have some kind of locking based on timings Signed-off-by: Andrii Korotkov <[email protected]> * Re-order some cleanups to not wait if something is already covered by no wait commands Signed-off-by: Andrii Korotkov <[email protected]> * Move rm temp dir to be together with re-creation, since otherwise repo server watcher seems to fail Signed-off-by: Andrii Korotkov <[email protected]> * Only update config maps and login as admin if there were any changes Signed-off-by: Andrii Korotkov <[email protected]> * Also check for user being logged in Signed-off-by: Andrii Korotkov <[email protected]> * Improve config maps equivalency check Signed-off-by: Andrii Korotkov <[email protected]> * Remove timing information Signed-off-by: Andrii Korotkov <[email protected]> * Improve loops with sleep Signed-off-by: Andrii Korotkov <[email protected]> * Properly propagate and handle errors for parallel functions Signed-off-by: Andrii Korotkov <[email protected]> * Add missing CheckError Signed-off-by: Andrii Korotkov <[email protected]> * Fix import Signed-off-by: Andrii Korotkov <[email protected]> * Fix imports linter error (try 2) Signed-off-by: Andrii Korotkov <[email protected]> * Parallelize EnsureCleanState for application sets Signed-off-by: Andrii Korotkov <[email protected]> * Fix errors.IsNotFound Signed-off-by: Andrii Korotkov <[email protected]> * Use errgroup to run functions in parallel Signed-off-by: Andrii Korotkov <[email protected]> * Remove name keys for functions as unused Signed-off-by: Andrii Korotkov <[email protected]> * Use apierr directly Signed-off-by: Andrii Korotkov <[email protected]> --------- Signed-off-by: Andrii Korotkov <[email protected]> Signed-off-by: Adrian Aneci <[email protected]>
Closes #20968
Closes #20967
A number of cleanup and create steps can be done in parallel, reducing the total runtime. Gather timing information as well and log it.
Checklist: