diff --git a/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md b/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md index 31aa0476..c512f09f 100644 --- a/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md +++ b/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md @@ -6,7 +6,8 @@ - **Recover:** Read saved inventory and restore missing tombstones into the current synthesis. A tombstone is a manifest with `Deleted: true`; normal resource reconciliation performs the downstream deletion. - **Coordinate:** Prevent reconstitution and readiness from advancing before the current synthesis has a terminal recovery decision. - **Record:** Save the current Ready synthesis's inventory as one or more Secrets for future recovery. -- **Not included:** General inventory garbage collection or cleanup on Composition deletion. Recording only rotates older lineage snapshots after writing their replacement. +- **Clean up:** Delete a Composition's lineage inventory once it starts deleting, unless its Symphony is being deleted. +- **Not included:** General inventory garbage collection. Recording only rotates older lineage snapshots after writing their replacement. - **Not guaranteed:** Recovery of every historical resource when inventory is missing or invalid, or atomic behavior across Composition status, ResourceSlices, and downstream resources. - **Related guide:** [Resource Reconciliation](./reconciliation.md) explains ordinary apply, deletion, readiness, and sharding behavior. @@ -65,7 +66,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement - **Inventory matches current identities:** Write no additional tombstones and record `FinishedTombstoneRecovery`. - **Historical identities are missing:** Persist tombstones for those identities and record `FinishedTombstoneRecovery`. - **Operator disabled:** Reconstitution records `TombstoneRecoveryOperatorNotEnabled` for an annotated, synthesized, non-deleting Composition without a terminal decision, then proceeds. -- **Unannotated or deleting Composition:** Bypass recovery gates and retain normal reconciliation/deletion behavior. +- **Unannotated or deleting Composition:** Bypass recovery gates and retain normal reconciliation/deletion behavior. A deleting opted-in Composition also has its lineage inventory deleted unless its Symphony is being deleted. ## Component Responsibilities @@ -82,7 +83,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement - **Does:** Treat current manifests, existing tombstones, and Eno Patch targets as present, using group/kind/namespace/name identity. - **Does:** Record retriable errors or terminal decisions, publishing overflow references together with completion. - **Does:** On a subsequent event, record inventory when the persisted current synthesis is Ready and its recovery decision is finished. -- **Does not:** Change an existing desired manifest into a tombstone, directly delete downstream resources, or garbage-collect inventory Secrets. +- **Does not:** Change an existing desired manifest into a tombstone, directly delete downstream resources, or garbage-collect inventory Secrets for Compositions that are already gone. - **Entry points:** [`Reconcile` and `recoverMissingTombstones`](../../internal/controllers/tombstonerecovery/controller.go), [`missingTombstones`](../../internal/controllers/tombstonerecovery/inventory.go). ### Packing Planner and Writer @@ -101,6 +102,14 @@ Synthesis detects incomplete history or inherits an outstanding requirement - **Does not:** Reopen a finished recovery decision, overwrite different contents under the same synthesis UUID, or perform general orphan garbage collection. - **Entry points:** [`recordInventory` and `writeInventoryChunk`](../../internal/controllers/tombstonerecovery/recording.go). +### Inventory Cleanup + +- **Does:** When an opted-in Composition has a `DeletionTimestamp`, list downstream `kube-system` Secrets by its `eno.azure.io/inventory-lineage` label and delete them all. `NotFound` counts as deleted. +- **Does:** Start as soon as deletion starts, without waiting for dependents, downstream resource deletion, or `CurrentSynthesis.Reconciled`. Retry list and delete errors while the Composition still exists. +- **Does not:** Clean up when the Composition has `eno.azure.io/symphony-deleting: "true"`; Symphony deletion orphans downstream resources, so their inventory is kept. +- **Does not:** Add a finalizer, block Composition deletion, change Composition status, or delete downstream resources. +- **Entry point:** [`cleanupInventory`](../../internal/controllers/tombstonerecovery/cleanup.go). + ### Reconstitution - **Does:** Wait for the current synthesis's terminal recovery decision when recovery is enabled and the Composition is opted in and not deleting. @@ -184,7 +193,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement ### Composition Deletion, Opt-Out, or Reconciler Restart - **Situation:** The Composition begins deleting, loses its opt-in annotation, or the reconciler restarts. -- **Handling:** Deleting/unannotated Compositions bypass recovery gates. Pre-write checks reject observed deletion or opt-out. A restarted controller reconstructs progress from persisted slices and status. +- **Handling:** Deleting/unannotated Compositions bypass recovery gates. Pre-write checks reject observed deletion or opt-out. A deleting opted-in Composition has its lineage inventory deleted unless its Symphony is being deleted. A restarted controller reconstructs progress from persisted slices and status. - **Boundary:** Removing the annotation does not roll back tombstones already written; recovery-specific cleanup protection ends for unreferenced overflow. ### Inventory Recording Fails or Is Superseded @@ -217,6 +226,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement - **Concurrent downstream deletion:** An already-running deletion can overlap a newer synthesis that wants the resource. Recovery status preconditions do not make downstream operations transactional. - **Progress depends on the environment:** Permanent errors, inhibited resynthesis, or continuous synthesis churn can prevent completion. - **Namespace and ownership configuration:** Matching reconciliation/recovery scopes and assigning a single recovery owner per Composition are deployment responsibilities, not guarantees enforced by the annotation. -- **Follow-up implementation:** General orphan inventory garbage collection and Composition-deletion handling. +- **Best-effort inventory cleanup:** Cleanup does not block Composition deletion. If the Composition disappears before cleanup succeeds, its inventory Secrets remain. +- **Follow-up implementation:** General orphan inventory garbage collection. - **Follow-up hardening:** Admission policies restricting unauthorized inventory or ResourceSlice changes, while permitting legitimate cleanup and garbage collection. - **Follow-up validation:** End-to-end coverage is deferred; focused unit regressions exercise recovery decisions, inheritance, partial-write retries, status races, and lifecycle guards. diff --git a/internal/controllers/tombstonerecovery/cleanup.go b/internal/controllers/tombstonerecovery/cleanup.go new file mode 100644 index 00000000..b3bf9209 --- /dev/null +++ b/internal/controllers/tombstonerecovery/cleanup.go @@ -0,0 +1,27 @@ +package tombstonerecovery + +import ( + "context" + "fmt" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/go-logr/logr" + apierrors "k8s.io/apimachinery/pkg/api/errors" +) + +// cleanupInventory deletes every inventory Secret for a deleting Composition's lineage. +// Best effort: it does not block Composition deletion. +func (c *tombstoneRecoveryController) cleanupInventory(ctx context.Context, comp *apiv1.Composition) error { + items, err := c.readInventories(ctx, comp) + if err != nil { + return err + } + for i := range items { + item := &items[i] + if err := c.downstream.Delete(ctx, item); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("deleting inventory Secret %q: %w", item.Name, err) + } + logr.FromContextOrDiscard(ctx).Info("deleted inventory chunk", "secretName", item.Name) + } + return nil +} diff --git a/internal/controllers/tombstonerecovery/controller.go b/internal/controllers/tombstonerecovery/controller.go index ff2a3fb2..f7387b0f 100644 --- a/internal/controllers/tombstonerecovery/controller.go +++ b/internal/controllers/tombstonerecovery/controller.go @@ -40,6 +40,8 @@ const ( compositionOperationRecover compositionOperation = "recoverMissingTombstones" compositionOperationNotNeeded compositionOperation = "recordRecoveryNotNeeded" compositionOperationRecord compositionOperation = "recordInventory" + compositionOperationCleanup compositionOperation = "cleanupInventory" + symphonyDeletingLabel = "eno.azure.io/symphony-deleting" ) var errSuperseded = errors.New("tombstone recovery operation superseded") @@ -141,24 +143,19 @@ func (c *tombstoneRecoveryController) Reconcile(ctx context.Context, req ctrl.Re logger.Error(err, "failed to get composition") return ctrl.Result{}, err } - // Deletion reuses existing slices and must not wait for tombstone recovery. - if comp.DeletionTimestamp != nil { - logger.Info("composition is deleting; skipping recovery and inventory recording", "compositionUID", comp.UID, - "synthesisUUID", comp.Status.GetCurrentSynthesisUUID()) - return ctrl.Result{}, nil - } syn := comp.Status.CurrentSynthesis - if syn == nil || syn.Synthesized == nil { + // Deletion cleanup finds inventory by lineage, so it does not need a synthesis. + if comp.DeletionTimestamp == nil && (syn == nil || syn.Synthesized == nil) { return ctrl.Result{}, nil } - logger = logger.WithValues("compositionUID", comp.UID, "compositionGeneration", comp.Generation, "synthesisUUID", syn.UUID, + logger = logger.WithValues("compositionUID", comp.UID, "compositionGeneration", comp.Generation, "synthesisUUID", comp.Status.GetCurrentSynthesisUUID(), "synthesizerName", comp.Spec.Synthesizer.Name, "operationID", comp.GetAzureOperationID(), "operationOrigin", comp.GetAzureOperationOrigin()) ctx = logr.NewContext(ctx, logger) if !comp.RecoveryEnabled() { logger.Info("composition has not opted into recovery") return ctrl.Result{}, nil } - if syn.UUID == "" { + if comp.DeletionTimestamp == nil && syn.UUID == "" { logger.Error(err, "invalid synthesis") return ctrl.Result{}, fmt.Errorf("published synthesis has no UUID") } @@ -173,6 +170,8 @@ func (c *tombstoneRecoveryController) Reconcile(ctx context.Context, req ctrl.Re err = c.recordRecoveryDecision(ctx, comp, reasonNotNeeded, "", nil) case compositionOperationRecord: err = c.recordInventory(ctx, comp) + case compositionOperationCleanup: + err = c.cleanupInventory(ctx, comp) } if errors.Is(err, errSuperseded) { @@ -187,10 +186,21 @@ func (c *tombstoneRecoveryController) Reconcile(ctx context.Context, req ctrl.Re // selectTombstoneRecoveryOperation selects one idempotent operation without performing side effects. func selectTombstoneRecoveryOperation(comp *apiv1.Composition) compositionOperation { - if comp == nil || comp.Status.CurrentSynthesis == nil || comp.Status.CurrentSynthesis.Synthesized == nil { + if comp == nil { return compositionOperationNone } + // Deletion reuses existing slices and must not wait for tombstone recovery or record inventory. + if comp.DeletionTimestamp != nil { + // Symphony deletion orphans downstream resources, so their inventory is kept. + if comp.Labels[symphonyDeletingLabel] == "true" { + return compositionOperationNone + } + return compositionOperationCleanup + } syn := comp.Status.CurrentSynthesis + if syn == nil || syn.Synthesized == nil { + return compositionOperationNone + } if !syn.IsTombstoneRecoveryFinished() { if syn.TombstoneRecoveryRequired { return compositionOperationRecover diff --git a/internal/controllers/tombstonerecovery/controller_test.go b/internal/controllers/tombstonerecovery/controller_test.go index c726df57..218ec3d0 100644 --- a/internal/controllers/tombstonerecovery/controller_test.go +++ b/internal/controllers/tombstonerecovery/controller_test.go @@ -242,6 +242,20 @@ func TestSelectTombstoneRecoveryOperation(t *testing.T) { }, }}, }, want: compositionOperationRecover}, + {name: "deleting cleans up inventory", comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: ptr.To(metav1.Now())}, + Status: apiv1.CompositionStatus{CurrentSynthesis: &apiv1.Synthesis{ + Synthesized: ptr.To(metav1.Now()), TombstoneRecoveryRequired: true, + }}, + }, want: compositionOperationCleanup}, + {name: "symphony deleting keeps inventory", comp: &apiv1.Composition{ + ObjectMeta: metav1.ObjectMeta{ + DeletionTimestamp: ptr.To(metav1.Now()), Labels: map[string]string{symphonyDeletingLabel: "true"}, + }, + Status: apiv1.CompositionStatus{CurrentSynthesis: &apiv1.Synthesis{ + Synthesized: ptr.To(metav1.Now()), TombstoneRecoveryRequired: true, + }}, + }}, } { t.Run(tc.name, func(t *testing.T) { assert.Equal(t, tc.want, selectTombstoneRecoveryOperation(tc.comp)) @@ -567,19 +581,37 @@ func TestTombstoneRecoveryOperatorEmptyCurrentRecoveryAnnotation(t *testing.T) { } } -func TestTombstoneRecoveryOperatorDeletingSkipsInventory(t *testing.T) { - for _, recording := range []bool{false, true} { - t.Run(fmt.Sprintf("recording-%t", recording), func(t *testing.T) { - f := newControllerTestFixture(t, !recording) +func TestTombstoneRecoveryOperatorDeletingCleansUpInventory(t *testing.T) { + for _, tc := range []struct { + name string + recording bool + change func(*apiv1.Composition) + keep bool + }{ + {name: "recovery-pending"}, + {name: "recording", recording: true}, + {name: "symphony-deleting", keep: true, change: func(comp *apiv1.Composition) { + comp.Labels = map[string]string{symphonyDeletingLabel: "true"} + }}, + {name: "not-opted-in", keep: true, change: func(comp *apiv1.Composition) { + delete(comp.Annotations, "eno.azure.io/recovery-enabled") + }}, + } { + t.Run(tc.name, func(t *testing.T) { + f := newControllerTestFixture(t, !tc.recording) history := f.history("removed") - if recording { + if tc.recording { f.finish(reasonNotNeeded) f.ready() } f.updateStatus(func(syn *apiv1.Synthesis) { syn.ResourceSlices = nil }) + if tc.change != nil { + comp := f.composition() + tc.change(comp) + require.NoError(t, f.upstream.Update(t.Context(), comp)) + } require.NoError(t, f.upstream.Delete(t.Context(), f.composition())) before := f.composition() - f.controller.downstream = nil f.controller.client = interceptor.NewClient(f.upstream, interceptor.Funcs{ SubResourcePatch: func(context.Context, client.Client, string, client.Object, client.Patch, ...client.SubResourcePatchOption) error { t.Error("deleting Composition must not receive tombstone recovery status writes") @@ -588,11 +620,44 @@ func TestTombstoneRecoveryOperatorDeletingSkipsInventory(t *testing.T) { }) require.NoError(t, f.reconcile()) assert.Equal(t, before, f.composition()) - assert.Equal(t, []corev1.Secret{*history}, f.inventories()) + if tc.keep { + assert.Equal(t, []corev1.Secret{*history}, f.inventories()) + } else { + assert.Empty(t, f.inventories()) + } + require.NoError(t, f.reconcile(), "repeated cleanup must be idempotent") }) } } +func TestTombstoneRecoveryOperatorDeletingRetriesInventoryCleanup(t *testing.T) { + f := newControllerTestFixture(t, true) + other := f.history("removed") + other.Name, other.UID, other.ResourceVersion = "unrelated", "unrelated-uid", "" + other.Labels = map[string]string{inventoryLineageLabel: "other-lineage"} + require.NoError(t, f.downstream.Create(t.Context(), other)) + require.NoError(t, f.upstream.Delete(t.Context(), f.composition())) + + injected := fmt.Errorf("injected delete failure") + fail := true + f.controller.downstream = interceptor.NewClient(f.downstream, interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if fail { + return injected + } + return c.Delete(ctx, obj, opts...) + }, + }) + require.ErrorIs(t, f.reconcile(), injected) + assert.Len(t, f.inventories(), 2) + + fail = false + require.NoError(t, f.reconcile()) + items := f.inventories() + require.Len(t, items, 1, "only the deleting Composition's lineage is removed") + assert.Equal(t, "unrelated", items[0].Name) +} + func TestTombstoneRecoveryOperatorObsoleteWork(t *testing.T) { for _, mode := range []string{"already-deleting", "deletion-during-read", "new-synthesis"} { t.Run(mode, func(t *testing.T) { @@ -603,7 +668,6 @@ func TestTombstoneRecoveryOperatorObsoleteWork(t *testing.T) { if mode == "already-deleting" { require.NoError(t, f.upstream.Delete(t.Context(), f.composition())) expected = f.composition() - f.controller.downstream = nil } else { f.controller.downstream = interceptor.NewClient(f.downstream, interceptor.Funcs{ List: func(ctx context.Context, cli client.WithWatch, obj client.ObjectList, opts ...client.ListOption) error { @@ -637,12 +701,16 @@ func TestTombstoneRecoveryOperatorObsoleteWork(t *testing.T) { require.NotNil(t, expected, "the external deletion or supersession must occur") assert.Equal(t, expected, f.composition(), "obsolete recovery must not acknowledge or change the new state") assert.Equal(t, original.Spec, f.slice("desired").Spec) - assert.Equal(t, []corev1.Secret{*old}, f.inventories()) + if mode == "already-deleting" { + assert.Empty(t, f.inventories(), "deleting Composition removes its lineage inventory") + } else { + assert.Equal(t, []corev1.Secret{*old}, f.inventories()) + } if mode != "new-synthesis" { f.restart() - f.controller.downstream = nil require.NoError(t, f.reconcile()) assert.Equal(t, expected, f.composition()) + assert.Empty(t, f.inventories(), "deleting Composition removes its lineage inventory") } }) } diff --git a/internal/controllers/tombstonerecovery/recording.go b/internal/controllers/tombstonerecovery/recording.go index bb9971d0..3fc209ea 100644 --- a/internal/controllers/tombstonerecovery/recording.go +++ b/internal/controllers/tombstonerecovery/recording.go @@ -33,7 +33,7 @@ func (c *tombstoneRecoveryController) recordInventory(ctx context.Context, comp return fmt.Errorf("building inventory: %w", err) } for i := range secrets { - if err := c.writeInventoryChunk(ctx, comp, &secrets[i]); err != nil { + if err := c.writeInventoryChunk(ctx, &secrets[i]); err != nil { return err } } @@ -44,7 +44,7 @@ func (c *tombstoneRecoveryController) recordInventory(ctx context.Context, comp return nil } -func (c *tombstoneRecoveryController) writeInventoryChunk(ctx context.Context, comp *apiv1.Composition, intended *corev1.Secret) error { +func (c *tombstoneRecoveryController) writeInventoryChunk(ctx context.Context, intended *corev1.Secret) error { existing := &corev1.Secret{} err := c.downstream.Get(ctx, client.ObjectKeyFromObject(intended), existing) if apierrors.IsNotFound(err) { diff --git a/internal/controllers/tombstonerecovery/recording_test.go b/internal/controllers/tombstonerecovery/recording_test.go index 81bc25fd..2fb92cc8 100644 --- a/internal/controllers/tombstonerecovery/recording_test.go +++ b/internal/controllers/tombstonerecovery/recording_test.go @@ -69,7 +69,9 @@ func TestInventoryRecordingEligibility(t *testing.T) { case "deleting": require.NoError(t, f.upstream.Delete(t.Context(), comp)) } - f.controller.downstream = nil // Neither inventory reads nor writes are allowed in these cases. + if mode != "deleting" { + f.controller.downstream = nil // Neither inventory reads nor writes are allowed in these cases. + } f.reconcileEvent() assert.Empty(t, f.inventories()) })