fix(rest): eager-place only on clone; bare restore stays an empty shell

PR #153 changed the bare snapshot-restore handler to synchronously
stamp a replica on EVERY snapshot node. That fixed the clone verb (a
one-shot CSI op with no follow-up autoplace) but regressed the staged
cross-node restore workflow: the e2e restore lanes place the
data-bearing replica on a snapshot node first, wait UpToDate, then add
a cross-node replica that SyncTargets it. With both snapshot nodes
already eagerly placed, place_count was satisfied and the cross-node
replica was never created (stuck Connecting / no devicePath).

Split the placement policy by caller. The clone path (cloneWithData)
eager-places on the snapshot nodes in the source pool. The bare
restore handler leaves an empty shell when no --node-name is given, so
the operator / linstor-csi drives placement — exactly the pre-#153 /
upstream restore-then-scale-out contract. Explicit node lists are
still stamped verbatim on both paths.

Cross-backend protection is unaffected: the placer's restore-source
backend pin keeps the operator's follow-up autoplace on the source
backend, and the REST autoplace handler still constrains a
no-node-list autoplace of a restore-marked RD to the snapshot nodes.

Co-Authored-By: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>

Author: kvaps

pkg/rest/rd_clone.go

@@ -226,7 +226,11 @@ func (s *Server) cloneWithData(w http.ResponseWriter, r *http.Request, src *apiv
 
 	restoreReq := &snapshotRestoreRequest{ToResource: req.Name}
 
-	_, err := s.materializeRestoredRD(ctx, src.Name, restoreReq, snap)
+	// Clone path: eagerPlace=true. `rd clone` is a one-shot CSI
+	// operation with no follow-up autoplace, so the clone replicas must
+	// materialise on the snapshot-holding nodes in the source pool here
+	// (same backend by construction — Bug 038).
+	_, err := s.materializeRestoredRD(ctx, src.Name, restoreReq, snap, true)
 	if err != nil {
 		writeCloneRefused(w, http.StatusInternalServerError, src.Name, req.Name, &apiv1.APICallRc{
 			RetCode: apiCallRcError,

pkg/rest/snapshot_restore.go

@@ -308,7 +308,11 @@ func (s *Server) handleSnapshotRestore(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	newRDName, err := s.materializeRestoredRD(r.Context(), srcRD, &req, &snap)
+	// Bare restore: eagerPlace=false. With no explicit --node-name the
+	// target is left an empty shell for the operator / linstor-csi to
+	// place (restore-then-scale-out); an explicit node list is still
+	// stamped verbatim inside materializeRestoredRD.
+	newRDName, err := s.materializeRestoredRD(r.Context(), srcRD, &req, &snap, false)
 	if err != nil {
 		writeStoreError(w, err)
 
@@ -411,7 +415,29 @@ func resolveSnapshotName(r *http.Request, req *snapshotRestoreRequest) string {
 // RD's LayerStack + Props (snapshot Props win when set) and hydrates
 // its VolumeDefinitions from the snapshot's recorded volume layout.
 // Returns the new RD's name on success.
-func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *snapshotRestoreRequest, snap *apiv1.Snapshot) (string, error) {
+//
+// eagerPlace selects the placement policy for an EMPTY caller node list:
+//
+//   - true (the clone path, cloneWithData): stamp one diskful replica on
+//     EVERY snapshot-holding node in the source pool. `rd clone` is a
+//     one-shot CSI operation with no follow-up autoplace, so the clone
+//     replicas must materialise here; keeping them on the snapshot nodes
+//     in the SOURCE backend is what closes Bug 038 (no cross-backend
+//     stream into `zfs recv`).
+//   - false (the bare snapshot-restore handler): leave an EMPTY shell —
+//     no Resources stamped — so the operator / linstor-csi drives
+//     placement explicitly via `rd ap`. This preserves upstream's
+//     restore-then-scale-out workflow and the legitimate STAGED
+//     cross-node bring-up the e2e restore lanes rely on (place the
+//     data-bearing replica on a snapshot node first, then add a
+//     cross-node replica that SyncTargets it). The placer's restore-
+//     source backend pin (constrainFilterToRestoreSource) keeps that
+//     later autoplace same-backend, so Bug 038 stays fixed without the
+//     eager all-nodes stamp.
+//
+// An explicit caller node list is always stamped verbatim, regardless of
+// eagerPlace.
+func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *snapshotRestoreRequest, snap *apiv1.Snapshot, eagerPlace bool) (string, error) {
 	srcRDObj, err := s.Store.ResourceDefinitions().Get(ctx, srcRD)
 	if err != nil {
 		return "", err //nolint:wrapcheck // surfaced via writeStoreError
@@ -464,7 +490,7 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s
 	// observed a Resource for the new RD, so the BlockstorRestoreFromSnapshot
 	// prop marker on the RD was dead code and the restored RD stayed an
 	// empty shell. Mirrors upstream CtrlSnapshotRestoreApiCallHandler.
-	err = s.placeRestoredResources(ctx, srcRD, &newRD, req, snap)
+	err = s.placeRestoredResources(ctx, srcRD, &newRD, req, snap, eagerPlace)
 	if err != nil {
 		return "", err
 	}
@@ -502,15 +528,29 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s
 //
 // The Nodes / NodeNames request fields are aliased — callers may use
 // either; we normalise to one canonical list before iterating.
-func (s *Server) placeRestoredResources(ctx context.Context, srcRDName string, newRD *apiv1.ResourceDefinition, req *snapshotRestoreRequest, snap *apiv1.Snapshot) error {
+func (s *Server) placeRestoredResources(ctx context.Context, srcRDName string, newRD *apiv1.ResourceDefinition, req *snapshotRestoreRequest, snap *apiv1.Snapshot, eagerPlace bool) error {
 	nodes := canonicalRestoreNodeList(req)
 
 	if len(nodes) == 0 {
-		// Upstream default: restore on every node the snapshot exists
-		// on. An empty snap.Nodes (legacy snapshot CRD without the
+		if !eagerPlace {
+			// Bare restore with no explicit nodes → EMPTY shell. The
+			// operator / linstor-csi drives placement via `rd ap`,
+			// which the placer keeps on the source backend
+			// (constrainFilterToRestoreSource). This preserves the
+			// upstream restore-then-scale-out workflow and the staged
+			// cross-node bring-up the e2e restore lanes rely on. _ =
+			// snap keeps the signature uniform with the eager branch.
+			_ = snap
+
+			return nil
+		}
+
+		// Clone path (eager): stamp one replica on every snapshot node
+		// in the source pool. `rd clone` is a one-shot CSI op with no
+		// follow-up autoplace, so the clone replicas must materialise
+		// here. An empty snap.Nodes (legacy snapshot CRD without the
 		// node list) stamps nothing — the operator can still drive
-		// placement explicitly via `linstor rd ap <new>`, which the
-		// autoplace handler constrains to snapshot-compatible pools.
+		// placement explicitly via `linstor rd ap <new>`.
 		nodes = snap.Nodes
 	}
 

pkg/rest/snapshot_restore_test.go

@@ -293,26 +293,27 @@ func TestSnapshotRestoreConstrainsProviderToSource(t *testing.T) {
 		t.Fatalf("restore status: got %d, want 201", resp.StatusCode)
 	}
 
-	// 2) the restore itself stamped replicas on the snapshot's nodes
-	// in the SOURCE pool (Bug 038 / upstream restore semantics).
+	// 2) the bare restore (no --node-name) leaves an EMPTY shell — the
+	// operator / linstor-csi drives placement, preserving the
+	// restore-then-scale-out workflow and the staged cross-node
+	// bring-up the e2e restore lanes rely on. The cross-backend
+	// protection moved to the placer, asserted in step 3.
 	got, err := st.Resources().ListByDefinition(ctx, "pvc-2")
 	if err != nil {
 		t.Fatalf("list pvc-2: %v", err)
 	}
 
-	if len(got) != 2 {
-		t.Fatalf("restored replicas: got %d, want 2 (one per snapshot node)", len(got))
-	}
-
-	for i := range got {
-		if stor := got[i].Props["StorPoolName"]; stor != "zpool" {
-			t.Errorf("replica on %s landed on %q, want the source pool %q",
-				got[i].NodeName, stor, "zpool")
-		}
+	if len(got) != 0 {
+		t.Fatalf("bare restore replicas: got %d, want 0 (empty shell — "+
+			"placement is the operator's call)", len(got))
 	}
 
-	// 3) a follow-up autoplace at the same place_count is an
-	// idempotent no-op — no extra replicas, no pool churn.
+	// 3) the operator's autoplace honours the restore-source BACKEND
+	// pin: even though the LVM_THIN candidates are roomier (FreeCapacity
+	// 9000 vs the ZFS_THIN 1000), every diskful replica must land on a
+	// ZFS_THIN pool — the placer's constrainFilterToRestoreSource drops
+	// the mismatched backend (Bug 038: a ZFS snapshot stream piped into
+	// an LVM receiver fails opaquely).
 	body, _ = json.Marshal(map[string]any{
 		"select_filter": map[string]any{"place_count": 2},
 	})
@@ -330,7 +331,16 @@ func TestSnapshotRestoreConstrainsProviderToSource(t *testing.T) {
 	}
 
 	if len(got) != 2 {
-		t.Fatalf("replicas after idempotent autoplace: got %d, want 2", len(got))
+		t.Fatalf("replicas after autoplace: got %d, want 2", len(got))
+	}
+
+	for i := range got {
+		stor := got[i].Props["StorPoolName"]
+		if stor != "zfs-target-n1" && stor != "zfs-target-n2" {
+			t.Errorf("replica on %s landed on %q, want a ZFS_THIN target "+
+				"(the LVM_THIN decoy must be filtered by the backend pin)",
+				got[i].NodeName, stor)
+		}
 	}
 }
 
@@ -922,39 +932,51 @@ func TestSnapshotRestoreBug354AutoplacesWhenNodesEmpty(t *testing.T) {
 		t.Fatalf("status: got %d, want 201", resp.StatusCode)
 	}
 
+	// The bare restore (no --node-name) leaves an EMPTY shell — the
+	// operator / linstor-csi drives placement. (Bug 354 was about
+	// stamping SOMETHING so satellites reconcile; the explicit
+	// autoplace below does that, while the placer's backend pin keeps
+	// it on the source backend — the Bug 038 fix without the eager
+	// all-nodes stamp that broke staged cross-node bring-up.)
 	got, err := st.Resources().ListByDefinition(ctx, "pvc-restored")
 	if err != nil {
 		t.Fatalf("list restored Resources: %v", err)
 	}
 
-	if len(got) != 2 {
-		t.Fatalf("Resource CRDs stamped: got %d, want %d (one per snapshot node)",
-			len(got), 2)
+	if len(got) != 0 {
+		t.Fatalf("bare restore Resource CRDs: got %d, want 0 (empty shell)", len(got))
 	}
 
-	// Both replicas must land on the snapshot's nodes, in the
-	// SOURCE pool — never on the roomier different-backend decoy
-	// (Bug 038: a FILE_THIN→ZFS placement loops forever on
-	// `zfs recv … bad magic number` at the satellite).
-	wantNodes := map[string]bool{"n1": false, "n2": false}
-	for _, res := range got {
-		if _, ok := wantNodes[res.NodeName]; !ok {
-			t.Errorf("placed off snapshot node set: got %q", res.NodeName)
+	// Operator autoplace: with the parent RG's empty SelectFilter the
+	// caller supplies place_count explicitly. The placer's restore-
+	// source backend pin must keep every replica on the ZFS_THIN source
+	// pool, never the roomier LVM_THIN decoy (Bug 038: a ZFS snapshot
+	// stream into an LVM receiver fails opaquely at the satellite).
+	body, _ = json.Marshal(map[string]any{
+		"select_filter": map[string]any{"place_count": 2},
+	})
 
-			continue
-		}
+	resp = httpPost(t, base+"/v1/resource-definitions/pvc-restored/autoplace", body)
+	_ = resp.Body.Close()
 
-		wantNodes[res.NodeName] = true
+	if resp.StatusCode != http.StatusOK {
+		t.Fatalf("autoplace status: got %d, want 200", resp.StatusCode)
+	}
 
-		if pool := res.Props["StorPoolName"]; pool != "zpool" {
-			t.Errorf("replica on %s landed on pool %q, want the source pool %q",
-				res.NodeName, pool, "zpool")
-		}
+	got, err = st.Resources().ListByDefinition(ctx, "pvc-restored")
+	if err != nil {
+		t.Fatalf("list restored Resources after autoplace: %v", err)
+	}
+
+	if len(got) != 2 {
+		t.Fatalf("Resource CRDs after autoplace: got %d, want 2", len(got))
 	}
 
-	for n, placed := range wantNodes {
-		if !placed {
-			t.Errorf("expected a replica on snapshot node %q, none placed", n)
+	for _, res := range got {
+		if pool := res.Props["StorPoolName"]; pool != "zpool" {
+			t.Errorf("replica on %s landed on pool %q, want the source pool %q "+
+				"(the LVM_THIN decoy must be filtered by the backend pin)",
+				res.NodeName, pool, "zpool")
 		}
 	}
 }