diff --git a/docs/cli-parity-known-deltas.md b/docs/cli-parity-known-deltas.md index 3e6416d8..653d17b8 100644 --- a/docs/cli-parity-known-deltas.md +++ b/docs/cli-parity-known-deltas.md @@ -35,6 +35,7 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see | 66 | `vd drbd-options --` (option-class enforcement) | VALIDATION | 2026-12-31 | Corner-case C6: BS does not reject a DRBD option set at the wrong object level (e.g. a `net{}`-class `--protocol` on a volume-definition, which accepts only disk/peer-device classes). BS merges the prop permissively; upstream rejects with a typed FAIL_INVLD_PROP. Pinned permissive behavior with a regression test pending the per-object-class validator. | | 67 | `r c ` (StorPoolName resolution chain) | BEHAVIOR | permanent | Corner D5: upstream resolves the pool via VD → Resource → RD → Node → literal `DfltStorPool`. BS resolves via sibling-diskful-replica → RG `SelectFilter.StoragePool` → RG `StoragePoolList[0]` (`resolveStorPoolForFreshCreate`). BS does NOT honor a per-VD `StorPoolName` prop (no two-volumes-in-two-pools layout) and has no literal `DfltStorPool` terminal fallback — an unresolvable pool yields an empty `StorPoolName` (a named-but-missing pool 404s via `refuseResourceCreateOnUnknownPool`). CSI always supplies the pool through the RG path, so the divergent source-chain is operator-invisible for Cozystack. | | 68 | `controller set-property Autoplacer/Weights/*` (default weights) | BEHAVIOR | permanent | Corner D9: upstream default weights are MaxFreeSpace=1, MinReservedSpace/MinRscCount/MaxThroughput=0. BS defaults ALL FOUR to 1.0 (`parseWeight` empty→1.0) — a richer equal-weight scorer. Per-strategy semantics match upstream (negative/zero ranks a pool lower but never excludes it; eligibility is gated separately in `matchesPoolFilter`). Only the unset-default composition differs; operators who set the weights explicitly get upstream-identical ranking. | +| 69 | `snapshot rollback` | MISSING_FEATURE | permanent | DELIBERATE (corner G1/G2): BS does NOT expose in-place rollback. `zfs rollback` / `lvconvert --merge` destroy every snapshot newer than the target, and upstream >=1.31.2 additionally RESURRECTS replicas on nodes whose resource was deleted — both silent footguns. BS returns 501 + actionable text pointing at `snapshot-restore-resource` (safe, non-destructive). InUse/Primary still 409s first, unknown snap still 404s first. Pinned by `TestSnapshotRollback*` (pkg/rest/snapshots_test.go). Oracle 1.33.2 confirmed: in-place rollback fails on a snapshot-less node WITHOUT resurrecting the deleted replica (the resurrection footgun is version-specific); BS's blanket 501 is the safe superset. | ## Open (block merge until addressed) diff --git a/internal/controller/autosnapshot_controller.go b/internal/controller/autosnapshot_controller.go index 50b99fce..9d34d9f4 100644 --- a/internal/controller/autosnapshot_controller.go +++ b/internal/controller/autosnapshot_controller.go @@ -48,8 +48,9 @@ const ( PropAutoSnapshotRunEvery = "AutoSnapshot/RunEvery" // PropAutoSnapshotKeep bounds the retained auto-snapshot count. - // Default 10 (DFLT_AUTO_SNAPSHOT_KEEP). A value <=0 disables the - // prune step, in which case every auto-snapshot is kept. + // Default 10 (DFLT_AUTO_SNAPSHOT_KEEP). Per upstream's documented + // semantic, a value <= 0 (or the prop being absent) falls back to + // that default of 10 — it does NOT disable the prune (G4). PropAutoSnapshotKeep = "AutoSnapshot/Keep" // PropAutoSnapshotNextID is the monotonically-incremented @@ -371,10 +372,11 @@ func (r *AutoSnapshotRunnable) stampRDAfterCreate( // "Manually-created snapshots NOT counted against the keep budget" // invariant. // -// A Keep value of 0 or negative disables the prune (every -// auto-snapshot is retained) — matches the upstream OpenAPI -// "Removing this property or having a value <= 0 disables -// auto-cleanup" wording. +// G4 (corner-case): a Keep value of 0 or negative falls back to the +// DEFAULT Keep (10) — it does NOT disable cleanup. This matches the +// upstream administration guide: "If AutoSnapshot/Keep is omitted (or +// <= 0), LINSTOR will keep the last 10 snapshots by default" +// (linstor-administration.adoc ~2467). func (r *AutoSnapshotRunnable) pruneOldAutoSnapshots( ctx context.Context, rd *blockstoriov1alpha1.ResourceDefinition, @@ -387,12 +389,16 @@ func (r *AutoSnapshotRunnable) pruneOldAutoSnapshots( return errors.Wrapf(err, "parse %s=%q", PropAutoSnapshotKeep, rawKeep) } - if parsed <= 0 { - // Disable cleanup — every auto-snapshot is kept. - return nil + // G4 (corner-case): upstream LINSTOR's documented semantic is + // "if AutoSnapshot/Keep is omitted (or <= 0), LINSTOR will keep + // the last 10 snapshots by default" (linstor-administration.adoc + // ~2467). A <= 0 value therefore falls back to the default Keep + // (10) — it does NOT disable cleanup. Pre-fix blockstor treated + // <= 0 as "keep everything", which let an unbounded number of + // auto-snapshots accumulate and silently diverged from upstream. + if parsed > 0 { + keep = int(parsed) } - - keep = int(parsed) } var snapList blockstoriov1alpha1.SnapshotList diff --git a/internal/controller/autosnapshot_controller_test.go b/internal/controller/autosnapshot_controller_test.go index 09869c27..9fbb8a71 100644 --- a/internal/controller/autosnapshot_controller_test.go +++ b/internal/controller/autosnapshot_controller_test.go @@ -390,11 +390,14 @@ func TestAutoSnapshotRunEveryDisabledSkipsRD(t *testing.T) { } } -// TestAutoSnapshotKeepZeroDisablesPrune: the OpenAPI doc says -// "Removing this property or having a value <= 0 disables -// auto-cleanup, all auto-snapshots will be kept". Pin that -// invariant — Keep=0 means never prune. -func TestAutoSnapshotKeepZeroDisablesPrune(t *testing.T) { +// TestAutoSnapshotKeepZeroFallsBackToDefault10 (G4): the upstream +// administration guide is explicit — "If AutoSnapshot/Keep is omitted +// (or <= 0), LINSTOR will keep the last 10 snapshots by default" +// (linstor-administration.adoc ~2467). A Keep value of 0 therefore +// falls back to the default budget of 10, it does NOT disable cleanup. +// (Pre-fix blockstor diverged here, treating <= 0 as "keep +// everything" — see git history for TestAutoSnapshotKeepZeroDisablesPrune.) +func TestAutoSnapshotKeepZeroFallsBackToDefault10(t *testing.T) { t.Parallel() scheme := newScheme(t) @@ -417,8 +420,40 @@ func TestAutoSnapshotKeepZeroDisablesPrune(t *testing.T) { } snaps := listAutoSnapshotsByRD(t, cli, "pvc-w05-keep0") - if len(snaps) != 15 { - t.Errorf("expected 15 auto-snapshots (cleanup disabled), got %d", len(snaps)) + if len(snaps) != controllerpkg.DefaultAutoSnapshotKeep { + t.Errorf("expected %d auto-snapshots (Keep=0 falls back to default 10), got %d", + controllerpkg.DefaultAutoSnapshotKeep, len(snaps)) + } +} + +// TestAutoSnapshotKeepNegativeFallsBackToDefault10 (G4): same contract +// for an explicitly negative Keep — `<= 0` is the documented trigger, +// so a negative value must also resolve to the default budget of 10. +func TestAutoSnapshotKeepNegativeFallsBackToDefault10(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + rd := makeRDWithAutoSnapshot(t, "pvc-w05-keepneg", 15, "-3") + + cli := fake.NewClientBuilder().WithScheme(scheme). + WithObjects(rd). + Build() + + clk := &stubClock{t: time.Date(2026, 5, 14, 12, 0, 0, 0, time.UTC)} + r := &controllerpkg.AutoSnapshotRunnable{Client: cli, Clock: clk} + + for i := range 14 { + if err := r.Tick(context.Background()); err != nil { + t.Fatalf("Tick #%d: %v", i, err) + } + + clk.advance(15*time.Minute + time.Second) + } + + snaps := listAutoSnapshotsByRD(t, cli, "pvc-w05-keepneg") + if len(snaps) != controllerpkg.DefaultAutoSnapshotKeep { + t.Errorf("expected %d auto-snapshots (Keep<0 falls back to default 10), got %d", + controllerpkg.DefaultAutoSnapshotKeep, len(snaps)) } } diff --git a/pkg/rest/api_call_rc.go b/pkg/rest/api_call_rc.go index 01a129b3..e4cb272d 100644 --- a/pkg/rest/api_call_rc.go +++ b/pkg/rest/api_call_rc.go @@ -207,6 +207,21 @@ const warnNodeAlreadyGone = maskWarn | int64(2060) // envelope wrapper at the call site. const apiCallRcFailSnapshotFinalizerStuck int64 = 998 +// apiCallRcFailSnapshotsNotSupported mirrors upstream LINSTOR's +// `ApiConsts.FAIL_SNAPSHOTS_NOT_SUPPORTED` (`994 | MASK_ERROR`). +// Emitted by `POST /v1/resource-definitions/{rd}/snapshots` (G5) when +// a targeted diskful replica lives in a storage pool whose provider +// cannot take copy-on-write snapshots (thick `LVM`, plain thick +// `FILE`, `DISKLESS`). The upstream controller refuses the snapshot +// at the API edge — thin LVM and ZFS (and `FILE_THIN` downstream) are +// the only snapshot-capable providers. blockstor's thick-LVM provider +// CAN technically `lvcreate --snapshot` (a 25%ORIGIN COW overlay that +// silently invalidates on overflow), so without this gate the +// satellite would happily stage a footgun snapshot the operator never +// asked to risk. The MASK_ERROR bit is OR'd in by the envelope wrapper +// at the call site. +const apiCallRcFailSnapshotsNotSupported int64 = 994 + // apiCallRcFailExistsSnapshotDfn mirrors upstream LINSTOR's // `ApiConsts.FAIL_EXISTS_SNAPSHOT_DFN` (`514 | MASK_ERROR`). Emitted by // `DELETE /v1/resource-definitions/{rd}` when the RD still has at diff --git a/pkg/rest/snapshot_restore.go b/pkg/rest/snapshot_restore.go index 81a34856..c01fb2ba 100644 --- a/pkg/rest/snapshot_restore.go +++ b/pkg/rest/snapshot_restore.go @@ -23,6 +23,7 @@ import ( "maps" "net/http" "slices" + "strconv" "strings" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" @@ -114,6 +115,19 @@ func (s *Server) handleSnapshotRestoreVolumeDefinition(w http.ResponseWriter, r return } + // G3b (corner-case): the VD-restore variant hydrates the snapshot's + // recorded volume layout onto a (typically pre-existing, empty) + // target RD. If the target RD ALREADY carries a volume-definition + // whose number collides with one of the snapshot's, refuse up front + // with a typed FAIL_EXISTS_VLM_DFN envelope naming the offending + // volume number — rather than letting hydrateVolumesFromSnapshot's + // per-VD Create surface a bare 409 only AFTER it has already + // partially mutated the target (an earlier non-colliding VD would + // land before the colliding one errored, leaving a half-restored RD). + if !s.refuseRestoreOnVolumeConflict(w, r, req.ToResource, &snap) { + return + } + err = hydrateVolumesFromSnapshot(r.Context(), s, req.ToResource, &snap) if err != nil { writeStoreError(w, err) @@ -128,6 +142,60 @@ func (s *Server) handleSnapshotRestoreVolumeDefinition(w http.ResponseWriter, r }}) } +// refuseRestoreOnVolumeConflict is the G3b pre-mutation guard for the +// VD-restore handler. It compares the snapshot's recorded volume +// numbers against the target RD's existing VolumeDefinitions and +// refuses with a typed FAIL_EXISTS_VLM_DFN (502) envelope when any +// number collides. Returns true (caller may proceed) when the target +// has no conflicting VD or its VD list could not be read (best-effort: +// the downstream hydrate Create still guards). Returns false (and +// writes the 409 envelope) on a collision. +func (s *Server) refuseRestoreOnVolumeConflict(w http.ResponseWriter, r *http.Request, toResource string, snap *apiv1.Snapshot) bool { + existing, err := s.Store.VolumeDefinitions().List(r.Context(), toResource) + if err != nil || len(existing) == 0 { + return true + } + + have := make(map[int32]struct{}, len(existing)) + for i := range existing { + have[existing[i].VolumeNumber] = struct{}{} + } + + var clashes []int32 + + for i := range snap.VolumeDefinitions { + if _, ok := have[snap.VolumeDefinitions[i].VolumeNumber]; ok { + clashes = append(clashes, snap.VolumeDefinitions[i].VolumeNumber) + } + } + + if len(clashes) == 0 { + return true + } + + slices.Sort(clashes) + + nums := make([]string, 0, len(clashes)) + for _, n := range clashes { + nums = append(nums, strconv.Itoa(int(n))) + } + + writeJSON(w, http.StatusConflict, []apiv1.APICallRc{{ + RetCode: apiCallRcError | apiCallRcFailExistsVlmDfn, + Message: "cannot restore snapshot volume definitions onto '" + + toResource + "': volume number(s) " + strings.Join(nums, ", ") + + " already exist on the target", + Cause: "the target resource definition already carries a volume " + + "definition with the same number as the snapshot's; restoring " + + "would collide on the volume-number key", + Correc: "restore into a resource definition with no conflicting " + + "volume definitions (e.g. a freshly-created empty RD), or remove " + + "the clashing volume definition(s) from '" + toResource + "' first", + }}) + + return false +} + // validateSnapshotRestoreRequest runs every pre-store wire-boundary // gate the new-RD-spawning restore handler needs: ToResource is set // (required field), and ToResource is a valid LINSTOR identifier diff --git a/pkg/rest/snapshot_restore_test.go b/pkg/rest/snapshot_restore_test.go index 18624540..f61183ae 100644 --- a/pkg/rest/snapshot_restore_test.go +++ b/pkg/rest/snapshot_restore_test.go @@ -74,6 +74,85 @@ func TestSnapshotRestoreCreatesNewRD(t *testing.T) { } } +// TestSnapshotRestoreAfterSourceResourcesDeleted_G3a pins corner-case +// G3a: the upstream administration guide states a snapshot can be +// restored "even when the original resource has been removed from the +// nodes where the snapshots were taken" (linstor-administration.adoc +// ~2480). E1 proved upstream BLOCKS `rd d` while snapshots exist, so +// the relevant sequence is `r d` of every replica (the RD + the +// snapshot survive), then restore from the surviving snapshot. The +// snapshot is stored independently of the (now-deleted) Resources, so +// the restore must still build the new RD + place its replicas on the +// snapshot's recorded nodes. +func TestSnapshotRestoreAfterSourceResourcesDeleted_G3a(t *testing.T) { + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-src"}); err != nil { + t.Fatalf("seed source RD: %v", err) + } + + // Snapshot recorded on n1/n2 — but NO Resources exist (every replica + // was `r d`'d). The RD shell + the snapshot both survive. + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-keep", + ResourceName: "pvc-src", + Nodes: []string{"n1", "n2"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 1024 * 1024}, + }, + }); err != nil { + t.Fatalf("seed snap: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + // Explicit node list (the snapshot's recorded nodes) so the restore + // places replicas without depending on a parent RG / autoplace. + body, _ := json.Marshal(snapshotRestoreRequest{ + ToResource: "pvc-restored", + FromSnapshot: "snap-keep", + Nodes: []string{"n1", "n2"}, + }) + + resp := httpPost(t, base+"/v1/resource-definitions/pvc-src/snapshot-restore-resource", body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status: got %d, want 201 (restore must work after source resources deleted)", resp.StatusCode) + } + + if _, err := st.ResourceDefinitions().Get(ctx, "pvc-restored"); err != nil { + t.Fatalf("restored RD missing: %v", err) + } + + // The restored RD must carry the snapshot's volume layout. + vds, err := st.VolumeDefinitions().List(ctx, "pvc-restored") + if err != nil { + t.Fatalf("list restored VDs: %v", err) + } + + if len(vds) != 1 || vds[0].VolumeNumber != 0 { + t.Errorf("restored VDs: got %+v, want exactly [vol 0]", vds) + } + + // Replicas must be stamped on the snapshot's recorded nodes. + resList, err := st.Resources().ListByDefinition(ctx, "pvc-restored") + if err != nil { + t.Fatalf("list restored resources: %v", err) + } + + placed := make(map[string]bool, len(resList)) + for i := range resList { + placed[resList[i].NodeName] = true + } + + if !placed["n1"] || !placed["n2"] { + t.Errorf("restored replicas: got nodes %v, want {n1,n2}", placed) + } +} + // TestSnapshotRestoreUnknownSnapshot: 404 if the snapshot doesn't exist. func TestSnapshotRestoreUnknownSnapshot(t *testing.T) { st := store.NewInMemory() @@ -846,3 +925,129 @@ func TestSnapshotRestoreBug354AutoplacesWhenNodesEmpty(t *testing.T) { } } } + +// TestSnapshotRestoreVolumeDefinitionIntoEmptyRD_G3b is the positive +// control for the VD-restore variant: restoring a snapshot's volume +// layout onto a freshly-created EMPTY target RD succeeds and hydrates +// the recorded volumes. Mirrors the documented two-phase workflow +// (`rd create resource2` → `snapshot volume-definition restore`). +func TestSnapshotRestoreVolumeDefinitionIntoEmptyRD_G3b(t *testing.T) { + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-src"}); err != nil { + t.Fatalf("seed source RD: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-1", + ResourceName: "pvc-src", + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 1024 * 1024}, + }, + }); err != nil { + t.Fatalf("seed snap: %v", err) + } + + // Empty target RD — no VDs of its own. + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-tgt"}); err != nil { + t.Fatalf("seed target RD: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + body, _ := json.Marshal(snapshotRestoreRequest{ToResource: "pvc-tgt"}) + + resp := httpPost(t, + base+"/v1/resource-definitions/pvc-src/snapshot-restore-volume-definition/snap-1", body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("status: got %d, want 200 (VD restore into empty RD)", resp.StatusCode) + } + + vds, err := st.VolumeDefinitions().List(ctx, "pvc-tgt") + if err != nil { + t.Fatalf("list target VDs: %v", err) + } + + if len(vds) != 1 || vds[0].VolumeNumber != 0 { + t.Errorf("target VDs: got %+v, want exactly [vol 0]", vds) + } +} + +// TestSnapshotRestoreVolumeDefinitionConflict_G3b pins corner-case G3b: +// restoring a snapshot's volume definitions onto an RD that ALREADY +// carries a volume-definition with a clashing number is refused with a +// 409 + FAIL_EXISTS_VLM_DFN envelope BEFORE any mutation, naming the +// offending volume number. Without the up-front guard the per-VD +// hydrate Create surfaces a bare 409 only after a partial restore. +func TestSnapshotRestoreVolumeDefinitionConflict_G3b(t *testing.T) { + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-src"}); err != nil { + t.Fatalf("seed source RD: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-1", + ResourceName: "pvc-src", + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 1024 * 1024}, + }, + }); err != nil { + t.Fatalf("seed snap: %v", err) + } + + // Target RD already has its OWN volume 0. + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-tgt"}); err != nil { + t.Fatalf("seed target RD: %v", err) + } + + if err := st.VolumeDefinitions().Create(ctx, "pvc-tgt", &apiv1.VolumeDefinition{ + VolumeNumber: 0, SizeKib: 2048 * 1024, + }); err != nil { + t.Fatalf("seed target VD: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + body, _ := json.Marshal(snapshotRestoreRequest{ToResource: "pvc-tgt"}) + + resp := httpPost(t, + base+"/v1/resource-definitions/pvc-src/snapshot-restore-volume-definition/snap-1", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusConflict { + t.Fatalf("status: got %d, want 409 (volume-number conflict)", resp.StatusCode) + } + + var rc []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rc); err != nil { + t.Fatalf("decode envelope: %v", err) + } + + if len(rc) == 0 || rc[0].RetCode >= 0 { + t.Fatalf("ret_code: got %+v, want a negative (error) code", rc) + } + + // FAIL_EXISTS_VLM_DFN (502) sub-code so golinstor's typed matcher fires. + const failBit = 502 + if rc[0].RetCode&failBit != failBit { + t.Errorf("ret_code: got %#x, want FAIL_EXISTS_VLM_DFN (502) bit set", rc[0].RetCode) + } + + // The target RD's original volume 0 must remain untouched (size + // 2048 MiB) — the guard must not have partially mutated it. + got, err := st.VolumeDefinitions().Get(ctx, "pvc-tgt", 0) + if err != nil { + t.Fatalf("target VD 0 vanished: %v", err) + } + + if got.SizeKib != 2048*1024 { + t.Errorf("target VD 0 size: got %d, want untouched 2048 MiB", got.SizeKib) + } +} diff --git a/pkg/rest/snapshots.go b/pkg/rest/snapshots.go index d76a5d3c..961aea0e 100644 --- a/pkg/rest/snapshots.go +++ b/pkg/rest/snapshots.go @@ -536,6 +536,19 @@ func (s *Server) handleSnapshotCreate(w http.ResponseWriter, r *http.Request) { return } + // G5 (corner-case): refuse the snapshot when ANY targeted diskful + // replica lives in a storage pool whose provider cannot take + // copy-on-write snapshots (thick `LVM`, plain thick `FILE`, + // `DISKLESS`). Upstream LINSTOR rejects this at the controller — + // only thin LVM and ZFS (and `FILE_THIN` downstream) are + // snapshot-capable. Without this gate, blockstor's thick-LVM + // provider would actually `lvcreate --snapshot` a 25%ORIGIN COW + // overlay that silently invalidates on overflow (Bug 245 footgun), + // so we refuse at the API edge BEFORE any Store mutation. + if !s.refuseSnapshotOnNonSnapshotPool(w, r, rd, snap.Nodes) { + return + } + // Bug 180 pre-write guard: refuse the create if the parent RD is // already mid-tear-down. Upstream LINSTOR stamps the `DELETE` flag // on a ResourceDefinition while CtrlRscDfnDeleteApiCallHandler is @@ -972,6 +985,145 @@ func (s *Server) refuseSnapshotOnOfflineTargets(w http.ResponseWriter, r *http.R return false } +// refuseSnapshotOnNonSnapshotPool is the G5 capability gate. It walks +// the RD's diskful replicas, resolves each one's storage pool, and +// refuses the snapshot when ANY pool reports `SupportsSnapshot == +// false` — i.e. a thick provider (thick `LVM`, plain thick `FILE`, +// `DISKLESS`) that cannot take a copy-on-write snapshot. Mirrors +// upstream LINSTOR's controller-side `FAIL_SNAPSHOTS_NOT_SUPPORTED` +// refusal. Returns true when the caller may continue (every diskful +// replica sits in a snapshot-capable pool, or capability is +// un-observed); false when it has already written the 400 + envelope. +// +// Best-effort resolution: a pool we cannot look up (Get error, missing +// StorPoolName prop, never-reported Status) is treated as capable so a +// transient store blip or an as-yet-unprobed pool doesn't spuriously +// refuse a legitimate snapshot. Only an explicit +// `SupportsSnapshot == false` on a resolvable pool refuses — matching +// the tri-state caution the rest of the snapshot path applies. +func (s *Server) refuseSnapshotOnNonSnapshotPool(w http.ResponseWriter, r *http.Request, rd string, targetNodes []string) bool { + resList, err := s.Store.Resources().ListByDefinition(r.Context(), rd) + if err != nil { + // Surface the store error rather than silently passing — the + // create would fail downstream anyway and the operator should + // see why. + writeStoreError(w, err) + + return false + } + + locs := s.nonSnapshotPoolLocations(r.Context(), resList, targetNodes) + if len(locs) == 0 { + return true + } + + writeJSON(w, http.StatusBadRequest, []apiv1.APICallRc{{ + RetCode: apiCallRcError | apiCallRcFailSnapshotsNotSupported, + Message: "snapshot of resource definition '" + rd + + "' refused: storage pool(s) " + strings.Join(locs, ", ") + + " do not support snapshots", + Cause: "the targeted storage pool uses a thick provider (thick LVM, " + + "plain FILE, or DISKLESS); only thin LVM and ZFS (and FILE_THIN) " + + "can take copy-on-write snapshots", + Correc: "place the resource on a thin-provisioned snapshot-capable " + + "pool (LVM_THIN / ZFS_THIN / FILE_THIN), or use `linstor rd clone` " + + "for a full-copy duplicate that does not require snapshot support", + }}) + + return false +} + +// nonSnapshotPoolLocations resolves each diskful/active replica's +// storage pool and returns a sorted, de-duplicated list of human- +// readable `" on "` locations whose pool reports +// `SupportsSnapshot == false`. An empty result means every targeted +// diskful replica sits in a snapshot-capable pool (or capability could +// not be resolved — treated as capable; see +// refuseSnapshotOnNonSnapshotPool). +// +// targetNodes scopes the check to the resolved snapshot target set +// (snap.Nodes) — the same set the offline pre-check uses — so a caller +// that snapshots only a snapshot-capable subset of an otherwise +// mixed-provider RD is not falsely refused. An empty targetNodes means +// "all diskful replicas" (the un-pinned default-fan-out case). +func (s *Server) nonSnapshotPoolLocations(ctx context.Context, resList []apiv1.Resource, targetNodes []string) []string { + want := make(map[string]struct{}, len(targetNodes)) + for _, n := range targetNodes { + want[n] = struct{}{} + } + + var offenders []poolOffender + + seen := make(map[string]struct{}, len(resList)) + + for i := range resList { + res := &resList[i] + + // Scope to the resolved target set when the caller pinned one. + if len(want) > 0 { + if _, ok := want[res.NodeName]; !ok { + continue + } + } + + // Only diskful, active replicas materialise a real backing + // volume to snapshot — diskless / inactive replicas are skipped + // by listDiskfulNodes too, so don't gate on their (absent) pool. + if slices.Contains(res.Flags, apiv1.ResourceFlagDiskless) || + slices.Contains(res.Flags, apiv1.ResourceFlagInactive) { + continue + } + + poolName := res.Props[storPoolPropKey] + if poolName == "" { + continue + } + + key := res.NodeName + "/" + poolName + if _, dup := seen[key]; dup { + continue + } + + seen[key] = struct{}{} + + pool, getErr := s.Store.StoragePools().Get(ctx, res.NodeName, poolName) + if getErr != nil { + continue + } + + if !pool.SupportsSnapshot { + offenders = append(offenders, poolOffender{node: res.NodeName, pool: poolName}) + } + } + + return formatPoolOffenders(offenders) +} + +// poolOffender pairs a node with a non-snapshot-capable pool it hosts. +type poolOffender struct { + node string + pool string +} + +// formatPoolOffenders sorts the offenders deterministically (by node, +// then pool) and renders each as `" on "`. +func formatPoolOffenders(offenders []poolOffender) []string { + slices.SortFunc(offenders, func(a, b poolOffender) int { + if a.node != b.node { + return strings.Compare(a.node, b.node) + } + + return strings.Compare(a.pool, b.pool) + }) + + locs := make([]string, 0, len(offenders)) + for _, o := range offenders { + locs = append(locs, o.pool+" on "+o.node) + } + + return locs +} + // offlineTargetNodes returns the subset of `targets` whose Node is // currently OFFLINE. Used as the snapshot-create fail-fast pre-check // (mirrors upstream LINSTOR's getOfflineNodes): refusing before the diff --git a/pkg/rest/snapshots_test.go b/pkg/rest/snapshots_test.go index 1ee1560d..ebaedad4 100644 --- a/pkg/rest/snapshots_test.go +++ b/pkg/rest/snapshots_test.go @@ -604,6 +604,69 @@ func TestSnapshotRollbackAllReplicasSecondaryReaches501(t *testing.T) { } } +// TestSnapshotRollbackSnapshotlessNodeReaches501_G2 pins corner-case +// G2 as a DELIBERATE DELTA. Upstream LINSTOR >= 1.31.2 changed +// `snapshot rollback` so a snapshot-less node (a replica added AFTER +// the snapshot was taken, or one whose resource was deleted) gets an +// empty resource + resync on rollback — and the rollback RESTORES +// replicas on nodes where the resource had been deleted +// (linstor-administration.adoc ~2512). blockstor deliberately does NOT +// expose `zfs rollback` at all (it destroys every newer snapshot and +// the deleted-replica-resurrection is a counterintuitive footgun), so +// the rollback endpoint returns a 501 that points the operator at the +// safe, non-destructive `snapshot-restore-resource` path. This test +// models the exact 1.31.2+ scenario — snapshot on n1/n2, a third +// diskful replica on n3 that does NOT hold the snapshot, all Secondary +// — and asserts blockstor still returns 501 (no destructive backend +// path is reached, no deleted replicas are resurrected). +func TestSnapshotRollbackSnapshotlessNodeReaches501_G2(t *testing.T) { + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-1"}); err != nil { + t.Fatalf("seed RD: %v", err) + } + + // Snapshot recorded only on n1/n2. + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "s1", ResourceName: "pvc-1", Nodes: []string{"n1", "n2"}, + }); err != nil { + t.Fatalf("seed snap: %v", err) + } + + // Three diskful replicas, all Secondary — n3 was added AFTER the + // snapshot, so it is snapshot-less (the upstream 1.31.2+ edge). + for _, n := range []string{"n1", "n2", "n3"} { + if err := st.Resources().Create(ctx, &apiv1.Resource{ + Name: "pvc-1", NodeName: n, + State: apiv1.ResourceState{InUse: boolPtr(false)}, + }); err != nil { + t.Fatalf("seed res %s: %v", n, err) + } + } + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := httpPost(t, base+"/v1/resource-definitions/pvc-1/snapshots/s1/rollback", []byte("{}")) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusNotImplemented { + t.Fatalf("status: got %d, want 501 (snapshot-less node → blockstor refuses rollback entirely)", resp.StatusCode) + } + + var rc []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rc); err != nil { + t.Fatalf("decode envelope: %v", err) + } + + // The 501 must point operators at the safe restore path rather than + // silently doing nothing. + if len(rc) == 0 || !strings.Contains(rc[0].Message, "snapshot-restore-resource") { + t.Errorf("501 message must point at snapshot-restore-resource, got %+v", rc) + } +} + // TestSnapshotsDeleteUnknownRD pins one half of the CSI idempotence // contract: DELETE on an unknown {rd} path segment returns 200 + // ApiCallRc("snapshot already absent: ..."), NOT 404. csi-sanity's @@ -2135,3 +2198,222 @@ func TestSnapshotStateIgnoresInactiveReplica_Bug394(t *testing.T) { "INACTIVE worker-3 MUST be excluded from the denominator)", got[0].Flags) } } + +// seedSnapshotPoolFixture wires a RD + one diskful Resource pinned to a +// storage pool with the given snapshot capability. Used by the G5 gate +// tests to exercise refuseSnapshotOnNonSnapshotPool. +func seedSnapshotPoolFixture(t *testing.T, st store.Store, rd, node, pool string, canSnap bool) { + t.Helper() + + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: rd}); err != nil { + t.Fatalf("seed RD: %v", err) + } + + if err := st.StoragePools().Create(ctx, &apiv1.StoragePool{ + StoragePoolName: pool, + NodeName: node, + ProviderKind: "LVM", + SupportsSnapshot: canSnap, + }); err != nil { + t.Fatalf("seed pool: %v", err) + } + + if err := st.Resources().Create(ctx, &apiv1.Resource{ + Name: rd, + NodeName: node, + Props: map[string]string{"StorPoolName": pool}, + }); err != nil { + t.Fatalf("seed resource: %v", err) + } +} + +// TestSnapshotCreateRefusedOnThickPool_G5 pins the corner-case G5 gate: +// `snapshot create` against an RD whose diskful replica sits in a +// thick (SupportsSnapshot=false) storage pool is refused with a 400 + +// FAIL_SNAPSHOTS_NOT_SUPPORTED envelope BEFORE any Store mutation — +// mirroring upstream LINSTOR which only allows snapshots on thin +// LVM / ZFS (and FILE_THIN downstream). Without the gate blockstor's +// thick-LVM provider would silently stage a 25%ORIGIN COW overlay +// (Bug 245 footgun). +func TestSnapshotCreateRefusedOnThickPool_G5(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedSnapshotPoolFixture(t, st, "pvc-thick", "n1", "thickpool", false) + + base, stop := startServerWithStore(t, st) + defer stop() + + body, err := json.Marshal(apiv1.Snapshot{Name: "snap-1"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + resp := httpPost(t, base+"/v1/resource-definitions/pvc-thick/snapshots", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusBadRequest { + t.Fatalf("status: got %d, want 400 (thick pool snapshot refusal)", resp.StatusCode) + } + + var rc []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rc); err != nil { + t.Fatalf("decode ApiCallRc envelope: %v", err) + } + + if len(rc) == 0 { + t.Fatalf("empty ApiCallRc envelope") + } + + // Negative ret_code — the `linstor` CLI treats ret_code >= 0 as + // success and would not surface the refusal to the operator. + if rc[0].RetCode >= 0 { + t.Errorf("ret_code: got %#x, want negative (error)", rc[0].RetCode) + } + + // FAIL_SNAPSHOTS_NOT_SUPPORTED (994) sub-code must be set so + // golinstor's typed-error matchers fire. + const failBit = 994 + if rc[0].RetCode&failBit != failBit { + t.Errorf("ret_code: got %#x, want FAIL_SNAPSHOTS_NOT_SUPPORTED (994) bit set", rc[0].RetCode) + } + + // Message must name the offending pool + node so the operator knows + // which replica to relocate. + if !strings.Contains(rc[0].Message, "thickpool") || !strings.Contains(rc[0].Message, "n1") { + t.Errorf("message missing offending pool/node: %q", rc[0].Message) + } + + // No Snapshot CRD must have been persisted — the gate runs before + // the Store.Create. + if _, err := st.Snapshots().Get(t.Context(), "pvc-thick", "snap-1"); err == nil { + t.Errorf("snapshot was persisted despite the refusal (gate ran too late)") + } +} + +// TestSnapshotCreateAllowedOnThinPool_G5 is the positive control: an RD +// on a thin (SupportsSnapshot=true) pool still snapshots normally, so +// the G5 gate does not over-reach onto capable providers. +func TestSnapshotCreateAllowedOnThinPool_G5(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedSnapshotPoolFixture(t, st, "pvc-thin", "n1", "thinpool", true) + + base, stop := startServerWithStore(t, st) + defer stop() + + body, err := json.Marshal(apiv1.Snapshot{Name: "snap-1"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + resp := httpPost(t, base+"/v1/resource-definitions/pvc-thin/snapshots", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status: got %d, want 201 (thin pool snapshot allowed)", resp.StatusCode) + } + + if _, err := st.Snapshots().Get(t.Context(), "pvc-thin", "snap-1"); err != nil { + t.Errorf("snapshot not persisted on a snapshot-capable pool: %v", err) + } +} + +// TestSnapshotCreateUnresolvablePoolPasses_G5 pins the best-effort +// fall-through: a diskful replica with NO StorPoolName prop (pool not +// resolvable) must NOT be refused — a transient store blip or an +// as-yet-unprobed pool should not spuriously block a legitimate +// snapshot. The gate only refuses an explicit SupportsSnapshot=false. +func TestSnapshotCreateUnresolvablePoolPasses_G5(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-x"}); err != nil { + t.Fatalf("seed RD: %v", err) + } + + // Diskful replica but no StorPoolName prop → pool unresolvable. + if err := st.Resources().Create(ctx, &apiv1.Resource{Name: "pvc-x", NodeName: "n1"}); err != nil { + t.Fatalf("seed resource: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + body, err := json.Marshal(apiv1.Snapshot{Name: "snap-1"}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + resp := httpPost(t, base+"/v1/resource-definitions/pvc-x/snapshots", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status: got %d, want 201 (unresolvable pool must pass through)", resp.StatusCode) + } +} + +// TestSnapshotCreateScopesGateToTargetNodes_G5 pins the subset-scoping +// contract: an RD with one thin replica (n1) and one thick replica (n2). +// A snapshot that explicitly targets ONLY the thin node n1 must be +// ALLOWED (the gate is scoped to the resolved target set, mirroring the +// offline pre-check). A snapshot targeting the thick node n2 is refused. +func TestSnapshotCreateScopesGateToTargetNodes_G5(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-mix"}); err != nil { + t.Fatalf("seed RD: %v", err) + } + + // n1 thin (capable), n2 thick (incapable). + for _, p := range []struct { + node, pool string + canSnap bool + }{ + {"n1", "thinpool", true}, + {"n2", "thickpool", false}, + } { + if err := st.StoragePools().Create(ctx, &apiv1.StoragePool{ + StoragePoolName: p.pool, NodeName: p.node, + ProviderKind: "LVM", SupportsSnapshot: p.canSnap, + }); err != nil { + t.Fatalf("seed pool %s: %v", p.pool, err) + } + + if err := st.Resources().Create(ctx, &apiv1.Resource{ + Name: "pvc-mix", NodeName: p.node, + Props: map[string]string{"StorPoolName": p.pool}, + }); err != nil { + t.Fatalf("seed resource %s: %v", p.node, err) + } + } + + base, stop := startServerWithStore(t, st) + defer stop() + + // Target ONLY the thin node n1 → allowed. + okBody, _ := json.Marshal(apiv1.Snapshot{Name: "snap-thin", Nodes: []string{"n1"}}) + okResp := httpPost(t, base+"/v1/resource-definitions/pvc-mix/snapshots", okBody) + _ = okResp.Body.Close() + + if okResp.StatusCode != http.StatusCreated { + t.Errorf("targeting thin-only n1: got %d, want 201 (gate scoped to target set)", okResp.StatusCode) + } + + // Target the thick node n2 → refused. + badBody, _ := json.Marshal(apiv1.Snapshot{Name: "snap-thick", Nodes: []string{"n2"}}) + badResp := httpPost(t, base+"/v1/resource-definitions/pvc-mix/snapshots", badBody) + defer func() { _ = badResp.Body.Close() }() + + if badResp.StatusCode != http.StatusBadRequest { + t.Errorf("targeting thick n2: got %d, want 400 (thick pool refused)", badResp.StatusCode) + } +} diff --git a/tests/e2e/cli-matrix/snap-create-thick-pool-rejected.sh b/tests/e2e/cli-matrix/snap-create-thick-pool-rejected.sh new file mode 100755 index 00000000..0b7b5238 --- /dev/null +++ b/tests/e2e/cli-matrix/snap-create-thick-pool-rejected.sh @@ -0,0 +1,133 @@ +#!/usr/bin/env bash +# +# usage: snap-create-thick-pool-rejected.sh WORK_DIR +# +# L6 cli-matrix cell — corner-case G5 (snapshot create on a thick pool). +# +# Upstream LINSTOR only allows snapshots on snapshot-capable providers +# (thin LVM, ZFS — and FILE_THIN downstream). A `snapshot create` on a +# THICK pool (classic `LVM`, plain thick `FILE`) is refused at the +# controller with FAIL_SNAPSHOTS_NOT_SUPPORTED. blockstor's thick-LVM +# provider CAN technically `lvcreate --snapshot` a 25%ORIGIN COW overlay +# that silently invalidates on overflow (Bug 245 footgun), so the REST +# surface must refuse the create at the API edge. +# +# The dev stand ships only thin pools (lvm-thin, stand=FILE_THIN, +# zfs-thin), so this cell SELF-PROVISIONS a throwaway thick LVM pool on +# a loop device on worker-1, asserts the rejection, and tears it down. +# If the thick pool cannot be provisioned (no loop device, no spare +# disk), the cell SKIPS (exit 0) rather than false-FAIL. +# +# Contract: +# 1. provision a loop-backed thick LVM VG + register a blockstor `LVM` +# pool (CanSnapshots=False) on worker-1. +# 2. spawn a tiny RD diskful on that thick pool. +# 3. `snapshot create` on it → MUST be rejected (non-zero), error +# mentions snapshots-not-supported / thin / capability. +# 4. teardown: rd, pool, VG, loop device. + +set -euo pipefail + +WORK_DIR=${1:?work_dir required} +export KUBECONFIG="$WORK_DIR/kubeconfig" + +SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=lib.sh +source "$SCRIPT_DIR/lib.sh" + +require_workers 1 + +linstor_cli_setup + +N1=$WORKER_1 +THICKVG=clim_g5_thickvg +THICKPOOL=cli-matrix-g5-thickpool +RD=cli-matrix-g5-thick +SNAP=snap-g5-1 +IMG=/tmp/cli-matrix-g5-thick.img + +provisioned=false + +teardown_thick() { + "${LCTL[@]}" snapshot delete "$RD" "$SNAP" 2>/dev/null || true + delete_rd "$RD" + "${LCTL[@]}" storage-pool delete "$N1" "$THICKPOOL" 2>/dev/null || true + if $provisioned; then + on_node "$N1" bash -c " + vgremove -f $THICKVG 2>/dev/null || true + LD=\$(losetup -j $IMG 2>/dev/null | cut -d: -f1) + [ -n \"\$LD\" ] && losetup -d \"\$LD\" 2>/dev/null || true + rm -f $IMG + " 2>/dev/null || true + fi + assert_no_orphans "$RD" + linstor_cli_teardown +} +trap teardown_thick EXIT + +echo ">> [G5] provision throwaway thick LVM VG on $N1 (loop-backed)" +if ! on_node "$N1" bash -c " + set -e + truncate -s 1G $IMG + LD=\$(losetup --find --show $IMG) + pvcreate -f \"\$LD\" >/dev/null + vgcreate $THICKVG \"\$LD\" >/dev/null + vgs $THICKVG >/dev/null +" 2>/dev/null; then + echo "SKIP (G5): could not provision a loop-backed thick VG on $N1 (no losetup/lvm or no space)" + provisioned=false + exit 0 +fi +provisioned=true + +echo ">> [G5] register thick LVM pool $THICKPOOL on $N1" +if ! _out=$("${LCTL[@]}" storage-pool create lvm "$N1" "$THICKPOOL" "$THICKVG" 2>&1); then + echo "SKIP (G5): blockstor refused to register a thick LVM pool: $_out" + exit 0 +fi +sleep 3 + +# Confirm the pool reports CanSnapshots=False (the whole premise of G5). +if "${LCTL[@]}" storage-pool list --storage-pools "$THICKPOOL" 2>/dev/null \ + | grep -qi "True"; then + echo "note (G5): thick pool unexpectedly reports CanSnapshots=True; continuing — REST gate is authoritative" +fi + +echo ">> [G5] spawn tiny RD diskful on the thick pool" +_out=$("${LCTL[@]}" resource-definition create "$RD" 2>&1) \ + || { echo "FAIL: rd c $RD: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" volume-definition create "$RD" 32M 2>&1) \ + || { echo "FAIL: vd c $RD: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" resource create "$N1" "$RD" --storage-pool="$THICKPOOL" 2>&1) \ + || { echo "FAIL: r c $N1 $RD on thick pool: $_out" >&2; exit 1; } +# Single-replica diskful on the thick pool: poll the single-node disk +# state (wait_uptodate is a 2-peer helper and needs a third positional). +wait_disk_state "$RD" "$N1" "UpToDate" 180 + +echo ">> [G5] snapshot create on the thick pool MUST be REJECTED" +err_file=$(mktemp) +if "${LCTL[@]}" snapshot create "$RD" "$SNAP" >"$err_file" 2>&1; then + echo "FAIL (G5): snapshot create on a thick LVM pool was ACCEPTED" >&2 + cat "$err_file" >&2 + rm -f "$err_file" + exit 1 +fi + +# Error should explain the capability gap. Soft-check wording. +if ! grep -qiE "snapshot|support|thin" "$err_file"; then + echo "note (G5): rejection did not mention snapshot capability:" >&2 + cat "$err_file" >&2 +fi +rm -f "$err_file" + +# No orphan snapshot CRD may survive the rejected create. +if kubectl get snapshots.blockstor.cozystack.io -o json 2>/dev/null \ + | jq -e --arg rd "$RD" --arg s "$SNAP" \ + '[.items[]? | select(.spec.resourceDefinitionName==$rd) | select(.spec.snapshotName==$s)] | length > 0' \ + >/dev/null 2>&1; then + echo "FAIL (G5): rejected snapshot create left an orphan Snapshot CRD" >&2 + exit 1 +fi + +echo ">> [G5] OK — thick-pool snapshot create rejected, no orphan snapshot" +echo "PASS: snap-create-thick-pool-rejected" diff --git a/tests/e2e/cli-matrix/snap-vd-restore-volume-conflict-rejected.sh b/tests/e2e/cli-matrix/snap-vd-restore-volume-conflict-rejected.sh new file mode 100755 index 00000000..b2a85d48 --- /dev/null +++ b/tests/e2e/cli-matrix/snap-vd-restore-volume-conflict-rejected.sh @@ -0,0 +1,134 @@ +#!/usr/bin/env bash +# +# usage: snap-vd-restore-volume-conflict-rejected.sh WORK_DIR +# +# L6 cli-matrix cell — corner-case G3b (two-phase restore edge). +# +# `linstor snapshot volume-definition restore` hydrates a snapshot's +# recorded volume layout onto a (typically pre-existing, EMPTY) target +# RD. If the target RD already carries a volume-definition whose number +# collides with one of the snapshot's, the restore must be REJECTED +# up front with a clear error naming the offending volume number — +# never partially mutate the target (an earlier non-colliding VD would +# land before the colliding one errored, leaving a half-restored RD). +# +# Contract: +# 1. source RD with one volume, 1-replica diskful on worker-1. +# 2. snapshot the source. +# 3. target RD created with its OWN volume 0 (a different size). +# 4. `snapshot volume-definition restore` into the target → MUST be +# rejected (non-zero), error names the volume number, and the +# target's original VD 0 is left untouched. +# 5. control: restore into a fresh EMPTY target RD → MUST succeed. + +set -euo pipefail + +WORK_DIR=${1:?work_dir required} +export KUBECONFIG="$WORK_DIR/kubeconfig" + +SCRIPT_DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) +# shellcheck source=lib.sh +source "$SCRIPT_DIR/lib.sh" + +require_workers 1 + +linstor_cli_setup + +SRC=cli-matrix-g3b-src +SNAP=snap-g3b-1 +TGT_CONFLICT=cli-matrix-g3b-tgt-conflict +TGT_OK=cli-matrix-g3b-tgt-ok + +N1=$WORKER_1 + +cleanup() { + "${LCTL[@]}" snapshot delete "$SRC" "$SNAP" 2>/dev/null || true + delete_rd "$TGT_OK" + delete_rd "$TGT_CONFLICT" + delete_rd "$SRC" + assert_no_orphans "$SRC" + assert_no_orphans "$TGT_OK" + assert_no_orphans "$TGT_CONFLICT" + linstor_cli_teardown +} +trap cleanup EXIT + +echo ">> source RD: 1-replica diskful on $N1 (64M volume), snapshot it" +_out=$("${LCTL[@]}" resource-definition create "$SRC" 2>&1) \ + || { echo "FAIL: rd c $SRC: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" volume-definition create "$SRC" 64M 2>&1) \ + || { echo "FAIL: vd c $SRC: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" resource create "$N1" "$SRC" --storage-pool=stand 2>&1) \ + || { echo "FAIL: r c $N1 $SRC: $_out" >&2; exit 1; } +# Single-replica source: wait_uptodate is a 2-peer convergence helper +# (requires a primary AND a peer); use the single-node disk-state poll. +wait_disk_state "$SRC" "$N1" "UpToDate" 180 + +_out=$("${LCTL[@]}" snapshot create "$SRC" "$SNAP" 2>&1) \ + || { echo "FAIL: snap c $SRC $SNAP: $_out" >&2; exit 1; } +sleep 4 + +# =========================================================================== +# G3b — VOLUME-NUMBER CONFLICT: VD-restore into an RD that already has vol 0. +# =========================================================================== +echo ">> target RD $TGT_CONFLICT created with its OWN volume 0 (128M)" +_out=$("${LCTL[@]}" resource-definition create "$TGT_CONFLICT" 2>&1) \ + || { echo "FAIL: rd c $TGT_CONFLICT: $_out" >&2; exit 1; } +_out=$("${LCTL[@]}" volume-definition create "$TGT_CONFLICT" 128M 2>&1) \ + || { echo "FAIL: vd c $TGT_CONFLICT: $_out" >&2; exit 1; } + +echo ">> [G3b] VD-restore into $TGT_CONFLICT (volume 0 clash) must be REJECTED" +err_file=$(mktemp) +if "${LCTL[@]}" snapshot volume-definition restore \ + --from-resource "$SRC" \ + --from-snapshot "$SNAP" \ + --to-resource "$TGT_CONFLICT" >"$err_file" 2>&1; then + echo "FAIL (G3b): VD-restore onto an RD with a clashing volume 0 was ACCEPTED" >&2 + cat "$err_file" >&2 + rm -f "$err_file" + exit 1 +fi + +# The error should mention the volume number so the operator knows what +# clashed. Soft-check: surface but don't fail if wording differs. +if ! grep -qiE "volume|already exist" "$err_file"; then + echo "note: rejection did not mention the volume clash explicitly:" >&2 + cat "$err_file" >&2 +fi +rm -f "$err_file" + +# The target's original VD 0 must remain at its 128M size (untouched). +sz=$("${LCTL[@]}" volume-definition list --resource-definitions "$TGT_CONFLICT" 2>/dev/null \ + | awk '/[0-9]/ && /MiB|GiB/ {print; exit}') +echo ">> [G3b] target $TGT_CONFLICT VD after rejected restore: ${sz:-}" +if ! echo "$sz" | grep -q "128"; then + echo "FAIL (G3b): target VD 0 size changed by the rejected restore (expected 128 MiB): ${sz:-}" >&2 + exit 1 +fi +echo ">> [G3b] OK — conflict rejected, target VD untouched" + +# =========================================================================== +# CONTROL — VD-restore into a fresh EMPTY RD must SUCCEED. +# =========================================================================== +echo ">> [G3b control] VD-restore into EMPTY $TGT_OK must SUCCEED" +_out=$("${LCTL[@]}" resource-definition create "$TGT_OK" 2>&1) \ + || { echo "FAIL: rd c $TGT_OK: $_out" >&2; exit 1; } + +if ! _out=$("${LCTL[@]}" snapshot volume-definition restore \ + --from-resource "$SRC" \ + --from-snapshot "$SNAP" \ + --to-resource "$TGT_OK" 2>&1); then + echo "FAIL (G3b control): VD-restore into an empty RD was rejected: $_out" >&2 + exit 1 +fi + +# The restored target must now carry volume 0 (the snapshot's 64M layout). +if ! "${LCTL[@]}" volume-definition list --resource-definitions "$TGT_OK" 2>/dev/null \ + | grep -qE "MiB|GiB"; then + echo "FAIL (G3b control): VD-restore into empty RD produced no volume definition" >&2 + "${LCTL[@]}" volume-definition list --resource-definitions "$TGT_OK" 2>&1 | tail -10 >&2 + exit 1 +fi + +echo ">> [G3b control] OK — VD-restore into empty RD succeeded" +echo "PASS: snap-vd-restore-volume-conflict-rejected" diff --git a/tests/operator-harness/replay/snap-vd-restore-volume-conflict-rejected.yaml b/tests/operator-harness/replay/snap-vd-restore-volume-conflict-rejected.yaml new file mode 100644 index 00000000..9e9006ef --- /dev/null +++ b/tests/operator-harness/replay/snap-vd-restore-volume-conflict-rejected.yaml @@ -0,0 +1,90 @@ +name: snap-vd-restore-volume-conflict-rejected +description: | + Corner-case G3b catcher (two-phase restore edge — volume-number + conflict). + + `linstor snapshot volume-definition restore` hydrates a snapshot's + recorded volume layout onto a (typically pre-existing, EMPTY) target + RD. If the target RD ALREADY carries a volume-definition whose number + collides with one of the snapshot's, the restore must be REJECTED up + front with a typed FAIL_EXISTS_VLM_DFN envelope naming the offending + volume number — never partially mutate the target. + + Pre-fix the per-VD hydrate Create surfaced a bare 409 only AFTER an + earlier non-colliding VD had already landed, leaving a half-restored + RD. The handler now pre-checks every snapshot volume number against + the target's existing VDs and refuses before any Store mutation. + + Sequence: source RD (1 volume) on node1 → snapshot → target RD created + with its OWN volume 0 → VD-restore into the clashing target MUST be + rejected (CLI exit 10), and the target's own VD stays intact → control + VD-restore into a fresh EMPTY RD succeeds. + + L6 companion: tests/e2e/cli-matrix/snap-vd-restore-volume-conflict-rejected.sh + (richer per-VD size assertion); this replay codifies the operator + sequence + the reject/accept contract the harness asserts directly. + +prerequisites: + min_nodes: 1 + storage_pool: stand + +vars: + sp: stand + rd: snapg3b-src + +steps: + - name: create-src-rd + cmd: ["resource-definition", "create", "{{rd}}"] + expect_exit: 0 + - name: create-src-vd + cmd: ["volume-definition", "create", "{{rd}}", "64M"] + expect_exit: 0 + - name: r-c-node1 + cmd: ["resource", "create", "{{node1}}", "{{rd}}", "--storage-pool={{sp}}"] + expect_exit: 0 + - name: wait-source-uptodate + cmd: ["resource", "list", "--resources", "{{rd}}"] + expect_exit: 0 + await: + kind: all_uptodate + rd: "{{rd}}" + timeout_s: 240 + - name: snapshot-create + cmd: ["snapshot", "create", "{{rd}}", "snapg3b"] + expect_exit: 0 + # ---- target RD with its OWN clashing volume 0 ---- + - name: create-conflict-rd + cmd: ["resource-definition", "create", "snapg3b-tgt-conflict"] + expect_exit: 0 + - name: create-conflict-vd + cmd: ["volume-definition", "create", "snapg3b-tgt-conflict", "128M"] + expect_exit: 0 + # ---- VD-restore into the clashing target MUST be rejected ---- + # The snapshot records volume 0; the target already has volume 0, so + # the restore collides on the volume-number key. The handler refuses + # with FAIL_EXISTS_VLM_DFN (apiCallRcError envelope → CLI exit 10). + - name: vd-restore-conflict-rejected + cmd: ["snapshot", "volume-definition", "restore", + "--from-resource", "{{rd}}", + "--from-snapshot", "snapg3b", + "--to-resource", "snapg3b-tgt-conflict"] + expect_exit: 10 + # ---- control: VD-restore into a fresh EMPTY RD succeeds ---- + - name: create-empty-rd + cmd: ["resource-definition", "create", "snapg3b-tgt-ok"] + expect_exit: 0 + - name: vd-restore-into-empty-ok + cmd: ["snapshot", "volume-definition", "restore", + "--from-resource", "{{rd}}", + "--from-snapshot", "snapg3b", + "--to-resource", "snapg3b-tgt-ok"] + expect_exit: 0 + +teardown: + - cmd: ["resource-definition", "delete", "snapg3b-tgt-ok"] + - cmd: ["resource-definition", "delete", "snapg3b-tgt-conflict"] + - cmd: ["snapshot", "delete", "{{rd}}", "snapg3b"] + - cmd: ["resource-definition", "delete", "{{rd}}"] + +invariants: + - no_orphans