From 4d8e62b0e1469b95a763c965ca86af1069657c8e Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Mon, 12 Oct 2026 22:35:11 +0000 Subject: [PATCH 1/4] initial commit --- ...source-synthesis-self-heal-and-recovery.md | 17 +- .../controllers/tombstonerecovery/cleanup.go | 6 +- .../tombstonerecovery/controller.go | 17 ++ internal/controllers/tombstonerecovery/gc.go | 162 ++++++++++++++ .../controllers/tombstonerecovery/gc_test.go | 211 ++++++++++++++++++ .../tombstonerecovery/recording.go | 4 +- 6 files changed, 407 insertions(+), 10 deletions(-) create mode 100644 internal/controllers/tombstonerecovery/gc.go create mode 100644 internal/controllers/tombstonerecovery/gc_test.go diff --git a/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md b/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md index c512f09f..5b55d4d0 100644 --- a/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md +++ b/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md @@ -7,7 +7,7 @@ - **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. - **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. +- **Garbage collect:** Periodically delete inventory for lineages whose Composition is gone and older snapshots of existing Compositions, while the reconciler is running. - **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. @@ -83,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 for Compositions that are already gone. +- **Does not:** Change an existing desired manifest into a tombstone or directly delete downstream resources. - **Entry points:** [`Reconcile` and `recoverMissingTombstones`](../../internal/controllers/tombstonerecovery/controller.go), [`missingTombstones`](../../internal/controllers/tombstonerecovery/inventory.go). ### Packing Planner and Writer @@ -110,6 +110,16 @@ Synthesis detects incomplete history or inherits an outstanding requirement - **Does not:** Add a finalizer, block Composition deletion, change Composition status, or delete downstream resources. - **Entry point:** [`cleanupInventory`](../../internal/controllers/tombstonerecovery/cleanup.go). +### Inventory Garbage Collector + +- **Does:** Run as a leader-elected manager runnable when the operator is enabled, starting a cycle every 3 to 4.5 minutes. A failed or panicking cycle is logged and the next cycle starts fresh. +- **Does:** Each cycle lists downstream `kube-system` Secrets that have the `eno.azure.io/inventory-lineage` label, then lists Compositions and Symphonies in the reconciler's namespace with the uncached API reader. Any read failure skips the cycle. +- **Does:** Only when the namespace's Symphony exists and is not deleting, delete Secrets whose lineage matches no Composition, and delete an existing Composition's Secrets from older synthesis UUIDs once its current synthesis UUID has a complete snapshot. Compositions are listed before the Symphony so a Composition missing from the list was gone before any observed Symphony deletion. +- **Does:** Delete with UID preconditions, treat `NotFound` as deleted, continue after a failed delete, and log a per-cycle summary of scanned, deleted, and failed Secrets. Failed deletes are retried by the next cycle. +- **Does not:** Touch a deleting Composition's inventory, current-UUID Secrets, or Secrets without the lineage label; change Composition status, ResourceSlices, or downstream resources; or add finalizers. +- **Assumes:** Only this reconciler writes inventory to its downstream `kube-system`, and each namespace has one Symphony. +- **Entry points:** [`inventoryGarbageCollector` and `findStaleInventory`](../../internal/controllers/tombstonerecovery/gc.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. @@ -226,7 +236,6 @@ 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. -- **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. +- **Best-effort inventory cleanup:** Cleanup does not block Composition deletion. If the Composition disappears before cleanup succeeds, the garbage collector removes its inventory while the Symphony is active. Inventory kept by Symphony deletion stays until a Composition with the same lineage rotates it out or the downstream cluster is deleted. - **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 index b3bf9209..c5d41d5c 100644 --- a/internal/controllers/tombstonerecovery/cleanup.go +++ b/internal/controllers/tombstonerecovery/cleanup.go @@ -2,11 +2,9 @@ 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. @@ -18,8 +16,8 @@ func (c *tombstoneRecoveryController) cleanupInventory(ctx context.Context, comp } 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) + if err := deleteInventorySecret(ctx, c.downstream, item); err != nil { + return err } logr.FromContextOrDiscard(ctx).Info("deleted inventory chunk", "secretName", item.Name) } diff --git a/internal/controllers/tombstonerecovery/controller.go b/internal/controllers/tombstonerecovery/controller.go index f7387b0f..b1e2773a 100644 --- a/internal/controllers/tombstonerecovery/controller.go +++ b/internal/controllers/tombstonerecovery/controller.go @@ -116,6 +116,14 @@ func NewController(mgr ctrl.Manager, opts Options) error { if err != nil { return fmt.Errorf("constructing tombstone recovery downstream client: %w", err) } + if err := mgr.Add(&inventoryGarbageCollector{ + reader: mgr.GetAPIReader(), + downstream: c.downstream, + namespace: opts.Namespace, + logger: mgr.GetLogger().WithName("inventoryGarbageCollector"), + }); err != nil { + return fmt.Errorf("registering inventory garbage collector: %w", err) + } slice := &metav1.PartialObjectMetadata{} slice.SetGroupVersionKind(apiv1.SchemeGroupVersion.WithKind("ResourceSlice")) return ctrl.NewControllerManagedBy(mgr). @@ -371,6 +379,15 @@ func (c *tombstoneRecoveryController) readInventories(ctx context.Context, comp return list.Items, nil } +// deleteInventorySecret deletes one inventory Secret. The UID precondition avoids deleting a recreated Secret with the same name, +// and NotFound counts as deleted. +func deleteInventorySecret(ctx context.Context, downstream client.Client, item *corev1.Secret) error { + if err := downstream.Delete(ctx, item, &client.Preconditions{UID: &item.UID}); err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("deleting inventory Secret %q: %w", item.Name, err) + } + return nil +} + type statusPatch struct { Op string `json:"op"` Path string `json:"path"` diff --git a/internal/controllers/tombstonerecovery/gc.go b/internal/controllers/tombstonerecovery/gc.go new file mode 100644 index 00000000..af305cf2 --- /dev/null +++ b/internal/controllers/tombstonerecovery/gc.go @@ -0,0 +1,162 @@ +package tombstonerecovery + +import ( + "context" + "fmt" + "sort" + "time" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/util/wait" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +const ( + inventoryGCInterval = 3 * time.Minute + inventoryGCJitter = 0.5 // Cycles start every 3 to 4.5 minutes. +) + +// inventoryGarbageCollector periodically deletes stale inventory Secrets while the reconciler is running. +type inventoryGarbageCollector struct { + reader client.Reader + downstream client.Client + namespace string + logger logr.Logger +} + +func (g *inventoryGarbageCollector) NeedLeaderElection() bool { + return true +} + +func (g *inventoryGarbageCollector) Start(ctx context.Context) error { + ctx = logr.NewContext(ctx, g.logger) + wait.JitterUntilWithContext(ctx, g.runCycle, inventoryGCInterval, inventoryGCJitter, true) + return nil +} + +// runCycle logs failures and panics so the loop keeps running; the next cycle starts fresh. +func (g *inventoryGarbageCollector) runCycle(ctx context.Context) { + logger := logr.FromContextOrDiscard(ctx) + defer func() { + if r := recover(); r != nil { + logger.Error(fmt.Errorf("panic: %v", r), "inventory garbage collection cycle panicked") + } + }() + if err := g.deleteStaleInventory(ctx); err != nil { + logger.Error(err, "skipping inventory garbage collection cycle") + } +} + +// deleteStaleInventory runs one cycle. Compositions are listed before Symphonies so a Composition missing from the list +// was already gone before any Symphony deletion that the later read could observe. +func (g *inventoryGarbageCollector) deleteStaleInventory(ctx context.Context) error { + logger := logr.FromContextOrDiscard(ctx) + secrets := &corev1.SecretList{} + if err := g.downstream.List(ctx, secrets, client.InNamespace(inventoryNamespace), client.HasLabels{inventoryLineageLabel}); err != nil { + return fmt.Errorf("listing downstream inventory Secrets: %w", err) + } + comps := &apiv1.CompositionList{} + if err := g.reader.List(ctx, comps, client.InNamespace(g.namespace)); err != nil { + return fmt.Errorf("listing Compositions: %w", err) + } + symphonies := &apiv1.SymphonyList{} + if err := g.reader.List(ctx, symphonies, client.InNamespace(g.namespace)); err != nil { + return fmt.Errorf("listing Symphonies: %w", err) + } + if !isSymphonyActive(symphonies.Items) { + logger.Info("symphony is deleting or missing; keeping inventory", "scanned", len(secrets.Items)) + return nil + } + + stale := findStaleInventory(secrets.Items, comps.Items) + deleted, failed := 0, 0 + for i := range stale { + item := &stale[i] + itemLogger := logger.WithValues("secretName", item.Name, "lineage", item.Labels[inventoryLineageLabel], + "synthesisUUID", item.Labels[inventorySynthesisUUIDLabel]) + if err := deleteInventorySecret(ctx, g.downstream, item); err != nil { + itemLogger.Error(err, "failed to delete stale inventory Secret") + failed++ + continue + } + itemLogger.Info("deleted stale inventory Secret") + deleted++ + } + logger.Info("inventory garbage collection cycle finished", "scanned", len(secrets.Items), "deleted", deleted, "failed", failed) + return nil +} + +// isSymphonyActive reports whether the namespace's only Symphony exists and is not deleting. +func isSymphonyActive(symphonies []apiv1.Symphony) bool { + return len(symphonies) == 1 && symphonies[0].DeletionTimestamp == nil +} + +// findStaleInventory returns the inventory Secrets that are safe to delete. +func findStaleInventory(secrets []corev1.Secret, comps []apiv1.Composition) []corev1.Secret { + stale := findInventoryWithoutComposition(secrets, comps) + stale = append(stale, findInventoryFromOldSyntheses(secrets, comps)...) + sort.Slice(stale, func(i, j int) bool { + return stale[i].Name < stale[j].Name + }) + return stale +} + +// findInventoryWithoutComposition returns every Secret whose lineage matches no Composition, meaning its Composition is gone. +func findInventoryWithoutComposition(secrets []corev1.Secret, comps []apiv1.Composition) []corev1.Secret { + liveLineages := make(map[string]bool, len(comps)) + for i := range comps { + liveLineages[inventoryLineage(&comps[i])] = true + } + var withoutComposition []corev1.Secret + for _, secret := range secrets { + if !liveLineages[secret.Labels[inventoryLineageLabel]] { + withoutComposition = append(withoutComposition, secret) + } + } + return withoutComposition +} + +// findInventoryFromOldSyntheses returns each existing Composition's Secrets from older syntheses, +// once its current synthesis has a complete snapshot. +func findInventoryFromOldSyntheses(secrets []corev1.Secret, comps []apiv1.Composition) []corev1.Secret { + secretsByLineage := groupInventoryByLineage(secrets) + var old []corev1.Secret + for i := range comps { + comp := &comps[i] + // Event-driven cleanup owns a deleting Composition's inventory. + if comp.DeletionTimestamp != nil || comp.Status.CurrentSynthesis == nil { + continue + } + items := secretsByLineage[inventoryLineage(comp)] + old = append(old, oldSnapshotsInLineage(items, comp.Status.CurrentSynthesis.UUID)...) + } + return old +} + +// groupInventoryByLineage groups inventory Secrets by their lineage label. +func groupInventoryByLineage(secrets []corev1.Secret) map[string][]corev1.Secret { + secretsByLineage := map[string][]corev1.Secret{} + for _, secret := range secrets { + lineage := secret.Labels[inventoryLineageLabel] + secretsByLineage[lineage] = append(secretsByLineage[lineage], secret) + } + return secretsByLineage +} + +// oldSnapshotsInLineage returns one lineage's Secrets from older syntheses, only when the current synthesis has a complete snapshot. +func oldSnapshotsInLineage(items []corev1.Secret, currentUUID string) []corev1.Secret { + var current, old []corev1.Secret + for _, item := range items { + if item.Labels[inventorySynthesisUUIDLabel] == currentUUID { + current = append(current, item) + } else { + old = append(old, item) + } + } + if _, complete := orderInventoryChunks(current); !complete { + return nil + } + return old +} diff --git a/internal/controllers/tombstonerecovery/gc_test.go b/internal/controllers/tombstonerecovery/gc_test.go new file mode 100644 index 00000000..958b6235 --- /dev/null +++ b/internal/controllers/tombstonerecovery/gc_test.go @@ -0,0 +1,211 @@ +package tombstonerecovery + +import ( + "context" + "fmt" + "testing" + + apiv1 "github.com/Azure/eno/api/v1" + "github.com/go-logr/logr" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + "k8s.io/utils/ptr" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" +) + +func gcTestComposition() *apiv1.Composition { + comp := inventoryTestComposition() + comp.Annotations["eno.azure.io/recovery-enabled"] = "true" + comp.Status.CurrentSynthesis.UUID = controllerTestUUID(2) + return comp +} + +func gcTestSnapshot(t *testing.T, comp *apiv1.Composition, sequence int) corev1.Secret { + t.Helper() + comp = comp.DeepCopy() + comp.Status.CurrentSynthesis.UUID = controllerTestUUID(sequence) + items, err := makeInventory(comp, nil) + require.NoError(t, err) + require.Len(t, items, 1) + items[0].UID = types.UID(fmt.Sprintf("snapshot-%d-uid", sequence)) + return items[0] +} + +func gcTestNames(items []corev1.Secret) []string { + names := []string{} + for _, item := range items { + names = append(names, item.Name) + } + return names +} + +func gcTestOrphan(t *testing.T, comp *apiv1.Composition) corev1.Secret { + t.Helper() + orphan := gcTestSnapshot(t, comp, 3) + orphan.Name = "orphan" + orphan.Labels = map[string]string{inventoryLineageLabel: "other-lineage", inventorySynthesisUUIDLabel: controllerTestUUID(3)} + return orphan +} + +func TestFindStaleInventory(t *testing.T) { + base := gcTestComposition() + old, current, orphan := gcTestSnapshot(t, base, 1), gcTestSnapshot(t, base, 2), gcTestOrphan(t, base) + + for _, tc := range []struct { + name string + change func(*apiv1.Composition) + secrets func() []corev1.Secret + want []string + }{ + {name: "composition gone and old snapshot", want: []string{old.Name, orphan.Name}}, + {name: "current snapshot missing", secrets: func() []corev1.Secret { + return []corev1.Secret{old, orphan} + }, want: []string{orphan.Name}}, + {name: "current snapshot incomplete", secrets: func() []corev1.Secret { + partial := *current.DeepCopy() + partial.Annotations["eno.azure.io/inventory-chunk-count"] = "2" + return []corev1.Secret{old, partial, orphan} + }, want: []string{orphan.Name}}, + {name: "deleting composition", change: func(comp *apiv1.Composition) { + comp.DeletionTimestamp = ptr.To(metav1.Now()) + }, want: []string{orphan.Name}}, + {name: "no current synthesis", change: func(comp *apiv1.Composition) { + comp.Status.CurrentSynthesis = nil + }, want: []string{orphan.Name}}, + } { + t.Run(tc.name, func(t *testing.T) { + comp := base.DeepCopy() + if tc.change != nil { + tc.change(comp) + } + secrets := []corev1.Secret{old, current, orphan} + if tc.secrets != nil { + secrets = tc.secrets() + } + assert.Equal(t, tc.want, gcTestNames(findStaleInventory(secrets, []apiv1.Composition{*comp}))) + }) + } +} + +func TestIsSymphonyActive(t *testing.T) { + assert.False(t, isSymphonyActive(nil), "missing") + assert.True(t, isSymphonyActive([]apiv1.Symphony{{}}), "active") + assert.False(t, isSymphonyActive([]apiv1.Symphony{{ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: ptr.To(metav1.Now())}}}), "deleting") +} + +func newGCTestCollector(t *testing.T, upstreamObjs []client.Object, secrets ...corev1.Secret) (*inventoryGarbageCollector, client.WithWatch) { + t.Helper() + scheme := runtime.NewScheme() + require.NoError(t, apiv1.SchemeBuilder.AddToScheme(scheme)) + require.NoError(t, corev1.AddToScheme(scheme)) + downstream := fake.NewClientBuilder().WithScheme(scheme).Build() + for i := range secrets { + require.NoError(t, downstream.Create(t.Context(), &secrets[i])) + } + return &inventoryGarbageCollector{ + reader: fake.NewClientBuilder().WithScheme(scheme).WithObjects(upstreamObjs...).Build(), + downstream: downstream, + namespace: "control", + logger: logr.Discard(), + }, downstream +} + +func gcTestRemaining(t *testing.T, downstream client.Client) []string { + t.Helper() + list := &corev1.SecretList{} + require.NoError(t, downstream.List(t.Context(), list, client.InNamespace(inventoryNamespace))) + return gcTestNames(list.Items) +} + +func TestInventoryGarbageCollectorDeleteStaleInventory(t *testing.T) { + comp := gcTestComposition() + old, current, orphan := gcTestSnapshot(t, comp, 1), gcTestSnapshot(t, comp, 2), gcTestOrphan(t, comp) + unrelated := corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "unrelated", Namespace: inventoryNamespace}} + + for _, tc := range []struct { + name string + symphony *apiv1.Symphony + want []string + }{ + {name: "active symphony", symphony: &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{Name: "symphony", Namespace: "control"}}, + want: []string{current.Name, "unrelated"}}, + {name: "deleting symphony", symphony: &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{ + Name: "symphony", Namespace: "control", DeletionTimestamp: ptr.To(metav1.Now()), Finalizers: []string{"test"}, + }}, want: []string{old.Name, current.Name, orphan.Name, "unrelated"}}, + {name: "missing symphony", want: []string{old.Name, current.Name, orphan.Name, "unrelated"}}, + } { + t.Run(tc.name, func(t *testing.T) { + objs := []client.Object{comp} + if tc.symphony != nil { + objs = append(objs, tc.symphony) + } + g, downstream := newGCTestCollector(t, objs, old, current, orphan, unrelated) + require.NoError(t, g.deleteStaleInventory(t.Context())) + assert.ElementsMatch(t, tc.want, gcTestRemaining(t, downstream)) + }) + } +} + +func TestInventoryGarbageCollectorDeleteFailureContinues(t *testing.T) { + comp := gcTestComposition() + current, first := gcTestSnapshot(t, comp, 2), gcTestOrphan(t, comp) + second := gcTestOrphan(t, comp) + second.Name, second.UID = "orphan-2", "orphan-2-uid" + symphony := &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{Name: "symphony", Namespace: "control"}} + + g, downstream := newGCTestCollector(t, []client.Object{comp, symphony}, current, first, second) + fail := true + g.downstream = interceptor.NewClient(downstream, interceptor.Funcs{ + Delete: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.DeleteOption) error { + if fail && obj.GetName() == first.Name { + return fmt.Errorf("injected delete failure") + } + return c.Delete(ctx, obj, opts...) + }, + }) + require.NoError(t, g.deleteStaleInventory(t.Context())) + assert.ElementsMatch(t, []string{current.Name, first.Name}, gcTestRemaining(t, downstream), "other stale Secrets are still deleted") + + fail = false + require.NoError(t, g.deleteStaleInventory(t.Context())) + assert.ElementsMatch(t, []string{current.Name}, gcTestRemaining(t, downstream), "the next cycle retries the failed delete") +} + +func TestInventoryGarbageCollectorReadFailureSkipsCycle(t *testing.T) { + comp := gcTestComposition() + orphan := gcTestOrphan(t, comp) + symphony := &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{Name: "symphony", Namespace: "control"}} + + for _, kind := range []string{"Composition", "Symphony"} { + t.Run(kind, func(t *testing.T) { + g, downstream := newGCTestCollector(t, []client.Object{comp, symphony}, orphan) + g.reader = interceptor.NewClient(g.reader.(client.WithWatch), interceptor.Funcs{ + List: func(ctx context.Context, c client.WithWatch, list client.ObjectList, opts ...client.ListOption) error { + if _, ok := list.(*apiv1.CompositionList); ok && kind == "Composition" { + return fmt.Errorf("injected list failure") + } + if _, ok := list.(*apiv1.SymphonyList); ok && kind == "Symphony" { + return fmt.Errorf("injected list failure") + } + return c.List(ctx, list, opts...) + }, + }) + require.Error(t, g.deleteStaleInventory(t.Context())) + assert.Equal(t, []string{orphan.Name}, gcTestRemaining(t, downstream)) + }) + } +} + +func TestInventoryGarbageCollectorRunCycleRecoversPanic(t *testing.T) { + g := &inventoryGarbageCollector{logger: logr.Discard()} + assert.NotPanics(t, func() { + g.runCycle(t.Context()) + }) + assert.True(t, g.NeedLeaderElection()) +} diff --git a/internal/controllers/tombstonerecovery/recording.go b/internal/controllers/tombstonerecovery/recording.go index 3fc209ea..f570b358 100644 --- a/internal/controllers/tombstonerecovery/recording.go +++ b/internal/controllers/tombstonerecovery/recording.go @@ -102,8 +102,8 @@ func (c *tombstoneRecoveryController) deleteOldInventorySnapshots(ctx context.Co if item.Labels[inventorySynthesisUUIDLabel] == currentUUID { continue } - if err := c.downstream.Delete(ctx, item, &client.Preconditions{UID: &item.UID}); err != nil && !apierrors.IsNotFound(err) { - return fmt.Errorf("deleting old inventory Secret %q: %w", item.Name, err) + if err := deleteInventorySecret(ctx, c.downstream, item); err != nil { + return err } logr.FromContextOrDiscard(ctx).Info("deleted old inventory chunk", "secretName", item.Name) } From 27263537b85a342a2e9e386fa2038325c8c750f2 Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Wed, 21 Oct 2026 23:00:10 +0000 Subject: [PATCH 2/4] Keep inventory for Symphony variations awaiting Composition recreation The inventory garbage collector treated a lineage without a Composition as orphaned. During AKS start the active Symphony exists before its Compositions are recreated, so the first GC cycle could delete inventory needed for tombstone recovery. Lineages listed by the active Symphony's variations are now kept. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8e985e7f-ffae-45b3-837c-a41cd34ec8e0 --- ...source-synthesis-self-heal-and-recovery.md | 2 +- internal/controllers/tombstonerecovery/gc.go | 16 +++++++++------ .../controllers/tombstonerecovery/gc_test.go | 20 ++++++++++++++++++- .../tombstonerecovery/inventory.go | 6 +++++- 4 files changed, 35 insertions(+), 9 deletions(-) diff --git a/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md b/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md index 5b55d4d0..8c3a08a2 100644 --- a/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md +++ b/docs/reconciliation/resource-synthesis-self-heal-and-recovery.md @@ -114,7 +114,7 @@ Synthesis detects incomplete history or inherits an outstanding requirement - **Does:** Run as a leader-elected manager runnable when the operator is enabled, starting a cycle every 3 to 4.5 minutes. A failed or panicking cycle is logged and the next cycle starts fresh. - **Does:** Each cycle lists downstream `kube-system` Secrets that have the `eno.azure.io/inventory-lineage` label, then lists Compositions and Symphonies in the reconciler's namespace with the uncached API reader. Any read failure skips the cycle. -- **Does:** Only when the namespace's Symphony exists and is not deleting, delete Secrets whose lineage matches no Composition, and delete an existing Composition's Secrets from older synthesis UUIDs once its current synthesis UUID has a complete snapshot. Compositions are listed before the Symphony so a Composition missing from the list was gone before any observed Symphony deletion. +- **Does:** Only when the namespace's Symphony exists and is not deleting, delete Secrets whose lineage matches no Composition and no Symphony variation (a variation whose Composition is being recreated keeps its inventory for recovery), and delete an existing Composition's Secrets from older synthesis UUIDs once its current synthesis UUID has a complete snapshot. Compositions are listed before the Symphony so a Composition missing from the list was gone before any observed Symphony deletion. - **Does:** Delete with UID preconditions, treat `NotFound` as deleted, continue after a failed delete, and log a per-cycle summary of scanned, deleted, and failed Secrets. Failed deletes are retried by the next cycle. - **Does not:** Touch a deleting Composition's inventory, current-UUID Secrets, or Secrets without the lineage label; change Composition status, ResourceSlices, or downstream resources; or add finalizers. - **Assumes:** Only this reconciler writes inventory to its downstream `kube-system`, and each namespace has one Symphony. diff --git a/internal/controllers/tombstonerecovery/gc.go b/internal/controllers/tombstonerecovery/gc.go index af305cf2..574f48fd 100644 --- a/internal/controllers/tombstonerecovery/gc.go +++ b/internal/controllers/tombstonerecovery/gc.go @@ -70,7 +70,7 @@ func (g *inventoryGarbageCollector) deleteStaleInventory(ctx context.Context) er return nil } - stale := findStaleInventory(secrets.Items, comps.Items) + stale := findStaleInventory(secrets.Items, comps.Items, &symphonies.Items[0]) deleted, failed := 0, 0 for i := range stale { item := &stale[i] @@ -94,8 +94,8 @@ func isSymphonyActive(symphonies []apiv1.Symphony) bool { } // findStaleInventory returns the inventory Secrets that are safe to delete. -func findStaleInventory(secrets []corev1.Secret, comps []apiv1.Composition) []corev1.Secret { - stale := findInventoryWithoutComposition(secrets, comps) +func findStaleInventory(secrets []corev1.Secret, comps []apiv1.Composition, symph *apiv1.Symphony) []corev1.Secret { + stale := findInventoryWithoutComposition(secrets, comps, symph) stale = append(stale, findInventoryFromOldSyntheses(secrets, comps)...) sort.Slice(stale, func(i, j int) bool { return stale[i].Name < stale[j].Name @@ -103,12 +103,16 @@ func findStaleInventory(secrets []corev1.Secret, comps []apiv1.Composition) []co return stale } -// findInventoryWithoutComposition returns every Secret whose lineage matches no Composition, meaning its Composition is gone. -func findInventoryWithoutComposition(secrets []corev1.Secret, comps []apiv1.Composition) []corev1.Secret { - liveLineages := make(map[string]bool, len(comps)) +// findInventoryWithoutComposition returns every Secret whose lineage matches no Composition and no Symphony variation. +// A variation without a Composition yet is being (re)created, e.g. after AKS stop/start, and needs its inventory for recovery. +func findInventoryWithoutComposition(secrets []corev1.Secret, comps []apiv1.Composition, symph *apiv1.Symphony) []corev1.Secret { + liveLineages := make(map[string]bool, len(comps)+len(symph.Spec.Variations)) for i := range comps { liveLineages[inventoryLineage(&comps[i])] = true } + for _, variation := range symph.Spec.Variations { + liveLineages[inventoryLineageFor(symph.Namespace, variation.Synthesizer.Name)] = true + } var withoutComposition []corev1.Secret for _, secret := range secrets { if !liveLineages[secret.Labels[inventoryLineageLabel]] { diff --git a/internal/controllers/tombstonerecovery/gc_test.go b/internal/controllers/tombstonerecovery/gc_test.go index 958b6235..3aa7f302 100644 --- a/internal/controllers/tombstonerecovery/gc_test.go +++ b/internal/controllers/tombstonerecovery/gc_test.go @@ -88,11 +88,29 @@ func TestFindStaleInventory(t *testing.T) { if tc.secrets != nil { secrets = tc.secrets() } - assert.Equal(t, tc.want, gcTestNames(findStaleInventory(secrets, []apiv1.Composition{*comp}))) + symph := &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{Namespace: comp.Namespace}} + assert.Equal(t, tc.want, gcTestNames(findStaleInventory(secrets, []apiv1.Composition{*comp}, symph))) }) } } +func TestFindStaleInventoryKeepsSymphonyVariations(t *testing.T) { + comp := gcTestComposition() + current, orphan := gcTestSnapshot(t, comp, 2), gcTestOrphan(t, comp) + secrets := []corev1.Secret{current, orphan} + withVariation := &apiv1.Symphony{ + ObjectMeta: metav1.ObjectMeta{Namespace: comp.Namespace}, + Spec: apiv1.SymphonySpec{Variations: []apiv1.Variation{{Synthesizer: comp.Spec.Synthesizer}}}, + } + withoutVariation := &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{Namespace: comp.Namespace}} + + got := findStaleInventory(secrets, nil, withVariation) + assert.Equal(t, []string{orphan.Name}, gcTestNames(got), "a variation whose Composition is being recreated keeps its inventory") + + got = findStaleInventory(secrets, nil, withoutVariation) + assert.ElementsMatch(t, []string{current.Name, orphan.Name}, gcTestNames(got), "a removed variation's inventory is stale") +} + func TestIsSymphonyActive(t *testing.T) { assert.False(t, isSymphonyActive(nil), "missing") assert.True(t, isSymphonyActive([]apiv1.Symphony{{}}), "active") diff --git a/internal/controllers/tombstonerecovery/inventory.go b/internal/controllers/tombstonerecovery/inventory.go index f5513150..42c4130c 100644 --- a/internal/controllers/tombstonerecovery/inventory.go +++ b/internal/controllers/tombstonerecovery/inventory.go @@ -51,7 +51,11 @@ type synthesisInventory struct { } func inventoryLineage(comp *apiv1.Composition) string { - hash := sha256.Sum256([]byte(comp.Namespace + "/" + comp.Spec.Synthesizer.Name)) + return inventoryLineageFor(comp.Namespace, comp.Spec.Synthesizer.Name) +} + +func inventoryLineageFor(namespace, synthesizer string) string { + hash := sha256.Sum256([]byte(namespace + "/" + synthesizer)) return hex.EncodeToString(hash[:16]) } From 9915820417f0f82e4ecac8d0d0a5d5e6ad74a251 Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Mon, 26 Oct 2026 18:52:20 +0000 Subject: [PATCH 3/4] Reject terminating replacement snapshots in inventory GC GC deleted older inventory once the current snapshot's chunks were complete, even if a current chunk was terminating. Recording already rejects terminating replacements. Both paths now share isCompleteSnapshot, so GC keeps the older snapshot until a non-terminating replacement exists. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8e985e7f-ffae-45b3-837c-a41cd34ec8e0 --- internal/controllers/tombstonerecovery/gc.go | 2 +- internal/controllers/tombstonerecovery/gc_test.go | 5 +++++ internal/controllers/tombstonerecovery/inventory.go | 12 ++++++++++++ internal/controllers/tombstonerecovery/recording.go | 5 ++--- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/internal/controllers/tombstonerecovery/gc.go b/internal/controllers/tombstonerecovery/gc.go index 574f48fd..be71b0e1 100644 --- a/internal/controllers/tombstonerecovery/gc.go +++ b/internal/controllers/tombstonerecovery/gc.go @@ -159,7 +159,7 @@ func oldSnapshotsInLineage(items []corev1.Secret, currentUUID string) []corev1.S old = append(old, item) } } - if _, complete := orderInventoryChunks(current); !complete { + if !isCompleteSnapshot(current) { return nil } return old diff --git a/internal/controllers/tombstonerecovery/gc_test.go b/internal/controllers/tombstonerecovery/gc_test.go index 3aa7f302..a85e9172 100644 --- a/internal/controllers/tombstonerecovery/gc_test.go +++ b/internal/controllers/tombstonerecovery/gc_test.go @@ -64,6 +64,11 @@ func TestFindStaleInventory(t *testing.T) { want []string }{ {name: "composition gone and old snapshot", want: []string{old.Name, orphan.Name}}, + {name: "current snapshot terminating", secrets: func() []corev1.Secret { + terminating := *current.DeepCopy() + terminating.DeletionTimestamp = ptr.To(metav1.Now()) + return []corev1.Secret{old, terminating, orphan} + }, want: []string{orphan.Name}}, {name: "current snapshot missing", secrets: func() []corev1.Secret { return []corev1.Secret{old, orphan} }, want: []string{orphan.Name}}, diff --git a/internal/controllers/tombstonerecovery/inventory.go b/internal/controllers/tombstonerecovery/inventory.go index 42c4130c..84b5bf9c 100644 --- a/internal/controllers/tombstonerecovery/inventory.go +++ b/internal/controllers/tombstonerecovery/inventory.go @@ -138,6 +138,18 @@ func selectNewestCompleteInventory(inventories []synthesisInventory) ([]corev1.S } // orderInventoryChunks returns chunks in index order and reports whether the set is complete. +// isCompleteSnapshot reports whether chunks form a full snapshot that is not being deleted. +// A terminating chunk will disappear, so its snapshot cannot replace an older one. +func isCompleteSnapshot(chunks []corev1.Secret) bool { + for i := range chunks { + if chunks[i].DeletionTimestamp != nil { + return false + } + } + _, complete := orderInventoryChunks(chunks) + return complete +} + func orderInventoryChunks(items []corev1.Secret) ([]corev1.Secret, bool) { if len(items) == 0 { return nil, false diff --git a/internal/controllers/tombstonerecovery/recording.go b/internal/controllers/tombstonerecovery/recording.go index f570b358..f70ec01c 100644 --- a/internal/controllers/tombstonerecovery/recording.go +++ b/internal/controllers/tombstonerecovery/recording.go @@ -74,12 +74,11 @@ func isInventoryAlreadyRecorded(items []corev1.Secret, currentUUID string) bool return false } for i := range items { - if items[i].Labels[inventorySynthesisUUIDLabel] != currentUUID || items[i].DeletionTimestamp != nil { + if items[i].Labels[inventorySynthesisUUIDLabel] != currentUUID { return false } } - _, complete := orderInventoryChunks(items) - return complete + return isCompleteSnapshot(items) } func inventorySecretMatches(existing, intended *corev1.Secret) bool { From 53206c764518dac6a72c50e3622d8c8306304560 Mon Sep 17 00:00:00 2001 From: Ruinan Liu Date: Tue, 27 Oct 2026 17:44:32 +0000 Subject: [PATCH 4/4] resolve comments --- internal/controllers/tombstonerecovery/gc.go | 34 +++++++++++-------- .../controllers/tombstonerecovery/gc_test.go | 27 +++++++-------- 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/internal/controllers/tombstonerecovery/gc.go b/internal/controllers/tombstonerecovery/gc.go index be71b0e1..4d66a9aa 100644 --- a/internal/controllers/tombstonerecovery/gc.go +++ b/internal/controllers/tombstonerecovery/gc.go @@ -3,6 +3,7 @@ package tombstonerecovery import ( "context" "fmt" + "runtime/debug" "sort" "time" @@ -41,7 +42,7 @@ func (g *inventoryGarbageCollector) runCycle(ctx context.Context) { logger := logr.FromContextOrDiscard(ctx) defer func() { if r := recover(); r != nil { - logger.Error(fmt.Errorf("panic: %v", r), "inventory garbage collection cycle panicked") + logger.Error(fmt.Errorf("panic: %v", r), "inventory garbage collection cycle panicked", "stack", string(debug.Stack())) } }() if err := g.deleteStaleInventory(ctx); err != nil { @@ -65,8 +66,18 @@ func (g *inventoryGarbageCollector) deleteStaleInventory(ctx context.Context) er if err := g.reader.List(ctx, symphonies, client.InNamespace(g.namespace)); err != nil { return fmt.Errorf("listing Symphonies: %w", err) } - if !isSymphonyActive(symphonies.Items) { - logger.Info("symphony is deleting or missing; keeping inventory", "scanned", len(secrets.Items)) + // Eno expects exactly one active Symphony per active cluster cluster; otherwise keep all inventory. + switch { + case len(symphonies.Items) == 0: + logger.Info("no symphony found; keeping inventory", "scanned", len(secrets.Items)) + return nil + case len(symphonies.Items) > 1: + logger.Error(nil, "found multiple symphonies, expected exactly one; keeping inventory", + "symphonyCount", len(symphonies.Items), "scanned", len(secrets.Items)) + return nil + case symphonies.Items[0].DeletionTimestamp != nil: + logger.Info("symphony is deleting; keeping inventory", + "symphonyName", symphonies.Items[0].Name, "scanned", len(secrets.Items)) return nil } @@ -88,14 +99,9 @@ func (g *inventoryGarbageCollector) deleteStaleInventory(ctx context.Context) er return nil } -// isSymphonyActive reports whether the namespace's only Symphony exists and is not deleting. -func isSymphonyActive(symphonies []apiv1.Symphony) bool { - return len(symphonies) == 1 && symphonies[0].DeletionTimestamp == nil -} - // findStaleInventory returns the inventory Secrets that are safe to delete. func findStaleInventory(secrets []corev1.Secret, comps []apiv1.Composition, symph *apiv1.Symphony) []corev1.Secret { - stale := findInventoryWithoutComposition(secrets, comps, symph) + stale := findOrphanedLineageInventory(secrets, comps, symph) stale = append(stale, findInventoryFromOldSyntheses(secrets, comps)...) sort.Slice(stale, func(i, j int) bool { return stale[i].Name < stale[j].Name @@ -103,9 +109,9 @@ func findStaleInventory(secrets []corev1.Secret, comps []apiv1.Composition, symp return stale } -// findInventoryWithoutComposition returns every Secret whose lineage matches no Composition and no Symphony variation. +// findOrphanedLineageInventory returns every Secret whose lineage matches no Composition and no Symphony variation. // A variation without a Composition yet is being (re)created, e.g. after AKS stop/start, and needs its inventory for recovery. -func findInventoryWithoutComposition(secrets []corev1.Secret, comps []apiv1.Composition, symph *apiv1.Symphony) []corev1.Secret { +func findOrphanedLineageInventory(secrets []corev1.Secret, comps []apiv1.Composition, symph *apiv1.Symphony) []corev1.Secret { liveLineages := make(map[string]bool, len(comps)+len(symph.Spec.Variations)) for i := range comps { liveLineages[inventoryLineage(&comps[i])] = true @@ -113,13 +119,13 @@ func findInventoryWithoutComposition(secrets []corev1.Secret, comps []apiv1.Comp for _, variation := range symph.Spec.Variations { liveLineages[inventoryLineageFor(symph.Namespace, variation.Synthesizer.Name)] = true } - var withoutComposition []corev1.Secret + var orphaned []corev1.Secret for _, secret := range secrets { if !liveLineages[secret.Labels[inventoryLineageLabel]] { - withoutComposition = append(withoutComposition, secret) + orphaned = append(orphaned, secret) } } - return withoutComposition + return orphaned } // findInventoryFromOldSyntheses returns each existing Composition's Secrets from older syntheses, diff --git a/internal/controllers/tombstonerecovery/gc_test.go b/internal/controllers/tombstonerecovery/gc_test.go index a85e9172..0524ce04 100644 --- a/internal/controllers/tombstonerecovery/gc_test.go +++ b/internal/controllers/tombstonerecovery/gc_test.go @@ -116,12 +116,6 @@ func TestFindStaleInventoryKeepsSymphonyVariations(t *testing.T) { assert.ElementsMatch(t, []string{current.Name, orphan.Name}, gcTestNames(got), "a removed variation's inventory is stale") } -func TestIsSymphonyActive(t *testing.T) { - assert.False(t, isSymphonyActive(nil), "missing") - assert.True(t, isSymphonyActive([]apiv1.Symphony{{}}), "active") - assert.False(t, isSymphonyActive([]apiv1.Symphony{{ObjectMeta: metav1.ObjectMeta{DeletionTimestamp: ptr.To(metav1.Now())}}}), "deleting") -} - func newGCTestCollector(t *testing.T, upstreamObjs []client.Object, secrets ...corev1.Secret) (*inventoryGarbageCollector, client.WithWatch) { t.Helper() scheme := runtime.NewScheme() @@ -151,22 +145,27 @@ func TestInventoryGarbageCollectorDeleteStaleInventory(t *testing.T) { old, current, orphan := gcTestSnapshot(t, comp, 1), gcTestSnapshot(t, comp, 2), gcTestOrphan(t, comp) unrelated := corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "unrelated", Namespace: inventoryNamespace}} + active := func(name string) *apiv1.Symphony { + return &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "control"}} + } for _, tc := range []struct { - name string - symphony *apiv1.Symphony - want []string + name string + symphonies []*apiv1.Symphony + want []string }{ - {name: "active symphony", symphony: &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{Name: "symphony", Namespace: "control"}}, + {name: "active symphony", symphonies: []*apiv1.Symphony{active("symphony")}, want: []string{current.Name, "unrelated"}}, - {name: "deleting symphony", symphony: &apiv1.Symphony{ObjectMeta: metav1.ObjectMeta{ + {name: "deleting symphony", symphonies: []*apiv1.Symphony{{ObjectMeta: metav1.ObjectMeta{ Name: "symphony", Namespace: "control", DeletionTimestamp: ptr.To(metav1.Now()), Finalizers: []string{"test"}, - }}, want: []string{old.Name, current.Name, orphan.Name, "unrelated"}}, + }}}, want: []string{old.Name, current.Name, orphan.Name, "unrelated"}}, {name: "missing symphony", want: []string{old.Name, current.Name, orphan.Name, "unrelated"}}, + {name: "multiple symphonies", symphonies: []*apiv1.Symphony{active("symphony"), active("symphony-2")}, + want: []string{old.Name, current.Name, orphan.Name, "unrelated"}}, } { t.Run(tc.name, func(t *testing.T) { objs := []client.Object{comp} - if tc.symphony != nil { - objs = append(objs, tc.symphony) + for _, symph := range tc.symphonies { + objs = append(objs, symph) } g, downstream := newGCTestCollector(t, objs, old, current, orphan, unrelated) require.NoError(t, g.deleteStaleInventory(t.Context()))