diff --git a/cocoonset/delete.go b/cocoonset/delete.go index f6edfed..9326e7f 100644 --- a/cocoonset/delete.go +++ b/cocoonset/delete.go @@ -58,19 +58,21 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cs *cocoonv1.CocoonSet return ctrl.Result{RequeueAfter: requeueWaitForMain}, nil } - // :hibernate is always orphaned at teardown — drop unconditionally. :latest - // is kept when shouldKeepLatestTag says vk-cocoon pushed it for retag. + // :hibernate is always orphaned at teardown. :latest is kept when + // shouldKeepLatestTag says vk-cocoon pushed it for retag. Probe before each + // delete because some registries materialize an empty repository while + // authorizing DELETE for a tag that never existed. if r.Registry != nil { for _, name := range parseVMNamesAnnotation(cs.Annotations[annotationDeleteVMNames]) { // Non-fatal, but log at error: a persistent delete failure (e.g. the // registry SA lacking delete permission) silently leaks snapshots. - if err := r.Registry.DeleteManifest(ctx, name, meta.HibernateSnapshotTag); err != nil { + if err := r.deleteManifestIfPresent(ctx, name, meta.HibernateSnapshotTag); err != nil { logger.Errorf(ctx, err, "delete snapshot %s:%s", name, meta.HibernateSnapshotTag) } if shouldKeepLatestTag(cs, name) { continue } - if err := r.Registry.DeleteManifest(ctx, name, meta.DefaultSnapshotTag); err != nil { + if err := r.deleteManifestIfPresent(ctx, name, meta.DefaultSnapshotTag); err != nil { logger.Errorf(ctx, err, "delete snapshot %s:%s", name, meta.DefaultSnapshotTag) } } @@ -86,6 +88,17 @@ func (r *Reconciler) reconcileDelete(ctx context.Context, cs *cocoonv1.CocoonSet return ctrl.Result{}, nil } +func (r *Reconciler) deleteManifestIfPresent(ctx context.Context, name, reference string) error { + present, err := r.Registry.HasManifest(ctx, name, reference) + if err != nil { + return fmt.Errorf("probe snapshot %s:%s: %w", name, reference, err) + } + if !present { + return nil + } + return r.Registry.DeleteManifest(ctx, name, reference) +} + // stashDeleteVMNames merges VM names from Status, the previously stashed // annotation, and live pods, then re-writes the annotation if anything changed. func (r *Reconciler) stashDeleteVMNames(ctx context.Context, cs *cocoonv1.CocoonSet, owned []corev1.Pod) error { diff --git a/cocoonset/reconciler_test.go b/cocoonset/reconciler_test.go index 2fdc959..6e6694d 100644 --- a/cocoonset/reconciler_test.go +++ b/cocoonset/reconciler_test.go @@ -715,7 +715,11 @@ func TestReconcileDeleteSnapshotPolicyGC(t *testing.T) { cs.Status.Toolboxes = c.toolboxes cli := ctrlfake.NewClientBuilder().WithScheme(scheme).WithObjects(cs).Build() - reg := &fakeRegistry{} + present := make(map[string]bool, len(c.want)) + for _, ref := range c.want { + present[ref] = true + } + reg := &fakeRegistry{present: present} r := &Reconciler{Client: cli, Scheme: scheme, Registry: reg} if _, err := r.reconcileDelete(t.Context(), cs); err != nil { @@ -728,6 +732,34 @@ func TestReconcileDeleteSnapshotPolicyGC(t *testing.T) { } } +func TestReconcileDeleteSkipsAbsentSnapshotTags(t *testing.T) { + scheme := testScheme(t) + cs := newCocoonSet("demo") + cs.Finalizers = []string{finalizerName} + cs.Spec.SnapshotPolicy = cocoonv1.SnapshotPolicyNever + cs.Status.Agents = []cocoonv1.AgentStatus{ + {Slot: 0, Role: "main", PodName: "demo-0", VMName: "vk-ns-demo-0"}, + } + + cli := ctrlfake.NewClientBuilder().WithScheme(scheme).WithObjects(cs).Build() + reg := &fakeRegistry{} + r := &Reconciler{Client: cli, Scheme: scheme, Registry: reg} + + if _, err := r.reconcileDelete(t.Context(), cs); err != nil { + t.Fatalf("reconcileDelete: %v", err) + } + if len(reg.deleted) != 0 { + t.Errorf("DeleteManifest calls = %v, want none", reg.deleted) + } + wantProbed := []string{ + "vk-ns-demo-0:" + meta.HibernateSnapshotTag, + "vk-ns-demo-0:" + meta.DefaultSnapshotTag, + } + if !slices.Equal(reg.probed, wantProbed) { + t.Errorf("HasManifest calls = %v, want %v", reg.probed, wantProbed) + } +} + // Race window: pod exists but Status.Agents lacks VMName yet — pass 1 stashes // the pod's VMName onto the annotation so pass 2 still GCs :hibernate. func TestReconcileDeleteStashesPodVMNamesEvenWhenStatusIsEmpty(t *testing.T) { @@ -739,7 +771,10 @@ func TestReconcileDeleteStashesPodVMNamesEvenWhenStatusIsEmpty(t *testing.T) { WithScheme(scheme). WithObjects(cs, mustBuildAgentPod(t, cs, 0, "", "", scheme)). Build() - reg := &fakeRegistry{} + reg := &fakeRegistry{present: map[string]bool{ + "vk-ns-demo-0:" + meta.HibernateSnapshotTag: true, + "vk-ns-demo-0:" + meta.DefaultSnapshotTag: true, + }} r := &Reconciler{Client: cli, Scheme: scheme, Registry: reg} if _, err := r.reconcileDelete(t.Context(), cs); err != nil { @@ -767,7 +802,11 @@ func TestReconcileDeleteCleansTagsAfterPodsGone(t *testing.T) { } cli := ctrlfake.NewClientBuilder().WithScheme(scheme).WithObjects(cs).Build() - reg := &fakeRegistry{} + reg := &fakeRegistry{present: map[string]bool{ + "vk-ns-demo-0:" + meta.HibernateSnapshotTag: true, + "vk-ns-demo-1:" + meta.HibernateSnapshotTag: true, + "vk-ns-demo-tb:" + meta.HibernateSnapshotTag: true, + }} r := &Reconciler{Client: cli, Scheme: scheme, Registry: reg} if _, err := r.reconcileDelete(t.Context(), cs); err != nil { @@ -913,11 +952,16 @@ type fakeRegistry struct { delay time.Duration block map[string]chan struct{} entered chan string + probedMu sync.Mutex + probed []string deletedMu sync.Mutex deleted []string } func (f *fakeRegistry) HasManifest(_ context.Context, name, tag string) (bool, error) { + f.probedMu.Lock() + f.probed = append(f.probed, name+":"+tag) + f.probedMu.Unlock() if f.probeErr != nil { return false, f.probeErr } diff --git a/docs/cocoonset.md b/docs/cocoonset.md index 73f1a79..960b3d1 100644 --- a/docs/cocoonset.md +++ b/docs/cocoonset.md @@ -1,7 +1,7 @@ # CocoonSet reconcile loop 1. Fetch the CocoonSet (return early on NotFound). -2. If `DeletionTimestamp` is set, walk owned pods, delete them, then GC registry tags on every owned VM: `:hibernate` unconditionally (DeleteManifest is 404-tolerant, and hibernate pushes ignore snapshotPolicy so any main-only gate would orphan `:hibernate` tags pushed by sub-agents), and `:latest` only where `shouldKeepLatestTag` says vk-cocoon never pushed one for that VM (`always` keeps every `:latest`, `main-only` keeps slot 0's, `never` drops them all). Then drop the finalizer. VM names to GC are stashed onto an annotation before pod deletion so the cleanup survives a CocoonSet deleted before `Status.Agents` was ever patched. +2. If `DeletionTimestamp` is set, walk owned pods, delete them, then GC registry tags on every owned VM: probe and delete `:hibernate` when present (hibernate pushes ignore snapshotPolicy, so any main-only gate would orphan tags pushed by sub-agents), and probe/delete `:latest` only where `shouldKeepLatestTag` says vk-cocoon never pushed one for that VM (`always` keeps every `:latest`, `main-only` keeps slot 0's, `never` drops stale tags). Probing avoids issuing DELETE for a nonexistent manifest, which can materialize an empty repository on some registries. Then drop the finalizer. VM names to GC are stashed onto an annotation before pod deletion so the cleanup survives a CocoonSet deleted before `Status.Agents` was ever patched. 3. Ensure the `cocoonset.cocoonstack.io/finalizer` is in place. 4. List owned pods by `cocoonset.cocoonstack.io/name=`, drop any with stale labels that aren't actually controller-owned, and classify the rest by role label. 5. **Lifecycle-bridge stamp**: patch `cs.Generation` onto each owned pod's `cocoonset.cocoonstack.io/generation` annotation so vk-cocoon can echo it back as `lifecycle-observed-generation`, giving clients a counter-based completion signal immune to wallclock skew.