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: factor optional "soak time" into freight availability #3100

Merged
merged 17 commits into from
Jan 10, 2025

Conversation

hiddeco
Copy link
Contributor

@hiddeco hiddeco commented Dec 9, 2024

Fixes: #2069

With this addition, it is possible to configure a duration for which the Freight of an upstream Stage has to be verified, effectively allowing it to "soak" in this Stage.

apiVersion: kargo.akuity.io/v1alpha1
kind: Stage
metadata:
  name: prod
  namespace: guestbook
spec:
  # ...omitted for brevity 
  requestedFreight:
  - origin:
      kind: Warehouse
      name: guestbook
    sources:
      stages: ["staging"]
      # Freight needs to be verified for 12 hours in "staging"
      # before being allowed to be auto-Promoted to "prod".
      verifiedFor: 12h0m

Copy link

netlify bot commented Dec 9, 2024

Deploy Preview for docs-kargo-io ready!

Name Link
🔨 Latest commit df8c497
🔍 Latest deploy log https://app.netlify.com/sites/docs-kargo-io/deploys/67806a023a11b00008487999
😎 Deploy Preview https://deploy-preview-3100.docs.kargo.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.

Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 88.70056% with 40 lines in your changes missing coverage. Please review.

Project coverage is 51.55%. Comparing base (4972b6c) to head (df8c497).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
internal/controller/stages/regular_stages.go 67.12% 19 Missing and 5 partials ⚠️
internal/controller/stages/control_flow_stages.go 71.42% 8 Missing ⚠️
internal/indexer/indexer.go 75.00% 2 Missing and 1 partial ⚠️
internal/api/promote_to_stage_v1alpha1.go 0.00% 2 Missing ⚠️
internal/api/approve_freight_v1alpha1.go 80.00% 0 Missing and 1 partial ⚠️
internal/api/query_freights_v1alpha1.go 50.00% 1 Missing ⚠️
internal/controller/promotions/promotions.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3100      +/-   ##
==========================================
+ Coverage   51.40%   51.55%   +0.15%     
==========================================
  Files         288      288              
  Lines       26108    26188      +80     
==========================================
+ Hits        13421    13502      +81     
+ Misses      11961    11960       -1     
  Partials      726      726              

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

@hiddeco hiddeco force-pushed the delay-freight-promotion branch from 7a1ebc7 to 6248d3f Compare December 9, 2024 16:20
@hiddeco hiddeco self-assigned this Dec 9, 2024
@hiddeco hiddeco marked this pull request as ready for review December 11, 2024 07:53
@hiddeco hiddeco requested a review from a team as a code owner December 11, 2024 07:53
@krancour
Copy link
Member

krancour commented Jan 2, 2025

This looks like it doesn't comprehensively address the question of a piece of Freight's overall availability to a given Stage. The required soak time isn't, for instance, being enforced in the case of a manual Promotion.

I realize that based on wording of this PR's title and issue #2069, that the scope might have been narrowly about auto-promotion, in which case this PR checks that box perfectly, but imho, the soak time should also be enforced for manual promotions, and if someone absolutely needs to manually Promote a piece of Freight whose soak time hasn't elapsed, they should add a manual Approval to it first. (I don't think soak time should apply to manual approvals.)

cc @jessesuen for his opinion on what the scope is supposed to be.

@hiddeco since you're out for a bit, I will take up any remaining work on this pending @jessesuen's clarification.

@jessesuen
Copy link
Member

The required soak time isn't, for instance, being enforced in the case of a manual Promotion.
if someone absolutely needs to manually Promote a piece of Freight whose soak time hasn't elapsed, they should add a manual Approval to it first. (I don't think soak time should apply to manual approvals.)

Thanks for catching this. I agree. We can consider this feature similar to a policy is enforced regardless of auto vs. manual promotion and it would be good to have the guard rail in place for both humans and machines. Manual approval seems like the appropriate way to go around it.

@krancour
Copy link
Member

krancour commented Jan 2, 2025

Thanks @jessesuen. I think this is relatively easy to resolve.

@krancour krancour assigned krancour and unassigned hiddeco Jan 3, 2025
@krancour
Copy link
Member

krancour commented Jan 3, 2025

I have this well underway now. I should have something ready late tomorrow.

@krancour krancour changed the title feat: allow to delay auto-Promotion of Freight feat: factor optional "soak time" into freight availability Jan 4, 2025
@krancour
Copy link
Member

krancour commented Jan 4, 2025

The last four commits I added move Freight availability logic to one central place and it's applied consistently whether you're promoting, auto-promoting, or just querying for available Freight.

I'm changing this to a draft, because there is still an outstanding issue here that I discovered and @jessesuen and I discussed offline. This will currently count a piece of Freight as available to a given Stage if its successful verification merely occurred long-enough ago, which turns out to be insufficient given the following scenario:

Given a pipeline test --> uat, if Freight A were promoted to test and successfully verified and almost immediately afterward, Freight B were promoted to test, Freight A will still become available to uat after the specified time has elapsed, but Freight A didn't "soak" adequately in test.

This is going to require additional bookkeeping to keep track of how long Freight verified in a given Stage actually spent in that Stage.

My best thinking on how to achieve this, which needs to be sanity checked by @hiddeco:

  1. Make Freight status track two new things:

    1. What Stages currently have it and when it got there.
    2. In verification records, track the max duration the Freight spent in that Stage.
  2. Make Stage reconciliation look at the status of every piece of Freight in the current Freight collection (top of the history stack) and, only if necessary, update the above information accordingly.

    In the case where a new Freight collection is pushed on to the top of the Freight history stack, updates need to be applied to the union of all Freight in the old top Freight collection and the new one.

    If a piece of Freight's status is updated to reflect that it is no longer in the Stage, the time it spent there should be calculated. If that Freight's status has a verification record for that Stage and the calculated duration recently spent in that Stage exceeds the currently recorded max duration, the max duration should be updated.

  3. Freight is available to a given Stage under any of the following conditions:

    1. Freight is approved for the Stage
    2. Freight is verified for the Stage with no soak time criteria
    3. Freight is verified for the Stage and the recorded max time spent in that Stage meets or exceeds soak time criteria
    4. Freight is verified for the Stage, currently in the Stage, and the elapsed time since it entered the Stage meets or exceeds soak time criteria

A tangential benefit to this is that with every piece of Freight having a record of what Stages are currently using it, the UI, which I believe is currently crunching that information itself, will no longer have to.

@hiddeco might be able to refine this idea or suggest an even better alternative.

@krancour
Copy link
Member

krancour commented Jan 7, 2025

This is working e2e and now conforms to the requirements that were clarified by @jessesuen in our offline discussions.

@krancour
Copy link
Member

krancour commented Jan 7, 2025

The merge conflicts are all with codegen. I'll resolve those once this is approved because a rebase on main is probably the best way and I don't want to force push while two maintainers are potentially working on this.

api/v1alpha1/freight_helpers_test.go Outdated Show resolved Hide resolved

// IsFreightAvailable answers whether the specified Freight is available to the
// Stage.
func (s *Stage) IsFreightAvailable(freight *Freight) bool {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any specific reason for putting this method on the Stage instead of the Freight? E.g.

Suggested change
func (s *Stage) IsFreightAvailable(freight *Freight) bool {
func (f *Freight) IsEligibleForStage(stage *Stage) bool {

Copy link
Member

Choose a reason for hiding this comment

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

Nice catch.

Copy link
Member

Choose a reason for hiding this comment

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

Oh. Sorry. I misread this.

There wasn't any specific reason.

It's a function for which Stage and Freight are equally important arguments.

I think I could have gone either way on this and this is just where I landed.

If you think there's a compelling reason to do it the other way, I'm unopposed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I felt it should be put on the Freight instead of the Stage because, assuming the Freight != nil, it is always available. If it is available to something appears to be the responsibility of the Freight (see also .IsApprovedFor and .IsVerifiedIn), and not the entity having the desire.

api/v1alpha1/freight_types.go Show resolved Hide resolved
@@ -26,10 +26,9 @@ func freightByCurrentStagesIndexer(obj client.Object) []string {
return nil
}
currentStages := make([]string, len(freight.Status.CurrentlyIn))
var i int
currentStages := make([]string, 0, len(freight.Status.CurrentlyIn))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Something went wrong here when the suggestion was auto-applied.

Copy link
Member

Choose a reason for hiding this comment

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

Will fix.

hiddeco and others added 16 commits January 9, 2025 19:23
Signed-off-by: Kent Rancourt <[email protected]>
Signed-off-by: Kent Rancourt <[email protected]>

Co-authored-by: Hidde Beydals <[email protected]>
Signed-off-by: Kent Rancourt <[email protected]>
@krancour krancour force-pushed the delay-freight-promotion branch from c03ec00 to df8c497 Compare January 10, 2025 00:29
@krancour
Copy link
Member

I removed four codegen commits, rebased on main, re-ran codegen, and force pushed.

@krancour krancour enabled auto-merge January 10, 2025 00:31
@krancour krancour added this pull request to the merge queue Jan 10, 2025
Merged via the queue into akuity:main with commit 759a417 Jan 10, 2025
17 checks passed
fykaa pushed a commit to fykaa/kargo that referenced this pull request Jan 16, 2025
)

Signed-off-by: Hidde Beydals <[email protected]>
Signed-off-by: Kent Rancourt <[email protected]>
Co-authored-by: Kent Rancourt <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to delay Freight verification in a Stage
3 participants