diff --git a/docs/component.md b/docs/component.md index e6af17b4..402b8aff 100644 --- a/docs/component.md +++ b/docs/component.md @@ -815,22 +815,28 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) 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. +writes stale values back over newer ones. With +[`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), the write is a merge patch of the +declared fields and the conditions instead. 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. +type this flush does not own, and retries. The staged owner is never replaced, and no other status field is taken from +the server. 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. -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. +Two things follow from that. A status field that the flush writes keeps its staged value across a conflict, because the +object holding it 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. +Without `WithOwnedStatusFields`, the flush writes every non-condition status field, so a retry also writes back the +stale copy of a field that another controller changed after the fetch. When another controller writes non-condition +fields of the same status, see +[Sharing the owner's status with another controller](#sharing-the-owners-status-with-another-controller). Which types count as owned comes from the components passed to `FlushStatus`: @@ -917,6 +923,61 @@ Server-Side Apply is for managed resources, never for the owner's status subreso depends on which call runs last: the `Update` can overwrite the patched condition, or the patch can overwrite what the `Update` wrote. Keep every owner-status write on the `meta.SetStatusCondition` plus `FlushStatus` path. +### Sharing the owner's status with another controller + +Sometimes two controllers write the status of one CR. For example, a status-only controller owns an `Accepted` +condition, and a second controller owns a `Ready` condition and `status.observedGeneration`. Each controller still +writes through `FlushStatus`. Condition ownership comes from the components passed to it. For non-condition fields, pass +`component.WithOwnedStatusFields` with the fields that this controller writes: + +```go +// In the status-only controller, which writes status.phase and no other non-condition field: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) + +// In the other controller, which writes status.observedGeneration: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("observedGeneration")) +``` + +With the option, every attempt of `FlushStatus` is a JSON merge patch of the status instead of a `Status().Update`. The +patch holds only: + +- the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the + owner changed after the controller read it; +- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field but the + server does, so the server removes it. A declared object is owned whole: the patch sends `null` for each child that + the server stores and the staged owner does not carry, so a child that `omitempty` dropped is removed too; +- the conditions list as staged. On the first attempt that includes any unowned condition the controller staged by hand, + such as an owner-level aggregate; the precondition makes the patch fail if the server changed after the read. After a + 409, the unowned conditions are first refreshed from the server. + +To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` +otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. When that read finds an owner with +another UID, because the owner was deleted and recreated under the same name after the controller read it, `FlushStatus` +returns an error and does not write, as `Status().Update` would refuse the write. A field that the controller did not +declare is never sent, on the first attempt or on a retry, so it cannot overwrite another writer's value. On a 409, +`FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the server, as described above, builds the +patch again, and retries. After a successful write, the controller-runtime client decodes the server's response into the +owner, so a value staged in a field that you did not declare does not survive in memory. + +Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, +`"summary.ready"`. Do not include the `status.` prefix. `WithOwnedStatusFields()` with no paths declares that the flush +owns no non-condition field, which fits a controller that writes only conditions. Paths from several options are +combined. Declare every non-condition field that the controller writes, including `observedGeneration` when it is this +controller's field: a field that you stage but do not declare is not written. + +`FlushStatus` returns an error, with no write, when a path: + +- is empty or has an empty segment, such as `"summary..ready"`; +- names the conditions, contains them, or lies below them, because conditions are owned through components. + `FlushStatus` finds the JSON path of the list that `GetStatusConditions` returns, so this holds whatever that field is + called; +- runs below a field that the staged owner holds as a value that is not an object, such as `"phase.value"` when `phase` + is a string. + +The framework does not check a path against the CRD schema. A misspelled path, such as `"phsae"`, sends a field that the +schema does not declare. With a structural schema the API server prunes that field, so that part of the write does +nothing and `FlushStatus` returns no error. Check each path against the CRD type when you add it. + ### Where the owner's observedGeneration is set The framework sets `ObservedGeneration` on every **component condition** it writes, from `owner.GetGeneration()`. It @@ -925,7 +986,9 @@ never writes `status.observedGeneration` on the owner itself. `OperatorCRD` requ An owner-level `status.observedGeneration` is the controller's own field. Assign it on the owner before `FlushStatus` runs, in the same staging step that writes the owner's own condition, and the single status write persists both. This -applies to a status-only controller too, as in the validation-only example above. +applies to a status-only controller too, as in the validation-only example above. When the status is +[shared with another controller](#sharing-the-owners-status-with-another-controller), only the controller that owns +`observedGeneration` assigns it, and that controller includes it in `WithOwnedStatusFields`. ```go app.Status.ObservedGeneration = app.Generation diff --git a/docs/guidelines.md b/docs/guidelines.md index 13b07842..b0c28a6d 100644 --- a/docs/guidelines.md +++ b/docs/guidelines.md @@ -190,6 +190,12 @@ returns an error. If you do not want condition metrics, leave `ReconcileContext.Metrics` as `nil`; `FlushStatus` tolerates a nil recorder and skips metric emission. +When two controllers write the status of one CR, give each status field one owner, as with conditions. Each controller +passes the components that own its conditions and declares its non-condition fields with +[`WithOwnedStatusFields`](component.md#sharing-the-owners-status-with-another-controller), so its flush patches only +those fields and the conditions. Without the declaration, the flush writes the whole status, and a conflict retry writes +the controller's stale copy of the other controller's fields back over newer values. + Building the component set from a pure resolver `(spec, version) -> []*component.Component` keeps the loop stable: enabling an optional feature changes which components the resolver returns without touching the reconcile loop. diff --git a/pkg/component/component.go b/pkg/component/component.go index 1f9af1eb..7537a3b0 100644 --- a/pkg/component/component.go +++ b/pkg/component/component.go @@ -655,11 +655,21 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // validation-only CRD reports, it is unowned. On a conflict an unowned condition // follows the server, and the next reconcile stages it again. // -// 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. +// By default 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. +// +// With [WithOwnedStatusFields], every attempt is instead a JSON merge patch of +// the status that holds only the declared fields and the conditions, with the +// staged resourceVersion as a precondition when the owner has one. A field the +// controller did not declare is never sent, so it cannot overwrite another +// writer's value. Use it when another controller writes non-condition fields of +// the same status. After a successful write rec.Client decodes the server's +// response into rec.Owner; with the controller-runtime client a staged value of +// an undeclared field does not survive in memory. // // Controllers must call FlushStatus exactly once per reconciliation, typically // via defer so that conditions set on error paths are still persisted. Declare @@ -693,7 +703,10 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // 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. +// only the conditions whose type this flush does not own. It then retries. No +// other status field is taken from the server. Without [WithOwnedStatusFields] +// the retry therefore writes every staged non-condition field again, including +// a stale copy of a field that another writer has changed since the fetch. // // 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 @@ -714,24 +727,35 @@ func fail(rec ReconcileContext, conditionType ConditionType, err error) error { // 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 { +// be non-nil; a nil component is a programming error. With +// [WithOwnedStatusFields], FlushStatus returns an error without writing for an +// invalid path. If rec.Metrics is nil, metric recording is skipped. +func FlushStatus(ctx context.Context, rec ReconcileContext, comps []*Component, opts ...FlushOption) error { + var options flushOptions + for _, opt := range opts { + opt(&options) + } + 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) { + var err error + if options.fieldsDeclared { + err = patchOwnedStatus(ctx, rec, owned, options.ownedFields) + } else { + 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 refreshErr := refreshUnownedConditions(ctx, rec, owned); refreshErr != nil { - return refreshErr - } - return updateErr - }) + }) + } if err != nil { return err } @@ -779,14 +803,7 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m // 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 { + if err := liveReader(rec).Get(ctx, client.ObjectKeyFromObject(rec.Owner), server); err != nil { return err } @@ -819,6 +836,17 @@ func refreshUnownedConditions(ctx context.Context, rec ReconcileContext, owned m return nil } +// liveReader returns rec.APIReader when one is set, and rec.Client otherwise. +// The manager's Client serves reads from the informer cache, which can lag the +// write that caused a conflict and return the same stale resourceVersion, +// defeating the retry. +func liveReader(rec ReconcileContext) client.Reader { + if rec.APIReader != nil { + return rec.APIReader + } + return rec.Client +} + // replaceStatusCondition sets cond by type, taking its value verbatim. // // meta.SetStatusCondition is deliberately not used here: it preserves the diff --git a/pkg/component/flush_owned_fields_test.go b/pkg/component/flush_owned_fields_test.go new file mode 100644 index 00000000..692315bc --- /dev/null +++ b/pkg/component/flush_owned_fields_test.go @@ -0,0 +1,513 @@ +package component + +import ( + "context" + "encoding/json" + "testing" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/api/meta" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// These specs run against envtest because they depend on what the API server +// does with a merge patch: null removes a field, a resourceVersion in the body +// is a precondition, and a structural schema prunes unknown fields. +var _ = Describe("FlushStatus with WithOwnedStatusFields", func() { + var ( + ctx = context.Background() + namespace string + ) + + infraComponent := func() *Component { + GinkgoHelper() + comp, err := NewComponentBuilder().WithName("infra").WithConditionType("InfraReady").Build() + Expect(err).NotTo(HaveOccurred()) + return comp + } + + // readThenRace creates the owner with the given status and reads it as a + // controller does at the top of a reconcile. It then applies the second + // writer's change on the server, so the returned owner is stale and its + // first write conflicts. + readThenRace := func(status MockStatus, secondWriter func(*MockOperatorCRD)) *MockOperatorCRD { + GinkgoHelper() + created := &MockOperatorCRD{ObjectMeta: metav1.ObjectMeta{Name: "owner", Namespace: namespace}} + Expect(k8sClient.Create(ctx, created)).To(Succeed()) + created.Status = status + Expect(k8sClient.Status().Update(ctx, created)).To(Succeed()) + + owner := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(created), owner)).To(Succeed()) + + if secondWriter != nil { + other := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(created), other)).To(Succeed()) + secondWriter(other) + Expect(k8sClient.Status().Update(ctx, other)).To(Succeed()) + } + return owner + } + + persisted := func(owner *MockOperatorCRD) *MockOperatorCRD { + GinkgoHelper() + live := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), live)).To(Succeed()) + return live + } + + condition := func(ctype, reason string) metav1.Condition { + return metav1.Condition{ + Type: ctype, Status: metav1.ConditionTrue, Reason: reason, LastTransitionTime: metav1.Now(), + } + } + + BeforeEach(func() { + namespace = createNamespace(ctx, "flush-owned-fields-") + }) + + It("keeps a second writer's observedGeneration across the conflict retry", func() { + owner := readThenRace(MockStatus{ObservedGeneration: 1}, func(other *MockOperatorCRD) { + other.Status.ObservedGeneration = 2 + }) + recording := &patchRecordingClient{Client: k8sClient} + + owner.Status.Phase = "Accepted" + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, []*Component{infraComponent()}, + WithOwnedStatusFields("phase"), + )).To(Succeed()) + + Expect(recording.bodies).To(HaveLen(2), "the first patch must conflict and the retry must succeed") + live := persisted(owner) + Expect(live.Status.ObservedGeneration).To(Equal(int64(2))) + Expect(live.Status.Phase).To(Equal("Accepted")) + Expect(meta.FindStatusCondition(live.Status.Conditions, "InfraReady")).NotTo(BeNil()) + }) + + It("sends only the declared fields and the conditions, even without a conflict", func() { + owner := readThenRace(MockStatus{ObservedGeneration: 1}, nil) + recording := &patchRecordingClient{Client: k8sClient} + + owner.Status.ObservedGeneration = 99 // staged but not declared + owner.Status.Phase = "Accepted" + readVersion := owner.GetResourceVersion() + + Expect(FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(Succeed()) + + Expect(recording.bodies).To(HaveLen(1)) + var body map[string]any + Expect(json.Unmarshal(recording.bodies[0], &body)).To(Succeed()) + Expect(body).To(HaveKey("metadata")) + Expect(body["metadata"]).To(Equal(map[string]any{"resourceVersion": readVersion})) + Expect(body["status"]).To(HaveKeyWithValue("phase", "Accepted")) + Expect(body["status"]).NotTo(HaveKey("observedGeneration")) + + live := persisted(owner) + Expect(live.Status.ObservedGeneration).To(Equal(int64(1)), "an undeclared field is never written") + Expect(live.Status.Phase).To(Equal("Accepted")) + }) + + It("leaves the server's state in memory, not undeclared staged values", func() { + owner := readThenRace(MockStatus{}, nil) + + owner.Status.ObservedGeneration = 99 // undeclared, and absent on the server + owner.Status.Summary = &MockStatusSummary{Ready: 5} + owner.Status.Phase = "Accepted" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(Succeed()) + + Expect(owner.Status.ObservedGeneration).To(BeZero()) + Expect(owner.Status.Summary).To(BeNil()) + Expect(owner.Status.Phase).To(Equal("Accepted")) + }) + + It("removes a child that the staged owner dropped from a wholly declared object", func() { + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1, Total: 3}}, nil) + + owner.Status.Summary = &MockStatusSummary{Ready: 2} // total cleared, so omitempty drops it + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2})) + }) + + It("removes a dropped child against the server's version after a conflict", func() { + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1}}, nil) + // The competing write lands after FlushStatus read the server's copy + // and before its first patch, so that patch conflicts and the retry + // must compute the removals against the newer copy. + recording := &patchRecordingClient{Client: k8sClient, beforeFirstPatch: func() { + other := &MockOperatorCRD{} + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), other)).To(Succeed()) + other.Status.Summary.Total = 4 + Expect(k8sClient.Status().Update(ctx, other)).To(Succeed()) + }} + + owner.Status.Summary = &MockStatusSummary{Ready: 2} + + Expect(FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(recording.bodies).To(HaveLen(2), "the first patch must conflict and the retry must succeed") + Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2}), + "the declared object is the staged one, so the other writer's total is removed") + }) + + It("keeps a staged unowned condition when a declared object needs the server's copy", func() { + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1}}, nil) + + owner.Status.Summary = &MockStatusSummary{Ready: 2} + // An owner-level aggregate, owned by no component and not yet on the server. + meta.SetStatusCondition(&owner.Status.Conditions, metav1.Condition{ + Type: "Ready", Status: metav1.ConditionTrue, Reason: "AllReady", + }) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(meta.FindStatusCondition(persisted(owner).Status.Conditions, "Ready")).NotTo(BeNil(), + "without a conflict the staged conditions are written as staged") + }) + + It("removes a declared field that the staged owner cleared", func() { + owner := readThenRace(MockStatus{Phase: "Pending", ObservedGeneration: 1}, func(other *MockOperatorCRD) { + other.Status.ObservedGeneration = 2 + }) + + owner.Status.Phase = "" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(Succeed()) + + live := persisted(owner) + Expect(live.Status.Phase).To(BeEmpty()) + Expect(live.Status.ObservedGeneration).To(Equal(int64(2))) + }) + + It("writes a nested declared field and leaves its siblings to the server", func() { + owner := readThenRace( + MockStatus{Summary: &MockStatusSummary{Ready: 1, Total: 3}}, + func(other *MockOperatorCRD) { other.Status.Summary.Total = 4 }, + ) + + owner.Status.Summary.Ready = 2 + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary.ready"), + )).To(Succeed()) + + Expect(persisted(owner).Status.Summary).To(Equal(&MockStatusSummary{Ready: 2, Total: 4})) + }) + + It("removes an explicitly stored zero child of a wholly declared object", func() { + owner := readThenRace(MockStatus{Summary: &MockStatusSummary{Ready: 1}}, nil) + // A typed write drops total: 0 through omitempty, so store it raw. + Expect(k8sClient.Status().Patch(ctx, owner, client.RawPatch( + types.MergePatchType, []byte(`{"status":{"summary":{"ready":1,"total":0}}}`), + ))).To(Succeed()) + Expect(rawStatus(ctx, owner)["summary"]).To(HaveKey("total")) + + owner.Status.Summary = &MockStatusSummary{Ready: 2} + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary"), + )).To(Succeed()) + + Expect(rawStatus(ctx, owner)["summary"]).To(Equal(map[string]any{"ready": int64(2)})) + }) + + It("leaves an absent parent absent when a declared child has nothing to remove", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("summary.ready"), + )).To(Succeed()) + + Expect(rawStatus(ctx, owner)).NotTo(HaveKey("summary")) + }) + + It("does not write into a replacement created under the same name", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + + Expect(k8sClient.Delete(ctx, owner.DeepCopy())).To(Succeed()) + replacement := &MockOperatorCRD{ + ObjectMeta: metav1.ObjectMeta{Name: owner.Name, Namespace: owner.Namespace}, + } + Expect(k8sClient.Create(ctx, replacement)).To(Succeed()) + Expect(replacement.UID).NotTo(Equal(owner.UID)) + + owner.Status.Phase = "Accepted" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phase"), + )).To(MatchError(ContainSubstring("was replaced"))) + + Expect(persisted(owner).Status.Phase).To(BeEmpty(), "the replacement must stay as it was created") + }) + + It("lets owned conditions win and unowned conditions follow the server on a retry", func() { + owner := readThenRace( + MockStatus{Conditions: []metav1.Condition{ + condition("InfraReady", "StaleServerValue"), + condition("ExternalReady", "BeforeOtherWrite"), + }}, + func(other *MockOperatorCRD) { + meta.SetStatusCondition(&other.Status.Conditions, metav1.Condition{ + Type: "ExternalReady", Status: metav1.ConditionFalse, Reason: "WrittenByOtherController", + }) + meta.SetStatusCondition(&other.Status.Conditions, condition("AddedByOther", "BrandNew")) + }, + ) + + applyStatusCondition(ReconcileContext{Owner: owner}, Condition{ + Type: "InfraReady", Status: metav1.ConditionTrue, Reason: "Healthy", + }) + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, []*Component{infraComponent()}, + WithOwnedStatusFields(), + )).To(Succeed()) + + live := persisted(owner) + Expect(meta.FindStatusCondition(live.Status.Conditions, "InfraReady").Reason).To(Equal("Healthy")) + Expect(meta.FindStatusCondition(live.Status.Conditions, "ExternalReady").Reason). + To(Equal("WrittenByOtherController")) + Expect(meta.FindStatusCondition(live.Status.Conditions, "AddedByOther")).NotTo(BeNil()) + }) + + It("does nothing for a misspelled path, because the schema prunes the unknown field", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + + owner.Status.Phase = "Accepted" + + Expect(FlushStatus( + ctx, ReconcileContext{Client: k8sClient, Owner: owner}, nil, WithOwnedStatusFields("phsae"), + )).To(Succeed()) + + Expect(persisted(owner).Status.Phase).To(Equal("Pending"), "the declared path names no field, so nothing is written") + }) + + It("fails without writing for a declared path below a scalar the staged owner holds", func() { + owner := readThenRace(MockStatus{Phase: "Pending"}, nil) + recording := &patchRecordingClient{Client: k8sClient} + + err := FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields("phase.value"), + ) + + Expect(err).To(MatchError(ContainSubstring(`"phase.value"`))) + Expect(recording.bodies).To(BeEmpty()) + Expect(persisted(owner).Status.Phase).To(Equal("Pending")) + }) + + DescribeTable("rejects invalid path syntax before any write", + func(path string) { + owner := readThenRace(MockStatus{}, nil) + recording := &patchRecordingClient{Client: k8sClient} + + err := FlushStatus( + ctx, ReconcileContext{Client: recording, Owner: owner}, nil, WithOwnedStatusFields(path), + ) + + Expect(err).To(MatchError(ContainSubstring(`"` + path + `"`))) + Expect(recording.bodies).To(BeEmpty()) + }, + Entry("empty", ""), + Entry("empty segment", "summary..ready"), + Entry("leading separator", ".phase"), + Entry("trailing separator", "phase."), + Entry("the conditions", "conditions"), + Entry("below the conditions", "conditions.type"), + ) +}) + +func TestConditionsJSONPath(t *testing.T) { + t.Run("finds the conditions directly under status", func(t *testing.T) { + path, err := conditionsJSONPath(&MockOperatorCRD{}) + + require.NoError(t, err) + assert.Equal(t, []string{"conditions"}, path) + }) + + t.Run("finds conditions nested below status under any JSON name", func(t *testing.T) { + path, err := conditionsJSONPath(&nestedConditionsOwner{}) + + require.NoError(t, err) + assert.Equal(t, []string{"health", "checks"}, path) + }) + + t.Run("leaves the staged conditions as they were", func(t *testing.T) { + staged := []metav1.Condition{{Type: "InfraReady", Reason: "Healthy"}} + owner := &MockOperatorCRD{Status: MockStatus{Conditions: staged}} + + _, err := conditionsJSONPath(owner) + + require.NoError(t, err) + assert.Equal(t, staged, owner.Status.Conditions) + }) +} + +func TestParseOwnedStatusPaths(t *testing.T) { + conditions := []string{"health", "checks"} + + t.Run("splits a valid path into its segments", func(t *testing.T) { + paths, err := parseOwnedStatusPaths([]string{"phase", "health.score"}, conditions) + + require.NoError(t, err) + assert.Equal(t, [][]string{{"phase"}, {"health", "score"}}, paths) + }) + + for name, path := range map[string]string{ + "the conditions": "health.checks", + "a path that contains them": "health", + "a path below them": "health.checks.type", + } { + t.Run("rejects "+name, func(t *testing.T) { + _, err := parseOwnedStatusPaths([]string{path}, conditions) + + require.Error(t, err) + assert.Contains(t, err.Error(), `"`+path+`"`) + }) + } +} + +// rawStatus reads the owner's status as the server stores it, where a typed +// read cannot tell a stored zero from an absent field. +func rawStatus(ctx context.Context, owner *MockOperatorCRD) map[string]any { + GinkgoHelper() + live := &unstructured.Unstructured{} + live.SetGroupVersionKind(GroupVersion.WithKind("MockOperatorCRD")) + Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(owner), live)).To(Succeed()) + status, _ := live.Object["status"].(map[string]any) + return status +} + +func TestOwnedStatusPatch(t *testing.T) { + owner := &MockOperatorCRD{Status: MockStatus{Summary: &MockStatusSummary{Ready: 2}}} + conditionsPath := []string{"conditions"} + + statusOf := func(t *testing.T, body []byte) map[string]any { + t.Helper() + var patch map[string]any + require.NoError(t, json.Unmarshal(body, &patch)) + status, _ := patch["status"].(map[string]any) + return status + } + + t.Run("does not depend on the order of overlapping declarations", func(t *testing.T) { + server := map[string]any{"summary": map[string]any{"ready": int64(1)}} + forward, err := parseOwnedStatusPaths([]string{"summary", "summary.details.ready"}, conditionsPath) + require.NoError(t, err) + backward, err := parseOwnedStatusPaths([]string{"summary.details.ready", "summary"}, conditionsPath) + require.NoError(t, err) + + forwardBody, err := ownedStatusPatch(owner, server, forward, conditionsPath) + require.NoError(t, err) + backwardBody, err := ownedStatusPatch(owner, server, backward, conditionsPath) + require.NoError(t, err) + + assert.Equal(t, statusOf(t, forwardBody), statusOf(t, backwardBody)) + assert.Equal(t, map[string]any{"ready": float64(2)}, statusOf(t, forwardBody)["summary"]) + }) + + for name, server := range map[string]map[string]any{ + "absent": {}, + "null": {"summary": nil}, + } { + t.Run("sends nothing to remove below a parent the server holds as "+name, func(t *testing.T) { + stagedWithout := &MockOperatorCRD{} + paths, err := parseOwnedStatusPaths([]string{"summary.ready"}, conditionsPath) + require.NoError(t, err) + + body, err := ownedStatusPatch(stagedWithout, server, paths, conditionsPath) + require.NoError(t, err) + + assert.NotContains(t, statusOf(t, body), "summary") + }) + } +} + +// patchRecordingClient records the body of every status patch it forwards. +type patchRecordingClient struct { + client.Client + bodies [][]byte + // beforeFirstPatch, when set, runs once before the first patch is forwarded. + beforeFirstPatch func() +} + +func (c *patchRecordingClient) Status() client.SubResourceWriter { + return &patchRecordingStatusWriter{SubResourceWriter: c.Client.Status(), parent: c} +} + +type patchRecordingStatusWriter struct { + client.SubResourceWriter + parent *patchRecordingClient +} + +func (w *patchRecordingStatusWriter) Patch( + ctx context.Context, obj client.Object, patch client.Patch, opts ...client.SubResourcePatchOption, +) error { + body, err := patch.Data(obj) + if err != nil { + return err + } + if w.parent.beforeFirstPatch != nil && len(w.parent.bodies) == 0 { + w.parent.beforeFirstPatch() + } + w.parent.bodies = append(w.parent.bodies, body) + return w.SubResourceWriter.Patch(ctx, obj, patch, opts...) +} + +// nestedConditionsOwner keeps its conditions below status.health.checks, so +// the conditions JSON path has more than one segment. +type nestedConditionsOwner struct { + metav1.TypeMeta `json:",inline"` + metav1.ObjectMeta `json:"metadata,omitempty"` + + Status nestedConditionsStatus `json:"status,omitempty"` +} + +type nestedConditionsStatus struct { + Phase string `json:"phase,omitempty"` + Health healthStatus `json:"health,omitzero"` +} + +type healthStatus struct { + Checks []metav1.Condition `json:"checks,omitempty"` +} + +func (o *nestedConditionsOwner) GetStatusConditions() *[]metav1.Condition { + return &o.Status.Health.Checks +} + +func (o *nestedConditionsOwner) GetKind() string { + return "nestedConditionsOwner" +} + +func (o *nestedConditionsOwner) DeepCopyObject() runtime.Object { + out := *o + o.ObjectMeta.DeepCopyInto(&out.ObjectMeta) //nolint:staticcheck // naming ObjectMeta keeps the copy readable + out.Status.Health.Checks = append([]metav1.Condition(nil), o.Status.Health.Checks...) + return &out +} diff --git a/pkg/component/status_patch.go b/pkg/component/status_patch.go new file mode 100644 index 00000000..8369d26b --- /dev/null +++ b/pkg/component/status_patch.go @@ -0,0 +1,391 @@ +package component + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "fmt" + "maps" + "slices" + "strings" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/apiutil" +) + +// FlushOption configures a [FlushStatus] call. +type FlushOption func(*flushOptions) + +type flushOptions struct { + // fieldsDeclared is set by any WithOwnedStatusFields option, including one + // with no paths: declaring that a flush owns no field differs from not + // declaring at all. + fieldsDeclared bool + ownedFields []string +} + +// conditionsMarker is the condition type staged on the owner while +// conditionsJSONPath looks for it in the owner's JSON. +const conditionsMarker = "ocf.sourcehawk.io/flush-status-conditions-marker" + +// WithOwnedStatusFields declares the non-condition status fields that this +// flush owns, for an owner whose status is shared with other writers. It turns +// every write of [FlushStatus] into a JSON merge patch of the status that holds +// only the declared fields and the conditions, so a field that another writer +// owns is never sent, on the first attempt or on a retry. Each attempt first +// reads the owner's status through rec.APIReader when set, and through +// rec.Client otherwise; a cache-backed rec.Client can return a stale status. +// +// Each path names a field under status by its JSON name, with nested fields +// separated by dots, for example "observedGeneration" or "summary.ready". +// Paths are relative to status: write "phase", not "status.phase". A declared +// field takes the staged owner's value, and a declared field that the staged +// owner does not carry is removed on the server. A declared object is owned +// whole: a child that the staged owner does not carry is removed too. +// +// Calling it with no paths declares that the flush owns no non-condition status +// field, as for a controller that writes only conditions. Paths from several +// options are combined. +// +// FlushStatus returns an error without writing when a path has an empty +// segment, names the conditions, contains them, or lies below them; condition +// ownership comes from the components passed to FlushStatus. It also fails +// when a declared path runs below a field that the staged owner holds as a +// non-object value. The framework does not check a path against the CRD +// schema: with a structural schema the API server prunes a misspelled field, +// so that part of the write does nothing. +func WithOwnedStatusFields(paths ...string) FlushOption { + return func(o *flushOptions) { + o.fieldsDeclared = true + o.ownedFields = append(o.ownedFields, paths...) + } +} + +// patchOwnedStatus writes the declared fields and the conditions of rec.Owner +// with a merge patch guarded by the staged resourceVersion, and retries on a +// conflict after refreshing the staged owner's resourceVersion and unowned +// conditions from the server. +func patchOwnedStatus( + ctx context.Context, rec ReconcileContext, ownedConditions map[string]struct{}, rawPaths []string, +) error { + invalid := func(err error) error { + return fmt.Errorf( + "flushing status of %s %s: %w", rec.Owner.GetKind(), client.ObjectKeyFromObject(rec.Owner), err, + ) + } + + conditionsPath, err := conditionsJSONPath(rec.Owner) + if err != nil { + return invalid(err) + } + + paths, err := parseOwnedStatusPaths(rawPaths, conditionsPath) + if err != nil { + return invalid(err) + } + + staged, err := statusContent(rec.Owner) + if err != nil { + return invalid(err) + } + + // Read every declared path before any request, so a path that cannot be + // read from the staged owner fails without touching the server. + for _, path := range paths { + if _, err := valueAt(staged, path); err != nil { + return invalid(fmt.Errorf("owned status field %q: %w", strings.Join(path, "."), err)) + } + } + + server, err := readServerStatus(ctx, rec) + if err != nil { + return err + } + + body, err := ownedStatusPatch(rec.Owner, server, paths, conditionsPath) + if err != nil { + return invalid(err) + } + + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + patchErr := rec.Client.Status().Patch(ctx, rec.Owner, client.RawPatch(types.MergePatchType, body)) + if patchErr == nil || !apierrors.IsConflict(patchErr) { + return patchErr + } + + if refreshErr := refreshUnownedConditions(ctx, rec, ownedConditions); refreshErr != nil { + return refreshErr + } + + // Read the status after the refresh, so it is at least as new as the + // resourceVersion the patch is guarded by. + var readErr error + if server, readErr = readServerStatus(ctx, rec); readErr != nil { + return readErr + } + + var buildErr error + if body, buildErr = ownedStatusPatch(rec.Owner, server, paths, conditionsPath); buildErr != nil { + return buildErr + } + return patchErr + }) +} + +// readServerStatus returns the status that the server stores for the owner. +// It reads into an unstructured object, because a typed read drops a stored +// zero value that omitempty skips, and the patch must still remove it. +func readServerStatus(ctx context.Context, rec ReconcileContext) (map[string]any, error) { + gvk, err := apiutil.GVKForObject(rec.Owner, rec.Client.Scheme()) + if err != nil { + return nil, err + } + + live := &unstructured.Unstructured{} + live.SetGroupVersionKind(gvk) + if err := liveReader(rec).Get(ctx, client.ObjectKeyFromObject(rec.Owner), live); err != nil { + return nil, err + } + + // Status().Update sends the uid and the API server rejects a mismatch. A + // merge patch does not, so a retry would patch an owner that was deleted + // and recreated under the same name, taking its resourceVersion. + if staged := rec.Owner.GetUID(); staged != "" && live.GetUID() != staged { + return nil, fmt.Errorf( + "owner %s was replaced: the server holds uid %s, the staged owner has uid %s", + client.ObjectKeyFromObject(rec.Owner), live.GetUID(), staged, + ) + } + + status, _ := live.Object["status"].(map[string]any) + return status, nil +} + +// conditionsJSONPath returns the JSON path under status of the list that +// GetStatusConditions returns. OperatorCRD does not fix that list's JSON name +// or depth, so a marker condition is staged for a moment and found in the +// owner's JSON. +func conditionsJSONPath(owner OperatorCRD) ([]string, error) { + conditions := owner.GetStatusConditions() + staged := *conditions + *conditions = []metav1.Condition{{Type: conditionsMarker}} + data, err := json.Marshal(owner) + *conditions = staged + if err != nil { + return nil, fmt.Errorf("encoding %T: %w", owner, err) + } + + var content map[string]any + if err := json.Unmarshal(data, &content); err != nil { + return nil, fmt.Errorf("decoding %T: %w", owner, err) + } + + status, _ := content["status"].(map[string]any) + path, found := findConditionsMarker(status) + if !found { + return nil, fmt.Errorf("%T does not encode its status conditions under status", owner) + } + return path, nil +} + +func findConditionsMarker(object map[string]any) ([]string, bool) { + for key, value := range object { + switch typed := value.(type) { + case []any: + if len(typed) != 1 { + continue + } + if item, ok := typed[0].(map[string]any); ok && item["type"] == conditionsMarker { + return []string{key}, true + } + case map[string]any: + if path, found := findConditionsMarker(typed); found { + return append([]string{key}, path...), true + } + } + } + return nil, false +} + +func parseOwnedStatusPaths(paths []string, conditionsPath []string) ([][]string, error) { + parsed := make([][]string, 0, len(paths)) + for _, path := range paths { + segments := strings.Split(path, ".") + if slices.Contains(segments, "") { + return nil, fmt.Errorf("owned status field %q: path has an empty segment", path) + } + + // The patch writes the conditions under their own path, so a declared + // path that overlaps it would collide with that write. + if isPrefix(segments, conditionsPath) || isPrefix(conditionsPath, segments) { + return nil, fmt.Errorf( + "owned status field %q: overlaps the conditions at %q, which are owned through the components passed to FlushStatus", + path, strings.Join(conditionsPath, "."), + ) + } + parsed = append(parsed, segments) + } + + // A declared ancestor writes its whole subtree. A descendant written into + // it as well would make the patch depend on the order of the paths. + var kept [][]string + for i, path := range parsed { + covered := slices.ContainsFunc(parsed[:i], func(other []string) bool { return isPrefix(other, path) }) || + slices.ContainsFunc(parsed[i+1:], func(other []string) bool { + return len(other) < len(path) && isPrefix(other, path) + }) + if !covered { + kept = append(kept, path) + } + } + return kept, nil +} + +func isPrefix(prefix, path []string) bool { + return len(prefix) <= len(path) && slices.Equal(prefix, path[:len(prefix)]) +} + +// ownedStatusPatch returns the merge patch body that takes each declared path +// and the conditions from the server's status to the staged owner's, with the +// staged resourceVersion as a precondition. +func ownedStatusPatch(owner OperatorCRD, server map[string]any, paths [][]string, conditionsPath []string) ([]byte, error) { + staged, err := statusContent(owner) + if err != nil { + return nil, err + } + + status := map[string]any{} + for _, path := range append(slices.Clone(paths), conditionsPath) { + value, err := valueAt(staged, path) + if err != nil { + return nil, fmt.Errorf("owned status field %q: %w", strings.Join(path, "."), err) + } + + // A server value below a non-object parent counts as absent. + current, _ := valueAt(server, path) + switch typed := value.(type) { + case nil: + // A null below a parent the server does not hold as an object + // would create that parent, so remove only what is there. + if !holds(server, path) { + continue + } + case map[string]any: + value = withRemovals(typed, current) + } + setPatchValue(status, path, value) + } + + patch := map[string]any{"status": status} + if resourceVersion := owner.GetResourceVersion(); resourceVersion != "" { + patch["metadata"] = map[string]any{"resourceVersion": resourceVersion} + } + return json.Marshal(patch) +} + +// holds reports whether status has a key at path, below parents that are all +// objects. +func holds(status map[string]any, path []string) bool { + parent, err := valueAt(status, path[:len(path)-1]) + if err != nil { + return false + } + + object, ok := parent.(map[string]any) + if !ok { + return false + } + _, present := object[path[len(path)-1]] + return present +} + +// statusContent returns the status of obj as JSON values, or nil when obj has +// no status. +func statusContent(obj OperatorCRD) (map[string]any, error) { + data, err := json.Marshal(obj) + if err != nil { + return nil, fmt.Errorf("encoding %T: %w", obj, err) + } + + // json.Number re-encodes a number exactly as it was, so an int64 is not + // rounded through float64. + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + var content map[string]any + if err := decoder.Decode(&content); err != nil { + return nil, fmt.Errorf("decoding %T: %w", obj, err) + } + + status, _ := content["status"].(map[string]any) + return status, nil +} + +// valueAt returns the value at path in status, or nil when status does not +// carry it, which the patch sends as null to remove the field. An absent or +// null parent counts as absent. +func valueAt(status map[string]any, path []string) (any, error) { + var current any = status + for i, segment := range path { + if current == nil { + return nil, nil + } + + object, ok := current.(map[string]any) + if !ok { + return nil, errors.New(strings.Join(path[:i], ".") + " is not an object in the staged status") + } + current = object[segment] + } + return current, nil +} + +// withRemovals returns staged with null for each key that server holds and +// staged does not, at any depth, so the merge patch removes it. +func withRemovals(staged map[string]any, server any) map[string]any { + serverObject, ok := server.(map[string]any) + if !ok { + return staged + } + + result := maps.Clone(staged) + for key, serverValue := range serverObject { + value, present := result[key] + if !present { + result[key] = nil + continue + } + if child, ok := value.(map[string]any); ok { + result[key] = withRemovals(child, serverValue) + } + } + return result +} + +// setPatchValue sets value at path in the patch, creating parent objects. A +// parent already set to a non-object value by a shorter declared path covers +// the longer one, so the longer path is skipped. +func setPatchValue(patch map[string]any, path []string, value any) { + current := patch + for _, segment := range path[:len(path)-1] { + next, exists := current[segment] + if !exists { + child := map[string]any{} + current[segment] = child + current = child + continue + } + + child, ok := next.(map[string]any) + if !ok { + return + } + current = child + } + current[path[len(path)-1]] = value +} diff --git a/pkg/component/zz_mock_crd_test.go b/pkg/component/zz_mock_crd_test.go index 2e0173ee..380f8f41 100644 --- a/pkg/component/zz_mock_crd_test.go +++ b/pkg/component/zz_mock_crd_test.go @@ -37,6 +37,15 @@ type MockStatus struct { // prove that FlushStatus preserves staged status fields the framework does // not manage, including across a conflict retry. ObservedGeneration int64 `json:"observedGeneration,omitempty"` + // Phase and Summary are further non-condition status fields, so tests can + // split status ownership between controllers by field and by nested path. + Phase string `json:"phase,omitempty"` + Summary *MockStatusSummary `json:"summary,omitempty"` +} + +type MockStatusSummary struct { + Ready int32 `json:"ready,omitempty"` + Total int32 `json:"total,omitempty"` } func (m *MockOperatorCRD) GetStatusConditions() *[]metav1.Condition { @@ -90,6 +99,14 @@ var ( Type: "object", Properties: map[string]apiextensionsv1.JSONSchemaProps{ "observedGeneration": {Type: "integer", Format: "int64"}, + "phase": {Type: "string"}, + "summary": { + Type: "object", + Properties: map[string]apiextensionsv1.JSONSchemaProps{ + "ready": {Type: "integer", Format: "int32"}, + "total": {Type: "integer", Format: "int32"}, + }, + }, "conditions": { Type: "array", Items: &apiextensionsv1.JSONSchemaPropsOrArray{ @@ -130,6 +147,10 @@ func (m *MockOperatorCRD) DeepCopyInto(out *MockOperatorCRD) { *out = make([]metav1.Condition, len(*in)) copy(*out, *in) } + if m.Status.Summary != nil { + summary := *m.Status.Summary + out.Status.Summary = &summary + } } func (m *MockOperatorCRD) DeepCopy() *MockOperatorCRD { diff --git a/plugin/skills/building-components/SKILL.md b/plugin/skills/building-components/SKILL.md index d4d42c51..e7cd44c6 100644 --- a/plugin/skills/building-components/SKILL.md +++ b/plugin/skills/building-components/SKILL.md @@ -233,7 +233,8 @@ the list. This split lets a controller with several components stage several con 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. +top of every reconcile: an owner carried over from an earlier pass writes stale values back over newer ones. With +`WithOwnedStatusFields` the write is a merge patch of the declared fields and the conditions instead (see below). **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. @@ -250,10 +251,11 @@ 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. +that copy's `resourceVersion`, restores from it only the conditions whose type this flush does not own, and retries. No +other status field is taken from the server. Two consequences: the status fields the flush writes keep their staged +values, 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. Without `WithOwnedStatusFields` the flush writes +every non-condition field, so a retry also writes back a stale copy of another controller's field. 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 @@ -295,6 +297,24 @@ return reconcile.Result{}, component.FlushStatus(ctx, component.ReconcileContext }, nil) // nil: owns no component conditions; on a conflict this follows the server ``` +**A status shared with another controller needs `WithOwnedStatusFields`.** When a second controller writes non-condition +fields of the same status, for example `observedGeneration`, a conflict retry without the option writes this +controller's stale copy of those fields back. Declare the fields this flush writes, by JSON name under `status` with +dots for nested fields: + +```go +component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) +``` + +With the option, every attempt reads the stored status and then sends a JSON merge patch of the status that holds only +the staged `resourceVersion` (as a precondition), the declared fields (a field the staged owner does not carry is sent +as `null` and removed, and a declared object is owned whole, so its dropped children are removed too), and the +conditions. An undeclared field is never sent, so declare every field the controller writes, including +`observedGeneration` when it is this controller's. `WithOwnedStatusFields()` with no paths owns no non-condition field. +`FlushStatus` returns an error without writing for an empty segment, a path that overlaps the conditions, or a path +below a non-object value. A misspelled path is not caught: a structural CRD schema prunes the unknown field, so that +part of the write does nothing. + **Server-side apply is for managed resources, never for the owner's status subresource.** An SSA status patch alongside `FlushStatus` puts two field managers on one conditions list, and because `FlushStatus` writes the full status the outcome depends on which call runs last: the `Update` can clobber the patched condition, or the patch can clobber what diff --git a/plugin/skills/building-components/references/component.md b/plugin/skills/building-components/references/component.md index 185ef7be..0c1de1f1 100644 --- a/plugin/skills/building-components/references/component.md +++ b/plugin/skills/building-components/references/component.md @@ -818,22 +818,28 @@ func (r *WebAppReconciler) Reconcile(ctx context.Context, req reconcile.Request) 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. +writes stale values back over newer ones. With +[`WithOwnedStatusFields`](#sharing-the-owners-status-with-another-controller), the write is a merge patch of the +declared fields and the conditions instead. 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. +type this flush does not own, and retries. The staged owner is never replaced, and no other status field is taken from +the server. 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. -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. +Two things follow from that. A status field that the flush writes keeps its staged value across a conflict, because the +object holding it 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. +Without `WithOwnedStatusFields`, the flush writes every non-condition status field, so a retry also writes back the +stale copy of a field that another controller changed after the fetch. When another controller writes non-condition +fields of the same status, see +[Sharing the owner's status with another controller](#sharing-the-owners-status-with-another-controller). Which types count as owned comes from the components passed to `FlushStatus`: @@ -920,6 +926,61 @@ Server-Side Apply is for managed resources, never for the owner's status subreso depends on which call runs last: the `Update` can overwrite the patched condition, or the patch can overwrite what the `Update` wrote. Keep every owner-status write on the `meta.SetStatusCondition` plus `FlushStatus` path. +### Sharing the owner's status with another controller + +Sometimes two controllers write the status of one CR. For example, a status-only controller owns an `Accepted` +condition, and a second controller owns a `Ready` condition and `status.observedGeneration`. Each controller still +writes through `FlushStatus`. Condition ownership comes from the components passed to it. For non-condition fields, pass +`component.WithOwnedStatusFields` with the fields that this controller writes: + +```go +// In the status-only controller, which writes status.phase and no other non-condition field: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("phase")) + +// In the other controller, which writes status.observedGeneration: +flushErr := component.FlushStatus(ctx, recCtx, comps, component.WithOwnedStatusFields("observedGeneration")) +``` + +With the option, every attempt of `FlushStatus` is a JSON merge patch of the status instead of a `Status().Update`. The +patch holds only: + +- the staged owner's `metadata.resourceVersion`, as a precondition: the API server rejects the patch with a 409 when the + owner changed after the controller read it; +- each declared field with the staged owner's value, or `null` when the staged owner does not carry the field but the + server does, so the server removes it. A declared object is owned whole: the patch sends `null` for each child that + the server stores and the staged owner does not carry, so a child that `omitempty` dropped is removed too; +- the conditions list as staged. On the first attempt that includes any unowned condition the controller staged by hand, + such as an owner-level aggregate; the precondition makes the patch fail if the server changed after the read. After a + 409, the unowned conditions are first refreshed from the server. + +To build the patch, each attempt first reads the owner's status through `APIReader` when it is set, and through `Client` +otherwise. A cache-backed `Client` can return a stale status, so set `APIReader`. When that read finds an owner with +another UID, because the owner was deleted and recreated under the same name after the controller read it, `FlushStatus` +returns an error and does not write, as `Status().Update` would refuse the write. A field that the controller did not +declare is never sent, on the first attempt or on a retry, so it cannot overwrite another writer's value. On a 409, +`FlushStatus` refreshes the `resourceVersion` and the unowned conditions from the server, as described above, builds the +patch again, and retries. After a successful write, the controller-runtime client decodes the server's response into the +owner, so a value staged in a field that you did not declare does not survive in memory. + +Name each field by its JSON name under `status`, with dots between nested fields: `"observedGeneration"`, +`"summary.ready"`. Do not include the `status.` prefix. `WithOwnedStatusFields()` with no paths declares that the flush +owns no non-condition field, which fits a controller that writes only conditions. Paths from several options are +combined. Declare every non-condition field that the controller writes, including `observedGeneration` when it is this +controller's field: a field that you stage but do not declare is not written. + +`FlushStatus` returns an error, with no write, when a path: + +- is empty or has an empty segment, such as `"summary..ready"`; +- names the conditions, contains them, or lies below them, because conditions are owned through components. + `FlushStatus` finds the JSON path of the list that `GetStatusConditions` returns, so this holds whatever that field is + called; +- runs below a field that the staged owner holds as a value that is not an object, such as `"phase.value"` when `phase` + is a string. + +The framework does not check a path against the CRD schema. A misspelled path, such as `"phsae"`, sends a field that the +schema does not declare. With a structural schema the API server prunes that field, so that part of the write does +nothing and `FlushStatus` returns no error. Check each path against the CRD type when you add it. + ### Where the owner's observedGeneration is set The framework sets `ObservedGeneration` on every **component condition** it writes, from `owner.GetGeneration()`. It @@ -928,7 +989,9 @@ never writes `status.observedGeneration` on the owner itself. `OperatorCRD` requ An owner-level `status.observedGeneration` is the controller's own field. Assign it on the owner before `FlushStatus` runs, in the same staging step that writes the owner's own condition, and the single status write persists both. This -applies to a status-only controller too, as in the validation-only example above. +applies to a status-only controller too, as in the validation-only example above. When the status is +[shared with another controller](#sharing-the-owners-status-with-another-controller), only the controller that owns +`observedGeneration` assigns it, and that controller includes it in `WithOwnedStatusFields`. ```go app.Status.ObservedGeneration = app.Generation diff --git a/plugin/skills/structuring-operators/SKILL.md b/plugin/skills/structuring-operators/SKILL.md index cd4b82e5..e462aad3 100644 --- a/plugin/skills/structuring-operators/SKILL.md +++ b/plugin/skills/structuring-operators/SKILL.md @@ -101,6 +101,9 @@ Consequences worth holding on to: them with `meta.RemoveStatusCondition` when a component is retired. - **Set the owner's `status.observedGeneration` yourself.** The framework stamps `ObservedGeneration` on component conditions only. +- **A status shared by two controllers gives each field one owner.** Each controller declares its non-condition fields + with `FlushStatus(..., component.WithOwnedStatusFields("phase"))`, so the flush patches only those fields and the + conditions and never writes the other controller's fields. Only the owner of `observedGeneration` sets it. Full behavior table and reference: `ocf:building-components`, and `references/guidelines.md`, "Derive the Owner's Aggregate Condition from Component Conditions", for the practice framing. diff --git a/plugin/skills/structuring-operators/references/guidelines.md b/plugin/skills/structuring-operators/references/guidelines.md index 8f7e9440..de1e049f 100644 --- a/plugin/skills/structuring-operators/references/guidelines.md +++ b/plugin/skills/structuring-operators/references/guidelines.md @@ -190,6 +190,12 @@ returns an error. If you do not want condition metrics, leave `ReconcileContext.Metrics` as `nil`; `FlushStatus` tolerates a nil recorder and skips metric emission. +When two controllers write the status of one CR, give each status field one owner, as with conditions. Each controller +passes the components that own its conditions and declares its non-condition fields with +[`WithOwnedStatusFields`](../../building-components/references/component.md#sharing-the-owners-status-with-another-controller), +so its flush patches only those fields and the conditions. Without the declaration, the flush writes the whole status, +and a conflict retry writes the controller's stale copy of the other controller's fields back over newer values. + Building the component set from a pure resolver `(spec, version) -> []*component.Component` keeps the loop stable: enabling an optional feature changes which components the resolver returns without touching the reconcile loop.