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

feat(promo): support long-lived promotions and improve reactiveness #1038

Merged
merged 2 commits into from
Oct 30, 2023

Conversation

jessesuen
Copy link
Member

@jessesuen jessesuen commented Oct 27, 2023

This change refactors the promo controller to support long lived promotions as well as make it more reactive to completions of other promos on the stage.

Changes:

  1. PromoteFn is called during normal Reconcile() instead of background go routine
  2. Reconcile() is a no-op for promos that are already terminal, or is not their turn to run
  3. If no promotions for a stage is currently running, Reconcile() will only allow the highest priority promo to reconcile (based on creationTimestamp and name). All others will be a no-op
  4. Reconcile() allows any Promotion that is already Running to continue to run. This set us up for long-lived promotions (to be implemented in future PR).
  5. PriorityQueue has a Peek() function
  6. PriorityQueue ignores nil objects if pushed
  7. PriorityQueue is a no-op if the pushed object already exists in the queue. This allows Push() to idempotent because Reconcile() calls it repeatedly

The following video shows 20 promotions getting created via a bash command, and then the promo reconciler working through them one by one, from earliest to latest:

Screen.Recording.2023-10-26.at.6.46.24.PM.mov

Ignore the fact that they error, that is expected.

@netlify
Copy link

netlify bot commented Oct 27, 2023

Deploy Preview for docs-kargo-akuity-io ready!

Name Link
🔨 Latest commit 9b886f1
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-akuity-io/deploys/65403acd956eaf000875c108
😎 Deploy Preview https://deploy-preview-1038.kargo.akuity.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jessesuen jessesuen force-pushed the feat/long-lived-promotions branch 2 times, most recently from b3f37e4 to 1176066 Compare October 28, 2023 04:00
@codecov
Copy link

codecov bot commented Oct 28, 2023

Codecov Report

Attention: 179 lines in your changes are missing coverage. Please review.

Comparison is base (70ec2ca) 51.41% compared to head (9b886f1) 51.83%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
+ Coverage   51.41%   51.83%   +0.41%     
==========================================
  Files         104      106       +2     
  Lines        7680     7802     +122     
==========================================
+ Hits         3949     4044      +95     
- Misses       3586     3612      +26     
- Partials      145      146       +1     
Files Coverage Δ
internal/controller/stages/watches.go 0.00% <ø> (ø)
internal/controller/stages/stages.go 77.71% <0.00%> (+0.27%) ⬆️
internal/controller/runtime/queues.go 90.47% <83.33%> (+7.71%) ⬆️
internal/controller/promotions/promoqueues.go 84.15% <84.15%> (ø)
internal/kargo/kargo.go 44.11% <0.00%> (-55.89%) ⬇️
internal/controller/promotions/promotions.go 41.63% <58.41%> (+4.86%) ⬆️
internal/controller/promotions/watches.go 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jessesuen jessesuen force-pushed the feat/long-lived-promotions branch 3 times, most recently from 2bc95bd to 4c1aeee Compare October 28, 2023 04:17
@jessesuen jessesuen marked this pull request as ready for review October 28, 2023 04:17
@jessesuen jessesuen requested a review from a team as a code owner October 28, 2023 04:17
@jessesuen jessesuen force-pushed the feat/long-lived-promotions branch 2 times, most recently from a781a5b to f370a6f Compare October 28, 2023 04:46
@krancour
Copy link
Member

Super excited for this! Reviewing is my top priority on Monday!


// If we get here, the Stage does not have any Promotions Running against it.
// Now check if it this promo is the one that should run next.
first := pq.Peek()
Copy link
Member

Choose a reason for hiding this comment

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

I really like this approach and wish I'd thought of it from the start.

@krancour
Copy link
Member

krancour commented Oct 30, 2023

@jessesuen with only a few non-blocking nits, this all looks good to me.

Want to make sure that I properly understand that the extent of the "long-running" promotion support here is that running promotions can be re-reconciled, whereas they were not before. That doesn't do much for us at the moment, but opens some doors. Have I got that right?

@krancour krancour self-requested a review October 30, 2023 19:19
@jessesuen
Copy link
Member Author

jessesuen commented Oct 30, 2023

Want to make sure that I properly understand that the extent of the "long-running" promotion support here is that running promotions can be re-reconciled, whereas they were not before. That doesn't do much for us at the moment, but opens some doors. Have I got that right?

You have it exactly right. All of our promo mechanisms happen synchronously so it doesn't really help, yet. In the future, a call to promoteFn might leave the promotion still in a Running phase. And we will allow the promo to keep reconciling after indefinitely until promoteFn indicates the promotion is terminal.

The next PR may need to add concepts like Resume() to the promo interface, so that a promo that has already started, might do a different piece of work (e.g. check on the status of pull request) for the 2nd reconcile vs. the first reconcile (create the pull request).

@jessesuen jessesuen force-pushed the feat/long-lived-promotions branch from 2f89d34 to b0df0a8 Compare October 30, 2023 23:04
Signed-off-by: Jesse Suen <jesse@akuity.io>
@jessesuen jessesuen force-pushed the feat/long-lived-promotions branch 2 times, most recently from aaafba7 to 939e02f Compare October 30, 2023 23:19
Signed-off-by: Jesse Suen <jesse@akuity.io>
@jessesuen jessesuen force-pushed the feat/long-lived-promotions branch from 939e02f to 9b886f1 Compare October 30, 2023 23:22
@jessesuen jessesuen merged commit c9062d9 into akuity:main Oct 30, 2023
@jessesuen jessesuen deleted the feat/long-lived-promotions branch October 30, 2023 23:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants