From aa3fae820d5dad1b49d83ecf6135800a4726597d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86gir=20M=C3=A1ni=20Hauksson?= <54936225+sourcehawk@users.noreply.github.com> Date: Sat, 17 Oct 2026 14:18:04 +0100 Subject: [PATCH] fix(component)!: stop FlushStatus from writing stale status back on a conflict On a 409 Conflict, FlushStatus wrote the staged owner again with the resourceVersion of the server. The staged owner came from the read at the top of the reconcile, so the retry wrote old values over the status fields of other writers. This defeated optimistic locking. FlushStatus now makes one Status().Update. On any error, a conflict included, it writes nothing and returns the error, wrapped with %w so that apierrors.IsConflict still reports true. The controller requeues, and the next reconcile reads the current owner. A stale write never lands, so a status field or condition of another writer is never rolled back. BREAKING CHANGE: FlushStatus no longer takes the comps argument. The signature is FlushStatus(ctx, rec). Remove the third argument at each call site. A conflict is now returned as a reconcile error instead of being retried. ReconcileContext.APIReader stays for the resource reads of BlockOnForeignController and DeleteOnlyIfOwned. Fixes #238 Co-Authored-By: Claude Opus 5.5 --- docs/component.md | 118 ++-- docs/getting-started.md | 34 +- docs/guidelines.md | 35 +- e2e/framework/cluster_reconciler.go | 7 +- e2e/framework/reconciler.go | 7 +- .../component-prerequisites/app/controller.go | 14 +- examples/custom-resource/app/controller.go | 13 +- .../extraction-and-guards/app/controller.go | 13 +- .../grace-inconsistency/app/controller.go | 13 +- .../mutations-and-gating/app/controller.go | 13 +- pkg/component/aggregate.go | 4 +- pkg/component/component.go | 220 ++----- pkg/component/component_test.go | 4 +- pkg/component/conditions_test.go | 536 ------------------ pkg/component/flush_status_test.go | 195 +++++++ pkg/component/zz_mock_crd_test.go | 3 +- plugin/agents/reviewer.md | 10 +- plugin/skills/building-components/SKILL.md | 59 +- .../references/component.md | 118 ++-- plugin/skills/structuring-operators/SKILL.md | 9 +- .../references/guidelines.md | 37 +- 21 files changed, 441 insertions(+), 1021 deletions(-) create mode 100644 pkg/component/flush_status_test.go diff --git a/docs/component.md b/docs/component.md index 6e84ee83..b37ca6d5 100644 --- a/docs/component.md +++ b/docs/component.md @@ -656,7 +656,8 @@ Four properties of the aggregate condition are easy to miss: - A component disabled by a feature gate reports `True`/`Disabled` and therefore counts as ready, for the same reason. - `Aggregate` looks only at the components that you pass in. If the controller no longer builds a component, the condition of that component stays on the owner. `Aggregate` ignores that stale condition, but users still see it, - because [`FlushStatus`](#persisting-status-with-flushstatus) merges conditions by type and never prunes them. + because [`FlushStatus`](#persisting-status-with-flushstatus) writes the conditions on the owner as they are and never + prunes them. - The returned `Condition` is a plain struct. A caller that wants different prose can change the message before it stages the condition with `meta.SetStatusCondition`. @@ -700,9 +701,8 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) } recCtx := component.ReconcileContext{ /* ... */ Owner: owner} - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -711,7 +711,6 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) if err != nil { return reconcile.Result{}, err } - comps = append(comps, comp) if err := comp.Reconcile(ctx, recCtx); err != nil { return reconcile.Result{}, err @@ -788,7 +787,7 @@ recCtx := component.ReconcileContext{ Scheme: r.Scheme, // *runtime.Scheme EventRecorder: r.EventRecorder, // events.EventRecorder, from manager.GetEventRecorder(name) Metrics: r.Metrics, // component.MetricsRecorder, optional; see Metrics - APIReader: r.APIReader, // client.Reader, from manager.GetAPIReader(); direct reads on a status conflict + APIReader: r.APIReader, // client.Reader, from manager.GetAPIReader(); direct reads of live objects Owner: owner, // the CRD that owns this component } @@ -825,10 +824,8 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees every component built below. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -837,80 +834,61 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) if err != nil { return reconcile.Result{}, err } - comps = append(comps, comp) return reconcile.Result{}, comp.Reconcile(ctx, recCtx) } ``` -`FlushStatus` performs one `Status().Update` call, wrapped in `retry.RetryOnConflict`. That call writes the **whole -status subresource**, not only the conditions. Every status field the controller staged in memory is sent, and every -field it did not stage is sent exactly as it stands on the in-memory object. Fetch the owner fresh at the top of each -reconcile, so the fields nobody staged still hold current server state. An owner carried over from an earlier pass -writes stale values back over newer ones. +`FlushStatus(ctx context.Context, rec ReconcileContext) error` performs one `Status().Update` call. That call writes the +**whole status subresource**, not only the conditions. Every status field on the in-memory owner is sent, whether the +controller staged it or not. -On a 409 Conflict, for example when another writer updated the owner between the controller's `Get` and this call, -`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object, -takes that copy's `resourceVersion` so the retry targets the live object, restores from it only the conditions whose -type this flush does not own, and retries. The staged owner is never replaced. +The API server accepts the write only if the `resourceVersion` of the owner still matches the stored object. The +`resourceVersion` that the `Get` at the top of the reconcile read is therefore the guard for the write. Fetch the owner +fresh at the top of each reconcile. An owner carried over from an earlier pass holds an old `resourceVersion`, so its +flush can only conflict. -That fetch goes through `ReconcileContext.APIReader` when it is set, and through `Client` otherwise. Set it from the -manager's `GetAPIReader()`. The manager's default `Client` serves reads from the informer cache, which can lag the very -write that caused the conflict and hand back the same stale `resourceVersion`, so a retry through the cache may conflict -again until the cache catches up. A direct read sees the server as it is. +When another writer updates the owner between the `Get` and the flush, the API server rejects the write with a 409 +Conflict. `FlushStatus` does not retry. Nothing is written, and it returns the error, for which `apierrors.IsConflict` +(`k8s.io/apimachinery/pkg/api/errors`) reports `true`. Return the error from `Reconcile`, as the deferred call above +does. controller-runtime then requeues the owner with rate limiting, and the next reconcile reads the current owner and +computes its status again. -Two things follow from that. Non-condition status fields staged during the reconcile survive a conflict, because the -object holding them is the object that gets written. And a condition type this flush does not own keeps the server's -value rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled -back. +Because a stale write never lands, `FlushStatus` never rolls back a status field or a condition that another writer set. +Two controllers can share the status of one owner, each one writing its own conditions and fields with `FlushStatus`, +and neither one has to declare which fields it owns. -Which types count as owned comes from the components passed to `FlushStatus`: - -```go -component.FlushStatus(ctx, recCtx, []*component.Component{backendComp, frontendComp}) -``` - -Their condition types are the owned set, so it cannot drift from what the components actually write. - -Note that this is a slice, while [`Aggregate`](#aggregating-components-into-one-owner-condition) takes the same -components variadically. The difference is deliberate, for the reason below. - -The parameter is required rather than variadic on purpose. A variadic would let every existing call keep compiling while -silently retaining the old, wider ownership, which would make a correctness fix opt-in and invisible. Requiring the -argument forces each call site to be looked at exactly once. - -**You own exactly what you pass, and nothing else.** A controller that manages no components passes `nil`, and `nil` and -an empty slice behave identically: neither owns any condition type. Ownership is by condition type, not by who staged -the condition. A condition the controller stages by hand is owned only if its type belongs to a passed component; -otherwise, as with an owner-level aggregate or the only condition a -[validation-only CRD](#one-write-path-for-the-owners-status) reports, it is unowned. On a conflict an unowned condition -follows the server, and the next reconcile stages it again. - -!!! note "An owner-level aggregate is not owned by any component" - - A condition the controller stages itself, such as a `Ready` produced by - [`component.Aggregate`](#aggregating-components-into-one-owner-condition), belongs to no component, so a conflict - reverts it to the value the server already holds and the next reconcile stages it again. Passing the components - covers their own conditions; the aggregate is recomputed from them each pass anyway. - - For unowned types the server is the source of truth, absence included. An unowned condition the server no longer - carries is dropped from the staged owner rather than written back, so a condition another writer removed is not - resurrected. A condition the server has never held, such as the first write of the aggregate, is dropped for the - same reason and staged again on the next reconcile. - - "The next reconcile" depends on one happening. With controller-runtime's defaults, every update to the owner - triggers a reconcile, status-only ones included, so the successful write itself re-queues the owner and the aggregate - is restaged promptly. A controller that filters status-only updates out of its watch, for example with a - generation-based predicate, does not get that re-queue and the aggregate lags until the next reconcile from another - source. - -After a successful update, `FlushStatus` records metrics for every condition on the owner. If `Metrics` is `nil`, -recording is skipped. See [Metrics](#metrics). +After a successful update, `FlushStatus` records metrics for every condition on the owner. A failed update records none. +If `Metrics` is `nil`, recording is skipped. See [Metrics](#metrics). This split is what lets a controller with several components stage several conditions during one reconcile and persist them in a single write. Persisting after each component would race the components' writes and produce 409 conflicts. See [Keep Controllers Thin](guidelines.md#keep-controllers-thin) and [One Component Per Logical Condition](guidelines.md#one-component-per-logical-condition). +### Requeue on a conflict without a reconcile error + +A returned conflict is logged as a reconciler error. A conflict is normal when other writers share the owner, so a +controller can requeue it quietly instead. In the deferred call, check for a conflict and return a short `RequeueAfter` +with a `nil` error: + +```go +defer func() { + flushErr := component.FlushStatus(ctx, recCtx) + switch { + case flushErr == nil: + case err != nil: + // Reconcile already returns an error, and that error requeues the owner. + case apierrors.IsConflict(flushErr): + res, err = reconcile.Result{RequeueAfter: time.Second}, nil + default: + err = flushErr + } +}() +``` + +The deferred call sets the named results, so declare them as `(res reconcile.Result, err error)`. Use `RequeueAfter`, +not `Requeue`, which is deprecated as of controller-runtime v0.25. + ### One write path for the owner's status A CR's status is written once per reconcile, through `FlushStatus`. This rule also holds for a controller that builds no @@ -934,12 +912,10 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, req reconcile.Request) } meta.SetStatusCondition(policy.GetStatusConditions(), cond) - // nil: this controller owns no component conditions. On a conflict the staged - // condition follows the server and is written again on the next reconcile. return reconcile.Result{}, component.FlushStatus(ctx, component.ReconcileContext{ Client: r.Client, Owner: policy, - }, nil) + }) } ``` diff --git a/docs/getting-started.md b/docs/getting-started.md index 0a8e011d..544f0b34 100644 --- a/docs/getting-started.md +++ b/docs/getting-started.md @@ -266,22 +266,22 @@ self-induced update conflicts. `ReconcileContext` has six fields: -| Field | Type | Notes | -| --------------- | --------------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `Client` | `client.Client` | The controller-runtime client. | -| `Scheme` | `*runtime.Scheme` | The operator scheme. | -| `EventRecorder` | `events.EventRecorder` | For Kubernetes events. `GetEventRecorder(name)` on the manager, v0.23 and later. | -| `Metrics` | `component.MetricsRecorder` | Optional. `metrics.NewRecorder(...)` records condition and resource metrics; pass `nil` to skip both. See [Metrics](component.md#metrics). | -| `APIReader` | `client.Reader` | Optional, recommended. `GetAPIReader()` on the manager; `FlushStatus` reads through it on a 409 so the retry sees the live owner, not the informer cache. | -| `Owner` | `component.OperatorCRD` | The owner object you fetched. Your CRD satisfies this via `GetStatusConditions`. | +| Field | Type | Notes | +| --------------- | --------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------ | +| `Client` | `client.Client` | The controller-runtime client. | +| `Scheme` | `*runtime.Scheme` | The operator scheme. | +| `EventRecorder` | `events.EventRecorder` | For Kubernetes events. `GetEventRecorder(name)` on the manager, v0.23 and later. | +| `Metrics` | `component.MetricsRecorder` | Optional. `metrics.NewRecorder(...)` records condition and resource metrics; pass `nil` to skip both. See [Metrics](component.md#metrics). | +| `APIReader` | `client.Reader` | Optional, recommended. `GetAPIReader()` on the manager; the component reads live objects through it instead of the informer cache. | +| `Owner` | `component.OperatorCRD` | The owner object you fetched. Your CRD satisfies this via `GetStatusConditions`. | !!! note - `FlushStatus` performs the status write itself: it calls `Client.Status().Update` on the owner (retrying on - conflict). Do not also call `Status().Update` for the conditions the framework manages, or you will double-write. - Because the call is deferred, the conditions are flushed even if `Reconcile` returns an error. Pass the components - you built: on a conflict their condition types stay on the staged owner, while every other condition is refreshed - from the server. + `FlushStatus` performs the status write itself: it calls `Client.Status().Update` on the owner once. Do not also + call `Status().Update` for the conditions the framework manages, or you will double-write. Because the call is + deferred, the conditions are flushed even if `Reconcile` returns an error. If another writer updated the owner after + you fetched it, the write fails with a conflict and nothing is written. The error requeues the owner, and the next + reconcile starts from the current object. ```go package app @@ -300,7 +300,7 @@ type Controller struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - APIReader client.Reader // from mgr.GetAPIReader(); direct reads on a status conflict + APIReader client.Reader // from mgr.GetAPIReader(); direct reads of live objects NewDeploymentResource func(*ExampleApp) (component.Resource, error) NewConfigMapResource func(*ExampleApp) (component.Resource, error) @@ -315,10 +315,8 @@ func (r *Controller) reconcile(ctx context.Context, owner *ExampleApp) (err erro APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees the component built below. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -343,8 +341,6 @@ func (r *Controller) reconcile(ctx context.Context, owner *ExampleApp) (err erro if err != nil { return err } - comps = append(comps, comp) - return comp.Reconcile(ctx, recCtx) } ``` diff --git a/docs/guidelines.md b/docs/guidelines.md index bffa32c0..8d528ad4 100644 --- a/docs/guidelines.md +++ b/docs/guidelines.md @@ -148,24 +148,20 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) Scheme: r.Scheme, EventRecorder: r.EventRecorder, Metrics: r.Metrics, - APIReader: r.APIReader, // direct reads on a status conflict + APIReader: r.APIReader, // direct reads of live objects Owner: app, } - // Persist all staged conditions exactly once, even on the error path. comps is - // declared first so the closure sees whatever the resolver returns, and - // FlushStatus derives the condition types it owns from those components. - var comps []*component.Component + // Persist all staged conditions exactly once, even on the error path. defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() - built, buildErr := buildComponents(app) - if buildErr != nil { - return reconcile.Result{}, buildErr + comps, err := buildComponents(app) + if err != nil { + return reconcile.Result{}, err } - comps = built var firstErr error for _, comp := range comps { @@ -235,22 +231,14 @@ particular, and metrics, dashboards, and downstream automation key on the reason different prose, and leave the reason alone. **Remove the conditions of components you retire.** `Aggregate` reads only the components you pass it, and -[`FlushStatus`](component.md#persisting-status-with-flushstatus) merges conditions by type and never prunes, so a -component the controller has stopped building leaves its last condition on the owner indefinitely. The aggregate ignores -it, and users still see it in `kubectl describe`. Delete it explicitly when you retire a component: +[`FlushStatus`](component.md#persisting-status-with-flushstatus) writes the owner's conditions as they are and never +prunes them, so a component the controller has stopped building leaves its last condition on the owner indefinitely. The +aggregate ignores it, and users still see it in `kubectl describe`. Delete it explicitly when you retire a component: ```go meta.RemoveStatusCondition(owner.GetStatusConditions(), "OldComponentReady") ``` -Ownership decides whether that removal survives a conflict. `FlushStatus` owns exactly the condition types of the -components you pass it, and on a 409 it takes the server's value for every unowned type. If the retired component is -already absent from the slice when its condition is removed, the type is unowned, the conflict refresh puts the -condition back, and the removal lands only on a later, conflict-free pass. To make it land on the first pass, retire in -two steps: keep building the component and passing it to `FlushStatus` for the release that runs -`meta.RemoveStatusCondition`, so its type stays owned and the removal is what gets written, and delete the component in -the release after. Either way the condition disappears; the two-step form makes it deterministic. - ## Reconciler Error Handling and Requeueing The framework distinguishes between conditions and errors. A resource that is merely converging (a rolling Deployment, a @@ -266,6 +254,11 @@ grace period is the exception: no watch event arrives when its grace period ends conditions are written before the error propagates, so the failure is visible in status even while controller-runtime backs off. +A 409 Conflict from `FlushStatus` reaches `Reconcile` the same way. It means that another writer updated the owner after +the controller read it. Nothing was written, and the next reconcile reads the current owner and computes its status +again. To requeue a conflict without a logged reconciler error, see +[Requeue on a conflict without a reconcile error](component.md#requeue-on-a-conflict-without-a-reconcile-error). + Only `Reconcile` decides when to requeue. A helper or a sub-reconciler returns an error or a domain value, not a `reconcile.Result`. See [Give Each Controller Layer One Visible Responsibility](#give-each-controller-layer-one-visible-responsibility). diff --git a/e2e/framework/cluster_reconciler.go b/e2e/framework/cluster_reconciler.go index 54ebdb13..447ce281 100644 --- a/e2e/framework/cluster_reconciler.go +++ b/e2e/framework/cluster_reconciler.go @@ -31,8 +31,9 @@ type ClusterE2EReconciler struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - // APIReader serves the conflict-path refetch in FlushStatus with a direct - // read, so the retry sees the live owner rather than the informer cache. + // APIReader reads straight from the API server. The component reads live + // objects through it for resources registered with BlockOnForeignController + // or DeleteOnlyIfOwned. APIReader client.Reader mu sync.RWMutex @@ -136,7 +137,7 @@ func (r *ClusterE2EReconciler) Reconcile(ctx context.Context, req reconcile.Requ Owner: owner, } defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, []*component.Component{comp}); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() diff --git a/e2e/framework/reconciler.go b/e2e/framework/reconciler.go index 96133da1..73abc306 100644 --- a/e2e/framework/reconciler.go +++ b/e2e/framework/reconciler.go @@ -31,8 +31,9 @@ type E2EReconciler struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - // APIReader serves the conflict-path refetch in FlushStatus with a direct - // read, so the retry sees the live owner rather than the informer cache. + // APIReader reads straight from the API server. The component reads live + // objects through it for resources registered with BlockOnForeignController + // or DeleteOnlyIfOwned. APIReader client.Reader mu sync.RWMutex @@ -136,7 +137,7 @@ func (r *E2EReconciler) Reconcile(ctx context.Context, req reconcile.Request) (_ Owner: owner, } defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, []*component.Component{comp}); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() diff --git a/examples/component-prerequisites/app/controller.go b/examples/component-prerequisites/app/controller.go index 56635755..313c67ae 100644 --- a/examples/component-prerequisites/app/controller.go +++ b/examples/component-prerequisites/app/controller.go @@ -21,8 +21,9 @@ type Controller struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - // APIReader reads straight from the API server; FlushStatus uses it on a - // conflict so the retry sees the live owner rather than a stale cache entry. + // APIReader reads straight from the API server. The component reads live + // objects through it for resources registered with BlockOnForeignController + // or DeleteOnlyIfOwned. APIReader client.Reader NewConfigMapResource func(*ExampleApp) (component.Resource, error) @@ -45,13 +46,8 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees every component - // that gets built below. FlushStatus derives the condition types it owns - // from these: on a conflict the owned conditions stay on this staged owner - // while the unowned ones are refreshed from the server. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -61,7 +57,6 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro if err != nil { return err } - comps = append(comps, infra) if err := infra.Reconcile(ctx, recCtx); err != nil { return err @@ -72,7 +67,6 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro if err != nil { return err } - comps = append(comps, app) return app.Reconcile(ctx, recCtx) } diff --git a/examples/custom-resource/app/controller.go b/examples/custom-resource/app/controller.go index df30e43b..f0532aaf 100644 --- a/examples/custom-resource/app/controller.go +++ b/examples/custom-resource/app/controller.go @@ -18,8 +18,9 @@ type Controller struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - // APIReader reads straight from the API server; FlushStatus uses it on a - // conflict so the retry sees the live owner rather than a stale cache entry. + // APIReader reads straight from the API server. The component reads live + // objects through it for resources registered with BlockOnForeignController + // or DeleteOnlyIfOwned. APIReader client.Reader NewCertificateResource func(*ExampleApp) (component.Resource, error) @@ -35,13 +36,8 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees every component - // that gets built below. FlushStatus derives the condition types it owns - // from these: on a conflict the owned conditions stay on this staged owner - // while the unowned ones are refreshed from the server. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -59,7 +55,6 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro if err != nil { return err } - comps = []*component.Component{comp} return comp.Reconcile(ctx, recCtx) } diff --git a/examples/extraction-and-guards/app/controller.go b/examples/extraction-and-guards/app/controller.go index 9dff94c7..a914f9b8 100644 --- a/examples/extraction-and-guards/app/controller.go +++ b/examples/extraction-and-guards/app/controller.go @@ -19,8 +19,9 @@ type Controller struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - // APIReader reads straight from the API server; FlushStatus uses it on a - // conflict so the retry sees the live owner rather than a stale cache entry. + // APIReader reads straight from the API server. The component reads live + // objects through it for resources registered with BlockOnForeignController + // or DeleteOnlyIfOwned. APIReader client.Reader // NewConfigMapResource builds the ConfigMap and declares the extraction @@ -44,13 +45,8 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees every component - // that gets built below. FlushStatus derives the condition types it owns - // from these: on a conflict the owned conditions stay on this staged owner - // while the unowned ones are refreshed from the server. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -59,7 +55,6 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro if err != nil { return err } - comps = []*component.Component{comp} return comp.Reconcile(ctx, recCtx) } diff --git a/examples/grace-inconsistency/app/controller.go b/examples/grace-inconsistency/app/controller.go index 3fb6d34a..d61e683c 100644 --- a/examples/grace-inconsistency/app/controller.go +++ b/examples/grace-inconsistency/app/controller.go @@ -21,8 +21,9 @@ type Controller struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - // APIReader reads straight from the API server; FlushStatus uses it on a - // conflict so the retry sees the live owner rather than a stale cache entry. + // APIReader reads straight from the API server. The component reads live + // objects through it for resources registered with BlockOnForeignController + // or DeleteOnlyIfOwned. APIReader client.Reader NewDeploymentResource func(*ExampleApp) (component.Resource, error) @@ -40,13 +41,8 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (res reco APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees every component - // that gets built below. FlushStatus derives the condition types it owns - // from these: on a conflict the owned conditions stay on this staged owner - // while the unowned ones are refreshed from the server. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -55,7 +51,6 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (res reco if err != nil { return reconcile.Result{}, err } - comps = []*component.Component{comp} if err := comp.Reconcile(ctx, recCtx); err != nil { return reconcile.Result{}, err diff --git a/examples/mutations-and-gating/app/controller.go b/examples/mutations-and-gating/app/controller.go index 0147ea92..24dbc4b0 100644 --- a/examples/mutations-and-gating/app/controller.go +++ b/examples/mutations-and-gating/app/controller.go @@ -18,8 +18,9 @@ type Controller struct { Scheme *runtime.Scheme EventRecorder events.EventRecorder Metrics component.MetricsRecorder - // APIReader reads straight from the API server; FlushStatus uses it on a - // conflict so the retry sees the live owner rather than a stale cache entry. + // APIReader reads straight from the API server. The component reads live + // objects through it for resources registered with BlockOnForeignController + // or DeleteOnlyIfOwned. APIReader client.Reader NewDeploymentResource func(*ExampleApp) (component.Resource, error) @@ -36,13 +37,8 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees every component - // that gets built below. FlushStatus derives the condition types it owns - // from these: on a conflict the owned conditions stay on this staged owner - // while the unowned ones are refreshed from the server. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -51,7 +47,6 @@ func (r *Controller) Reconcile(ctx context.Context, owner *ExampleApp) (err erro if err != nil { return err } - comps = []*component.Component{comp} return comp.Reconcile(ctx, recCtx) } diff --git a/pkg/component/aggregate.go b/pkg/component/aggregate.go index 935b457b..eb3cf327 100644 --- a/pkg/component/aggregate.go +++ b/pkg/component/aggregate.go @@ -51,8 +51,8 @@ import ( // and therefore counts as ready, for the same reason. // - Only the components passed in are considered. A component the controller // has stopped building leaves a stale condition on the owner that Aggregate -// ignores but users still see, because [FlushStatus] merges conditions by type -// and never prunes. +// ignores but users still see, because [FlushStatus] writes the conditions on +// the owner as they are and never prunes them. // // The returned [Condition] is a plain struct. A caller that wants different prose // can adjust the Message before staging it: diff --git a/pkg/component/component.go b/pkg/component/component.go index 1f9af1eb..a483e35a 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -3,15 +3,12 @@ package component import ( "context" "fmt" - "slices" "time" - apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/tools/events" - "k8s.io/client-go/util/retry" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/log" @@ -97,12 +94,11 @@ type ReconcileContext struct { Metrics MetricsRecorder // APIReader reads straight from the API server, bypassing the informer // cache. Obtain it from the controller-runtime manager with GetAPIReader(). - // [FlushStatus] uses it on a 409 conflict to fetch the owner as the server - // holds it: the manager's default Client serves reads from the cache, which - // can lag the write that caused the conflict and hand back the same stale - // resourceVersion, so a retry through it may conflict again. When APIReader - // is nil, FlushStatus falls back to Client. It is optional but recommended - // for any controller whose owner has other status writers. + // The component reads live objects through it before it writes or deletes + // a resource registered with BlockOnForeignController or DeleteOnlyIfOwned, + // so that a decision about who controls or owns the object does not rest on + // a cache that can lag the API server. When APIReader is nil, those reads go + // through Client. APIReader client.Reader // Owner is the custom resource that owns and is updated by the components. Owner OperatorCRD @@ -550,6 +546,21 @@ func (c *Component) Reconcile(ctx context.Context, rec ReconcileContext) error { return nil } +// replaceStatusCondition sets cond by type, taking its value verbatim. +// +// meta.SetStatusCondition is deliberately not used here: it keeps the existing +// LastTransitionTime when the status is unchanged, and the caller needs the +// transition time it stamped to land. +func replaceStatusCondition(conditions *[]metav1.Condition, cond metav1.Condition) { + for i := range *conditions { + if (*conditions)[i].Type == cond.Type { + (*conditions)[i] = cond + return + } + } + *conditions = append(*conditions, cond) +} + func (c *Component) now() time.Time { if c.clock == nil { return time.Now() @@ -633,38 +644,27 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { return err } -// FlushStatus persists the owner's status to the Kubernetes API and records -// condition metrics for every condition on the owner. -// -// Pass the components whose conditions this flush is responsible for. Their -// condition types are the types FlushStatus treats as its own on a conflict, so -// the set can never drift from what the components actually write. -// -// You own exactly what you pass, and nothing else. Pass nil when the controller -// manages no components; nil and an empty slice behave identically and own no -// condition types at all. -// -// comps is a required parameter rather than a variadic one on purpose. A -// variadic would let every existing call keep compiling while silently retaining -// the old, wider ownership, which would make a correctness fix opt-in and -// invisible. Requiring the argument forces each call site to be looked at once. +// FlushStatus writes the owner's status to the Kubernetes API in one +// Status().Update and, after a successful write, records condition metrics for +// every condition on the owner. // -// Ownership is by condition type, not by who staged the condition. A condition -// the controller stages by hand is owned only if its type belongs to a passed -// component; otherwise, as with an owner-level aggregate or the only condition a -// validation-only CRD reports, it is unowned. On a conflict an unowned condition -// follows the server, and the next reconcile stages it again. +// The update writes the whole status subresource, not only the conditions. The +// API server accepts it only while the owner's resourceVersion still matches +// the stored object, so the write is guarded by the read the controller made at +// the top of the reconcile. Fetch the owner fresh at the top of each reconcile: +// an owner carried over from an earlier pass holds an old resourceVersion, and +// its flush can only conflict. // -// FlushStatus issues a single Status().Update, which writes the entire status -// subresource, not only the conditions. Fields the controller never staged are -// sent exactly as they stand on the in-memory owner. The owner must therefore be -// fetched fresh at the top of each reconcile: an owner carried over from an -// earlier pass writes stale status values back over newer ones. +// On a 409 Conflict, which means that another writer updated the owner after +// the controller read it, nothing is written and the error is returned. +// apierrors.IsConflict reports true for it. Return it from Reconcile: +// controller-runtime requeues the request with rate limiting, and the next +// reconcile reads the current owner and computes the status again. Because a +// stale write never lands, a status field or condition that another controller +// wrote is never rolled back. // -// Controllers must call FlushStatus exactly once per reconciliation, typically -// via defer so that conditions set on error paths are still persisted. Declare -// the component slice before the deferred call so the closure observes every -// component built during the reconcile: +// Call FlushStatus exactly once per reconciliation, typically deferred so that +// conditions set on error paths are still written: // // func (r *MyReconciler) Reconcile(ctx context.Context, req reconcile.Request) (res reconcile.Result, err error) { // owner := &v1alpha1.MyApp{} @@ -673,9 +673,8 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // } // rec := component.ReconcileContext{ /* ... */ Owner: owner} // -// var comps []*component.Component // defer func() { -// if flushErr := component.FlushStatus(ctx, rec, comps); flushErr != nil && err == nil { +// if flushErr := component.FlushStatus(ctx, rec); flushErr != nil && err == nil { // err = flushErr // } // }() @@ -684,56 +683,16 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // if err != nil { // return reconcile.Result{}, err // } -// comps = append(comps, comp) // return reconcile.Result{}, comp.Reconcile(ctx, rec) // } // -// On a 409 Conflict (for example if an external writer updated the owner between -// the controller fetching it and this call) FlushStatus keeps the staged owner as -// the object being written. It fetches the server's copy into a separate object -// through rec.APIReader when set (rec.Client otherwise), takes that object's -// resourceVersion so the retry targets the live object, and restores from it -// only the conditions whose type this flush does not own. It then retries. -// -// Set rec.APIReader from the manager's GetAPIReader. The manager's default -// Client serves reads from the informer cache, which can lag the very write -// that caused the conflict and return the same stale resourceVersion, so a -// retry through it may conflict again until the cache catches up. -// -// Two consequences follow from never replacing the staged owner. Non-condition -// status fields staged during the reconcile survive a conflict. And a condition -// type this flush does not own keeps the server's value rather than the possibly -// stale copy the controller happens to be holding, so a concurrent update by -// another writer is not rolled back. -// -// For unowned types the server is the source of truth, absence included. An -// unowned condition the server no longer carries is dropped from the staged -// owner rather than written back, so a condition another writer removed is not -// resurrected. A condition the server has never held, such as the first write of -// an owner-level aggregate, is dropped for the same reason and staged again on -// the next reconcile. -// -// rec.Client and rec.Owner must be populated, and every component in comps must -// be non-nil; a nil component is a programming error. If rec.Metrics is nil, -// metric recording is skipped. -func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component) error { - owned := ownedConditionTypes(comps) - - 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 - } - if refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { - return refreshErr - } - return updateErr - }) - if err != nil { - return err +// rec.Client and rec.Owner must be set. If rec.Metrics is nil, metric recording +// is skipped. +func FlushStatus(ctx context.Context, rec ReconcileContext) error { + if err := rec.Client.Status().Update(ctx, rec.Owner); err != nil { + return fmt.Errorf( + "updating status of %s %s: %w", rec.Owner.GetKind(), client.ObjectKeyFromObject(rec.Owner), err, + ) } if rec.Metrics == nil { @@ -747,90 +706,3 @@ func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component) } return nil } - -// ownedConditionTypes returns the condition types this flush is responsible for: -// exactly the types the given components write, so the set can never drift from -// what those components actually report. No components means no owned types. -func ownedConditionTypes(comps []*Component) map[string]struct{} { - owned := make(map[string]struct{}, len(comps)) - for _, comp := range comps { - owned[string(comp.conditionType)] = struct{}{} - } - return owned -} - -// refreshUnownedConditions prepares the staged owner for a retry after a -// conflict, without ever replacing it. -// -// The staged owner remains the object that will be written, so every field the -// controller staged during this reconcile survives, conditions and non-condition -// status fields alike. Only two things are taken from the server: the current -// resourceVersion, so the retry targets the live object, and the value of every -// condition type this flush does not own, so a concurrent update by another -// writer is not overwritten by the copy this controller happens to be holding. -func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned map[string]struct{}) error { - server, ok := rec.Owner.DeepCopyObject().(OperatorCRD) - if !ok { - return fmt.Errorf("failed to deep copy owner of type %T", rec.Owner) - } - // Clear the copy's conditions before the fetch. Decoding a response into a - // non-empty struct does not remove fields the response omits, so an owner - // with no conditions on the server would otherwise leave the staged - // conditions in place here and they would read as the server's. - *server.GetStatusConditions() = nil - - // Read through APIReader when one is set: the manager's Client serves reads - // from the informer cache, which can lag the write that caused the conflict - // and return the same stale resourceVersion, defeating the retry. - reader := client.Reader(rec.Client) - if rec.APIReader != nil { - reader = rec.APIReader - } - if err := reader.Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil { - return err - } - - rec.Owner.SetResourceVersion(server.GetResourceVersion()) - - serverConds := *server.GetStatusConditions() - onServer := make(map[string]struct{}, len(serverConds)) - for _, cond := range serverConds { - onServer[cond.Type] = struct{}{} - } - - // Absence is server state too. An unowned condition the server no longer - // carries has been removed by whoever owns it, so drop the local copy - // instead of writing it back and resurrecting it. - staged := rec.Owner.GetStatusConditions() - *staged = slices.DeleteFunc(*staged, func(cond metav1.Condition) bool { - if _, isOwned := owned[cond.Type]; isOwned { - return false - } - _, present := onServer[cond.Type] - return !present - }) - - for _, cond := range serverConds { - if _, isOwned := owned[cond.Type]; isOwned { - continue - } - replaceStatusCondition(staged, cond) - } - return nil -} - -// replaceStatusCondition sets cond by type, taking its value verbatim. -// -// meta.SetStatusCondition is deliberately not used here: it preserves the -// existing LastTransitionTime when the status is unchanged, which would blend -// the server's condition with the stale local copy this function exists to -// discard. -func replaceStatusCondition(conditions *[]metav1.Condition, cond metav1.Condition) { - for i := range *conditions { - if (*conditions)[i].Type == cond.Type { - (*conditions)[i] = cond - return - } - } - *conditions = append(*conditions, cond) -} diff --git a/pkg/component/component_test.go b/pkg/component/component_test.go index 93dcef4c..43e41b07 100644 --- a/pkg/component/component_test.go +++ b/pkg/component/component_test.go @@ -52,7 +52,7 @@ var _ = Describe("Component Reconciler", func() { // that the returned condition reflects the persisted state, including // error conditions set via fail(). getOwnerCondition := func() Condition { - Expect(FlushStatus(ctx, recCtx, []*Component{comp})).To(Succeed()) + Expect(FlushStatus(ctx, recCtx)).To(Succeed()) updatedOwner := &MockOperatorCRD{} Expect(k8sClient.Get(ctx, client.ObjectKey{Name: owner.Name, Namespace: namespace}, updatedOwner)).To(Succeed()) return comp.GetCondition(updatedOwner) @@ -977,7 +977,7 @@ var _ = Describe("Component Reconciler", func() { // transition time that the next reconcile reads. remaining, ok := c.GraceRemaining(owner) Expect(ok).To(BeTrue()) - Expect(FlushStatus(ctx, recCtx, []*Component{c})).To(Succeed()) + Expect(FlushStatus(ctx, recCtx)).To(Succeed()) time.Sleep(remaining) diff --git a/pkg/component/conditions_test.go b/pkg/component/conditions_test.go index 9801491e..9f2d398d 100644 --- a/pkg/component/conditions_test.go +++ b/pkg/component/conditions_test.go @@ -3,7 +3,6 @@ package component import ( "context" "errors" - "fmt" "testing" "time" @@ -11,13 +10,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/runtime" - "k8s.io/apimachinery/pkg/runtime/schema" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/fake" ocm "github.com/sourcehawk/go-crd-condition-metrics/pkg/crd-condition-metrics" ) @@ -238,170 +232,6 @@ func TestApplyStatusCondition(t *testing.T) { assert.Equal(t, cond.Reason, (*conditions)[0].Reason) } -func TestFlushStatus(t *testing.T) { - ctx := context.Background() - scheme := runtime.NewScheme() - require.NoError(t, AddToScheme(scheme)) - - newOwner := func() *MockOperatorCRD { - return &MockOperatorCRD{ - ObjectMeta: metav1.ObjectMeta{ - Name: "test-owner", - Namespace: "default", - Generation: 1, - }, - } - } - - cond := func(ctype, reason string, status metav1.ConditionStatus) Condition { - return Condition{Type: ctype, Status: status, Reason: reason, ObservedGeneration: 1} - } - - t.Run("persists every condition on the owner and records a metric per condition", func(t *testing.T) { - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, cond("InfraReady", "Ready", metav1.ConditionTrue)) - applyStatusCondition(ReconcileContext{Owner: owner}, cond("AppReady", "Ready", metav1.ConditionTrue)) - - k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() - metrics := &MockMetrics{} - metrics.On("RecordConditionFor", owner.GetKind(), owner, "InfraReady", - string(metav1.ConditionTrue), "Ready", mock.Anything, mock.Anything).Return().Once() - metrics.On("RecordConditionFor", owner.GetKind(), owner, "AppReady", - string(metav1.ConditionTrue), "Ready", mock.Anything, mock.Anything).Return().Once() - - require.NoError(t, FlushStatus(ctx, ReconcileContext{Client: k8sClient, Metrics: metrics, Owner: owner}, nil)) - - persisted := &MockOperatorCRD{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) - assert.Len(t, persisted.Status.Conditions, 2) - metrics.AssertExpectations(t) - }) - - t.Run("is a no-op when metrics recorder is nil", func(t *testing.T) { - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, cond("InfraReady", "Ready", metav1.ConditionTrue)) - - k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() - - require.NoError(t, FlushStatus(ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil)) - - persisted := &MockOperatorCRD{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) - assert.Len(t, persisted.Status.Conditions, 1) - }) - - t.Run("surfaces non-conflict update errors without recording metrics", func(t *testing.T) { - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, cond("InfraReady", "Ready", metav1.ConditionTrue)) - - inner := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() - k8sClient := &errorMockClient{Client: inner} - metrics := &MockMetrics{} - - err := FlushStatus(ctx, ReconcileContext{Client: k8sClient, Metrics: metrics, Owner: owner}, nil) - require.Error(t, err) - assert.Equal(t, "update failed", err.Error()) - - metrics.AssertNotCalled(t, "RecordConditionFor") - }) - - t.Run("retries on conflict by refetching the owner and reapplying staged conditions", func(t *testing.T) { - owner := newOwner() - - // External writer got there first with a condition the framework does not manage. - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "ExternalReady", - Status: metav1.ConditionTrue, - Reason: "ExternalReason", - LastTransitionTime: metav1.Now(), - }} - inner := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(owner).WithObjects(serverSide).Build() - // The in-memory owner has a stale ResourceVersion (empty), so the first - // Update must conflict. The conflict wrapper refetches, reapplies our - // staged condition, and lets retry.RetryOnConflict succeed on the retry. - k8sClient := &conflictOnceClient{Client: inner} - - applyStatusCondition(ReconcileContext{Owner: owner}, cond("InfraReady", "Ready", metav1.ConditionTrue)) - - infra, err := NewComponentBuilder().WithName("infra").WithConditionType("InfraReady").Build() - require.NoError(t, err) - - metrics := &MockMetrics{} - metrics.On("RecordConditionFor", owner.GetKind(), owner, "ExternalReady", - string(metav1.ConditionTrue), "ExternalReason", mock.Anything, mock.Anything).Return().Once() - metrics.On("RecordConditionFor", owner.GetKind(), owner, "InfraReady", - string(metav1.ConditionTrue), "Ready", mock.Anything, mock.Anything).Return().Once() - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Metrics: metrics, Owner: owner}, []*Component{infra}, - )) - - assert.GreaterOrEqual(t, k8sClient.gets, 1, "expected at least one Get for conflict refetch") - persisted := &MockOperatorCRD{} - require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) - // Both the externally written condition and the framework-managed - // condition must be present on the persisted owner. - types := make([]string, 0, len(persisted.Status.Conditions)) - for _, c := range persisted.Status.Conditions { - types = append(types, c.Type) - } - assert.ElementsMatch(t, []string{"ExternalReady", "InfraReady"}, types) - metrics.AssertExpectations(t) - }) -} - -type errorMockClient struct { - client.Client -} - -func (e *errorMockClient) Status() client.SubResourceWriter { - return &errorStatusWriter{SubResourceWriter: e.Client.Status()} -} - -type errorStatusWriter struct { - client.SubResourceWriter -} - -func (e *errorStatusWriter) Update(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { - return errors.New("update failed") -} - -// conflictOnceClient wraps a real client and causes the first status Update to -// return a Conflict error, forcing FlushStatus to refetch and retry. Subsequent -// Updates fall through to the underlying client. -type conflictOnceClient struct { - client.Client - conflicts int - gets int -} - -func (c *conflictOnceClient) Status() client.SubResourceWriter { - return &conflictOnceStatusWriter{SubResourceWriter: c.Client.Status(), parent: c} -} - -func (c *conflictOnceClient) Get(ctx context.Context, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { - c.gets++ - return c.Client.Get(ctx, key, obj, opts...) -} - -type conflictOnceStatusWriter struct { - client.SubResourceWriter - parent *conflictOnceClient -} - -func (w *conflictOnceStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.SubResourceUpdateOption) error { - if w.parent.conflicts == 0 { - w.parent.conflicts++ - return apierrors.NewConflict( - schema.GroupResource{Group: "example.io", Resource: "mockoperatorcrds"}, - obj.GetName(), - fmt.Errorf("the object has been modified; please apply your changes to the latest version and try again"), - ) - } - return w.SubResourceWriter.Update(ctx, obj, opts...) -} - // failingClient is a client whose every method panics. Used to prove that // applyStatusCondition never calls the API. type failingClient struct { @@ -430,369 +260,3 @@ func (metricsThatPanic) RecordResourceApply(ResourceMetricLabels, concepts.Conve func (metricsThatPanic) RecordResourceApplyError(ResourceMetricLabels) { panic("applyStatusCondition must not record metrics") } - -// TestFlushStatusConflictOwnership covers the conflict path, where FlushStatus -// must keep the staged owner as the object being written and take the server's -// value only for condition types the framework does not own. -func TestFlushStatusConflictOwnership(t *testing.T) { - ctx := context.Background() - scheme := runtime.NewScheme() - require.NoError(t, AddToScheme(scheme)) - - newOwner := func() *MockOperatorCRD { - return &MockOperatorCRD{ - ObjectMeta: metav1.ObjectMeta{Name: "test-owner", Namespace: "default", Generation: 1}, - } - } - - // infraComponent owns the "InfraReady" condition type. - infraComponent := func(t *testing.T) *Component { - t.Helper() - comp, err := NewComponentBuilder().WithName("infra").WithConditionType("InfraReady").Build() - require.NoError(t, err) - return comp - } - - // conflictingClient returns a client whose first status Update conflicts, - // backed by a server object in the given state. - conflictingClient := func(serverSide *MockOperatorCRD) *conflictOnceClient { - inner := fake.NewClientBuilder(). - WithScheme(scheme).WithStatusSubresource(serverSide).WithObjects(serverSide).Build() - return &conflictOnceClient{Client: inner} - } - - persistedOf := func(t *testing.T, c client.Client, owner *MockOperatorCRD) *MockOperatorCRD { - t.Helper() - persisted := &MockOperatorCRD{} - require.NoError(t, c.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) - return persisted - } - - conditionOf := func(t *testing.T, o *MockOperatorCRD, ctype string) metav1.Condition { - t.Helper() - found := meta.FindStatusCondition(o.Status.Conditions, ctype) - require.NotNil(t, found, "expected condition %q on the persisted owner", ctype) - return *found - } - - t.Run("preserves a staged non-condition status field across a conflict", func(t *testing.T) { - serverSide := newOwner() - serverSide.Status.ObservedGeneration = 0 - k8sClient := conflictingClient(serverSide) - - owner := newOwner() - owner.Status.ObservedGeneration = 7 // staged by the controller, not a condition - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - assert.Equal(t, int64(7), persistedOf(t, k8sClient, owner).Status.ObservedGeneration, - "the staged owner must remain the object written, so its non-condition fields survive") - }) - - t.Run("keeps our staged value for a condition type we own", func(t *testing.T) { - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "InfraReady", Status: metav1.ConditionFalse, Reason: "StaleServerValue", - LastTransitionTime: metav1.Now(), - }} - k8sClient := conflictingClient(serverSide) - - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - got := conditionOf(t, persistedOf(t, k8sClient, owner), "InfraReady") - assert.Equal(t, "Healthy", got.Reason, "an owned type must keep the value we staged") - assert.Equal(t, metav1.ConditionTrue, got.Status) - }) - - t.Run("keeps an owned condition removed from the staged owner removed across a conflict", func(t *testing.T) { - // This is what makes component retirement conflict-safe: the retired - // component is still passed for the pass that removes its condition, so - // the type is owned and the conflict refresh must not restore it from the - // server. Only unowned types follow the server. - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - LastTransitionTime: metav1.Now(), - }} - k8sClient := conflictingClient(serverSide) - - owner := newOwner() // InfraReady deliberately absent: the controller removed it - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Nil(t, meta.FindStatusCondition(persisted.Status.Conditions, "InfraReady"), - "an owned condition removed from the staged owner must stay removed; the server copy must not be restored") - }) - - t.Run("reads the server through APIReader on conflict, not through the cached client", func(t *testing.T) { - // A controller's Client reads from the informer cache, which can lag the - // write that caused the 409 and hand back the same stale resourceVersion. - // The conflict path must read through APIReader when one is set. Model - // the lag by giving the client and the reader different views of the same - // object: only the reader knows the other writer's newer condition. - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "SeenOnlyByAPIReader", - LastTransitionTime: metav1.Now(), - }} - apiReader := fake.NewClientBuilder(). - WithScheme(scheme).WithStatusSubresource(serverSide).WithObjects(serverSide).Build() - - cachedView := newOwner() // the cache never saw ExternalReady at all - k8sClient := conflictingClient(cachedView) - - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, - ReconcileContext{Client: k8sClient, APIReader: apiReader, Owner: owner}, - []*Component{infraComponent(t)}, - )) - - assert.Equal(t, 0, k8sClient.gets, "the conflict refetch must not go through the cached client") - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "SeenOnlyByAPIReader", conditionOf(t, persisted, "ExternalReady").Reason, - "the unowned condition must come from the direct read") - assert.Equal(t, "Healthy", conditionOf(t, persisted, "InfraReady").Reason) - }) - - t.Run("takes the server's newer value for a condition type we do not own", func(t *testing.T) { - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "WrittenByOtherController", - LastTransitionTime: metav1.Now(), - }} - k8sClient := conflictingClient(serverSide) - - // Our in-memory owner carries a stale copy of the other writer's condition, - // exactly as it would after fetching the owner at the top of the reconcile. - owner := newOwner() - owner.Status.Conditions = []metav1.Condition{{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "StaleValueFromOurGet", - LastTransitionTime: metav1.Now(), - }} - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "WrittenByOtherController", conditionOf(t, persisted, "ExternalReady").Reason, - "a type we do not own must take the server's value, not our stale copy") - assert.Equal(t, "Healthy", conditionOf(t, persisted, "InfraReady").Reason) - }) - - t.Run("takes the server's LastTransitionTime for an unowned condition, even when status is unchanged", func(t *testing.T) { - // This is the case that tells replaceStatusCondition apart from - // meta.SetStatusCondition. SetStatusCondition keeps the existing - // LastTransitionTime whenever the status did not change, so it would blend - // the server's condition with the stale local timestamp. The refresh must - // take the server's condition verbatim, transition time included. - serverTransition := metav1.NewTime(time.Date(2026, 8, 16, 12, 0, 0, 0, time.UTC)) - staleTransition := metav1.NewTime(serverTransition.Add(-time.Hour)) - - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "Same", - LastTransitionTime: serverTransition, - }} - k8sClient := conflictingClient(serverSide) - - owner := newOwner() - owner.Status.Conditions = []metav1.Condition{{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "Same", // same status and reason - LastTransitionTime: staleTransition, - }} - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - got := conditionOf(t, persistedOf(t, k8sClient, owner), "ExternalReady") - assert.True(t, got.LastTransitionTime.Equal(&serverTransition), - "an unowned condition must carry the server's LastTransitionTime, not the stale local one; got %v want %v", - got.LastTransitionTime, serverTransition) - }) - - t.Run("preserves a condition another writer added after our fetch", func(t *testing.T) { - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "AddedAfterOurGet", Status: metav1.ConditionTrue, Reason: "BrandNew", - LastTransitionTime: metav1.Now(), - }} - k8sClient := conflictingClient(serverSide) - - owner := newOwner() // never saw AddedAfterOurGet - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "BrandNew", conditionOf(t, persisted, "AddedAfterOurGet").Reason) - assert.Equal(t, "Healthy", conditionOf(t, persisted, "InfraReady").Reason) - }) - - t.Run("owns nothing when no components are passed", func(t *testing.T) { - // You own exactly what you pass. A validation-only controller passes nil, so - // its hand-staged condition is unowned and follows the server on a conflict, - // exactly as a controller-staged aggregate does. The next reconcile stages it - // again. Its non-condition status fields still survive, because the staged - // owner is never replaced. - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "Validated", Status: metav1.ConditionFalse, Reason: "StaleServerValue", - LastTransitionTime: metav1.Now(), - }} - k8sClient := conflictingClient(serverSide) - - owner := newOwner() - owner.Status.ObservedGeneration = 3 - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "Validated", Status: metav1.ConditionTrue, Reason: "SpecValid", - }) - - require.NoError(t, FlushStatus(ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil)) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "StaleServerValue", conditionOf(t, persisted, "Validated").Reason, - "nothing is owned, so the server's value wins on a conflict") - assert.Equal(t, int64(3), persisted.Status.ObservedGeneration, - "a staged non-condition status field still survives") - }) - - t.Run("treats a nil and an empty component slice identically", func(t *testing.T) { - // A reader will reasonably write either. Both own no condition types at - // all, so both must follow the server the same way on a conflict. - for name, comps := range map[string][]*Component{ - "nil": nil, - "empty non-nil": {}, - } { - t.Run(name, func(t *testing.T) { - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "Validated", Status: metav1.ConditionFalse, Reason: "StaleServerValue", - LastTransitionTime: metav1.Now(), - }} - k8sClient := conflictingClient(serverSide) - - owner := newOwner() - owner.Status.ObservedGeneration = 5 - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "Validated", Status: metav1.ConditionTrue, Reason: "SpecValid", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, comps, - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "StaleServerValue", conditionOf(t, persisted, "Validated").Reason, - "nothing is owned either way") - assert.Equal(t, int64(5), persisted.Status.ObservedGeneration) - }) - } - }) - - t.Run("drops an unowned condition the server no longer carries", func(t *testing.T) { - // Absence is server state. Another writer removing its condition must not - // be undone by this controller writing its stale local copy back. - // - // This also pins the cleared server copy: the fetch decodes into a separate - // object whose conditions are emptied first, so a response that omits - // conditions cannot leave the staged ones behind looking like the server's. - serverSide := newOwner() // the other writer deleted its condition - k8sClient := conflictingClient(serverSide) - - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "StaleLocalCopy", - }) - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "Healthy", conditionOf(t, persisted, "InfraReady").Reason, - "an owned condition is unaffected by the drop") - assert.Nil(t, meta.FindStatusCondition(persisted.Status.Conditions, "ExternalReady"), - "an unowned condition the server does not carry must be dropped, not resurrected") - }) - - t.Run("drops a first-time unowned condition the server has never held", func(t *testing.T) { - // Same rule seen from the other side: an owner-level aggregate the server - // has never held is not written on the conflict pass. The next reconcile - // stages it again. - serverSide := newOwner() - serverSide.Status.Conditions = []metav1.Condition{{ - Type: "ExternalReady", Status: metav1.ConditionTrue, Reason: "WrittenByOtherController", - LastTransitionTime: metav1.Now(), - }} - k8sClient := conflictingClient(serverSide) - - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "Ready", Status: metav1.ConditionTrue, Reason: "Healthy", // the aggregate, owned by nobody - }) - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent(t)}, - )) - - persisted := persistedOf(t, k8sClient, owner) - assert.Equal(t, "Healthy", conditionOf(t, persisted, "InfraReady").Reason) - assert.Equal(t, "WrittenByOtherController", conditionOf(t, persisted, "ExternalReady").Reason) - assert.Nil(t, meta.FindStatusCondition(persisted.Status.Conditions, "Ready"), - "an unowned aggregate the server has never held is not written on the conflict pass") - }) - - t.Run("does not fetch at all when the first update succeeds", func(t *testing.T) { - owner := newOwner() - applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ - Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", - }) - inner := fake.NewClientBuilder(). - WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() - counting := &conflictOnceClient{Client: inner, conflicts: 1} // never conflicts - - require.NoError(t, FlushStatus( - ctx, ReconcileContext{Client: counting, Owner: owner}, []*Component{infraComponent(t)}, - )) - - assert.Zero(t, counting.gets, "the non-conflict path must not fetch the owner") - }) -} diff --git a/pkg/component/flush_status_test.go b/pkg/component/flush_status_test.go new file mode 100644 index 00000000..a9b503e4 --- /dev/null +++ b/pkg/component/flush_status_test.go @@ -0,0 +1,195 @@ +package component + +import ( + "context" + "errors" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestFlushStatus(t *testing.T) { + ctx := context.Background() + scheme := runtime.NewScheme() + require.NoError(t, AddToScheme(scheme)) + + newOwner := func() *MockOperatorCRD { + return &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-owner", + Namespace: "default", + Generation: 1, + }, + } + } + + cond := func(ctype, reason string, status metav1.ConditionStatus) Condition { + return Condition{Type: ctype, Status: status, Reason: reason, ObservedGeneration: 1} + } + + t.Run("persists every condition on the owner and records a metric per condition", func(t *testing.T) { + owner := newOwner() + applyStatusCondition(ReconcileContext{Owner: owner}, cond("InfraReady", "Ready", metav1.ConditionTrue)) + applyStatusCondition(ReconcileContext{Owner: owner}, cond("AppReady", "Ready", metav1.ConditionTrue)) + + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() + metrics := &MockMetrics{} + metrics.On("RecordConditionFor", owner.GetKind(), owner, "InfraReady", + string(metav1.ConditionTrue), "Ready", mock.Anything, mock.Anything).Return().Once() + metrics.On("RecordConditionFor", owner.GetKind(), owner, "AppReady", + string(metav1.ConditionTrue), "Ready", mock.Anything, mock.Anything).Return().Once() + + require.NoError(t, FlushStatus(ctx, ReconcileContext{Client: k8sClient, Metrics: metrics, Owner: owner})) + + persisted := &MockOperatorCRD{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) + assert.Len(t, persisted.Status.Conditions, 2) + metrics.AssertExpectations(t) + }) + + t.Run("skips metrics when the recorder is nil", func(t *testing.T) { + owner := newOwner() + applyStatusCondition(ReconcileContext{Owner: owner}, cond("InfraReady", "Ready", metav1.ConditionTrue)) + + k8sClient := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() + + require.NoError(t, FlushStatus(ctx, ReconcileContext{Client: k8sClient, Owner: owner})) + + persisted := &MockOperatorCRD{} + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), persisted)) + assert.Len(t, persisted.Status.Conditions, 1) + }) + + t.Run("returns a non-conflict update error without recording metrics", func(t *testing.T) { + owner := newOwner() + applyStatusCondition(ReconcileContext{Owner: owner}, cond("InfraReady", "Ready", metav1.ConditionTrue)) + + inner := fake.NewClientBuilder().WithScheme(scheme).WithStatusSubresource(owner).WithObjects(owner).Build() + k8sClient := &errorMockClient{Client: inner} + metrics := &MockMetrics{} + + err := FlushStatus(ctx, ReconcileContext{Client: k8sClient, Metrics: metrics, Owner: owner}) + require.ErrorIs(t, err, errUpdateFailed) + assert.False(t, apierrors.IsConflict(err)) + + metrics.AssertNotCalled(t, "RecordConditionFor") + }) +} + +var errUpdateFailed = errors.New("update failed") + +type errorMockClient struct { + client.Client +} + +func (e *errorMockClient) Status() client.SubResourceWriter { + return &errorStatusWriter{SubResourceWriter: e.Client.Status()} +} + +type errorStatusWriter struct { + client.SubResourceWriter +} + +func (e *errorStatusWriter) Update(_ context.Context, _ client.Object, _ ...client.SubResourceUpdateOption) error { + return errUpdateFailed +} + +// Two controllers share the status of one owner: this one owns the Accepted +// condition, another owns Ready and status.observedGeneration. The specs run +// against a real API server because the guarantee rests on its +// resourceVersion check. +var _ = Describe("FlushStatus on an owner whose status has another writer", func() { + var ( + ctx = context.Background() + owner *MockOperatorCRD + ) + + fetch := func() *MockOperatorCRD { + GinkgoHelper() + fetched := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), fetched)).To(Succeed()) + return fetched + } + + // otherWriterUpdates writes the other controller's fields through its own + // fresh read, as a second controller would. + otherWriterUpdates := func() { + GinkgoHelper() + other := fetch() + other.Status.ObservedGeneration = 4 + meta.SetStatusCondition(&other.Status.Conditions, metav1.Condition{ + Type: "Ready", Status: metav1.ConditionTrue, Reason: "PluginReady", + }) + Expect(k8sClient.Status().Update(ctx, other)).To(Succeed()) + } + + stageAccepted := func(o *MockOperatorCRD) { + applyStatusCondition(ReconcileContext{Owner: o}, Condition{ + Type: "Accepted", Status: metav1.ConditionTrue, Reason: "Valid", + }) + } + + BeforeEach(func() { + owner = &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{ + Name: "shared-status-owner", + Namespace: createNamespace(ctx, "flush-status-"), + }, + } + Expect(k8sClient.Create(ctx, owner)).To(Succeed()) + }) + + AfterEach(func() { + Expect(k8sClient.Delete(ctx, owner)).To(Succeed()) + }) + + It("returns a conflict and writes nothing when the other writer changed the status after the read", func() { + stale := fetch() + otherWriterUpdates() + stageAccepted(stale) + metrics := &MockMetrics{} + + err := FlushStatus(ctx, ReconcileContext{Client: k8sClient, Metrics: metrics, Owner: stale}) + + Expect(apierrors.IsConflict(err)).To(BeTrue(), "expected a conflict, got %v", err) + server := fetch() + Expect(server.Status.ObservedGeneration).To(Equal(int64(4)), + "the other writer's observedGeneration must not be rolled back") + ready := meta.FindStatusCondition(server.Status.Conditions, "Ready") + Expect(ready).NotTo(BeNil()) + Expect(ready.Reason).To(Equal("PluginReady")) + Expect(meta.FindStatusCondition(server.Status.Conditions, "Accepted")).To(BeNil(), + "nothing from the stale write may land") + Expect(metrics.Calls).To(BeEmpty(), "a failed write records no condition metrics") + }) + + It("writes its own change and keeps the other writer's fields on the next reconcile", func() { + stale := fetch() + otherWriterUpdates() + stageAccepted(stale) + Expect(apierrors.IsConflict(FlushStatus(ctx, ReconcileContext{Client: k8sClient, Owner: stale}))).To(BeTrue()) + + fresh := fetch() + stageAccepted(fresh) + Expect(FlushStatus(ctx, ReconcileContext{Client: k8sClient, Owner: fresh})).To(Succeed()) + + server := fetch() + Expect(server.Status.ObservedGeneration).To(Equal(int64(4))) + ready := meta.FindStatusCondition(server.Status.Conditions, "Ready") + Expect(ready).NotTo(BeNil()) + Expect(ready.Reason).To(Equal("PluginReady")) + accepted := meta.FindStatusCondition(server.Status.Conditions, "Accepted") + Expect(accepted).NotTo(BeNil()) + Expect(accepted.Reason).To(Equal("Valid")) + }) +}) diff --git a/pkg/component/zz_mock_crd_test.go b/pkg/component/zz_mock_crd_test.go index 2e0173ee..58c2017f 100644 --- a/pkg/component/zz_mock_crd_test.go +++ b/pkg/component/zz_mock_crd_test.go @@ -34,8 +34,7 @@ type MockOperatorCRDList struct { type MockStatus struct { Conditions []metav1.Condition `json:"conditions,omitempty"` // ObservedGeneration is a non-condition status field. It exists so tests can - // prove that FlushStatus preserves staged status fields the framework does - // not manage, including across a conflict retry. + // prove that FlushStatus never rolls back a status field another writer set. ObservedGeneration int64 `json:"observedGeneration,omitempty"` } diff --git a/plugin/agents/reviewer.md b/plugin/agents/reviewer.md index 1734af16..436c70e8 100644 --- a/plugin/agents/reviewer.md +++ b/plugin/agents/reviewer.md @@ -38,11 +38,11 @@ actually violated. when the resources have no useful readiness independent of each other, adding noise without actionable information. 5. **Keep Controllers Thin.** Look for resource construction, feature decisions, or mutation logic living inline in the controller rather than in pure component-building functions. Look for `FlushStatus` called more than once per - reconcile, or called between component reconciles rather than deferred once at the end. Look for - `FlushStatus(ctx, rec, nil)` in a controller that did build components: a flush owns exactly the condition types of - the components it is passed, so `nil` there means the controller owns none of its own component conditions and every - one of them reverts to the server's value on a conflict retry. Look for a controller that stops reconciling remaining - components after the first error instead of collecting the first error and continuing. + reconcile, or called between component reconciles rather than deferred once at the end. Look for a controller that + drops the error `FlushStatus` returns, or that retries the status write itself after refreshing the + `resourceVersion`: a conflict must reach `Reconcile` as an error or a `RequeueAfter`, and a retry with a fresh + `resourceVersion` writes the stale status over another writer's values. Look for a controller that stops reconciling + remaining components after the first error instead of collecting the first error and continuing. 6. **Reconciler Error Handling and Requeueing.** Look for `Reconcile` returning an error for a resource that is merely converging (a rolling Deployment, a `Blocked` guard) instead of letting that state surface through its condition. Look for an explicit `reconcile.Result{RequeueAfter: ...}` set without a concrete reason to poll on a fixed cadence, diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index 57db9132..debac892 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -230,46 +230,27 @@ recorder. Before your controller records its own event or writes a log line, rea for example when `meta.SetStatusCondition` returns `true`. `Component.Reconcile` mutates the owner's status conditions only in memory. The controller persists them by calling -`component.FlushStatus(ctx, recCtx, comps)` once per reconcile, typically deferred so conditions set on error paths are -written. `FlushStatus` performs a single `Status().Update`, wrapped in `retry.RetryOnConflict`, that writes every -condition currently staged on the owner, merging by condition type via `meta.SetStatusCondition` rather than replacing -the list. This split lets a controller with several components stage several conditions in one reconcile and persist -them in one write, instead of racing a write per component. - -That `Status().Update` writes the **whole status subresource**, not only the conditions, so fetch the owner fresh at the -top of every reconcile: an owner carried over from an earlier pass writes stale values back over newer ones. - -**Pass the components whose conditions this flush owns:** `component.FlushStatus(ctx, recCtx, comps)`. Their condition -types are the owned set, so it cannot drift from what the components actually write. - -**You own exactly what you pass, and nothing else.** A controller that manages no components passes `nil`; `nil` and an -empty slice behave identically and own no condition types at all. Ownership is by condition type, not by who staged the -condition, so a hand-staged condition is owned only if its type belongs to a passed component; an aggregate or a -validation-only CRD's single condition is not, and on a conflict it follows the server and the next reconcile stages it -again. - -The parameter is required, not variadic: a variadic would let existing calls keep compiling while silently retaining the -old wider ownership, making a correctness fix opt-in and invisible. - -On a 409 conflict `FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a -separate object through `ReconcileContext.APIReader` (set it from the manager's `GetAPIReader()`; the default `Client` -reads from the informer cache, which can lag the conflicting write and return the same stale `resourceVersion`), takes -that copy's `resourceVersion`, restores from it only the conditions whose type this flush does not own, and retries. Two -consequences: non-condition status fields staged during the reconcile survive, and a type this flush does not own keeps -the server's value instead of the possibly stale copy the controller is holding, so another writer's concurrent update -is not rolled back. - -For unowned types the server is the source of truth, absence included. An unowned condition the server no longer carries -is dropped rather than written back, so a condition another writer removed is not resurrected, and one the server has -never held (a first aggregate write) is dropped for the same reason and staged again next reconcile. - -Declare the component slice before the deferred call, so the closure observes every component built during the -reconcile: +`component.FlushStatus(ctx, recCtx)` once per reconcile, typically deferred so conditions set on error paths are +written. `FlushStatus` performs a single `Status().Update` that writes the conditions currently staged on the owner. +This split lets a controller with several components stage several conditions in one reconcile and persist them in one +write, instead of racing a write per component. + +That `Status().Update` writes the **whole status subresource**, not only the conditions, and the API server accepts it +only while the owner's `resourceVersion` matches the stored object. Fetch the owner fresh at the top of every reconcile: +an owner carried over from an earlier pass holds an old `resourceVersion`, so its flush can only conflict. + +**A conflict is returned, not retried.** When another writer updated the owner after the controller read it, +`FlushStatus` writes nothing and returns the 409 error (`apierrors.IsConflict` is `true`). Return it from `Reconcile`: +controller-runtime requeues with rate limiting, and the next reconcile reads the current owner and recomputes. Because a +stale write never lands, a status field or condition another controller set is never rolled back, so two controllers can +share one owner's status without declaring which fields each owns. Condition metrics are recorded only after a +successful write. To requeue a conflict without a logged reconciler error, check `apierrors.IsConflict` in the deferred +call and return `reconcile.Result{RequeueAfter: time.Second}` with a `nil` error (not the deprecated `Requeue`); see +"Requeue on a conflict without a reconcile error" in `references/component.md`. ```go -var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -278,7 +259,7 @@ comp, err := buildComponent(owner) if err != nil { return err } -comps = append(comps, comp) +return comp.Reconcile(ctx, recCtx) ``` ## One write path for the owner's status @@ -296,7 +277,7 @@ meta.SetStatusCondition(policy.GetStatusConditions(), metav1.Condition{ policy.Status.ObservedGeneration = policy.Generation // owner-level field, see below return reconcile.Result{}, component.FlushStatus(ctx, component.ReconcileContext{ Client: r.Client, Owner: policy, -}, nil) // nil: owns no component conditions; on a conflict this follows the server +}) ``` **Server-side apply is for managed resources, never for the owner's status subresource.** An SSA status patch alongside diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 75ff2b26..8700e729 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -658,7 +658,8 @@ Four properties of the aggregate condition are easy to miss: - A component disabled by a feature gate reports `True`/`Disabled` and therefore counts as ready, for the same reason. - `Aggregate` looks only at the components that you pass in. If the controller no longer builds a component, the condition of that component stays on the owner. `Aggregate` ignores that stale condition, but users still see it, - because [`FlushStatus`](#persisting-status-with-flushstatus) merges conditions by type and never prunes them. + because [`FlushStatus`](#persisting-status-with-flushstatus) writes the conditions on the owner as they are and never + prunes them. - The returned `Condition` is a plain struct. A caller that wants different prose can change the message before it stages the condition with `meta.SetStatusCondition`. @@ -703,9 +704,8 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) } recCtx := component.ReconcileContext{ /* ... */ Owner: owner} - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -714,7 +714,6 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) if err != nil { return reconcile.Result{}, err } - comps = append(comps, comp) if err := comp.Reconcile(ctx, recCtx); err != nil { return reconcile.Result{}, err @@ -791,7 +790,7 @@ recCtx := component.ReconcileContext{ Scheme: r.Scheme, // *runtime.Scheme EventRecorder: r.EventRecorder, // events.EventRecorder, from manager.GetEventRecorder(name) Metrics: r.Metrics, // component.MetricsRecorder, optional; see Metrics - APIReader: r.APIReader, // client.Reader, from manager.GetAPIReader(); direct reads on a status conflict + APIReader: r.APIReader, // client.Reader, from manager.GetAPIReader(); direct reads of live objects Owner: owner, // the CRD that owns this component } @@ -828,10 +827,8 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) APIReader: r.APIReader, Owner: owner, } - // Declared before the deferred flush so the closure sees every component built below. - var comps []*component.Component defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() @@ -840,80 +837,61 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) if err != nil { return reconcile.Result{}, err } - comps = append(comps, comp) return reconcile.Result{}, comp.Reconcile(ctx, recCtx) } ``` -`FlushStatus` performs one `Status().Update` call, wrapped in `retry.RetryOnConflict`. That call writes the **whole -status subresource**, not only the conditions. Every status field the controller staged in memory is sent, and every -field it did not stage is sent exactly as it stands on the in-memory object. Fetch the owner fresh at the top of each -reconcile, so the fields nobody staged still hold current server state. An owner carried over from an earlier pass -writes stale values back over newer ones. +`FlushStatus(ctx context.Context, rec ReconcileContext) error` performs one `Status().Update` call. That call writes the +**whole status subresource**, not only the conditions. Every status field on the in-memory owner is sent, whether the +controller staged it or not. -On a 409 Conflict, for example when another writer updated the owner between the controller's `Get` and this call, -`FlushStatus` keeps the staged owner as the object it writes. It fetches the server's copy into a **separate** object, -takes that copy's `resourceVersion` so the retry targets the live object, restores from it only the conditions whose -type this flush does not own, and retries. The staged owner is never replaced. +The API server accepts the write only if the `resourceVersion` of the owner still matches the stored object. The +`resourceVersion` that the `Get` at the top of the reconcile read is therefore the guard for the write. Fetch the owner +fresh at the top of each reconcile. An owner carried over from an earlier pass holds an old `resourceVersion`, so its +flush can only conflict. -That fetch goes through `ReconcileContext.APIReader` when it is set, and through `Client` otherwise. Set it from the -manager's `GetAPIReader()`. The manager's default `Client` serves reads from the informer cache, which can lag the very -write that caused the conflict and hand back the same stale `resourceVersion`, so a retry through the cache may conflict -again until the cache catches up. A direct read sees the server as it is. +When another writer updates the owner between the `Get` and the flush, the API server rejects the write with a 409 +Conflict. `FlushStatus` does not retry. Nothing is written, and it returns the error, for which `apierrors.IsConflict` +(`k8s.io/apimachinery/pkg/api/errors`) reports `true`. Return the error from `Reconcile`, as the deferred call above +does. controller-runtime then requeues the owner with rate limiting, and the next reconcile reads the current owner and +computes its status again. -Two things follow from that. Non-condition status fields staged during the reconcile survive a conflict, because the -object holding them is the object that gets written. And a condition type this flush does not own keeps the server's -value rather than the copy the controller happens to be holding, so a concurrent update by another writer is not rolled -back. +Because a stale write never lands, `FlushStatus` never rolls back a status field or a condition that another writer set. +Two controllers can share the status of one owner, each one writing its own conditions and fields with `FlushStatus`, +and neither one has to declare which fields it owns. -Which types count as owned comes from the components passed to `FlushStatus`: - -```go -component.FlushStatus(ctx, recCtx, []*component.Component{backendComp, frontendComp}) -``` - -Their condition types are the owned set, so it cannot drift from what the components actually write. - -Note that this is a slice, while [`Aggregate`](#aggregating-components-into-one-owner-condition) takes the same -components variadically. The difference is deliberate, for the reason below. - -The parameter is required rather than variadic on purpose. A variadic would let every existing call keep compiling while -silently retaining the old, wider ownership, which would make a correctness fix opt-in and invisible. Requiring the -argument forces each call site to be looked at exactly once. - -**You own exactly what you pass, and nothing else.** A controller that manages no components passes `nil`, and `nil` and -an empty slice behave identically: neither owns any condition type. Ownership is by condition type, not by who staged -the condition. A condition the controller stages by hand is owned only if its type belongs to a passed component; -otherwise, as with an owner-level aggregate or the only condition a -[validation-only CRD](#one-write-path-for-the-owners-status) reports, it is unowned. On a conflict an unowned condition -follows the server, and the next reconcile stages it again. - -!!! note "An owner-level aggregate is not owned by any component" - - A condition the controller stages itself, such as a `Ready` produced by - [`component.Aggregate`](#aggregating-components-into-one-owner-condition), belongs to no component, so a conflict - reverts it to the value the server already holds and the next reconcile stages it again. Passing the components - covers their own conditions; the aggregate is recomputed from them each pass anyway. - - For unowned types the server is the source of truth, absence included. An unowned condition the server no longer - carries is dropped from the staged owner rather than written back, so a condition another writer removed is not - resurrected. A condition the server has never held, such as the first write of the aggregate, is dropped for the - same reason and staged again on the next reconcile. - - "The next reconcile" depends on one happening. With controller-runtime's defaults, every update to the owner - triggers a reconcile, status-only ones included, so the successful write itself re-queues the owner and the aggregate - is restaged promptly. A controller that filters status-only updates out of its watch, for example with a - generation-based predicate, does not get that re-queue and the aggregate lags until the next reconcile from another - source. - -After a successful update, `FlushStatus` records metrics for every condition on the owner. If `Metrics` is `nil`, -recording is skipped. See [Metrics](#metrics). +After a successful update, `FlushStatus` records metrics for every condition on the owner. A failed update records none. +If `Metrics` is `nil`, recording is skipped. See [Metrics](#metrics). This split is what lets a controller with several components stage several conditions during one reconcile and persist them in a single write. Persisting after each component would race the components' writes and produce 409 conflicts. See [Keep Controllers Thin](../../structuring-operators/references/guidelines.md#keep-controllers-thin) and [One Component Per Logical Condition](../../structuring-operators/references/guidelines.md#one-component-per-logical-condition). +### Requeue on a conflict without a reconcile error + +A returned conflict is logged as a reconciler error. A conflict is normal when other writers share the owner, so a +controller can requeue it quietly instead. In the deferred call, check for a conflict and return a short `RequeueAfter` +with a `nil` error: + +```go +defer func() { + flushErr := component.FlushStatus(ctx, recCtx) + switch { + case flushErr == nil: + case err != nil: + // Reconcile already returns an error, and that error requeues the owner. + case apierrors.IsConflict(flushErr): + res, err = reconcile.Result{RequeueAfter: time.Second}, nil + default: + err = flushErr + } +}() +``` + +The deferred call sets the named results, so declare them as `(res reconcile.Result, err error)`. Use `RequeueAfter`, +not `Requeue`, which is deprecated as of controller-runtime v0.25. + ### One write path for the owner's status A CR's status is written once per reconcile, through `FlushStatus`. This rule also holds for a controller that builds no @@ -937,12 +915,10 @@ func (r *PolicyReconciler) Reconcile(ctx context.Context, req reconcile.Request) } meta.SetStatusCondition(policy.GetStatusConditions(), cond) - // nil: this controller owns no component conditions. On a conflict the staged - // condition follows the server and is written again on the next reconcile. return reconcile.Result{}, component.FlushStatus(ctx, component.ReconcileContext{ Client: r.Client, Owner: policy, - }, nil) + }) } ``` diff --git a/plugin/skills/structuring-operators/SKILL.md b/plugin/skills/structuring-operators/SKILL.md index 947bd0fc..e54d68a1 100644 --- a/plugin/skills/structuring-operators/SKILL.md +++ b/plugin/skills/structuring-operators/SKILL.md @@ -38,7 +38,7 @@ checklist: a change that violates one of these rules is a candidate for rework, | Mutations Are Pure Functions of the Spec | A mutation is a pure function of the owner spec and build-time inputs; it must never read live cluster state. | | Leave Version-Dependent Fields Empty in the Baseline | Give each field exactly one owner; when a value depends on the spec version, leave it empty in the baseline and let a single mutation set it. | | One Component Per Logical Condition | Split components when users would ask about their health separately or a failure in one should not mask another; combine when resources have no independent readiness. | -| Keep Controllers Thin | A controller fetches the owner, builds and reconciles components, and defers one `FlushStatus(ctx, rec, comps)` passing the components it built, or `nil` if it built none; resource construction and mutation logic live in pure, testable component-building functions. | +| Keep Controllers Thin | A controller fetches the owner, builds and reconciles components, and defers one `FlushStatus(ctx, rec)` that returns a status conflict as a reconcile error; resource construction and mutation logic live in pure, testable component-building functions. | | Derive the Owner's Aggregate Condition from Component Conditions | Derive the owner's aggregate condition with `component.Aggregate` rather than by hand: it is `True` only when every component condition is `True` (unanimity), and its reason comes from the governing component, the highest `Status.Priority()` among the non-True conditions, or among all of them when every condition is `True`. | | Reconciler Error Handling and Requeueing | Return an error only for a genuine fault (a failed API call, a mutation that cannot apply, a version below the supported floor); let a merely converging resource report through its condition and requeue via normal watch and resync. | | Give Each Controller Layer One Visible Responsibility | A helper that changes an object does not also persist it, only `Reconcile` returns a `reconcile.Result`, and each layer is correct on its own inputs rather than on a check in its caller or a hidden side effect of its callee. | @@ -71,10 +71,9 @@ meta.SetStatusCondition(app.GetStatusConditions(), metav1.Condition(ready)) ``` Stage it after the component loop and before the deferred `FlushStatus`, so one status write carries the aggregate and -the component conditions together. The aggregate belongs to no component, so a conflict retry reverts it to the server's -value and the next reconcile stages it again. `Aggregate` writes nothing itself and reads each component condition -through `GetCondition`, so a component that has never reconciled contributes a synthetic `Unknown`/`False` and a fresh -CR never reports ready. +the component conditions together. `Aggregate` writes nothing itself and reads each component condition through +`GetCondition`, so a component that has never reconciled contributes a synthetic `Unknown`/`False` and a fresh CR never +reports ready. It makes **two separate decisions**, which is the part a hand-rolled version gets wrong: diff --git a/plugin/skills/structuring-operators/references/guidelines.md b/plugin/skills/structuring-operators/references/guidelines.md index cd549479..cdef4f49 100644 --- a/plugin/skills/structuring-operators/references/guidelines.md +++ b/plugin/skills/structuring-operators/references/guidelines.md @@ -148,24 +148,20 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) Scheme: r.Scheme, EventRecorder: r.EventRecorder, Metrics: r.Metrics, - APIReader: r.APIReader, // direct reads on a status conflict + APIReader: r.APIReader, // direct reads of live objects Owner: app, } - // Persist all staged conditions exactly once, even on the error path. comps is - // declared first so the closure sees whatever the resolver returns, and - // FlushStatus derives the condition types it owns from those components. - var comps []*component.Component + // Persist all staged conditions exactly once, even on the error path. defer func() { - if flushErr := component.FlushStatus(ctx, recCtx, comps); flushErr != nil && err == nil { + if flushErr := component.FlushStatus(ctx, recCtx); flushErr != nil && err == nil { err = flushErr } }() - built, buildErr := buildComponents(app) - if buildErr != nil { - return reconcile.Result{}, buildErr + comps, err := buildComponents(app) + if err != nil { + return reconcile.Result{}, err } - comps = built var firstErr error for _, comp := range comps { @@ -236,23 +232,15 @@ particular, and metrics, dashboards, and downstream automation key on the reason different prose, and leave the reason alone. **Remove the conditions of components you retire.** `Aggregate` reads only the components you pass it, and -[`FlushStatus`](../../building-components/references/component.md#persisting-status-with-flushstatus) merges conditions -by type and never prunes, so a component the controller has stopped building leaves its last condition on the owner -indefinitely. The aggregate ignores it, and users still see it in `kubectl describe`. Delete it explicitly when you -retire a component: +[`FlushStatus`](../../building-components/references/component.md#persisting-status-with-flushstatus) writes the owner's +conditions as they are and never prunes them, so a component the controller has stopped building leaves its last +condition on the owner indefinitely. The aggregate ignores it, and users still see it in `kubectl describe`. Delete it +explicitly when you retire a component: ```go meta.RemoveStatusCondition(owner.GetStatusConditions(), "OldComponentReady") ``` -Ownership decides whether that removal survives a conflict. `FlushStatus` owns exactly the condition types of the -components you pass it, and on a 409 it takes the server's value for every unowned type. If the retired component is -already absent from the slice when its condition is removed, the type is unowned, the conflict refresh puts the -condition back, and the removal lands only on a later, conflict-free pass. To make it land on the first pass, retire in -two steps: keep building the component and passing it to `FlushStatus` for the release that runs -`meta.RemoveStatusCondition`, so its type stays owned and the removal is what gets written, and delete the component in -the release after. Either way the condition disappears; the two-step form makes it deterministic. - ## Reconciler Error Handling and Requeueing The framework distinguishes between conditions and errors. A resource that is merely converging (a rolling Deployment, a @@ -268,6 +256,11 @@ grace period is the exception: no watch event arrives when its grace period ends `FlushStatus` is deferred, the owner's conditions are written before the error propagates, so the failure is visible in status even while controller-runtime backs off. +A 409 Conflict from `FlushStatus` reaches `Reconcile` the same way. It means that another writer updated the owner after +the controller read it. Nothing was written, and the next reconcile reads the current owner and computes its status +again. To requeue a conflict without a logged reconciler error, see +[Requeue on a conflict without a reconcile error](../../building-components/references/component.md#requeue-on-a-conflict-without-a-reconcile-error). + Only `Reconcile` decides when to requeue. A helper or a sub-reconciler returns an error or a domain value, not a `reconcile.Result`. See [Give Each Controller Layer One Visible Responsibility](#give-each-controller-layer-one-visible-responsibility).