-
Notifications
You must be signed in to change notification settings - Fork 372
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
Feature/deployment concurrency #1884
Feature/deployment concurrency #1884
Conversation
Co-authored-by: kuuji <ag94441@gmail.com>
Co-authored-by: erikkrieg <erik.c.krieg@gmail.com>
Co-authored-by: kuuji <ag94441@gmail.com>
Co-authored-by: erikkrieg <erik.c.krieg@gmail.com>
Co-authored-by: erikkrieg <erik.c.krieg@gmail.com>
@erikkrieg Thanks a lot for the PR! :) |
It'll also help if you could add e2e tests for this feature.
You can refer to some existing e2e tests under @FabianKramm WDYT? |
Thanks for the suggestion @pratikjagrut I also included some vscode config to run e2e, similar to what vcluster has. Let me know if you prefer that specific part to be in a different PR since it's not really related to this PR. |
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.
Thank you @kuuji :) E2E tests LGTM just some minor changes.
Personally, I would have prefered separate PR for this change as it is not an intrinsic part of this feature. |
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.
@erikkrieg @kuuji thanks so much for this PR! Thats really an improvement for DevSpace! Besides some small things in the e2e test the PR looks great and is ready to get merged!
That's fair, I'll put a separate PR 👍 |
Included documentation. Feeling pretty good about most of it but not sure whether the warning about an edge case with Helm hooks and concurrency is too specific or not. |
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.
@erikkrieg thanks a lot, LGTM now!
Thanks, @FabianKramm and @pratikjagrut! |
What issue type does this pull request address?
/kind enhancement
/kind feature
What does this pull request do? Which issues does it resolve?
Enables deployments to opt into deploying concurrently in order to improve performance for configs with many deployments.
resolves #1877
We did some tests with a modified version of
examples/quickstart
and found a roughly 35% - 50% reduction in deployment time. The more deployments, the greater the improvement.Please provide a short message that should be published in the DevSpace release notes
Allow select deployments to deploy concurrently by setting
concurrent: true
.What else do we need to know?
The logging was changed to resemble how concurrent image build logs are handled. We attempted to capture the effect the sequential deploys get from using
StartWait
but this is something we were less certain about.Example snippet of logging output:
Something we didn't love but are not sure how to address is that as the number of remaining deployments decrements a new
StartWait
call is made and therefore the count of seconds starts back at 0.We also wanted to wait for feedback before updating any documentation and will certainly do this if/when things are looking good :D