feat: pause auto-promotion after promoting older freight#6334
feat: pause auto-promotion after promoting older freight#6334jacobboykin wants to merge 110 commits into
Conversation
✅ Deploy Preview for docs-kargo-io ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
1491789 to
18a0ed0
Compare
c60a74e to
19e84b8
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6334 +/- ##
==========================================
+ Coverage 58.77% 59.08% +0.31%
==========================================
Files 506 507 +1
Lines 42928 43278 +350
==========================================
+ Hits 25232 25572 +340
+ Misses 16157 16151 -6
- Partials 1539 1555 +16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1d5dd27 to
76aa755
Compare
Yes please wait for that PR to merge. I will help with any UI conflict resolution in this PR, would be much easier than doing it in that PR |
|
Ok... I've obviously put a lot of effort into understanding the choices in this PR. I realize that my many in-line comments don't tell the whole story, as I see it. So, please just feel free to dismiss all of those. This one comment is a comprehensive snapshot of my thinking here. (I have, in fact, marked all my other comments as resolved to reduce overall noise.) First, credit where it's due: generally speaking, I think the contours of the proposed changes are logically correct. My concern isn't correctness; it's surface area. I think a few foundational choices are each pulling in a lot of machinery, and that removing them collapses most of the complexity without losing the behavior we actually want. What I think is driving the complexity
The leaner shape I'd like us to consider
Single writer means we can drop the optimistic-locking apparatus, the gate, Behavior is unchanged from a user's perspective: manually promoting older Freight pauses auto-promotion for that origin, promoting the current candidate resumes it, and pauses are per-origin. Two residual races remain, and I'm comfortable accepting both with a clear comment in the webhook: a benign, self-clearing intent misclassification, and a transient (self-correcting, because the queue runs oldest-first) case where newer Freight briefly promotes before the rollback lands. Neither is worth more machinery. Happy to talk it through. |
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
…otions
The Promotion webhook skipped hold/release intent inference for any
admission request originating from the Kargo control plane, on the
theory that such Promotions are system-generated and carry no user
intent. But the Kargo API server is part of the control plane and
creates Promotions on users' behalf using its own service account --
so Promotions created via the UI, CLI, or REST API (the primary user
flows) never received intent annotations, and manual promotions made
through them could neither establish nor release auto-promotion holds.
The feature effectively worked only for Promotions created directly
with kubectl.
The discriminator for "does this control-plane Promotion carry user
intent" already exists: the API server records the requesting user in
the create-actor annotation before creating a Promotion on their
behalf. The Stage controller's own auto-promotions set no create-actor
at all, and other controllers (e.g. a rollback controller) identify
themselves with a controller: actor. The webhook now infers intent for
control-plane requests whose Promotion names a non-controller actor,
and continues to leave genuinely system-generated Promotions untouched
-- including preserving any intent annotations they set explicitly.
Note that matching a user actor by prefix is not possible: the
create-actor value for users varies by identity ("admin", "email:...",
"subject:...", claim-based forms), so the rule keys on the absence of
an actor or the controller: prefix instead.
Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
efe8eb0 to
184679c
Compare
…ets set Signed-off-by: fuskovic <fhuskovic92@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
…er-wins patch status race Signed-off-by: fuskovic <fhuskovic92@gmail.com>
Reverts 2320ce0 and 7d2be5f. The state the first guarded against -- a terminal Promotion that was never acknowledged as current -- is unreachable: the promotions controller will not run a Promotion before the Stage records it as current. The race actually observed is elsewhere: a Promotion can go terminal between syncPromotions computing hold state and autoPromoteFreight acting on it. That will be fixed by having the auto-promotion guard also consider terminal hold-intent Promotions newer than status.lastPromotion. The second added optimistic locking to PatchStatus, which every status writer in the codebase uses. That is too large a behavioral change to adopt as a side effect of this PR -- and it would not have prevented the harmful outcome here, which is a Promotion creation, not a status write. Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
A Promotion can reach a terminal phase in the interval between syncPromotions computing hold state and autoPromoteFreight acting on it. The guard for in-flight hold intent only considered non-terminal Promotions, so one that succeeded moments earlier -- its hold not yet recorded -- did not block, and auto-promotion could supersede older Freight the user just deliberately promoted. The guard now also blocks on terminal hold-intent Promotions newer than status.lastPromotion, i.e., those whose outcome has not been recorded yet. Once the next reconciliation records the outcome, the hold itself takes over the blocking (or, for a failed Promotion, auto-promotion correctly resumes). Also documents the mirror ordering of accepted race 2 in the Promotion webhook: a queued auto-promotion created moments before a hold-intent Promotion may supersede it once; the hold survives (auto-promotions never imply intent) and recovery is re-promoting the older Freight. No read-time guard can see a Promotion created after the read. Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
…andidate Same window as the previous commit, different consumer: a fast Promotion can go from pending to succeeded between syncPromotions observing it and autoPromoteFreight acting. The duplicate-suppression check only matched non-terminal Promotions, and a succeeded Promotion for the candidate is deliberately not disqualifying (holds replaced the old any-Promotion-exists guard, which had been suppressing this duplicate incidentally). Auto-promotion could therefore create a second, identical Promotion moments after the first succeeded. The check now stands down while any Promotion for the candidate is either non-terminal or terminal but newer than status.lastPromotion. Once the outcome is recorded, a success is caught by the candidate-is-already-current check and a failure by the newest-terminal-not-successful check, so recorded history still never blocks -- the old guard stays removed. Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
controller-runtime demotes any untyped error returned from a defaulter to a generic admission denial, which kube-apiserver surfaces as HTTP 403 Forbidden. This made every failure in the Promotion mutating webhook -- including 'no auto-promotion candidate found' during promote-by-origin and transient freight-listing failures -- masquerade as an authorization problem. Return apierrors.NewInvalid (422) for user-correctable conditions (missing Stage, Stage with no promotion steps, no candidate for origin) and apierrors.NewInternalError (500) for infrastructure failures, matching the idiom the validating webhook already uses. Step inflation failures now pass through any underlying typed status error instead of being flattened. Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
The auto-promotion and hold documentation spoke loosely of pausing auto-promotion 'for an origin' without ever using the nomenclature Stages actually use: freight requests (the entries of spec.requestedFreight). Introduce the term where requestedFreight is first described, note that requests behave like slots each holding one piece of Freight from a specific origin, and define the per-request auto-promotion candidate. Holds are now described precisely as established on, recorded for, and lifted from a specific freight request, leaving the Stage's other freight requests unaffected. Also document that hold/resume intent takes effect only when a Promotion succeeds and that auto-promotion's own Promotions never carry intent. Signed-off-by: Kent Rancourt <kent.rancourt@gmail.com>
Signed-off-by: fuskovic <fhuskovic92@gmail.com>
| {field: indexer.FreightByWarehouseField, fn: indexer.FreightByWarehouse}, | ||
| {field: indexer.FreightByCurrentStagesField, fn: indexer.FreightByCurrentStages}, | ||
| {field: indexer.FreightByVerifiedStagesField, fn: indexer.FreightByVerifiedStages}, | ||
| {field: indexer.FreightApprovedForStagesField, fn: indexer.FreightApprovedForStages}, |
There was a problem hiding this comment.
@fuskovic, non-blocking, but it's worth checking that after all of the refactoring, all of these are actually still used.
Relates to #3016
Summary
This adds auto-promotion holds for Stages.
When a Promotion other than the Stage controller's own auto-promotion succeeds for Freight that is not the current auto-promotion candidate for the same origin, Kargo records a hold that pauses auto-promotion for that origin on that Stage. That keeps later auto-promotion from moving the Stage back to the candidate. Other origins on the same Stage can continue to auto-promote.
Auto-promotion resumes after a Promotion other than the Stage controller's own auto-promotion succeeds for the current candidate for that origin.
How It Works
For created Promotions on auto-promotion-enabled Stages, the Promotion webhook infers promotion intent unless the Promotion is marked as one created by the Stage controller's auto-promotion loop. That keeps Kargo's own auto-promotions from creating holds while allowing UI, CLI,
kubectl, external automation, and Enterprise auto-rollback Promotions to participate without special handling.The Stage controller is the only writer of committed hold state. It records active holds in
status.autoPromotionHolds, keyed by origin, after a hold-intent Promotion succeeds. A succeeded release-intent Promotion clears the hold.Hold processing uses a timestamp/name watermark so effects are applied once and remain stable even after Promotion GC.
autoPromoteFreightskips an origin when:That second check closes the window where a user-selected hold Promotion exists but has not yet reached Stage status.
Promote By Origin
This also adds a small general primitive: promote by origin.
REST, CLI, and raw
Promotionspecs can now ask Kargo to promote the current candidate for an origin in the direct Stage promotion path:kargo promote \ --project my-project \ --stage test \ --origin Warehouse/my-warehouseFor raw Kubernetes usage, create a
Promotionwithspec.origininstead ofspec.freight. The mutating webhook resolves the origin to the current candidate and fillsspec.freightbefore the Promotion is persisted.The REST promote request now accepts exactly one of
freight,freightAlias, ororigin. The Promotion CRD accepts exactly one ofspec.freightorspec.origin.ConnectRPC is intentionally left as-is because it is deprecated and has no current caller for promote-by-origin.
UI And Docs
The UI now surfaces active holds from
Stage.status.autoPromotionHolds:This PR does not add a dedicated resume button or pending-hold UI. Resume is already supported by promoting the current candidate, including with
--origin.Docs were updated to explain when auto-promotion pauses and how to resume it.