feat(clone): reap the internal snapshot a clone took on its source

internal/cli/definition.go

@@ -22,13 +22,15 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"maps"
 	"slices"
 	"sort"
 	"strings"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
 	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
 
 	"github.com/cozystack/blockstor/internal/cli/command"
 	"github.com/cozystack/blockstor/internal/cli/view"
@@ -156,7 +158,7 @@ func resourceDefinitionClone(ctx context.Context, run *runContext) error {
 
 	snapName := cloneSnapshotName(target)
 
-	err = ensureCloneSnapshot(ctx, run, &src, snapName)
+	err = ensureCloneSnapshot(ctx, run, &src, snapName, target)
 	if err != nil {
 		return err
 	}
@@ -234,7 +236,9 @@ func checkCloneSnapshotIsCurrent(
 	return nil
 }
 
-func ensureCloneSnapshot(ctx context.Context, run *runContext, src *apiv1.ResourceDefinition, snapName string) error {
+func ensureCloneSnapshot(
+	ctx context.Context, run *runContext, src *apiv1.ResourceDefinition, snapName, target string,
+) error {
 	existing, err := run.Store.Snapshots().Get(ctx, src.Name, snapName)
 	if err == nil {
 		return checkCloneSnapshotIsCurrent(ctx, run, src, &existing)
@@ -255,6 +259,15 @@ func ensureCloneSnapshot(ctx context.Context, run *runContext, src *apiv1.Resour
 		return err
 	}
 
+	// Stamped as the clone's own, the way the REST door stamps it, so that
+	// deleting the clone may reap it; see store.CloneSnapshotOwnerProp.
+	snap.Props = maps.Clone(snap.Props)
+	if snap.Props == nil {
+		snap.Props = map[string]string{}
+	}
+
+	snap.Props[store.CloneSnapshotOwnerProp] = target
+
 	err = run.Store.Snapshots().Create(ctx, snap)
 	if err != nil {
 		return fmt.Errorf("create clone snapshot %s: %w", snapName, err)

internal/cli/rd_delete_clone_snapshot_test.go

@@ -0,0 +1,112 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package cli_test
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+// `rd clone` takes the same internal snapshot the REST door takes, and `rd d`
+// of the clone reaped nothing, so the source could never be deleted through
+// the CLI either.
+func TestResourceDefinitionDeleteOfACloneReapsItsSnapshot(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, seedSnapshotSource)
+
+	if got := app.Run(t.Context(), []string{"rd", "clone", "pvc-x", "pvc-reap"}); got != 0 {
+		t.Fatalf("clone exit = %d (stderr: %s)", got, errBuf.String())
+	}
+
+	// The owner prop is the snapshot's. Carried onto the clone, it would be
+	// copied into every snapshot later taken of the clone as a claim of
+	// ownership nobody made.
+	clone, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-reap")
+	if err != nil {
+		t.Fatalf("get the clone: %v", err)
+	}
+
+	if owner, ok := clone.Props[store.CloneSnapshotOwnerProp]; ok {
+		t.Errorf("the clone carries the snapshot's owner prop %q", owner)
+	}
+
+	if got := app.Run(t.Context(), []string{"rd", "d", "pvc-reap"}); got != 0 {
+		t.Fatalf("delete of the clone exit = %d (stderr: %s)", got, errBuf.String())
+	}
+
+	backend := appStore(t, app)
+
+	if _, err := backend.Snapshots().Get(t.Context(), "pvc-x", "clone-pvc-reap"); !errors.Is(err, store.ErrNotFound) {
+		t.Errorf("the internal snapshot outlived the clone it backed: %v", err)
+	}
+
+	// seedSnapshotSource also leaves an operator snapshot on pvc-x, which the
+	// delete has to keep refusing over. The clone's own must not be among what
+	// it names.
+	app.Run(t.Context(), []string{"rd", "d", "pvc-x"})
+
+	if strings.Contains(errBuf.String(), "clone-pvc-reap") {
+		t.Errorf("the source delete still meets the clone's snapshot: %s", errBuf.String())
+	}
+}
+
+// An operator can name a snapshot anything, `clone-<target>` included, and
+// restore it under that target. Only the prop the clone path writes makes a
+// snapshot the clone's to reap.
+func TestResourceDefinitionDeleteKeepsAnOperatorSnapshotNamedLikeAClone(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) {
+		seedSnapshotSource(ctx, backend)
+		_ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{
+			Name: "clone-pvc-op", ResourceName: "pvc-x",
+			Nodes:             []string{"node-1", "node-2"},
+			VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}},
+		})
+	})
+
+	argv := []string{
+		"s", "resource", "restore",
+		"--from-resource", "pvc-x", "--from-snapshot", "clone-pvc-op", "--to-resource", "pvc-op",
+	}
+
+	if got := app.Run(t.Context(), argv); got != 0 {
+		t.Fatalf("restore exit = %d (stderr: %s)", got, errBuf.String())
+	}
+
+	if got := app.Run(t.Context(), []string{"rd", "d", "pvc-op"}); got != 0 {
+		t.Fatalf("delete exit = %d (stderr: %s)", got, errBuf.String())
+	}
+
+	if _, err := appStore(t, app).Snapshots().Get(t.Context(), "pvc-x", "clone-pvc-op"); err != nil {
+		t.Errorf("the operator's snapshot was reaped because of its name: %v", err)
+	}
+}
+
+// The CLI refusal over a source still carrying a snapshot its clone left had no
+// case that reached it: every fixture's list came back empty.
+func TestResourceDefinitionDeleteNamesACloneSnapshotLeftBehind(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) {
+		_ = backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-left"})
+		_ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{
+			Name: "clone-pvc-gone", ResourceName: "pvc-left",
+			Props: map[string]string{store.CloneSnapshotOwnerProp: "pvc-gone"},
+		})
+	})
+
+	if got := app.Run(t.Context(), []string{"rd", "d", "pvc-left"}); got == 0 {
+		t.Fatal("delete of a source still carrying a snapshot succeeded")
+	}
+
+	if !strings.Contains(errBuf.String(), "clone-pvc-gone") {
+		t.Errorf("refusal %q does not name the snapshot the clone left", errBuf.String())
+	}
+}

internal/cli/snapshot.go

@@ -347,6 +347,13 @@ func snapshotRestoreResource(ctx context.Context, run *runContext) error {
 		return fmt.Errorf("create resource definition %s: %w", def.Name, err)
 	}
 
+	// The snapshot was read before the definition existed; see
+	// store.ReapClonedSnapshot.
+	err = store.RestoreSourceWithdrawn(ctx, run.Store, snap.ResourceName, snap.Name)
+	if err != nil {
+		return rollbackRestore(ctx, run, def.Name, planned, err)
+	}
+
 	// The definition is this command's own, but a concurrent run may already
 	// have adopted it and be hydrating the same volumes. A volume that is
 	// already there at the snapshot's size is the restore's, not a collision
@@ -387,6 +394,10 @@ func newRestoredDefinition(rdName string, src *apiv1.ResourceDefinition, snap *a
 	// the placer looks the source half up as a store key.
 	def.Props = store.WithRestoreMarker(def.Props, snap)
 
+	// The owner prop belongs to the snapshot; see store.CloneSnapshotOwnerProp.
+	delete(def.Props, store.CloneSnapshotOwnerProp)
+	delete(def.Props, store.CloneSnapshotReapingProp)
+
 	return def
 }
 
@@ -469,6 +480,21 @@ func restoreIntoExistingTarget(
 		return finishConcurrentRestore(ctx, run, args, existing.Name, snap, createErr)
 	}
 
+	return placeOverMarkedTarget(ctx, run, existing.Name, snap, planned)
+}
+
+// placeOverMarkedTarget places the planned replicas on a target that carries
+// this restore's marker. The snapshot was read before the marker went on, or
+// before this run found it there, so it is read back first; see
+// store.ReapClonedSnapshot.
+func placeOverMarkedTarget(
+	ctx context.Context, run *runContext, rdName string, snap *apiv1.Snapshot, planned []apiv1.Resource,
+) error {
+	err := store.RestoreSourceWithdrawn(ctx, run.Store, snap.ResourceName, snap.Name)
+	if err != nil {
+		return fmt.Errorf("restore into %s: %w", rdName, err)
+	}
+
 	return placeOverExisting(ctx, run, planned)
 }
 
@@ -630,7 +656,7 @@ func finishRestoreLeftover(
 		return err
 	}
 
-	return placeOverExisting(ctx, run, planned)
+	return placeOverMarkedTarget(ctx, run, rdName, snap, planned)
 }
 
 // answerFinishedLeftover reports a finished leftover of this restore as

internal/cli/snapshot_restore_withdraw_test.go

@@ -0,0 +1,81 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package cli_test
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+	"time"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+// The CLI restore reads the snapshot back after it creates the definition,
+// the same way the REST door does, and withdraws from one a reap has marked.
+func TestSnapshotRestoreWithdrawsFromASnapshotBeingReaped(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) {
+		seedSnapshotSource(ctx, backend)
+		_ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{
+			Name: "clone-pvc-old", ResourceName: "pvc-x",
+			Nodes:             []string{"node-1", "node-2"},
+			VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}},
+			Props: map[string]string{
+				store.CloneSnapshotOwnerProp:   "pvc-old",
+				store.CloneSnapshotReapingProp: "pvc-old@" + time.Now().UTC().Format(time.RFC3339),
+			},
+		})
+	})
+
+	if got := app.Run(t.Context(), []string{
+		"s", "resource", "restore", "--from-resource", "pvc-x",
+		"--from-snapshot", "clone-pvc-old", "--to-resource", "pvc-third",
+	}); got == 0 {
+		t.Fatal("restore from a snapshot being reaped succeeded")
+	}
+
+	if !strings.Contains(errBuf.String(), "being deleted") {
+		t.Errorf("refusal %q does not say the snapshot is going", errBuf.String())
+	}
+
+	if _, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-third"); !errors.Is(err, store.ErrNotFound) {
+		t.Errorf("the definition restored from a snapshot being reaped was left behind: %v", err)
+	}
+}
+
+// The CLI restore drops the snapshot's reaping mark the way the REST door does.
+func TestSnapshotRestoreDropsAnExpiredReapingMark(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) {
+		seedSnapshotSource(ctx, backend)
+		_ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{
+			Name: "snap-stale", ResourceName: "pvc-x",
+			Nodes:             []string{"node-1", "node-2"},
+			VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}},
+			Props: map[string]string{
+				store.CloneSnapshotReapingProp: "gone@" + time.Now().Add(-time.Hour).UTC().Format(time.RFC3339),
+			},
+		})
+	})
+
+	if got := app.Run(t.Context(), []string{
+		"s", "resource", "restore", "--from-resource", "pvc-x",
+		"--from-snapshot", "snap-stale", "--to-resource", "pvc-stale",
+	}); got != 0 {
+		t.Fatalf("restore over an expired mark exit = %d (stderr: %s)", got, errBuf.String())
+	}
+
+	rd, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-stale")
+	if err != nil {
+		t.Fatalf("read the restored definition: %v", err)
+	}
+
+	if mark, ok := rd.Props[store.CloneSnapshotReapingProp]; ok {
+		t.Errorf("the restored definition carries the snapshot's reaping mark %q", mark)
+	}
+}

internal/cli/write.go

@@ -340,10 +340,27 @@ func resourceDefinitionDelete(ctx context.Context, run *runContext) error {
 	}
 
 	if len(snaps) > 0 {
+		// A snapshot a clone took for itself outlives the clone when its
+		// reap was skipped, failed or kept it for a restore; nothing else
+		// names it now.
+		left, leftErr := store.CloneSnapshotsLeftBehind(ctx, run.Store, snaps)
+		if leftErr != nil {
+			_, _ = fmt.Fprintf(run.Err, "warning: could not tell which snapshots clones left behind: %v\n", leftErr)
+		} else if !left.Empty() {
+			cause, correction := left.Explain(name)
+
+			return fmt.Errorf("%w: %s has %d snapshot(s): %s; %s",
+				errDefinitionHasSnapshots, name, len(snaps), cause, correction)
+		}
+
 		return fmt.Errorf("%w: %s has %d snapshot(s); delete them first",
 			errDefinitionHasSnapshots, name, len(snaps))
 	}
 
+	// Read before the delete: the definition's own props are the only record
+	// of the internal snapshot a clone took on its source.
+	clonedFrom := store.OwnedCloneSnapshot(ctx, run.Store, name)
+
 	// Then the replicas. Dropping the definition alone leaves them with no
 	// parent to stamp a deletion on, so the satellite finalizer never runs
 	// and the DRBD minor, port and peer entries stay live on every node.
@@ -357,6 +374,15 @@ func resourceDefinitionDelete(ctx context.Context, run *runContext) error {
 		return fmt.Errorf("delete resource definition %s: %w", name, err)
 	}
 
+	// The internal snapshot the clone took on its source goes with it, the
+	// way the REST door reaps it, or the source could never be deleted again.
+	// Not fatal: the definition is gone, and a kept snapshot is named by the
+	// refusal a later delete of the source meets.
+	reapErr := store.ReapClonedSnapshot(ctx, run.Store, clonedFrom)
+	if reapErr != nil {
+		_, _ = fmt.Fprintf(run.Err, "warning: internal clone snapshot kept: %v\n", reapErr)
+	}
+
 	return nil
 }
 

pkg/api/v1/snapshot.go

@@ -129,4 +129,7 @@ const (
 	SnapshotFlagFailedDeployment = "FAILED_DEPLOYMENT"
 	SnapshotFlagFailedDisconnect = "FAILED_DISCONNECT"
 	SnapshotFlagSuccessful       = "SUCCESSFUL"
+	// SnapshotFlagDelete marks a snapshot whose delete was issued and has
+	// not finished: a finalizer still holds it.
+	SnapshotFlagDelete = "DELETE"
 )

pkg/rest/cache_invalidation_bug_124_test.go

@@ -151,6 +151,10 @@ func (l *laggingRDs) List(ctx context.Context) ([]apiv1.ResourceDefinition, erro
 	return l.inner.List(ctx) //nolint:wrapcheck // test helper
 }
 
+func (l *laggingRDs) ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) {
+	return l.inner.ListUncached(ctx) //nolint:wrapcheck // test helper
+}
+
 func (l *laggingRDs) Get(ctx context.Context, name string) (apiv1.ResourceDefinition, error) {
 	return l.inner.Get(ctx, name) //nolint:wrapcheck // test helper
 }

pkg/rest/rd_clone.go

@@ -1474,6 +1474,8 @@ func (s *Server) ensureCloneSnapshot(
 		return nil, false, false
 	}
 
+	stampCloneSnapshotOwner(&snap, cloneName)
+
 	snap.Snapshots = makeSnapshotPerNode(snapName, snap.Nodes, snap.VolumeDefinitions)
 
 	err = s.Store.Snapshots().Create(ctx, &snap)
@@ -1490,6 +1492,20 @@ func (s *Server) ensureCloneSnapshot(
 	return &snap, false, true
 }
 
+// stampCloneSnapshotOwner marks the snapshot as the clone's own, so deleting
+// the clone may reap it. A name cannot carry that: an operator can call a
+// snapshot `clone-<target>` and restore it under that target, and the marker a
+// restore writes is the one a clone writes. The map is copied first, since the
+// hydration hands over the source definition's own.
+func stampCloneSnapshotOwner(snap *apiv1.Snapshot, cloneName string) {
+	snap.Props = maps.Clone(snap.Props)
+	if snap.Props == nil {
+		snap.Props = map[string]string{}
+	}
+
+	snap.Props[store.CloneSnapshotOwnerProp] = cloneName
+}
+
 // cloneSourceIsNotBeingDeleted mirrors the snapshot-create Bug 180 gate: a
 // source RD mid-tear-down would reap the internal snapshot and the clone
 // marker from under the satellite's restore.