fix(clone): land clone/restore replicas on snapshot nodes in the source pool

[Andrei Kvapil (kvaps)]

Problem

rd clone of a deployed, VD-bearing source on a FILE_THIN pool could materialise a target replica on a ZFS pool. The target satellite then piped the source's FILE_THIN snapshot stream into zfs recv, which loops forever on cannot receive: invalid stream (bad magic number) — the clone replicas never reach UpToDate. Reproduced on two stands via tests/e2e/cli-matrix/rd-clone-vd-data-plane.sh.

Root cause

Two placement gaps combined:

1. The snapshot-restore placement branch for the empty-node-list case (which the clone data plane reuses) deferred to the parent ResourceGroup's SelectFilter.PlaceCount. Under the stand-default empty-spec DfltRscGrp it placed zero replicas.
2. The controller-side ResourceGroup reconcilers then ran the placer with the raw RG filter — no snapshot-node constraint, no provider pin — so the capacity-weighted ranking could pick a pool of a different backend. Snapshot stream formats are not interchangeable across backends, so the cross-node restore fed a foreign stream into the ZFS receiver.

Fix (upstream-faithful)

Verified against the live LINSTOR 1.33.2 oracle: an upstream restore without an explicit node list lands on exactly the snapshot-holding nodes, in the snapshot's own storage pool, and never consults the autoplacer; upstream rd clone likewise operates on the source's own pools (it refuses sources on pools it cannot clone with "Clone source contains unsupported storage pools").

• pkg/rest: restore/clone with no explicit nodes now stamps one replica per snapshot-holding node, pinned to the source replica's storage pool — the clone data plane is node-local and same-backend by construction, independent of the RG's place_count.
• pkg/placer: Place() now applies the restore-source constraints (snapshot nodes + source provider kind) internally, so every caller — REST autoplace, RG apply, RG rebalance, node replacement — honours them. The REST handler's provider lookup delegates to the shared placer helper.
• harness: the cell aborted with cli_nodes[0]: unbound variable on failure (mapfile from a process substitution swallows the exit status), masking the real diagnostic; failures now surface the wait function's own message.
• docs/cli-parity-known-deltas.md row 82 updated with the placement semantics.

Validation

• L1: new regression tests in pkg/rest/clone_placement_bug038_test.go, pkg/placer/restore_source_constraints_bug038_test.go; updated restore-placement pins. go test ./... green, golangci-lint clean, -race green on satellite/placer/rest.
• L6: cli-matrix/rd-clone-vd-data-plane.sh green 3x on stand big at this branch's build (both wire variants: plain CLI clone and raw REST use_zfs_clone=true, byte-level marker verified on every replica). cli-matrix/r-full-lifecycle.sh green (placer/autoplace ground-truth gate).
• L7: replay/rd-clone-vd-data-plane.yaml PASS on stand big.

Summary by CodeRabbit

• New Features

  • Added snapshot-readiness gating to prevent race conditions during restore and clone operations.
  • Enhanced restore source backend constraints to pin replica placement to the source's storage pool.

• Bug Fixes

  • Fixed clone replica placement to land on snapshot-holding nodes in the source pool (Bug-038).
  • Improved local snapshot restoration with bounded retry logic before fallback.
  • Corrected restore operations to create empty shells initially, allowing operator-driven placement.
  • Fixed resource listing to properly handle mixed labeled and unlabeled replicas.

• Tests

  • Added comprehensive regression test coverage for clone/restore placement behavior.

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR implements Bug 038: a snapshot-based clone/restore mechanism that pins replica placement to the source pool backend, gates operations on snapshot materialization readiness, introduces satellite requeue defense for missing local snapshots, and fixes store resource listing to correctly handle unlabeled resources. The changes span placer constraints, REST handlers, satellite reconciliation, and data store operations.

Changes

Bug 038 - Snapshot clone/restore backend pinning and safety gates

Layer / File(s)	Summary
Placer restore-source backend constraint mechanism pkg/placer/placer.go, pkg/placer/restore_source_constraints_bug038_test.go, .golangci.yml	Placer.Place now applies backend constraints for restore-marked RDs by calling constrainFilterToRestoreSource, which resolves the source pool backend via SourceProviderKind and overrides the filter's ProviderList. Tests verify constraint application without filter mutation and that unmarked RDs preserve free placement.
Snapshot-readiness polling and gating pkg/rest/server.go, pkg/rest/snapshot_ready.go, pkg/rest/snapshot_ready_test.go, pkg/rest/nodes_test.go	Server gains SnapshotReadyTimeout/SnapshotReadyPoll tuning fields; waitSnapshotReadyOnNodes polls snapshot per-node CreateTimestamp entries until all target nodes materialize or timeout occurs, degrading gracefully. Tests verify polling correctness, timeout bounds, and empty-list no-op behavior.
Clone eager placement and snapshot-based stamping pkg/rest/rd_clone.go, pkg/rest/clone_placement_bug038_test.go	Clone requests now pass eagerPlace=true to materializeRestoredRD, which stamps replicas on snapshot-holding nodes with per-node backend-matched storage pools after waiting for snapshot readiness. Integration test verifies clones land on snapshot nodes in source backend pool, not on decoy pools.
Restore bare-shell semantics and backend-pinned stamping pkg/rest/snapshot_restore.go, pkg/rest/snapshot_restore_test.go	Snapshot restore distinguishes bare-restore (eagerPlace=false, empty shell + operator placement) from clone (eagerPlace=true, eager one-shot). placeRestoredResources branches: explicit nodes stamp on those nodes; empty list with eagerPlace=false stamps nothing; empty list with eagerPlace=true uses snap.Nodes with readiness wait. Per-node StorPoolName selection uses storPoolsByNodeFromSourceRD mapping. Tests verify bare-restore leaves empty shell and operator autoplace honors backend pin.
REST autoplace refactoring to delegate provider-kind resolution pkg/rest/autoplace.go	resolveCloneSourceProviderKind now delegates to placer.SourceProviderKind helper instead of inlining resource/pool iteration, simplifying the REST handler by reusing shared placer logic.
Satellite bounded requeue defense for missing snapshots pkg/satellite/reconciler.go, pkg/satellite/bug_397_restore_seed_test.go, pkg/satellite/cross_cluster_ship_test.go, pkg/satellite/ship_dispatch_test.go	Reconciler tracks restoreSnapshotMissingTries counter per resource/volume to bound requeue passes when local snapshot sources are missing. When RestoreVolumeFromSnapshot fails with ErrNotFound and no cross-node fetcher is present, reconcile requeues up to restoreSnapshotMissingBudget passes before falling back to blank CreateVolume and recording the blank-fallback marker. Tests verify requeue continues in budget and blank fallback fires only after budget exhaustion.
Store resource listing label-based lookup fix pkg/store/k8s/resources.go, pkg/store/k8s/list_by_definition_mixed_label_bug038_test.go	ListByDefinition now unconditionally lists all Resource CRDs and filters in-memory by ResourceDefinitionName, removing the label-selector fast path that could omit unlabeled resources created via kubectl apply or non-controller paths. Regression tests verify mixed labeled/unlabeled lists and kubectl-apply-only pool specs are correctly included.

Documentation and test tooling updates

Layer / File(s)	Summary
Documentation for clone/restore backend-pinning semantics docs/cli-parity-known-deltas.md	CLI parity delta #82 updated to explicitly document Bug 038 backend pinning: clone/restore replicas are stamped on snapshot-holding nodes in the source replica's storage pool, with refusal of cross-backend placement due to snapshot stream format incompatibility.
Shell script error propagation in e2e clone test tests/e2e/cli-matrix/rd-clone-vd-data-plane.sh	Both clone variants now capture wait_clone_replicas output into intermediate variables before mapfile array population, ensuring exit errors propagate under set -euo pipefail instead of being masked.

🎯 4 (Complex) | ⏱️ ~60 minutes

🐰 Replicas now follow their snapshot homes,
With readiness gates and requeue domes,
Backend pools pinned, no cross-stream sins,
Snapshot defense where patience begins. ✨

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely summarizes the main fix: ensuring clone/restore replicas land on snapshot nodes in the source pool, which directly addresses the bug described in the PR objectives.
Docstring Coverage	✅ Passed	Docstring coverage is 96.30% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/bug-038-rd-clone-cross-backend

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gemini-code-assist]

Code Review

This pull request addresses Bug 038 by enforcing restore-source constraints directly within the placer's Place method and refactoring the REST snapshot-restore handlers to place replicas on all snapshot-holding nodes in the source's storage pool by default. This prevents cross-backend placement issues, such as FILE_THIN snapshots being restored onto ZFS pools. The changes are accompanied by comprehensive unit and integration tests, as well as a minor fix to bash scripting in the E2E tests. Feedback on the implementation points out a potential nil pointer dereference in constrainFilterToRestoreSource if the filter parameter is nil.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

The filter parameter is dereferenced as out := *filter on line 1799. If filter is nil, this will cause a nil pointer dereference panic. To prevent this, add a defensive nil check at the beginning of the function.

func (p *Placer) constrainFilterToRestoreSource(ctx context.Context, rdName string, filter *apiv1.AutoSelectFilter) *apiv1.AutoSelectFilter {
	if filter == nil {
		return nil
	}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/satellite/reconciler.go (1)

1554-1564: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset the missing-snapshot budget on any non-ErrNotFound outcome.

restoreSnapshotMissingTries is documented as a count of consecutive local-miss passes, but this branch only clears it on success. A sequence like ErrNotFound → ErrNotFound → transient restore error → ErrNotFound resumes at the old count and can hit the blank-create fallback early, recreating the empty-target path this backstop is supposed to avoid.

Proposed fix

 	if !errors.Is(err, storage.ErrNotFound) {
+		// Break the "consecutive local snapshot miss" streak on any other outcome.
+		r.clearRestoreSnapshotMissing(rdName, vol.GetVolumeNumber())
+
 		if err == nil {
 			// Local clone succeeded: this replica holds the snapshot's
 			// data. Clear any stale blank-fallback marker so the
 			// legitimate all-clone restore fast-path keeps its skip, and
 			// reset the Bug 038 not-yet-materialised requeue budget.
 			r.recordRestoreBlankFallback(rdName, vol.GetVolumeNumber(), false)
-			r.clearRestoreSnapshotMissing(rdName, vol.GetVolumeNumber())
 		}
 
 		return err //nolint:wrapcheck // caller wraps
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/satellite/reconciler.go` around lines 1554 - 1564, The branch that
handles non-ErrNotFound outcomes currently only calls
r.clearRestoreSnapshotMissing(...) when err == nil; change this so the
missing-snapshot counter is cleared on any non-ErrNotFound outcome (i.e., when
!errors.Is(err, storage.ErrNotFound)), not just on success. Concretely, keep
r.recordRestoreBlankFallback(...) inside the err == nil block, but move or add
r.clearRestoreSnapshotMissing(rdName, vol.GetVolumeNumber()) to the outer
!errors.Is(err, storage.ErrNotFound) branch so it runs for both success and
transient errors before returning err from the function that contains these
calls.

🧹 Nitpick comments (1)

pkg/rest/snapshot_restore_test.go (1)

214-224: ⚡ Quick win

Align test docstrings with the current bare-restore behavior.

These comments still describe eager replica stamping, but the test body now asserts empty-shell restore followed by explicit autoplace. Please update wording to avoid future misreads of the intended contract.

✏️ Suggested comment-only fix

-// retargeted by Bug 038: the restore no longer defers placement to
-// the autoplacer — it stamps replicas on the snapshot's nodes in the
-// source replica's pool up front (...)
+// retargeted by Bug 038: bare restore with no explicit nodes now
+// leaves an empty shell, and follow-up autoplace must honor the
+// restore-source backend pin (never crossing backend kind).

-// empty-nodes branch: no `--node-name` arguments → the handler stamps
-// one replica on EVERY node holding the snapshot (...)
+// empty-nodes branch: no `--node-name` arguments → the handler creates
+// an empty shell; placement is done explicitly via autoplace and must
+// stay pinned to the source backend.

Also applies to: 843-849

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/rest/snapshot_restore_test.go` around lines 214 - 224, The docstring for
TestSnapshotRestoreConstrainsProviderToSource is outdated: it describes eager
replica stamping on restore but the test now asserts an empty-shell restore
followed by an explicit autoplace; update the comment above that test (and the
similar block at lines 843-849) to describe the current behavior—that restores
create an empty-shell in the source pool and then require an explicit autoplace
to populate replicas—removing any language about stamping replicas eagerly or
deferring placement to the autoplacer.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@pkg/satellite/reconciler.go`:
- Around line 1554-1564: The branch that handles non-ErrNotFound outcomes
currently only calls r.clearRestoreSnapshotMissing(...) when err == nil; change
this so the missing-snapshot counter is cleared on any non-ErrNotFound outcome
(i.e., when !errors.Is(err, storage.ErrNotFound)), not just on success.
Concretely, keep r.recordRestoreBlankFallback(...) inside the err == nil block,
but move or add r.clearRestoreSnapshotMissing(rdName, vol.GetVolumeNumber()) to
the outer !errors.Is(err, storage.ErrNotFound) branch so it runs for both
success and transient errors before returning err from the function that
contains these calls.

---

Nitpick comments:
In `@pkg/rest/snapshot_restore_test.go`:
- Around line 214-224: The docstring for
TestSnapshotRestoreConstrainsProviderToSource is outdated: it describes eager
replica stamping on restore but the test now asserts an empty-shell restore
followed by an explicit autoplace; update the comment above that test (and the
similar block at lines 843-849) to describe the current behavior—that restores
create an empty-shell in the source pool and then require an explicit autoplace
to populate replicas—removing any language about stamping replicas eagerly or
deferring placement to the autoplacer.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c0bf53d6-2f17-4186-9475-662ba484be79

📥 Commits

Reviewing files that changed from the base of the PR and between 170751b and 24baa83.

📒 Files selected for processing (20)

• .golangci.yml
• docs/cli-parity-known-deltas.md
• pkg/placer/placer.go
• pkg/placer/restore_source_constraints_bug038_test.go
• pkg/rest/autoplace.go
• pkg/rest/clone_placement_bug038_test.go
• pkg/rest/nodes_test.go
• pkg/rest/rd_clone.go
• pkg/rest/server.go
• pkg/rest/snapshot_ready.go
• pkg/rest/snapshot_ready_test.go
• pkg/rest/snapshot_restore.go
• pkg/rest/snapshot_restore_test.go
• pkg/satellite/bug_397_restore_seed_test.go
• pkg/satellite/cross_cluster_ship_test.go
• pkg/satellite/reconciler.go
• pkg/satellite/ship_dispatch_test.go
• pkg/store/k8s/list_by_definition_mixed_label_bug038_test.go
• pkg/store/k8s/resources.go
• tests/e2e/cli-matrix/rd-clone-vd-data-plane.sh