[eno][tombstone-recovery 7/7] Garbage collect stale inventory Secrets

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 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.
+- **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.

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

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"`

internal/controllers/tombstonerecovery/gc.go

@@ -0,0 +1,172 @@
+package tombstonerecovery
+
+import (
+	"context"
+	"fmt"
+	"runtime/debug"
+	"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", "stack", string(debug.Stack()))
+		}
+	}()
+	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)
+	}
+	// 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
+	}
+
+	stale := findStaleInventory(secrets.Items, comps.Items, &symphonies.Items[0])
+	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
+}
+
+// findStaleInventory returns the inventory Secrets that are safe to delete.
+func findStaleInventory(secrets []corev1.Secret, comps []apiv1.Composition, symph *apiv1.Symphony) []corev1.Secret {
+	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
+	})
+	return stale
+}
+
+// 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 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
+	}
+	for _, variation := range symph.Spec.Variations {
+		liveLineages[inventoryLineageFor(symph.Namespace, variation.Synthesizer.Name)] = true
+	}
+	var orphaned []corev1.Secret
+	for _, secret := range secrets {
+		if !liveLineages[secret.Labels[inventoryLineageLabel]] {
+			orphaned = append(orphaned, secret)
+		}
+	}
+	return orphaned
+}
+
+// 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 !isCompleteSnapshot(current) {
+		return nil
+	}
+	return old
+}