update owner status outside of component reconciliation#110
update owner status outside of component reconciliation#110sourcehawk wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the framework and its consumer patterns so components only stage status conditions in memory, while controllers persist status once per reconcile via a deferred component.FlushStatus call (to reduce self-induced 409 conflicts).
Changes:
- Introduces
FlushStatusand refactors component reconciliation to only apply conditions in-memory (applyStatusCondition), deferring status persistence to controllers. - Updates examples and e2e reconcilers to use named returns and a deferred
FlushStatusat the end of reconciliation. - Updates docs (
README.md,docs/component.md,docs/guidelines.md) to describe the new responsibility split and thatReconcileContext.Metricsis optional.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/component/conditions_test.go | Replaces setStatusCondition tests with coverage for applyStatusCondition and FlushStatus (incl. conflict retry). |
| pkg/component/conditions.go | Removes API-writing setStatusCondition in favor of purely in-memory applyStatusCondition. |
| pkg/component/component_test.go | Updates helper to flush and refetch so assertions reflect persisted status. |
| pkg/component/component.go | Moves status persistence/metrics to new exported FlushStatus; updates reconciliation/docs to stage conditions only. |
| examples/mutations-and-gating/app/controller.go | Defers FlushStatus and reuses a shared recCtx. |
| examples/grace-inconsistency/app/controller.go | Defers FlushStatus and reuses a shared recCtx. |
| examples/extraction-and-guards/app/controller.go | Defers FlushStatus and reuses a shared recCtx. |
| examples/custom-resource/app/controller.go | Defers FlushStatus and reuses a shared recCtx. |
| examples/component-prerequisites/app/controller.go | Defers FlushStatus to persist multiple component conditions in one write. |
| e2e/framework/reconciler.go | Defers FlushStatus; updates naming and return signature to support deferred error assignment. |
| e2e/framework/cluster_reconciler.go | Defers FlushStatus; updates naming and return signature to support deferred error assignment. |
| docs/guidelines.md | Documents controller responsibility for a single deferred status flush. |
| docs/component.md | Adds “Persisting Status with FlushStatus” section and clarifies optional metrics recorder. |
| README.md | Updates Quick Start reconciliation pattern to defer FlushStatus. |
| // If rec.Metrics is nil, metric recording is skipped. All other fields of rec | ||
| // must be populated. |
There was a problem hiding this comment.
The FlushStatus GoDoc says "All other fields of rec must be populated", but FlushStatus only depends on rec.Client, rec.Owner (and rec.Metrics optionally). This is misleading for callers/tests that only want to flush conditions; please tighten the wording to the fields actually required.
| // If rec.Metrics is nil, metric recording is skipped. All other fields of rec | |
| // must be populated. | |
| // rec.Client and rec.Owner must be populated. If rec.Metrics is nil, metric | |
| // recording is skipped. |
| desired := append([]metav1.Condition(nil), *rec.Owner.GetStatusConditions()...) | ||
|
|
||
| err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| updateErr := rec.Client.Status().Update(ctx, rec.Owner) | ||
| if updateErr == nil { | ||
| return nil | ||
| } | ||
| if !apierrors.IsConflict(updateErr) { | ||
| return updateErr | ||
| } | ||
| key := client.ObjectKeyFromObject(rec.Owner) | ||
| if getErr := rec.Client.Get(ctx, key, rec.Owner); getErr != nil { | ||
| return getErr | ||
| } | ||
| for _, cond := range desired { | ||
| meta.SetStatusCondition(rec.Owner.GetStatusConditions(), cond) | ||
| } |
There was a problem hiding this comment.
On conflict, FlushStatus refetches the owner and only re-applies the previously staged conditions before retrying. If the controller/component also staged other status subresource fields on rec.Owner (beyond conditions), those changes will be lost on the retry path. Consider snapshotting and reapplying the full desired status (or otherwise preserving any non-condition status updates) when handling conflicts so conflict retries don't change the semantics of what gets persisted.
| // failingClient is a client whose every method returns an error. Used to prove | ||
| // that applyStatusCondition never calls the API. | ||
| type failingClient struct { | ||
| client.Client | ||
| } | ||
|
|
||
| func (failingClient) Status() client.SubResourceWriter { | ||
| panic("applyStatusCondition must not reach the client") | ||
| } | ||
| func (failingClient) Get(context.Context, client.ObjectKey, client.Object, ...client.GetOption) error { | ||
| panic("applyStatusCondition must not reach the client") | ||
| } |
There was a problem hiding this comment.
The comment says failingClient methods "return an error", but the implementation intentionally panics to assert applyStatusCondition never reaches the client. Please update the comment to match the actual behavior (panic).
| func FlushStatus(ctx context.Context, rec ReconcileContext) error { | ||
| desired := append([]metav1.Condition(nil), *rec.Owner.GetStatusConditions()...) | ||
|
|
||
| err := retry.RetryOnConflict(retry.DefaultRetry, func() error { | ||
| updateErr := rec.Client.Status().Update(ctx, rec.Owner) | ||
| if updateErr == nil { | ||
| return nil |
There was a problem hiding this comment.
FlushStatus always calls Client.Status().Update even when the in-memory conditions are identical to what was originally fetched. Status updates typically bump ResourceVersion and will retrigger reconciliation; without change-detection this can create self-induced hot loops and unnecessary API traffic. Please add a guard so FlushStatus is a no-op when there are no effective status changes (e.g., by tracking a “dirty” flag when applying conditions, or by comparing against a snapshot of the pre-reconcile conditions before deciding to Update).
This pull request refactors controller and component reconciliation patterns across documentation, examples, and e2e test code to consistently use a deferred
component.FlushStatuscall at the end of each reconcile loop. This ensures that all status conditions staged in memory by components are persisted in a single API call, preventing self-induced 409 conflicts and making status persistence the explicit responsibility of the controller. The documentation is updated to describe and explain this pattern, and example code is modernized to match.Controller and Component Reconciliation Pattern
All controller
Reconcilemethods (in documentation, examples, and e2e test code) now use a deferred call tocomponent.FlushStatusat the end of reconciliation. This persists all staged status conditions to the Kubernetes API in one write, reducing the risk of status update conflicts. [1] [2] [3] [4] [5] [6] [7] [8]The signature of
Reconcilemethods is updated to use named return values (e.g.,(_ reconcile.Result, err error)) to support error assignment from the deferred function. [1] [2] [3] [4] [5] [6] [7] [8]Documentation Updates
Documentation in
docs/component.md,docs/guidelines.md, andREADME.mdis updated to describe the new pattern: components only mutate status in memory, and controllers are responsible for persisting status with a singleFlushStatuscall, typically deferred. This is explained in new and revised sections, with code examples. [1] [2] [3] [4] [5] [6]The
Metricsfield incomponent.ReconcileContextis now documented as optional; if omitted, condition metrics are not recorded. [1] [2] [3]Code Consistency and Minor Cleanups
Example and e2e test controllers are updated for consistency: variable naming is clarified (e.g.,
res→resource), and redundant error variable declarations are removed. [1] [2]All example controllers now use the new
recCtxvariable and pass it tocomp.Reconcile, instead of constructingReconcileContextinline. [1] [2] [3] [4]These changes standardize status persistence, improve documentation, and reduce the likelihood of status update conflicts in multi-component controllers.