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).