From d422218989e74423adcb32a87fe9e60aa6d9545d Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Sat, 24 Oct 2026 12:05:34 +0200 Subject: [PATCH 1/8] fix(snapshot): keep a restore's source until the restore is done A definition restored or cloned from a snapshot keeps its restore marker, and while the marker is there every new replica is brought up from that snapshot and placed on its nodes. Nothing protected the snapshot: `s d` deleted it straight away and the auto-snapshot prune deleted by age, so the definition's next replica came up blank and never reached UpToDate. A snapshot delete, on the REST door and in the CLI, now first ends the dependency: a definition whose restore is finished, by the rule an operator's own delete of the marker passes, has the marker taken off, and its later replicas sync from their peers. One whose restore is still running keeps it, and the delete is refused for as long as that lasts: a retryable 409 on the REST door, an error naming the definition in the CLI. The prune skips such a snapshot for the pass. The clone door already refused rewriting the marker through override_props. Fixes #197 Assisted-by: LLM Signed-off-by: Andrei Kvapil --- cmd/controller/main.go | 1 + .../snapshot_delete_restore_source_test.go | 66 ++++++++++ internal/cli/write_more.go | 8 +- .../controller/autosnapshot_controller.go | 53 +++++++- .../autosnapshot_restore_source_test.go | 114 ++++++++++++++++++ pkg/rest/restore_source_lifetime_test.go | 98 +++++++++++++++ pkg/rest/snapshots.go | 30 +++++ pkg/store/restore_source.go | 109 +++++++++++++++++ tests/integration/harness/manager.go | 6 +- 9 files changed, 480 insertions(+), 5 deletions(-) create mode 100644 internal/cli/snapshot_delete_restore_source_test.go create mode 100644 internal/controller/autosnapshot_restore_source_test.go create mode 100644 pkg/rest/restore_source_lifetime_test.go create mode 100644 pkg/store/restore_source.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 7ceaca77..d96ecaec 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -280,6 +280,7 @@ func main() { } if err := (&controller.AutoSnapshotRunnable{ Client: mgr.GetClient(), + Store: st, }).RegisterWithManager(mgr); err != nil { setupLog.Error(err, "Failed to register runnable", "runnable", "auto-snapshot") os.Exit(1) diff --git a/internal/cli/snapshot_delete_restore_source_test.go b/internal/cli/snapshot_delete_restore_source_test.go new file mode 100644 index 00000000..2c4cbbb7 --- /dev/null +++ b/internal/cli/snapshot_delete_restore_source_test.go @@ -0,0 +1,66 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cli + +import ( + "errors" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// The CLI deletes snapshots through the store directly, so it runs the same +// release the REST door does: a snapshot an unfinished restore reads from +// stays, with the reason. +func TestSnapshotDeleteRefusesWhileARestoreReadsFromIt(t *testing.T) { + t.Parallel() + + ctx := t.Context() + st := store.NewInMemory() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "src-cli"}); err != nil { + t.Fatalf("seed source: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{Name: "snap-cli", ResourceName: "src-cli"}); err != nil { + t.Fatalf("seed snapshot: %v", err) + } + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "dst-cli", + Props: map[string]string{store.RestoreFromSnapshotProp: "src-cli:snap-cli"}, + }); err != nil { + t.Fatalf("seed restored definition: %v", err) + } + + run := &runContext{ + Store: st, + Flags: &flagSet{Values: map[string]string{}, Positionals: []string{"src-cli", "snap-cli"}}, + } + + err := snapshotDelete(ctx, run) + if !errors.Is(err, store.ErrRestoreSourceInUse) { + t.Fatalf("snapshot delete under an unfinished restore = %v, want ErrRestoreSourceInUse", err) + } + + if _, err := st.Snapshots().Get(ctx, "src-cli", "snap-cli"); err != nil { + t.Fatalf("the refused delete took the snapshot: %v", err) + } +} diff --git a/internal/cli/write_more.go b/internal/cli/write_more.go index 94a31fb8..f115fefc 100644 --- a/internal/cli/write_more.go +++ b/internal/cli/write_more.go @@ -607,7 +607,13 @@ func snapshotDelete(ctx context.Context, run *runContext) error { rdName, snapName := run.Flags.Positionals[0], run.Flags.Positionals[1] - err := run.Store.Snapshots().Delete(ctx, rdName, snapName) + // The same release the REST door runs: see store.ReleaseRestoreSource. + err := store.ReleaseRestoreSource(ctx, run.Store, rdName, snapName) + if err != nil { + return fmt.Errorf("delete snapshot %s of %s: %w", snapName, rdName, err) + } + + err = run.Store.Snapshots().Delete(ctx, rdName, snapName) if err != nil && !isNotFound(err) { return fmt.Errorf("delete snapshot %s of %s: %w", snapName, rdName, err) } diff --git a/internal/controller/autosnapshot_controller.go b/internal/controller/autosnapshot_controller.go index 275926ab..02b9fad9 100644 --- a/internal/controller/autosnapshot_controller.go +++ b/internal/controller/autosnapshot_controller.go @@ -23,6 +23,7 @@ import ( "fmt" "sort" "strconv" + "strings" "time" "github.com/cockroachdb/errors" @@ -34,6 +35,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager" blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + "github.com/cozystack/blockstor/pkg/store" ) // AutoSnapshot prop keys mirror upstream LINSTOR's namespaced keys @@ -148,6 +150,11 @@ type AutoSnapshotRunnable struct { // Period overrides AutoSnapshotPeriod (test-only). A zero // Period falls back to the default. Period time.Duration + + // Store judges whether a restore from an auto-snapshot is finished + // before the prune deletes it (store.ReleaseRestoreSource). Nil keeps + // every auto-snapshot a definition was restored from. + Store store.Store } // NeedLeaderElection returns true: the cron writes Snapshot CRDs @@ -454,7 +461,18 @@ func (r *AutoSnapshotRunnable) pruneOldAutoSnapshots( continue } - err := r.Client.Delete(ctx, snap) + inUse, err := r.restoreReadsFrom(ctx, rd.Name, snap.Spec.SnapshotName) + if err != nil { + return err + } + + if inUse { + // Kept for this pass: a definition restored from it still needs + // it. It is pruned once that restore is finished. + continue + } + + err = r.Client.Delete(ctx, snap) if err != nil && !apierrors.IsNotFound(err) { return errors.Wrapf(err, "delete auto-snapshot %q", snap.Name) } @@ -463,6 +481,39 @@ func (r *AutoSnapshotRunnable) pruneOldAutoSnapshots( return nil } +// restoreReadsFrom reports whether a definition restored from rdName:snapName +// still needs the snapshot. A finished restore has its marker taken off and +// lets the snapshot go; see store.ReleaseRestoreSource. +func (r *AutoSnapshotRunnable) restoreReadsFrom(ctx context.Context, rdName, snapName string) (bool, error) { + if r.Store == nil { + var rds blockstoriov1alpha1.ResourceDefinitionList + + err := r.Client.List(ctx, &rds) + if err != nil { + return false, errors.Wrap(err, "list definitions restored from an auto-snapshot") + } + + for i := range rds.Items { + if strings.EqualFold(rds.Items[i].Spec.Props[store.RestoreFromSnapshotProp], rdName+":"+snapName) { + return true, nil + } + } + + return false, nil + } + + err := store.ReleaseRestoreSource(ctx, r.Store, rdName, snapName) + if errors.Is(err, store.ErrRestoreSourceInUse) { + return true, nil + } + + if err != nil { + return false, errors.Wrapf(err, "release auto-snapshot %s of %s", snapName, rdName) + } + + return false, nil +} + // RegisterWithManager wires the runnable into the controller-runtime // manager. Symmetrical to other Runnables in this package // (storage_sweeper, ObserverRunnable). diff --git a/internal/controller/autosnapshot_restore_source_test.go b/internal/controller/autosnapshot_restore_source_test.go new file mode 100644 index 00000000..e7a84ea5 --- /dev/null +++ b/internal/controller/autosnapshot_restore_source_test.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller_test + +import ( + "context" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + controllerpkg "github.com/cozystack/blockstor/internal/controller" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// The prune deleted by age alone. A definition restored from an +// auto-snapshot lost its source as soon as enough newer ones piled up, and +// its next replica came up blank. The prune keeps an auto-snapshot a +// definition still reads from. +func TestAutoSnapshotPruneKeepsTheSourceOfARestore(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + rd := makeRDWithAutoSnapshot(t, "pvc-keep-src", 15, "2") + + restored := &blockstoriov1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-restored"}, + Spec: blockstoriov1alpha1.ResourceDefinitionSpec{ + Props: map[string]string{store.RestoreFromSnapshotProp: "pvc-keep-src:auto-snap-00001"}, + }, + } + + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(rd, restored).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 4 { + if err := r.Tick(context.Background()); err != nil { + t.Fatalf("Tick #%d: %v", i, err) + } + + clk.advance(15*time.Minute + time.Second) + } + + kept := false + + for _, snap := range listAutoSnapshotsByRD(t, cli, "pvc-keep-src") { + if snap.Spec.SnapshotName == "auto-snap-00001" { + kept = true + } + } + + if !kept { + t.Fatal("the prune deleted the auto-snapshot a definition was restored from") + } +} + +// The production prune judges the restore through the store +// (store.ReleaseRestoreSource): a restore that is not finished keeps its +// auto-snapshot. +func TestAutoSnapshotPruneWithAStoreKeepsTheSourceOfAnUnfinishedRestore(t *testing.T) { + t.Parallel() + + scheme := newScheme(t) + rd := makeRDWithAutoSnapshot(t, "pvc-keep-st", 15, "2") + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(rd).Build() + + st := store.NewInMemory() + if err := st.ResourceDefinitions().Create(context.Background(), &apiv1.ResourceDefinition{ + Name: "pvc-restored-st", + Props: map[string]string{store.RestoreFromSnapshotProp: "pvc-keep-st:auto-snap-00001"}, + }); err != nil { + t.Fatalf("seed restored definition: %v", err) + } + + clk := &stubClock{t: time.Date(2026, 5, 14, 12, 0, 0, 0, time.UTC)} + r := &controllerpkg.AutoSnapshotRunnable{Client: cli, Clock: clk, Store: st} + + for i := range 4 { + if err := r.Tick(context.Background()); err != nil { + t.Fatalf("Tick #%d: %v", i, err) + } + + clk.advance(15*time.Minute + time.Second) + } + + for _, snap := range listAutoSnapshotsByRD(t, cli, "pvc-keep-st") { + if snap.Spec.SnapshotName == "auto-snap-00001" { + return + } + } + + t.Fatal("the prune deleted the auto-snapshot an unfinished restore reads from") +} diff --git a/pkg/rest/restore_source_lifetime_test.go b/pkg/rest/restore_source_lifetime_test.go new file mode 100644 index 00000000..cf458d3e --- /dev/null +++ b/pkg/rest/restore_source_lifetime_test.go @@ -0,0 +1,98 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "errors" + "net/http" + "testing" + + "github.com/cozystack/blockstor/pkg/store" +) + +// A restored definition reads from its snapshot until a replica is up from +// it. `s d` under a restore still running went straight through, and the +// replicas the restore had not brought up yet came up blank. +func TestSnapshotDeleteIsRefusedWhileARestoreReadsFromIt(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedDeployedCloneSource(t, st, "src-busy") + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := cloneOnce(t, base, "src-busy", "dst-busy", nil); code != http.StatusCreated { + t.Fatalf("clone = %d, want 201", code) + } + + snap := cloneSnapshotName("dst-busy") + + resp := httpDelete(t, base+"/v1/resource-definitions/src-busy/snapshots/"+snap) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusConflict { + t.Fatalf("snapshot delete under an unfinished restore = %d, want 409", resp.StatusCode) + } + + if _, err := st.Snapshots().Get(t.Context(), "src-busy", snap); err != nil { + t.Fatalf("the refused delete took the snapshot: %v", err) + } +} + +// Once the restore is done the dependency ends: the delete takes the marker +// off the restored definition first, so its later replicas sync from their +// peers instead of looking for a snapshot that is gone. +func TestSnapshotDeleteReleasesAFinishedRestore(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedDeployedCloneSource(t, st, "src-done") + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := cloneOnce(t, base, "src-done", "dst-done", nil); code != http.StatusCreated { + t.Fatalf("clone = %d, want 201", code) + } + + reportVolumesUpToDate(t, st, "dst-done") + + snap := cloneSnapshotName("dst-done") + + resp := httpDelete(t, base+"/v1/resource-definitions/src-done/snapshots/"+snap) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("snapshot delete after the restore finished = %d, want 200", resp.StatusCode) + } + + rd, err := st.ResourceDefinitions().Get(t.Context(), "dst-done") + if err != nil { + t.Fatalf("read back: %v", err) + } + + if mark, ok := rd.Props[store.RestoreFromSnapshotProp]; ok { + t.Errorf("the restored definition still names the deleted snapshot: %q", mark) + } + + if _, err := st.Snapshots().Get(t.Context(), "src-done", snap); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the snapshot is still there: err=%v", err) + } +} diff --git a/pkg/rest/snapshots.go b/pkg/rest/snapshots.go index 874ef3df..892fa806 100644 --- a/pkg/rest/snapshots.go +++ b/pkg/rest/snapshots.go @@ -1201,6 +1201,32 @@ func listDiskfulNodes(ctx context.Context, s *Server, rd string) ([]string, erro return out, nil } +// refuseSnapshotDeleteUnderRestore runs store.ReleaseRestoreSource before a +// snapshot delete: a definition restored from the snapshot reads from it +// until its restore is done. True means the answer has been written. +func (s *Server) refuseSnapshotDeleteUnderRestore(w http.ResponseWriter, r *http.Request, rd, snapName string) bool { + err := store.ReleaseRestoreSource(r.Context(), s.Store, rd, snapName) + if errors.Is(err, store.ErrRestoreSourceInUse) { + writeJSON(w, http.StatusConflict, []apiv1.APICallRc{{ + RetCode: apiCallRcError, + Message: "snapshot '" + snapName + "' of '" + rd + "' cannot be deleted yet: " + err.Error(), + Cause: "a restore from this snapshot has not brought up a replica with every volume UpToDate", + Correc: "retry the delete once that restore has finished", + ObjRefs: map[string]string{objRefRscDfn: rd, objRefSnapshotDfn: snapName}, + }}) + + return true + } + + if err != nil { + writeStoreError(w, err) + + return true + } + + return false +} + // handleSnapshotDelete answers `DELETE /v1/resource-definitions/{rd}/snapshots/{snap}` // with an idempotent 200 + ApiCallRc envelope. CSI spec §DeleteSnapshot // mandates idempotence: the driver retries until it sees success, so @@ -1237,6 +1263,10 @@ func (s *Server) handleSnapshotDelete(w http.ResponseWriter, r *http.Request) { rd := r.PathValue("rd") snapName := r.PathValue("snap") + if s.refuseSnapshotDeleteUnderRestore(w, r, rd, snapName) { + return + } + err := s.Store.Snapshots().Delete(r.Context(), rd, snapName) if err != nil { if errors.Is(err, store.ErrNotFound) { diff --git a/pkg/store/restore_source.go b/pkg/store/restore_source.go new file mode 100644 index 00000000..f222d354 --- /dev/null +++ b/pkg/store/restore_source.go @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package store + +import ( + "context" + "errors" + "fmt" + "slices" + "strings" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" +) + +// ErrRestoreSourceInUse refuses deleting a snapshot that a restore which is +// not finished yet still reads from. +var ErrRestoreSourceInUse = errors.New("a restore from this snapshot is not finished") + +// ReleaseRestoreSource gets snapshot srcRD:snapName ready to be deleted. +// +// A restored or cloned definition keeps its restore marker after the restore +// is done, and while the marker is there every new replica is brought up from +// that snapshot and placed on its nodes. Deleting the snapshot under it left +// the definition looking fine until it needed a replica (node replacement, +// `rd ap`, a toggle back to diskful), which then came up blank and never +// reached UpToDate. +// +// So a delete first ends the dependency: every definition whose marker names +// the snapshot and whose restore is finished (RestoreMarkerClearRefusal, the +// rule an operator's own delete of the marker passes) has the marker taken +// off, and its later replicas sync from their peers. A definition whose +// restore is still running keeps it, and the delete is refused with +// ErrRestoreSourceInUse; that is retryable, and lasts as long as the restore. +// A definition being deleted is not waited for. +func ReleaseRestoreSource(ctx context.Context, st Store, srcRD, snapName string) error { + marker := srcRD + ":" + snapName + + rds, err := st.ResourceDefinitions().List(ctx) + if err != nil { + return fmt.Errorf("list the definitions restored from %s: %w", marker, err) + } + + for i := range rds { + if !strings.EqualFold(rds[i].Props[RestoreFromSnapshotProp], marker) { + continue + } + + if slices.Contains(rds[i].Flags, apiv1.ResourceFlagDelete) { + continue + } + + err = releaseRestoreMarker(ctx, st, rds[i].Name, marker) + if err != nil { + return err + } + } + + return nil +} + +func releaseRestoreMarker(ctx context.Context, st Store, rdName, marker string) error { + refusal := RestoreMarkerClearRefusal(ctx, st, rdName) + if errors.Is(refusal, ErrRestoreUnfinished) { + return fmt.Errorf("%w: %q is still being restored from it (%w)", ErrRestoreSourceInUse, rdName, refusal) + } + + if errors.Is(refusal, ErrNotFound) { + return nil + } + + if refusal != nil { + return refusal + } + + err := st.ResourceDefinitions().PatchResourceDefinitionSpec(ctx, rdName, func(rd *apiv1.ResourceDefinition) error { + // Re-checked on the object the patch writes: a marker rewritten in + // between is somebody else's restore. + if strings.EqualFold(rd.Props[RestoreFromSnapshotProp], marker) { + delete(rd.Props, RestoreFromSnapshotProp) + } + + return nil + }) + if errors.Is(err, ErrNotFound) { + return nil + } + + if err != nil { + return fmt.Errorf("take the restore marker off %q: %w", rdName, err) + } + + return nil +} diff --git a/tests/integration/harness/manager.go b/tests/integration/harness/manager.go index cf4ebb47..66de560d 100644 --- a/tests/integration/harness/manager.go +++ b/tests/integration/harness/manager.go @@ -237,7 +237,7 @@ func wireReconcilers(mgr manager.Manager, store *storek8s.Store) error { return err } - return wireBackgroundRunnables(mgr) + return wireBackgroundRunnables(mgr, store) } func wireNodeReconcilers(mgr manager.Manager, store *storek8s.Store) error { @@ -338,8 +338,8 @@ func wireResourceReconcilers(mgr manager.Manager, store *storek8s.Store) error { return nil } -func wireBackgroundRunnables(mgr manager.Manager) error { - err := (&controller.AutoSnapshotRunnable{Client: mgr.GetClient()}).RegisterWithManager(mgr) +func wireBackgroundRunnables(mgr manager.Manager, store *storek8s.Store) error { + err := (&controller.AutoSnapshotRunnable{Client: mgr.GetClient(), Store: store}).RegisterWithManager(mgr) if err != nil { return fmt.Errorf("auto-snapshot: %w", err) } From 4578438578fddfcc68898beb67ab7a887b3bec0e Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Sat, 24 Oct 2026 12:05:34 +0200 Subject: [PATCH 2/8] fix(restore): refuse a target name with orphan replicas holding no data A restore or clone into a name with no definition but with a diskless replica under it created the definition over that replica. The stamp on its node then failed, and the rollback of the definition cascaded over every replica under the name, deleting one the request had not made. Both restore doors, REST and CLI, now refuse such a name with 409 before they write anything, and name the replicas. A replica holding data is still taken as placed, the controller's tie-breaker witness is still promoted by the stamp, and a replica already being deleted is no obstacle. Fixes #203 Assisted-by: LLM Signed-off-by: Andrei Kvapil --- internal/cli/snapshot.go | 10 ++- internal/cli/snapshot_restore_orphans_test.go | 56 ++++++++++++ pkg/rest/restore_orphan_replicas_test.go | 87 +++++++++++++++++++ pkg/rest/snapshot_restore.go | 13 ++- pkg/store/restore_orphans_test.go | 43 +++++++++ pkg/store/restore_source.go | 63 ++++++++++++++ 6 files changed, 270 insertions(+), 2 deletions(-) create mode 100644 internal/cli/snapshot_restore_orphans_test.go create mode 100644 pkg/rest/restore_orphan_replicas_test.go create mode 100644 pkg/store/restore_orphans_test.go diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 76b3fc6e..70fc0209 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -1097,13 +1097,21 @@ func unwindVolumes(ctx context.Context, run *runContext, rdName string, added [] func planRestoredReplicas( ctx context.Context, run *runContext, srcRD, rdName string, snap *apiv1.Snapshot, ) ([]apiv1.Resource, error) { + // The same refusal the REST door makes, before anything is written: see + // store.RefuseReplicasWithoutDefinition. A target that already has its + // definition passes it. + err := store.RefuseReplicasWithoutDefinition(ctx, run.Store, rdName) + if err != nil { + return nil, err //nolint:wrapcheck // names the definition and the replicas itself + } + nodes := run.Flags.Nodes // Named nodes have to actually hold the snapshot. Restoring onto one // that does not produces a replica with nothing behind it: the objects // are created, the satellite finds no snapshot to receive, and the // command reports success over an empty volume. - err := validate.RestoreNodesHoldSnapshot(nodes, snap.Nodes) + err = validate.RestoreNodesHoldSnapshot(nodes, snap.Nodes) if err != nil { //nolint:wrapcheck // a semantic refusal; see checkVolumeSize return nil, err diff --git a/internal/cli/snapshot_restore_orphans_test.go b/internal/cli/snapshot_restore_orphans_test.go new file mode 100644 index 00000000..b0915ae7 --- /dev/null +++ b/internal/cli/snapshot_restore_orphans_test.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cli_test + +import ( + "context" + "errors" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// `s resource restore` into a name with a diskless replica and no +// definition over it is refused before anything is written, the way the +// REST door refuses it, and the replica stays. +func TestSnapshotRestoreRefusesATargetWithOrphanReplicas(t *testing.T) { + t.Parallel() + + app, _, _ := newApp(t, func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + _ = backend.Resources().Create(ctx, &apiv1.Resource{ + Name: "pvc-orph", NodeName: "node-2", Flags: []string{apiv1.ResourceFlagDiskless}, + }) + }) + + if got := app.Run(t.Context(), restoreResourceArgv("pvc-orph")); got == 0 { + t.Fatal("restore over an orphan diskless replica succeeded") + } + + st := appStore(t, app) + + if _, err := st.Resources().Get(t.Context(), "pvc-orph", "node-2"); err != nil { + t.Fatalf("the refusal took the orphan: %v", err) + } + + if _, err := st.ResourceDefinitions().Get(t.Context(), "pvc-orph"); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("the refusal left a definition behind: err=%v", err) + } +} diff --git a/pkg/rest/restore_orphan_replicas_test.go b/pkg/rest/restore_orphan_replicas_test.go new file mode 100644 index 00000000..854a0689 --- /dev/null +++ b/pkg/rest/restore_orphan_replicas_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "errors" + "net/http" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// Replicas under a name with no definition over them are not this request's +// work. Created over them, the restore adopted them, and its rollback after a +// failed placement cascaded over them too. +func TestCloneRefusesATargetNameWithReplicasAndNoDefinition(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, st, "src-orph") + + if err := st.Resources().Create(ctx, &apiv1.Resource{ + Name: "dst-orph", NodeName: "node-x", Flags: []string{apiv1.ResourceFlagDiskless}, + }); err != nil { + t.Fatalf("seed the replica: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := cloneOnce(t, base, "src-orph", "dst-orph", nil); code != http.StatusConflict { + t.Fatalf("clone over replicas with no definition = %d, want 409", code) + } + + if _, err := st.Resources().Get(ctx, "dst-orph", "node-x"); err != nil { + t.Fatalf("the refusal took the replica that was there: %v", err) + } + + if _, err := st.ResourceDefinitions().Get(ctx, "dst-orph"); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("the refusal left a definition behind: err=%v", err) + } +} + +// The controller's tie-breaker witness is not refused: the stamp promotes it +// into the replica the restore asked for, as it did before. +func TestRestoreStillPromotesAnOrphanWitness(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, backend, "src-tbo") + seedRestoreSnapshot(t, backend, "src-tbo", "snap-tbo", []string{"node-a"}) + + if err := backend.Resources().Create(ctx, &apiv1.Resource{ + Name: "dst-tbo", NodeName: "node-a", + Flags: []string{apiv1.ResourceFlagDiskless, apiv1.ResourceFlagTieBreaker}, + }); err != nil { + t.Fatalf("seed the witness: %v", err) + } + + base, stop := startServerWithStore(t, backend) + defer stop() + + if code := restoreOnce(t, base, "src-tbo", "snap-tbo", map[string]any{ + "to_resource": "dst-tbo", "node_names": []string{"node-a"}, + }); code != http.StatusCreated { + t.Fatalf("restore onto a node holding an orphan witness = %d, want 201", code) + } +} diff --git a/pkg/rest/snapshot_restore.go b/pkg/rest/snapshot_restore.go index 22789e54..3555fa5c 100644 --- a/pkg/rest/snapshot_restore.go +++ b/pkg/rest/snapshot_restore.go @@ -1472,7 +1472,12 @@ func flagUnappliedGroupStack(ctx context.Context, st store.Store, rd *apiv1.Reso // what makes the decision on fresh state rather than on the read that lost the // race. func (s *Server) createOrAdoptRestoredRD(ctx context.Context, newRD *apiv1.ResourceDefinition, snap *apiv1.Snapshot, overrides *rdShapeOverrides) (bool, string, error) { - err := s.Store.ResourceDefinitions().Create(ctx, newRD) + err := s.refuseReplicasWithoutDefinition(ctx, newRD.Name) + if err != nil { + return false, "", err + } + + err = s.Store.ResourceDefinitions().Create(ctx, newRD) if err == nil { return true, "", nil } @@ -1803,6 +1808,12 @@ func (s *Server) stampRestoredResourcesOnNodes(ctx context.Context, srcRDName, n return placed, nil } +// refuseReplicasWithoutDefinition is store.RefuseReplicasWithoutDefinition +// on this server's store, run before the restored definition is created. +func (s *Server) refuseReplicasWithoutDefinition(ctx context.Context, rdName string) error { + return store.RefuseReplicasWithoutDefinition(ctx, s.Store, rdName) //nolint:wrapcheck // surfaced via writeStoreError +} + // errReplicaStillDeleting marks a stamp refused because the replica already // under that (definition, node) is going away. It wraps ErrAlreadyExists, but // is answered with no FAIL_EXISTS band; see materialiseRefusalKind. diff --git a/pkg/store/restore_orphans_test.go b/pkg/store/restore_orphans_test.go new file mode 100644 index 00000000..ee5bba03 --- /dev/null +++ b/pkg/store/restore_orphans_test.go @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package store + +import ( + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" +) + +// A diskless replica already being deleted is going whatever the restore does: +// it does not refuse the name. +func TestRefuseReplicasWithoutDefinitionPassesOverOneBeingDeleted(t *testing.T) { + t.Parallel() + + st := NewInMemory() + if err := st.Resources().Create(t.Context(), &apiv1.Resource{ + Name: "dst-going", NodeName: "node-a", + Flags: []string{apiv1.ResourceFlagDiskless, apiv1.ResourceFlagDelete}, + }); err != nil { + t.Fatalf("seed: %v", err) + } + + if err := RefuseReplicasWithoutDefinition(t.Context(), st, "dst-going"); err != nil { + t.Fatalf("refused over a replica being deleted: %v", err) + } +} diff --git a/pkg/store/restore_source.go b/pkg/store/restore_source.go index f222d354..c5d085d3 100644 --- a/pkg/store/restore_source.go +++ b/pkg/store/restore_source.go @@ -107,3 +107,66 @@ func releaseRestoreMarker(ctx context.Context, st Store, rdName, marker string) return nil } + +// ErrReplicasWithoutDefinition refuses a restore or clone into a name that +// has no definition but already has diskless replicas under it. +var ErrReplicasWithoutDefinition = errors.New("replicas already exist under that name with no definition over them") + +// RefuseReplicasWithoutDefinition refuses materialising a new definition over +// diskless replicas that are already there with none over them: an +// operator's diskless replica left by a cascade that did not finish, or +// applied by hand. Both restore doors, REST and CLI, run it before they +// create the definition. +// +// Created over them, the definition adopted them, the stamp on their node +// failed because a replica with no data is not one a restore can use, and the +// rollback of the definition it had created cascaded over every replica under +// the name, taking those with it although the request had not made them. +// Refusing before anything is written leaves them where they were and names +// them. +// +// A replica that holds data is not refused here: the stamp takes it as +// placed, and it is the restore's to keep or roll back. Nor is the +// controller's tie-breaker witness, which the stamp promotes into the replica +// the restore asked for. A replica already being deleted is no obstacle +// either: it is going whatever the restore does. +// +// Read past the cache on both sides: a definition the cache has not seen yet +// would make its own replicas read as orphans. The answer wraps +// ErrAlreadyExists, so the doors answer it with 409. +func RefuseReplicasWithoutDefinition(ctx context.Context, st Store, rdName string) error { + _, err := st.ResourceDefinitions().GetUncached(ctx, rdName) + if err == nil { + return nil + } + + if !errors.Is(err, ErrNotFound) { + return fmt.Errorf("read %q to check for replicas without it: %w", rdName, err) + } + + replicas, err := LiveReplicas(ctx, st, rdName) + if err != nil { + return fmt.Errorf("list the replicas under %q: %w", rdName, err) + } + + var orphans []string + + for i := range replicas { + flags := replicas[i].Flags + if slices.Contains(flags, apiv1.ResourceFlagDelete) || slices.Contains(flags, apiv1.ResourceFlagTieBreaker) { + continue + } + + if slices.Contains(flags, apiv1.ResourceFlagDiskless) { + orphans = append(orphans, replicas[i].NodeName) + } + } + + if len(orphans) == 0 { + return nil + } + + return fmt.Errorf("%w: %q has diskless replicas on %s but no resource definition: %w; "+ + "delete those Resource objects, then retry", ErrAlreadyExists, rdName, strings.Join(orphans, ", "), + ErrReplicasWithoutDefinition) +} From c56786dc28cdd379b3e8c485a2d6cfbae060ea7d Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 18:29:24 +0100 Subject: [PATCH 3/8] fix(rollback): let a retry take over an in-progress mark nobody holds A clone or restore rollback marks the definition in-progress while it runs, and every retry over that mark answers 409. An apiserver replica OOM-killed or lost between writing the mark and taking it off left it for good, and linstor-csi retried CreateVolume against the 409 until an operator cleared the prop by hand. The mark now carries the time it was written. A retry, on the REST doors and in the CLI, takes over a mark older than two minutes and resumes the leftover; a mark another retry already took off counts as taken over. Every rollback that writes the mark runs on a budget well under that: the REST door's detached one, and a one-minute budget the CLI rollback now gets as well, which had no deadline before. The mark only ever stands over a definition left whole, and a rollback still alive past its budget finds the mark gone at its next step and stands down without deleting. Fixes #213 Assisted-by: LLM Signed-off-by: Andrei Kvapil --- internal/cli/rollback_budget_test.go | 93 +++++++++++++++++++ internal/cli/snapshot.go | 8 ++ pkg/rest/rd_clone.go | 65 ++++++++++--- pkg/rest/rg_deleted_race.go | 10 +- .../rollback_handshake_failclosed_test.go | 3 +- pkg/rest/rollback_mark_stale_test.go | 68 ++++++++++++++ pkg/store/local_props.go | 69 +++++++++++++- pkg/store/restore_target.go | 84 ++++++++++++++--- pkg/store/rollback_stale_test.go | 87 +++++++++++++++++ 9 files changed, 451 insertions(+), 36 deletions(-) create mode 100644 internal/cli/rollback_budget_test.go create mode 100644 pkg/rest/rollback_mark_stale_test.go create mode 100644 pkg/store/rollback_stale_test.go diff --git a/internal/cli/rollback_budget_test.go b/internal/cli/rollback_budget_test.go new file mode 100644 index 00000000..ce979dac --- /dev/null +++ b/internal/cli/rollback_budget_test.go @@ -0,0 +1,93 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cli + +import ( + "context" + "errors" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +type deadlineRecordingRDs struct { + store.ResourceDefinitionStore + + deadlines *[]time.Duration +} + +func (d deadlineRecordingRDs) PatchResourceDefinitionSpec( + ctx context.Context, name string, mutate func(*apiv1.ResourceDefinition) error, +) error { + if deadline, ok := ctx.Deadline(); ok { + *d.deadlines = append(*d.deadlines, time.Until(deadline)) + } else { + *d.deadlines = append(*d.deadlines, -1) + } + + return d.ResourceDefinitionStore.PatchResourceDefinitionSpec(ctx, name, mutate) //nolint:wrapcheck // test double +} + +type deadlineRecordingStore struct { + store.Store + + deadlines *[]time.Duration +} + +func (s deadlineRecordingStore) ResourceDefinitions() store.ResourceDefinitionStore { + return deadlineRecordingRDs{ResourceDefinitionStore: s.Store.ResourceDefinitions(), deadlines: s.deadlines} +} + +var errRestorePlacement = errors.New("placement failed") + +// A CLI rollback writes the same in-progress mark a REST one does, and a +// retry takes a mark over once it is stale on the grounds that no rollback +// can still be running. The CLI command has no deadline of its own, so the +// rollback must carry one, shorter than the staleness bound. +func TestCLIRollbackRunsOnABudget(t *testing.T) { + t.Parallel() + + ctx := t.Context() + backend := store.NewInMemory() + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-budget"}); err != nil { + t.Fatalf("seed: %v", err) + } + + var deadlines []time.Duration + + run := &runContext{ + Store: deadlineRecordingStore{Store: backend, deadlines: &deadlines}, + Flags: &flagSet{Values: map[string]string{}}, + } + + _ = rollbackRestore(context.Background(), run, "pvc-budget", nil, errRestorePlacement) + + if len(deadlines) == 0 { + t.Fatal("the rollback wrote no mark") + } + + for _, left := range deadlines { + if left < 0 || left > store.CLIRollbackBudget { + t.Fatalf("a rollback write ran with %s left, want a deadline within %s", left, store.CLIRollbackBudget) + } + } +} diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 70fc0209..1dc0d3f9 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -778,6 +778,14 @@ func volumeAlreadyRestored(ctx context.Context, run *runContext, rdName string, func rollbackRestore( ctx context.Context, run *runContext, rdName string, planned []apiv1.Resource, cause error, ) error { + // The rollback runs on a budget of its own, like the REST door's. A retry + // takes over an in-progress mark older than store.StaleRollbackAfter on + // the grounds that nobody is behind it; a CLI rollback without a + // deadline could still be, and could then take apart a definition the + // retry had answered for. + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), store.CLIRollbackBudget) + defer cancel() + err := store.HoldRollback(ctx, run.Store, rdName, true) if errors.Is(err, store.ErrRollbackGone) { return cause diff --git a/pkg/rest/rd_clone.go b/pkg/rest/rd_clone.go index b0c7272a..87b91e98 100644 --- a/pkg/rest/rd_clone.go +++ b/pkg/rest/rd_clone.go @@ -25,6 +25,7 @@ import ( "slices" "strconv" "strings" + "time" "github.com/LINBIT/golinstor/client" "github.com/LINBIT/golinstor/clonestatus" @@ -1472,22 +1473,8 @@ func (s *Server) abandonedRollbackRefusal( return 0, nil } - // A rollback that has not reported a step is one still running, or one - // whose process stopped mid-way. Both are refused, but the first ends on - // its own, so the advice says so before it says to delete by hand; the - // CLI door words it the same way (store.JudgeRestoreLeftover). if spelled == rollbackInProgress { - return http.StatusConflict, &apiv1.APICallRc{ - RetCode: apiCallRcError, - Message: operation + " target '" + rdName + "' is being rolled back by the attempt " + - "that created it", - Cause: "an earlier attempt at this " + operation + " started rolling the definition " + - "back and has not reported how it ended: it is still running, or it stopped mid-way", - Correc: "retry the " + operation + " once that rollback has finished, which takes under a " + - "minute; if the mark is still there a minute after the failure, " + clearRollbackMarkCommand(rdName) + - " to keep '" + rdName + "' as it stands, or delete it by hand. Cleared while the rollback " + - "still runs, it stops that rollback before it deletes anything", - } + return s.inProgressRollbackRefusal(ctx, operation, rdName) } step, known := rollbackStepByName(spelled) @@ -1515,6 +1502,54 @@ func (s *Server) knownOrReadCloneSnapshot( return readCloneSnapshot(r.Context(), w, s.Store, srcName, cloneName) } +// inProgressRollbackRefusal answers a retry over an in-progress rollback mark. +// A rollback that has not reported a step is one still running, or one whose +// process stopped mid-way. A mark older than store.StaleRollbackAfter has +// nobody behind it and is taken over, and the retry resumes; a younger one is +// refused, with the advice that it ends on its own. The CLI door does the same +// (store.JudgeRestoreLeftover). +func (s *Server) inProgressRollbackRefusal(ctx context.Context, operation, rdName string) (int, *apiv1.APICallRc) { + took, status, rc := s.takeOverStaleRollbackMark(ctx, operation, rdName) + if rc != nil { + return status, rc + } + + if took { + return 0, nil + } + + return http.StatusConflict, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: operation + " target '" + rdName + "' is being rolled back by the attempt " + + "that created it", + Cause: "an earlier attempt at this " + operation + " started rolling the definition " + + "back and has not reported how it ended: it is still running, or it stopped mid-way", + Correc: "retry the " + operation + " once that rollback has finished, which takes under a " + + "minute; a retry takes over a mark nobody has touched for two minutes. To keep '" + rdName + + "' as it stands sooner, " + clearRollbackMarkCommand(rdName) + ", or delete it by hand. Cleared " + + "while the rollback still runs, it stops that rollback before it deletes anything", + } +} + +// takeOverStaleRollbackMark takes over an in-progress rollback mark older than +// store.StaleRollbackAfter, whose writer is gone, so the retry can resume the +// leftover. A non-nil rc is the answer when the check itself failed. +func (s *Server) takeOverStaleRollbackMark( + ctx context.Context, operation, rdName string, +) (bool, int, *apiv1.APICallRc) { + took, err := store.TakeOverStaleRollback(ctx, s.Store, rdName, time.Now()) + if err != nil { + return false, http.StatusInternalServerError, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: operation + " target '" + rdName + "' carries a rollback mark that could not be " + + "checked for staleness: " + scrubImplDetails(err.Error()), + Correc: "retry the " + operation, + } + } + + return took, 0, nil +} + // ensureCloneSnapshot takes (or reuses) the internal snapshot backing // a data-plane clone. Returns (snap, true) when the caller may // proceed; (nil, false) when a refusal envelope was already written. diff --git a/pkg/rest/rg_deleted_race.go b/pkg/rest/rg_deleted_race.go index c9bb05da..d3189cf6 100644 --- a/pkg/rest/rg_deleted_race.go +++ b/pkg/rest/rg_deleted_race.go @@ -267,7 +267,7 @@ func (s *Server) enterDestructiveRollback(ctx context.Context, rdName string) (b return errRollbackMarkNotOurs } - rd.Props[rollbackAbandonedKey] = rollbackStepNames[rollbackStepReapReplicas] + rd.Props = store.MarkRollback(rd.Props, rollbackStepNames[rollbackStepReapReplicas], time.Now()) return nil }) @@ -465,7 +465,7 @@ func (s *Server) patchRollbackMarkAway(ctx context.Context, rdName string) error return s.Store.ResourceDefinitions().PatchResourceDefinitionSpec(markCtx, rdName, //nolint:wrapcheck // logged by the caller func(rd *apiv1.ResourceDefinition) error { if rd.Props[rollbackAbandonedKey] == rollbackInProgress { - delete(rd.Props, rollbackAbandonedKey) + store.UnmarkRollback(rd.Props) } return nil @@ -514,11 +514,7 @@ func (s *Server) markRollbackAbandoned(ctx context.Context, rdName, step string) go func() { done <- s.Store.ResourceDefinitions().PatchResourceDefinitionSpec(markCtx, rdName, func(rd *apiv1.ResourceDefinition) error { - if rd.Props == nil { - rd.Props = map[string]string{} - } - - rd.Props[rollbackAbandonedKey] = step + rd.Props = store.MarkRollback(rd.Props, step, time.Now()) return nil }) diff --git a/pkg/rest/rollback_handshake_failclosed_test.go b/pkg/rest/rollback_handshake_failclosed_test.go index 09b066f8..8a6a8e6a 100644 --- a/pkg/rest/rollback_handshake_failclosed_test.go +++ b/pkg/rest/rollback_handshake_failclosed_test.go @@ -409,7 +409,8 @@ func TestTheInProgressRefusalNamesTheClearAndWhenItIsSafe(t *testing.T) { } for _, want := range []string{ - "set-property dst-inprog " + store.RollbackAbandonedProp, "a minute after the failure", "stops that rollback before it deletes anything", + "set-property dst-inprog " + store.RollbackAbandonedProp, "takes over a mark nobody has touched for two minutes", + "stops that rollback before it deletes anything", } { if !strings.Contains(refusal.Correc, want) { t.Errorf("correction %q does not say %q", refusal.Correc, want) diff --git a/pkg/rest/rollback_mark_stale_test.go b/pkg/rest/rollback_mark_stale_test.go new file mode 100644 index 00000000..e1835ff7 --- /dev/null +++ b/pkg/rest/rollback_mark_stale_test.go @@ -0,0 +1,68 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "net/http" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// An in-progress mark whose writer was killed stood for good, and every +// retry answered 409 until somebody cleared it by hand. A retry now takes +// over a mark older than store.StaleRollbackAfter and resumes; a fresh one +// is still refused. +func TestCloneRetryTakesOverAStaleRollbackMark(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + age time.Duration + want int + }{ + {name: "fresh", age: 10 * time.Second, want: http.StatusConflict}, + {name: "stale", age: store.StaleRollbackAfter + time.Minute, want: http.StatusCreated}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + src, dst := "src-stale-"+tc.name, "dst-stale-"+tc.name + seedDeployedCloneSource(t, st, src) + + props := store.MarkRollback(map[string]string{ + restoreFromSnapshotKey: restoreMarker(src, cloneSnapshotName(dst)), + }, rollbackInProgress, time.Now().Add(-tc.age)) + + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{Name: dst, Props: props}); err != nil { + t.Fatalf("seed the leftover: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := cloneOnce(t, base, src, dst, nil); code != tc.want { + t.Fatalf("retry over a %s in-progress mark = %d, want %d", tc.name, code, tc.want) + } + }) + } +} diff --git a/pkg/store/local_props.go b/pkg/store/local_props.go index 1dc85ce5..a37f66f0 100644 --- a/pkg/store/local_props.go +++ b/pkg/store/local_props.go @@ -24,6 +24,7 @@ import ( "slices" "strconv" "strings" + "time" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/passphrase" @@ -44,6 +45,70 @@ const RollbackAbandonedProp = "BlockstorRollbackAbandoned" // rollback is running, before it reports the step it stopped at. const RollbackInProgress = "in-progress" +// RollbackSinceProp records when the in-progress mark was written, in RFC +// 3339. A rollback that is still running holds the mark for less than its +// budget; one whose process was killed between writing the mark and taking it +// off leaves it for good, and every retry over it answered 409 until somebody +// cleared it by hand. With the time on it, a retry takes over a mark older +// than StaleRollbackAfter (TakeOverStaleRollback). +const RollbackSinceProp = "Blockstor/RollbackSince" + +// StaleRollbackAfter is how long an in-progress mark may stand before a retry +// takes it over. Every rollback runs on a budget, the REST door's detached one +// or CLIRollbackBudget, and every write it makes after that lands on an +// expired context, so a mark older than this has nobody behind it. The margin +// covers clock skew between the machines that wrote and read the mark: the +// API server replicas, and the operator's machine for a CLI rollback. Should a +// skewed clock let a retry take over early, a rollback still running finds +// the mark gone at its next step and stands down without deleting. +const StaleRollbackAfter = 2 * time.Minute + +// CLIRollbackBudget bounds a CLI rollback the way the REST door's detached +// budget bounds its own, so every rollback that writes an in-progress mark +// is over well before StaleRollbackAfter lets a retry take the mark over. +const CLIRollbackBudget = time.Minute + +// MarkRollback writes the rollback mark into props: the in-progress mark with +// the time it was written, or a step, which stands until an operator acts and +// carries no time. +func MarkRollback(props map[string]string, step string, now time.Time) map[string]string { + if props == nil { + props = map[string]string{} + } + + props[RollbackAbandonedProp] = step + + if step == RollbackInProgress { + props[RollbackSinceProp] = now.UTC().Format(time.RFC3339) + } else { + delete(props, RollbackSinceProp) + } + + return props +} + +// UnmarkRollback removes the rollback mark and its time. +func UnmarkRollback(props map[string]string) { + delete(props, RollbackAbandonedProp) + delete(props, RollbackSinceProp) +} + +// rollbackMarkStale reports whether props carry an in-progress mark written +// more than StaleRollbackAfter before now. A mark with no readable time is +// never stale: it is taken off by hand, as before. +func rollbackMarkStale(props map[string]string, now time.Time) bool { + if props[RollbackAbandonedProp] != RollbackInProgress { + return false + } + + since, err := time.Parse(time.RFC3339, props[RollbackSinceProp]) + if err != nil { + return false + } + + return now.Sub(since) > StaleRollbackAfter +} + // RestoreAdoptedProp marks a definition that a retry adopted as the leftover of // an earlier attempt at the same clone or restore, before it wrote anything // into it. The attempt that created the definition reads it before rolling @@ -158,6 +223,7 @@ func RecordedRestoreVolumes(props map[string]string) ([]apiv1.SnapshotVolumeDef, func TravellingProps(props map[string]string) map[string]string { out := maps.Clone(props) delete(out, RollbackAbandonedProp) + delete(out, RollbackSinceProp) delete(out, RestoreAdoptedProp) delete(out, RestoreFromSnapshotProp) delete(out, RestoreVolumesProp) @@ -183,6 +249,7 @@ func TravellingProps(props map[string]string) map[string]string { var serverOwnedDefinitionProps = []string{ //nolint:gochecknoglobals // a fixed list, read only RestoreFromSnapshotProp, RollbackAbandonedProp, + RollbackSinceProp, RestoreAdoptedProp, RestoreVolumesProp, CloneSnapshotOwnerProp, @@ -261,7 +328,7 @@ func RollbackMarkClearRefusal(before, after map[string]string) error { // same delete (`set-property KEY ""`), and is read as one. func ServerOwnedPropEditByOperator(overrideProps map[string]string, deleteProps, deleteNamespaces []string) string { clearable := func(k string) bool { - return k == RollbackAbandonedProp || k == RestoreAdoptedProp || k == RestoreFromSnapshotProp + return k == RollbackAbandonedProp || k == RollbackSinceProp || k == RestoreAdoptedProp || k == RestoreFromSnapshotProp } kept := slices.DeleteFunc(slices.Clone(deleteProps), clearable) diff --git a/pkg/store/restore_target.go b/pkg/store/restore_target.go index 374ac34a..0e42c1d3 100644 --- a/pkg/store/restore_target.go +++ b/pkg/store/restore_target.go @@ -477,7 +477,12 @@ func JudgeRestoreLeftoverOf( return progress, true, err } - err = rollbackMarkRefusal(rd.Name, rd.Props[RollbackAbandonedProp], snap) + mark, err := liveRollbackMark(ctx, st, &rd) + if err != nil { + return progress, true, err + } + + err = rollbackMarkRefusal(rd.Name, mark, snap) if err != nil { return progress, true, err } @@ -485,6 +490,27 @@ func JudgeRestoreLeftoverOf( return progress, true, nil } +// liveRollbackMark is the rollback mark a retry has to answer for: the one on +// rd, unless it is an in-progress mark nobody holds any more, which the retry +// takes over (TakeOverStaleRollback). +func liveRollbackMark(ctx context.Context, st Store, rd *apiv1.ResourceDefinition) (string, error) { + mark := rd.Props[RollbackAbandonedProp] + if mark != RollbackInProgress { + return mark, nil + } + + took, err := TakeOverStaleRollback(ctx, st, rd.Name, time.Now()) + if err != nil { + return "", err + } + + if took { + return "", nil + } + + return mark, nil +} + // rollbackMarkRefusal is the CLI door's refusal over a rollback mark, worded // for the step it names; nil when there is none. func rollbackMarkRefusal(rdName, step string, snap *apiv1.Snapshot) error { @@ -495,9 +521,9 @@ func rollbackMarkRefusal(rdName, step string, snap *apiv1.Snapshot) error { return nil case RollbackInProgress: return fmt.Errorf("%w: %q is being rolled back by the attempt that created it, "+ - "or that attempt stopped mid-way; %s once it has finished, which on the REST door takes under a minute; "+ - "if the mark is still there a minute after the failure, run "+ - "`blockstor resource-definition set-property %s %s` with no value to keep it as it stands, "+ + "or that attempt stopped mid-way; %s once it has finished, which on the REST door takes under a minute, "+ + "and a retry takes over a mark nobody has touched for two minutes; to keep it as it stands sooner, run "+ + "`blockstor resource-definition set-property %s %s` with no value, "+ "or delete it by hand. "+ "Cleared while the rollback still runs, it stops that rollback before it deletes anything", ErrRestoreRollbackAbandoned, rdName, again, rdName, RollbackAbandonedProp) @@ -866,7 +892,7 @@ func EnterDestructiveRollback(ctx context.Context, st Store, rdName, step string return errRollbackMarkNotOurs } - rd.Props[RollbackAbandonedProp] = step + rd.Props = MarkRollback(rd.Props, step, time.Now()) return nil }) @@ -951,7 +977,7 @@ func releaseInProgress(ctx context.Context, st Store, rdName string) error { err := st.ResourceDefinitions().PatchResourceDefinitionSpec(ctx, rdName, func(rd *apiv1.ResourceDefinition) error { if rd.Props[RollbackAbandonedProp] == RollbackInProgress { - delete(rd.Props, RollbackAbandonedProp) + UnmarkRollback(rd.Props) } return nil @@ -963,21 +989,55 @@ func releaseInProgress(ctx context.Context, st Store, rdName string) error { return nil } +// TakeOverStaleRollback takes an in-progress mark off a definition when it is +// older than StaleRollbackAfter, and reports whether it did. A retry calls it +// before refusing over the mark. +// +// The in-progress mark only ever stands over a definition left whole: a +// rollback swaps it for the step it is about to run before it deletes +// anything (EnterDestructiveRollback). A stale one is a rollback whose process +// stopped before either, so taking it off hands the retry an unfinished +// leftover, which it resumes. Should that rollback still be alive past its +// budget, its swap finds the mark gone and it stands down without deleting. +// +// The check runs again on the object the patch writes, so a mark a newer +// rollback wrote in between is left alone. +func TakeOverStaleRollback(ctx context.Context, st Store, rdName string, now time.Time) (bool, error) { + took := false + + err := st.ResourceDefinitions().PatchResourceDefinitionSpec(ctx, rdName, + func(rd *apiv1.ResourceDefinition) error { + // A mark another retry already took off counts as taken: there + // is no rollback left to refuse over. + took = rd.Props[RollbackAbandonedProp] == "" || rollbackMarkStale(rd.Props, now) + if took { + UnmarkRollback(rd.Props) + } + + return nil + }) + if errors.Is(err, ErrNotFound) { + return false, nil + } + + if err != nil { + return false, fmt.Errorf("take over the stale rollback mark on %q: %w", rdName, err) + } + + return took, nil +} + // setRollbackMark writes the rollback mark, or removes it when step is empty. func setRollbackMark(ctx context.Context, st Store, rdName, step string) error { return st.ResourceDefinitions().PatchResourceDefinitionSpec(ctx, rdName, //nolint:wrapcheck // callers wrap with the definition they name func(rd *apiv1.ResourceDefinition) error { if step == "" { - delete(rd.Props, RollbackAbandonedProp) + UnmarkRollback(rd.Props) return nil } - if rd.Props == nil { - rd.Props = map[string]string{} - } - - rd.Props[RollbackAbandonedProp] = step + rd.Props = MarkRollback(rd.Props, step, time.Now()) return nil }) diff --git a/pkg/store/rollback_stale_test.go b/pkg/store/rollback_stale_test.go new file mode 100644 index 00000000..99b60dd6 --- /dev/null +++ b/pkg/store/rollback_stale_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package store + +import ( + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" +) + +// The CLI door asks liveRollbackMark which mark a retry answers for: a stale +// in-progress mark is taken over and none is left, a fresh one stands. +func TestLiveRollbackMarkTakesOverOnlyAStaleMark(t *testing.T) { + t.Parallel() + + for name, age := range map[string]time.Duration{"fresh": 10 * time.Second, "stale": StaleRollbackAfter + time.Minute} { + t.Run(name, func(t *testing.T) { + t.Parallel() + + st := NewInMemory() + rd := apiv1.ResourceDefinition{ + Name: "dst-" + name, + Props: MarkRollback(map[string]string{}, RollbackInProgress, time.Now().Add(-age)), + } + + if err := st.ResourceDefinitions().Create(t.Context(), &rd); err != nil { + t.Fatalf("seed: %v", err) + } + + mark, err := liveRollbackMark(t.Context(), st, &rd) + if err != nil { + t.Fatalf("liveRollbackMark: %v", err) + } + + want := RollbackInProgress + if name == "stale" { + want = "" + } + + if mark != want { + t.Fatalf("mark = %q, want %q", mark, want) + } + }) + } +} + +// A mark another retry already took off is no rollback to refuse over: the +// second of two retries racing over a stale mark proceeds too. +func TestTakeOverStaleRollbackCountsAMarkAlreadyTakenOff(t *testing.T) { + t.Parallel() + + st := NewInMemory() + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{Name: "dst-gone-mark"}); err != nil { + t.Fatalf("seed: %v", err) + } + + took, err := TakeOverStaleRollback(t.Context(), st, "dst-gone-mark", time.Now()) + if err != nil || !took { + t.Fatalf("took=%v err=%v over a definition with no mark, want took", took, err) + } +} + +// The takeover rests on every rollback being over before a mark turns stale. +func TestCLIRollbackBudgetEndsBeforeAMarkTurnsStale(t *testing.T) { + t.Parallel() + + if CLIRollbackBudget >= StaleRollbackAfter { + t.Fatalf("CLIRollbackBudget %s is not shorter than StaleRollbackAfter %s", CLIRollbackBudget, StaleRollbackAfter) + } +} From 042feef5c69d015b466ab45ffa6aced36f8424a4 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 18:50:49 +0100 Subject: [PATCH 4/8] fix(snapshot): hold off a restore that races a snapshot delete A snapshot delete listed the definitions restored from the snapshot once, and a restore read the snapshot once, before it created its marked definition. A delete landing between the two found no dependent and took the snapshot, and the restore then answered 201 over a definition whose marker named a snapshot that was gone, so its next replica came up blank. The list also went through the informer cache, which can miss a definition created a moment earlier. Every operator-facing delete, on the REST door, in the CLI and in the auto-snapshot prune, now marks the snapshot first, lists the definitions past the cache, and only then deletes; a snapshot it keeps gets its mark back off. A restore, on both doors, reads the snapshot back past the cache once its marked definition stands, and withdraws when the snapshot is gone or carries a live delete mark, taking back a definition it created; the REST door answers 404 and a retryable 409 for the two. Whichever of the list and the create comes second sees the other. The mark expires, so one a dead process left stops refusing restores. The mark goes on and comes off as a patch of the props the API server holds, read past the cache. A clone's reaping mark written in between survives it, a snapshot the cache has not seen yet is marked rather than reported absent, and a delete takes off only its own mark, never another delete's. An in-memory patch that empties the props leaves none, as the API server does. A delete of a snapshot that does not exist is judged before any restore again: a definition whose marker named a missing snapshot, with a restore that could never finish, turned the idempotent 200 into a 409 that linstor-csi retried forever. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- internal/cli/snapshot.go | 4 +- internal/cli/snapshot_delete_race_test.go | 112 ++++++++ internal/cli/write_more.go | 10 +- .../controller/autosnapshot_controller.go | 77 +++--- .../autosnapshot_restore_source_test.go | 44 +-- pkg/rest/nodes.go | 8 +- pkg/rest/snapshot_delete_restore_race_test.go | 169 ++++++++++++ pkg/rest/snapshot_restore.go | 16 +- pkg/rest/snapshots.go | 31 +-- pkg/store/clone_snapshot.go | 50 +++- pkg/store/inmemory_snapshot.go | 6 + .../k8s/snapshot_delete_cache_miss_test.go | 87 ++++++ pkg/store/k8s/snapshots.go | 5 +- pkg/store/local_props.go | 1 + pkg/store/restore_source.go | 157 ++++++++++- pkg/store/snapshot_delete_reap_mark_test.go | 166 +++++++++++ pkg/store/snapshot_delete_test.go | 259 ++++++++++++++++++ tests/e2e/cli-matrix/README.md | 1 + tests/e2e/cli-matrix/snap-d-restore-source.sh | 116 ++++++++ .../replay/snap-d-restore-source.yaml | 83 ++++++ 20 files changed, 1287 insertions(+), 115 deletions(-) create mode 100644 internal/cli/snapshot_delete_race_test.go create mode 100644 pkg/rest/snapshot_delete_restore_race_test.go create mode 100644 pkg/store/k8s/snapshot_delete_cache_miss_test.go create mode 100644 pkg/store/snapshot_delete_reap_mark_test.go create mode 100644 pkg/store/snapshot_delete_test.go create mode 100755 tests/e2e/cli-matrix/snap-d-restore-source.sh create mode 100644 tests/operator-harness/replay/snap-d-restore-source.yaml diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 1dc0d3f9..64593c0b 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -351,7 +351,7 @@ func snapshotRestoreResource(ctx context.Context, run *runContext) error { } // The snapshot was read before the definition existed; see - // store.ReapClonedSnapshot. + // store.ReapClonedSnapshot and store.DeleteSnapshot. err = store.RestoreSourceWithdrawn(ctx, run.Store, snap.ResourceName, snap.Name) if err != nil { return rollbackRestore(ctx, run, def.Name, planned, err) @@ -508,7 +508,7 @@ func restoreIntoExistingTarget( // 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. +// store.ReapClonedSnapshot and store.DeleteSnapshot. func placeOverMarkedTarget( ctx context.Context, run *runContext, rdName string, snap *apiv1.Snapshot, planned []apiv1.Resource, ) error { diff --git a/internal/cli/snapshot_delete_race_test.go b/internal/cli/snapshot_delete_race_test.go new file mode 100644 index 00000000..1b53968b --- /dev/null +++ b/internal/cli/snapshot_delete_race_test.go @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package cli_test + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// A delete of a snapshot that is not there is the no-op it always was, even +// with a definition whose marker names it and whose restore can never finish. +func TestSnapshotDeleteOfAMissingSnapshotIgnoresAHangingMarker(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + _ = backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "pvc-hang", Props: map[string]string{store.RestoreFromSnapshotProp: "pvc-x:snap-gone"}, + }) + }) + + if got := app.Run(t.Context(), []string{"s", "d", "pvc-x", "snap-gone"}); got != 0 { + t.Fatalf("delete of a missing snapshot exit = %d (stderr: %s)", got, errBuf.String()) + } +} + +// The CLI restore reads its snapshot back once the marked definition stands, +// the way the REST door does, and withdraws from one a delete has marked: the +// delete listed dependents before the definition existed and will not see it. +func TestSnapshotRestoreWithdrawsFromASnapshotBeingDeleted(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + + snap, _ := backend.Snapshots().Get(ctx, "pvc-x", "snap-1") + snap.Props = map[string]string{store.SnapshotDeletingProp: "tok@" + time.Now().UTC().Format(time.RFC3339)} + _ = backend.Snapshots().Update(ctx, &snap) + }) + + if got := app.Run(t.Context(), restoreResourceArgv("pvc-wd")); got == 0 { + t.Fatal("restore from a snapshot being deleted succeeded") + } + + if !strings.Contains(errBuf.String(), "retry once it finishes") { + t.Errorf("refusal %q does not say to retry once the delete finishes", errBuf.String()) + } + + if _, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-wd"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the withdrawn restore left its definition behind: %v", err) + } +} + +// A prepared target the restore marked reads the snapshot back before it +// places anything. +func TestSnapshotRestoreIntoAPreparedTargetWithdrawsFromASnapshotBeingDeleted(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, seedSnapshotSource) + + for _, argv := range [][]string{ + {"rd", "c", "pvc-prep-wd"}, + {"s", "vd", "restore", "--from-resource", "pvc-x", "--from-snapshot", "snap-1", "--to-resource", "pvc-prep-wd"}, + } { + if got := app.Run(t.Context(), argv); got != 0 { + t.Fatalf("%v exit = %d (stderr: %s)", argv, got, errBuf.String()) + } + } + + st := appStore(t, app) + + snap, err := st.Snapshots().Get(t.Context(), "pvc-x", "snap-1") + if err != nil { + t.Fatalf("read the snapshot: %v", err) + } + + snap.Props = map[string]string{store.SnapshotDeletingProp: "tok@" + time.Now().UTC().Format(time.RFC3339)} + if err := st.Snapshots().Update(t.Context(), &snap); err != nil { + t.Fatalf("mark the snapshot: %v", err) + } + + if got := app.Run(t.Context(), restoreResourceArgv("pvc-prep-wd")); got == 0 { + t.Fatal("restore into a prepared target from a snapshot being deleted succeeded") + } + + replicas, err := st.Resources().ListByDefinition(t.Context(), "pvc-prep-wd") + if err != nil || len(replicas) != 0 { + t.Errorf("replicas placed after the refusal = %d (%v), want none", len(replicas), err) + } +} diff --git a/internal/cli/write_more.go b/internal/cli/write_more.go index f115fefc..34f719a3 100644 --- a/internal/cli/write_more.go +++ b/internal/cli/write_more.go @@ -607,13 +607,9 @@ func snapshotDelete(ctx context.Context, run *runContext) error { rdName, snapName := run.Flags.Positionals[0], run.Flags.Positionals[1] - // The same release the REST door runs: see store.ReleaseRestoreSource. - err := store.ReleaseRestoreSource(ctx, run.Store, rdName, snapName) - if err != nil { - return fmt.Errorf("delete snapshot %s of %s: %w", snapName, rdName, err) - } - - err = run.Store.Snapshots().Delete(ctx, rdName, snapName) + // The same delete the REST door runs: see store.DeleteSnapshot. A + // snapshot that is not there is the idempotent no-op it always was. + err := store.DeleteSnapshot(ctx, run.Store, rdName, snapName) if err != nil && !isNotFound(err) { return fmt.Errorf("delete snapshot %s of %s: %w", snapName, rdName, err) } diff --git a/internal/controller/autosnapshot_controller.go b/internal/controller/autosnapshot_controller.go index 02b9fad9..f566bdb2 100644 --- a/internal/controller/autosnapshot_controller.go +++ b/internal/controller/autosnapshot_controller.go @@ -151,9 +151,9 @@ type AutoSnapshotRunnable struct { // Period falls back to the default. Period time.Duration - // Store judges whether a restore from an auto-snapshot is finished - // before the prune deletes it (store.ReleaseRestoreSource). Nil keeps - // every auto-snapshot a definition was restored from. + // Store deletes an auto-snapshot through store.DeleteSnapshot, which + // judges whether a restore from it is finished. Nil keeps every + // auto-snapshot a definition was restored from. Store store.Store } @@ -461,54 +461,61 @@ func (r *AutoSnapshotRunnable) pruneOldAutoSnapshots( continue } - inUse, err := r.restoreReadsFrom(ctx, rd.Name, snap.Spec.SnapshotName) + err := r.pruneAutoSnapshot(ctx, rd.Name, snap) if err != nil { return err } - - if inUse { - // Kept for this pass: a definition restored from it still needs - // it. It is pruned once that restore is finished. - continue - } - - err = r.Client.Delete(ctx, snap) - if err != nil && !apierrors.IsNotFound(err) { - return errors.Wrapf(err, "delete auto-snapshot %q", snap.Name) - } } return nil } -// restoreReadsFrom reports whether a definition restored from rdName:snapName -// still needs the snapshot. A finished restore has its marker taken off and -// lets the snapshot go; see store.ReleaseRestoreSource. -func (r *AutoSnapshotRunnable) restoreReadsFrom(ctx context.Context, rdName, snapName string) (bool, error) { - if r.Store == nil { - var rds blockstoriov1alpha1.ResourceDefinitionList - - err := r.Client.List(ctx, &rds) - if err != nil { - return false, errors.Wrap(err, "list definitions restored from an auto-snapshot") +// pruneAutoSnapshot deletes one auto-snapshot past the keep budget, unless a +// definition restored from it still needs it; it is then kept for this pass +// and pruned once that restore is finished. With a store the delete is +// store.DeleteSnapshot, the one every snapshot delete door runs, which takes +// a finished restore's marker off and holds off a restore racing the delete. +func (r *AutoSnapshotRunnable) pruneAutoSnapshot( + ctx context.Context, rdName string, snap *blockstoriov1alpha1.Snapshot, +) error { + if r.Store != nil { + err := store.DeleteSnapshot(ctx, r.Store, rdName, snap.Spec.SnapshotName) + if errors.Is(err, store.ErrRestoreSourceInUse) || errors.Is(err, store.ErrNotFound) { + return nil } - for i := range rds.Items { - if strings.EqualFold(rds.Items[i].Spec.Props[store.RestoreFromSnapshotProp], rdName+":"+snapName) { - return true, nil - } - } + return errors.Wrapf(err, "delete auto-snapshot %q", snap.Name) + } - return false, nil + inUse, err := r.restoreReadsFrom(ctx, rdName, snap.Spec.SnapshotName) + if err != nil || inUse { + return err } - err := store.ReleaseRestoreSource(ctx, r.Store, rdName, snapName) - if errors.Is(err, store.ErrRestoreSourceInUse) { - return true, nil + err = r.Client.Delete(ctx, snap) + if err != nil && !apierrors.IsNotFound(err) { + return errors.Wrapf(err, "delete auto-snapshot %q", snap.Name) } + return nil +} + +// restoreReadsFrom reports, for a runnable with no store, whether any +// definition was restored from rdName:snapName. Without a store a finished +// restore cannot be told from an unfinished one, so every such snapshot is +// kept. +func (r *AutoSnapshotRunnable) restoreReadsFrom(ctx context.Context, rdName, snapName string) (bool, error) { + var rds blockstoriov1alpha1.ResourceDefinitionList + + err := r.Client.List(ctx, &rds) if err != nil { - return false, errors.Wrapf(err, "release auto-snapshot %s of %s", snapName, rdName) + return false, errors.Wrap(err, "list definitions restored from an auto-snapshot") + } + + for i := range rds.Items { + if strings.EqualFold(rds.Items[i].Spec.Props[store.RestoreFromSnapshotProp], rdName+":"+snapName) { + return true, nil + } } return false, nil diff --git a/internal/controller/autosnapshot_restore_source_test.go b/internal/controller/autosnapshot_restore_source_test.go index e7a84ea5..56ebaac8 100644 --- a/internal/controller/autosnapshot_restore_source_test.go +++ b/internal/controller/autosnapshot_restore_source_test.go @@ -28,8 +28,8 @@ import ( blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" controllerpkg "github.com/cozystack/blockstor/internal/controller" - apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/store" + storek8s "github.com/cozystack/blockstor/pkg/store/k8s" ) // The prune deleted by age alone. A definition restored from an @@ -75,28 +75,28 @@ func TestAutoSnapshotPruneKeepsTheSourceOfARestore(t *testing.T) { } } -// The production prune judges the restore through the store -// (store.ReleaseRestoreSource): a restore that is not finished keeps its -// auto-snapshot. +// The production prune deletes through the store (store.DeleteSnapshot), +// over the same API server the cron writes to: a restore that is not finished +// keeps its auto-snapshot, and the ones nothing was restored from go. func TestAutoSnapshotPruneWithAStoreKeepsTheSourceOfAnUnfinishedRestore(t *testing.T) { t.Parallel() scheme := newScheme(t) rd := makeRDWithAutoSnapshot(t, "pvc-keep-st", 15, "2") - cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(rd).Build() - - st := store.NewInMemory() - if err := st.ResourceDefinitions().Create(context.Background(), &apiv1.ResourceDefinition{ - Name: "pvc-restored-st", - Props: map[string]string{store.RestoreFromSnapshotProp: "pvc-keep-st:auto-snap-00001"}, - }); err != nil { - t.Fatalf("seed restored definition: %v", err) + + restored := &blockstoriov1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-restored-st"}, + Spec: blockstoriov1alpha1.ResourceDefinitionSpec{ + Props: map[string]string{store.RestoreFromSnapshotProp: "pvc-keep-st:auto-snap-00001"}, + }, } + cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(rd, restored).Build() + clk := &stubClock{t: time.Date(2026, 5, 14, 12, 0, 0, 0, time.UTC)} - r := &controllerpkg.AutoSnapshotRunnable{Client: cli, Clock: clk, Store: st} + r := &controllerpkg.AutoSnapshotRunnable{Client: cli, Clock: clk, Store: storek8s.New(cli)} - for i := range 4 { + for i := range 5 { if err := r.Tick(context.Background()); err != nil { t.Fatalf("Tick #%d: %v", i, err) } @@ -104,11 +104,19 @@ func TestAutoSnapshotPruneWithAStoreKeepsTheSourceOfAnUnfinishedRestore(t *testi clk.advance(15*time.Minute + time.Second) } + names := map[string]bool{} for _, snap := range listAutoSnapshotsByRD(t, cli, "pvc-keep-st") { - if snap.Spec.SnapshotName == "auto-snap-00001" { - return - } + names[snap.Spec.SnapshotName] = true } - t.Fatal("the prune deleted the auto-snapshot an unfinished restore reads from") + if !names["auto-snap-00001"] { + t.Fatal("the prune deleted the auto-snapshot an unfinished restore reads from") + } + + // Without the second check a prune that deleted nothing at all would + // pass the first: the store path has to delete what nothing restores + // from. + if names["auto-snap-00002"] { + t.Fatalf("the prune kept auto-snap-00002, which nothing was restored from: %v", names) + } } diff --git a/pkg/rest/nodes.go b/pkg/rest/nodes.go index afe2728c..6212b114 100644 --- a/pkg/rest/nodes.go +++ b/pkg/rest/nodes.go @@ -1515,9 +1515,11 @@ func writeStoreErrorTyped(w http.ResponseWriter, err error, kind storeResourceKi // answers in CloneStarted, which python-linstor decodes whatever the status. func storeErrorRc(err error, kind storeResourceKind) (int, apiv1.APICallRc) { switch { - case errors.Is(err, store.ErrRestoreSourceReaping): - // Retryable, and no FAIL_EXISTS band: linstor-csi reads that as a - // volume already there. + case errors.Is(err, store.ErrRestoreSourceReaping), errors.Is(err, store.ErrRestoreSourceDeleting): + // A reap or a delete is judging the snapshot right now and may keep + // it: the restore retried once it is over can go through. Retryable, + // and no FAIL_EXISTS band: linstor-csi reads that as a volume already + // there. return http.StatusConflict, apiv1.APICallRc{ RetCode: apiCallRcError, Message: scrubImplDetails(err.Error()), diff --git a/pkg/rest/snapshot_delete_restore_race_test.go b/pkg/rest/snapshot_delete_restore_race_test.go new file mode 100644 index 00000000..4ff4443b --- /dev/null +++ b/pkg/rest/snapshot_delete_restore_race_test.go @@ -0,0 +1,169 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "context" + "errors" + "net/http" + "sync" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// deleteOnMarkedCreate deletes the snapshot through the running server just +// before the first definition carrying a restore marker is created: a delete +// that lands between the restore's read of its snapshot and its create. +type deleteOnMarkedCreate struct { + store.Store + + t *testing.T + base *string + snapPath string + once *sync.Once + status *int +} + +type deleteOnMarkedCreateRDs struct { + store.ResourceDefinitionStore + + outer deleteOnMarkedCreate +} + +func (d deleteOnMarkedCreate) ResourceDefinitions() store.ResourceDefinitionStore { + return deleteOnMarkedCreateRDs{ResourceDefinitionStore: d.Store.ResourceDefinitions(), outer: d} +} + +func (r deleteOnMarkedCreateRDs) Create(ctx context.Context, rd *apiv1.ResourceDefinition) error { + if rd.Props[store.RestoreFromSnapshotProp] != "" { + r.outer.once.Do(func() { //nolint:contextcheck // the helper sends on t's own context + resp := httpDelete(r.outer.t, *r.outer.base+r.outer.snapPath) + _ = resp.Body.Close() + *r.outer.status = resp.StatusCode + }) + } + + return r.ResourceDefinitionStore.Create(ctx, rd) //nolint:wrapcheck // pass-through test double +} + +// A snapshot delete that lands between a restore's read of the snapshot and +// its create of the marked definition found no dependent, deleted the +// snapshot, and the restore then answered 201 over a definition whose marker +// named a snapshot that no longer existed: every later replica came up blank. +// The restore now reads the snapshot back once its definition stands, and +// takes the definition back when the snapshot is gone. +func TestSnapshotDeleteRacingARestoreLeavesNoMarkerOnAGoneSnapshot(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedTwoNodeCloneSource(t, backend, "src-race") + seedRestoreSnapshot(t, backend, "src-race", "snap-race", []string{"node-a", "node-b"}) + + var ( + base string + deleteStatus int + ) + + st := deleteOnMarkedCreate{ + Store: backend, t: t, base: &base, snapPath: "/v1/resource-definitions/src-race/snapshots/snap-race", + once: &sync.Once{}, status: &deleteStatus, + } + + base, stop := startServerWithStore(t, st) + defer stop() + + code := restoreOnce(t, base, "src-race", "snap-race", map[string]any{"to_resource": "dst-race"}) + + if deleteStatus != http.StatusOK { + t.Fatalf("the racing delete = %d, want 200", deleteStatus) + } + + if code != http.StatusNotFound { + t.Errorf("restore whose snapshot was deleted under it = %d, want 404", code) + } + + if _, err := backend.Snapshots().Get(t.Context(), "src-race", "snap-race"); !errors.Is(err, store.ErrNotFound) { + t.Fatalf("fixture: the snapshot is still there: %v", err) + } + + rd, err := backend.ResourceDefinitions().Get(t.Context(), "dst-race") + if err == nil && rd.Props[store.RestoreFromSnapshotProp] != "" { + t.Fatalf("dst-race names the deleted snapshot: %q", rd.Props[store.RestoreFromSnapshotProp]) + } +} + +// A snapshot that is not there is the idempotent 200 it always was, even +// with a definition whose marker still names it and whose restore can never +// finish: answered 409, linstor-csi retried DeleteSnapshot forever. +func TestSnapshotDeleteOfAMissingSnapshotIgnoresAHangingMarker(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedTwoNodeCloneSource(t, st, "src-hang") + + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: "dst-hang", Props: map[string]string{store.RestoreFromSnapshotProp: "src-hang:snap-gone"}, + }); err != nil { + t.Fatalf("seed the definition: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := httpDelete(t, base+"/v1/resource-definitions/src-hang/snapshots/snap-gone") + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + t.Fatalf("delete of a missing snapshot under a hanging marker = %d, want 200", resp.StatusCode) + } +} + +// A restore whose snapshot carries a live delete mark withdraws with a +// retryable 409: the delete may still keep the snapshot. +func TestSnapshotRestoreFromASnapshotBeingDeletedIsRetryable(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedTwoNodeCloneSource(t, st, "src-mark") + seedRestoreSnapshot(t, st, "src-mark", "snap-mark", []string{"node-a", "node-b"}) + + snap, err := st.Snapshots().Get(t.Context(), "src-mark", "snap-mark") + if err != nil { + t.Fatalf("read the snapshot: %v", err) + } + + snap.Props = map[string]string{store.SnapshotDeletingProp: "tok@" + time.Now().UTC().Format(time.RFC3339)} + if err := st.Snapshots().Update(t.Context(), &snap); err != nil { + t.Fatalf("mark the snapshot: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + if code := restoreOnce(t, base, "src-mark", "snap-mark", map[string]any{"to_resource": "dst-mark"}); code != http.StatusConflict { + t.Errorf("restore from a snapshot being deleted = %d, want 409", code) + } + + if _, err := st.ResourceDefinitions().Get(t.Context(), "dst-mark"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the withdrawn restore left its definition behind: %v", err) + } +} diff --git a/pkg/rest/snapshot_restore.go b/pkg/rest/snapshot_restore.go index 3555fa5c..63436d44 100644 --- a/pkg/rest/snapshot_restore.go +++ b/pkg/rest/snapshot_restore.go @@ -1411,9 +1411,10 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s made = createdRD(newRD.Name, newRD.ResourceGroupName) } - // The snapshot was read before the definition existed, and a reap that - // listed dependents in between could not see this one. Read it back now - // the definition is there; see store.ReapClonedSnapshot. The withdraw has + // The snapshot was read before the marked definition existed, and a reap + // or a snapshot delete that listed dependents in between could not see + // this one. Read it back now the definition is there; see + // store.ReapClonedSnapshot and store.DeleteSnapshot. The withdraw has // already taken back what this request created, so its error is not one // to roll back over. err = store.RestoreSourceWithdrawn(ctx, s.Store, snap.ResourceName, snap.Name) @@ -1552,9 +1553,9 @@ func (s *Server) claimAdoptedLeftover(ctx context.Context, rdName string, snap * // withdrawRestoredRD takes back a definition this request created from a // snapshot that went away under it, before anything was hydrated into it, and -// answers as for a snapshot that does not exist, or, when only a reap's mark -// is on the snapshot and no delete was issued, with a retryable 409: the reap -// may still keep the snapshot. +// answers as for a snapshot that does not exist, or, when only a reap's or a +// delete's mark is on the snapshot, with a retryable 409: the reap or the +// delete may still keep the snapshot. // // The take-back goes through the rollback and its handshake, not a bare // delete: between this request's create and its read of the snapshot, a @@ -1581,7 +1582,8 @@ func (s *Server) withdrawRestoredRD( } } - if !errors.Is(cause, store.ErrRestoreSourceWithdrawn) || errors.Is(cause, store.ErrRestoreSourceReaping) { + if !errors.Is(cause, store.ErrRestoreSourceWithdrawn) || + errors.Is(cause, store.ErrRestoreSourceReaping) || errors.Is(cause, store.ErrRestoreSourceDeleting) { return cause } diff --git a/pkg/rest/snapshots.go b/pkg/rest/snapshots.go index 892fa806..55672bf6 100644 --- a/pkg/rest/snapshots.go +++ b/pkg/rest/snapshots.go @@ -1201,30 +1201,23 @@ func listDiskfulNodes(ctx context.Context, s *Server, rd string) ([]string, erro return out, nil } -// refuseSnapshotDeleteUnderRestore runs store.ReleaseRestoreSource before a -// snapshot delete: a definition restored from the snapshot reads from it -// until its restore is done. True means the answer has been written. -func (s *Server) refuseSnapshotDeleteUnderRestore(w http.ResponseWriter, r *http.Request, rd, snapName string) bool { - err := store.ReleaseRestoreSource(r.Context(), s.Store, rd, snapName) +// writeSnapshotDeleteRefusal answers a snapshot delete that store.DeleteSnapshot +// refused: a definition restored from the snapshot reads from it until its +// restore is done. +func writeSnapshotDeleteRefusal(w http.ResponseWriter, rd, snapName string, err error) { if errors.Is(err, store.ErrRestoreSourceInUse) { writeJSON(w, http.StatusConflict, []apiv1.APICallRc{{ RetCode: apiCallRcError, - Message: "snapshot '" + snapName + "' of '" + rd + "' cannot be deleted yet: " + err.Error(), + Message: "snapshot '" + snapName + "' of '" + rd + "' cannot be deleted yet: " + scrubImplDetails(err.Error()), Cause: "a restore from this snapshot has not brought up a replica with every volume UpToDate", Correc: "retry the delete once that restore has finished", ObjRefs: map[string]string{objRefRscDfn: rd, objRefSnapshotDfn: snapName}, }}) - return true - } - - if err != nil { - writeStoreError(w, err) - - return true + return } - return false + writeStoreError(w, err) } // handleSnapshotDelete answers `DELETE /v1/resource-definitions/{rd}/snapshots/{snap}` @@ -1263,11 +1256,9 @@ func (s *Server) handleSnapshotDelete(w http.ResponseWriter, r *http.Request) { rd := r.PathValue("rd") snapName := r.PathValue("snap") - if s.refuseSnapshotDeleteUnderRestore(w, r, rd, snapName) { - return - } - - err := s.Store.Snapshots().Delete(r.Context(), rd, snapName) + // A missing snapshot is answered before any restore is judged: there is + // nothing to hold, and the idempotent 200 is what a CSI retry needs. + err := store.DeleteSnapshot(r.Context(), s.Store, rd, snapName) if err != nil { if errors.Is(err, store.ErrNotFound) { writeJSON(w, http.StatusOK, []apiv1.APICallRc{{ @@ -1282,7 +1273,7 @@ func (s *Server) handleSnapshotDelete(w http.ResponseWriter, r *http.Request) { return } - writeStoreError(w, err) + writeSnapshotDeleteRefusal(w, rd, snapName, err) return } diff --git a/pkg/store/clone_snapshot.go b/pkg/store/clone_snapshot.go index c46e488d..aaa3db3c 100644 --- a/pkg/store/clone_snapshot.go +++ b/pkg/store/clone_snapshot.go @@ -120,8 +120,9 @@ const cloneSnapshotReapingMarkTTL = 5 * time.Minute // was restored from, which is therefore kept. var ErrCloneSnapshotInUse = errors.New("internal clone snapshot is still a restore source") -// ErrRestoreSourceWithdrawn reports a snapshot that was deleted, or is being -// reaped, while a definition was being restored from it. +// ErrRestoreSourceWithdrawn reports a snapshot that was deleted, or that a +// reap or a delete has started on, while a definition was being restored +// from it. var ErrRestoreSourceWithdrawn = errors.New("snapshot was withdrawn while a definition was restored from it") // ErrRestoreSourceReaping is the withdrawal a live reaping mark alone causes: @@ -263,20 +264,30 @@ func dropReapingMark(ctx context.Context, st Store, source, snapshot, mark strin return err //nolint:wrapcheck // callers wrap with the snapshot they name } -// uncachedSnapshot reads one snapshot from the API server. -func uncachedSnapshot(ctx context.Context, st Store, source, snapshot string) (apiv1.Snapshot, error) { - snaps, err := st.Snapshots().ListByDefinitionUncached(ctx, source) +// uncachedSnapshot reads one snapshot from the API server, both names +// compared the way LINSTOR compares names. The scoped read compares the +// definition name verbatim, so it asks under the spelling the definition is +// stored with; a snapshot whose definition is gone is asked for as named. +func uncachedSnapshot(ctx context.Context, st Store, srcRD, snapName string) (apiv1.Snapshot, error) { + rd, err := st.ResourceDefinitions().GetUncached(ctx, srcRD) + if err == nil { + srcRD = rd.Name + } else if !errors.Is(err, ErrNotFound) { + return apiv1.Snapshot{}, fmt.Errorf("read %q to find snapshot %s on it: %w", srcRD, snapName, err) + } + + snaps, err := st.Snapshots().ListByDefinitionUncached(ctx, srcRD) if err != nil { - return apiv1.Snapshot{}, fmt.Errorf("read snapshot %s/%s: %w", source, snapshot, err) + return apiv1.Snapshot{}, fmt.Errorf("read snapshot %s of %s: %w", snapName, srcRD, err) } for i := range snaps { - if strings.EqualFold(snaps[i].Name, snapshot) { + if strings.EqualFold(snaps[i].Name, snapName) { return snaps[i], nil } } - return apiv1.Snapshot{}, fmt.Errorf("snapshot %s/%s: %w", source, snapshot, ErrNotFound) + return apiv1.Snapshot{}, fmt.Errorf("snapshot %s of %s: %w", snapName, srcRD, ErrNotFound) } // reapingMarkIsLive reports whether a reaping mark belongs to a reap that can @@ -295,15 +306,18 @@ func reapingMarkIsLive(mark string, now time.Time) bool { return now.Sub(written) < cloneSnapshotReapingMarkTTL } -// RestoreSourceWithdrawn is the restore's half of the protocol described on -// ReapClonedSnapshot: called after the restored definition was created, it -// reads the snapshot back past the cache and reports ErrRestoreSourceWithdrawn -// when it is gone or a reap that is still live has started on it. Any other -// error means the answer could not be had, and the caller withdraws as well. +// RestoreSourceWithdrawn is the restore's half of the protocols described on +// ReapClonedSnapshot and DeleteSnapshot: called once the restored definition +// carries its marker, it reads the snapshot back past the cache and reports +// ErrRestoreSourceWithdrawn when it is gone, wrapping ErrNotFound, when a +// delete was issued on it, or when a reap or a delete that is still live has +// started on it, wrapping ErrRestoreSourceReaping or ErrRestoreSourceDeleting. +// Any other error means the answer could not be had, and the caller +// withdraws as well. func RestoreSourceWithdrawn(ctx context.Context, st Store, source, snapshot string) error { snap, err := uncachedSnapshot(ctx, st, source, snapshot) if errors.Is(err, ErrNotFound) { - return fmt.Errorf("%w: %s/%s was deleted", ErrRestoreSourceWithdrawn, source, snapshot) + return fmt.Errorf("%w: %s/%s was deleted: %w", ErrRestoreSourceWithdrawn, source, snapshot, err) } if err != nil { @@ -314,10 +328,16 @@ func RestoreSourceWithdrawn(ctx context.Context, st Store, source, snapshot stri return fmt.Errorf("%w: %s/%s is being deleted", ErrRestoreSourceWithdrawn, source, snapshot) } - if reapingMarkIsLive(snap.Props[CloneSnapshotReapingProp], time.Now()) { + now := time.Now() + + if reapingMarkIsLive(snap.Props[CloneSnapshotReapingProp], now) { return fmt.Errorf("%w: %s/%s: %w", ErrRestoreSourceWithdrawn, source, snapshot, ErrRestoreSourceReaping) } + if deletingMarkIsLive(snap.Props[SnapshotDeletingProp], now) { + return fmt.Errorf("%w: %s/%s: %w", ErrRestoreSourceWithdrawn, source, snapshot, ErrRestoreSourceDeleting) + } + return nil } diff --git a/pkg/store/inmemory_snapshot.go b/pkg/store/inmemory_snapshot.go index eeaedcc9..cc627bc8 100644 --- a/pkg/store/inmemory_snapshot.go +++ b/pkg/store/inmemory_snapshot.go @@ -161,6 +161,12 @@ func (s *inMemorySnapshots) PatchSnapshotProps( return err } + // Empty props read back as none, the way the API server drops an empty + // map. + if len(props) == 0 { + props = nil + } + snap.Props = props s.m[key] = snap diff --git a/pkg/store/k8s/snapshot_delete_cache_miss_test.go b/pkg/store/k8s/snapshot_delete_cache_miss_test.go new file mode 100644 index 00000000..6cb282a0 --- /dev/null +++ b/pkg/store/k8s/snapshot_delete_cache_miss_test.go @@ -0,0 +1,87 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package k8s_test + +import ( + "context" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + "github.com/cozystack/blockstor/pkg/store" + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// A snapshot the replica's cache has not seen yet is still deleted: the +// delete finds it on the API server, and the patch that writes its mark +// reads past the cache too, so a cache miss is never reported as "already +// absent" with the snapshot left behind. +func TestDeletingASnapshotTheCacheHasNotSeenDeletesIt(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := crdv1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("AddToScheme: %v", err) + } + + apiServer := fake.NewClientBuilder().WithScheme(scheme).WithIndex(&crdv1alpha1.Snapshot{}, k8s.FieldSnapshotDefinitionName, + func(o client.Object) []string { + return []string{o.(*crdv1alpha1.Snapshot).Spec.ResourceDefinitionName} + }).Build() + + if err := apiServer.Create(context.Background(), &crdv1alpha1.Snapshot{ + ObjectMeta: metav1.ObjectMeta{Name: "src.snap1"}, + Spec: crdv1alpha1.SnapshotSpec{ResourceDefinitionName: "src", SnapshotName: "snap1"}, + }); err != nil { + t.Fatalf("seed: %v", err) + } + + // The cache: it has not seen the snapshot, so a Get answers NotFound. + cached := interceptor.NewClient(apiServer, interceptor.Funcs{ + Get: func(ctx context.Context, c client.WithWatch, key client.ObjectKey, obj client.Object, opts ...client.GetOption) error { + if _, ok := obj.(*crdv1alpha1.Snapshot); ok { + return apierrors.NewNotFound(schema.GroupResource{Resource: "snapshots"}, key.Name) + } + + return c.Get(ctx, key, obj, opts...) + }, + }) + + st := k8s.NewWithAPIReader(cached, apiServer) + + if err := store.DeleteSnapshot(context.Background(), st, "src", "snap1"); err != nil { + t.Fatalf("DeleteSnapshot: %v", err) + } + + var left crdv1alpha1.SnapshotList + if err := apiServer.List(context.Background(), &left); err != nil { + t.Fatalf("list: %v", err) + } + + if n := len(left.Items); n != 0 { + t.Errorf("%d snapshot(s) remain after the delete", n) + } +} diff --git a/pkg/store/k8s/snapshots.go b/pkg/store/k8s/snapshots.go index c4936c70..60bf39a8 100644 --- a/pkg/store/k8s/snapshots.go +++ b/pkg/store/k8s/snapshots.go @@ -50,8 +50,9 @@ const LabelSnapshotGroupID = "blockstor.io/snapshot-group-id" type snapshots struct { c ctrlclient.Client - // apiReader is the manager's direct reader when the store has one. Only - // ListByDefinitionUncached reads through it. + // apiReader is the manager's direct reader when the store has one. + // ListByDefinitionUncached and PatchSnapshotProps read through it, so a + // prop write never starts from a copy the cache has not caught up with. apiReader ctrlclient.Reader } diff --git a/pkg/store/local_props.go b/pkg/store/local_props.go index a37f66f0..7a9b83d2 100644 --- a/pkg/store/local_props.go +++ b/pkg/store/local_props.go @@ -232,6 +232,7 @@ func TravellingProps(props map[string]string) map[string]string { // snapshot claim the clone path took it. delete(out, CloneSnapshotOwnerProp) delete(out, CloneSnapshotReapingProp) + delete(out, SnapshotDeletingProp) // A passphrase property left on a group or a definition from before the // Secret was the only source is never read, and copying it would put the diff --git a/pkg/store/restore_source.go b/pkg/store/restore_source.go index c5d085d3..af3a947a 100644 --- a/pkg/store/restore_source.go +++ b/pkg/store/restore_source.go @@ -20,10 +20,12 @@ package store import ( "context" + "crypto/rand" "errors" "fmt" "slices" "strings" + "time" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" ) @@ -32,7 +34,33 @@ import ( // not finished yet still reads from. var ErrRestoreSourceInUse = errors.New("a restore from this snapshot is not finished") -// ReleaseRestoreSource gets snapshot srcRD:snapName ready to be deleted. +// ErrRestoreSourceDeleting is the withdrawal a live deleting mark alone +// causes: the delete may still keep the snapshot, for a restore it found +// unfinished, so the same restore retried once the delete is over can go +// through. +var ErrRestoreSourceDeleting = errors.New("a delete of the snapshot is under way; retry once it finishes") + +// SnapshotDeletingProp marks a snapshot a delete has started on, as +// `@`. The token is random per delete, so a delete +// only ever takes its own mark back off. See DeleteSnapshot. +const SnapshotDeletingProp = "Blockstor/SnapshotDeleting" + +// snapshotDeleteBudget bounds a delete from its mark to its last write: no +// call it makes can land after it. +const snapshotDeleteBudget = 30 * time.Second + +// snapshotDeletingMarkTTL is how long a deleting mark refuses restores. A +// mark older than this belongs to a delete that can no longer issue one, +// since snapshotDeleteBudget is far shorter, so a mark a dead process left +// on a snapshot it never deleted stops refusing restores from it. The gap +// between the two also absorbs clock skew between the replicas that write +// and read the mark. +const snapshotDeletingMarkTTL = 5 * time.Minute + +// DeleteSnapshot deletes snapshot srcRD:snapName for an operator-facing door, +// the REST delete, the CLI and the auto-snapshot prune, once no restore needs +// it. A snapshot that does not exist answers ErrNotFound, before any +// definition is looked at: there is nothing to hold. // // A restored or cloned definition keeps its restore marker after the restore // is done, and while the marker is there every new replica is brought up from @@ -41,17 +69,70 @@ var ErrRestoreSourceInUse = errors.New("a restore from this snapshot is not fini // `rd ap`, a toggle back to diskful), which then came up blank and never // reached UpToDate. // -// So a delete first ends the dependency: every definition whose marker names -// the snapshot and whose restore is finished (RestoreMarkerClearRefusal, the -// rule an operator's own delete of the marker passes) has the marker taken +// So the delete first ends the dependency: every definition whose marker +// names the snapshot and whose restore is finished (RestoreMarkerClearRefusal, +// the rule an operator's own delete of the marker passes) has the marker taken // off, and its later replicas sync from their peers. A definition whose // restore is still running keeps it, and the delete is refused with // ErrRestoreSourceInUse; that is retryable, and lasts as long as the restore. // A definition being deleted is not waited for. -func ReleaseRestoreSource(ctx context.Context, st Store, srcRD, snapName string) error { +// +// A restore makes its dependent in two steps, the snapshot read and the +// definition create, so no list taken here alone can see one that is between +// them. The delete therefore marks the snapshot first, then lists the +// definitions past the cache, and only then deletes; a restore creates its +// definition first, then reads the snapshot back past the cache and withdraws +// on a live mark or on a snapshot that is gone (RestoreSourceWithdrawn). +// Whichever of the list and the create comes second sees the other. The mark +// stays on a snapshot whose delete was issued, and is taken back off one the +// delete keeps. +// +// The delete runs on a context the caller cannot cancel, bounded by +// snapshotDeleteBudget, so a caller that goes away between the mark and its +// clean-up does not leave the mark behind; one a dead process leaves expires +// (snapshotDeletingMarkTTL). +func DeleteSnapshot(ctx context.Context, st Store, srcRD, snapName string) error { + ctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), snapshotDeleteBudget) + defer cancel() + + snap, err := uncachedSnapshot(ctx, st, srcRD, snapName) + if err != nil { + return err + } + + mark := rand.Text() + "@" + time.Now().UTC().Format(time.RFC3339) + + err = setDeletingMark(ctx, st, &snap, mark) + if err != nil { + return fmt.Errorf("mark snapshot %s of %s as being deleted: %w", snapName, srcRD, err) + } + + // The stored names from here on: LINSTOR folds name case, and a marker + // is written from the stored snapshot. + srcRD, snapName = snap.ResourceName, snap.Name + + err = releaseRestoreSource(ctx, st, srcRD, snapName) + if err != nil { + return keepSnapshot(ctx, st, srcRD, snapName, mark, err) + } + + err = st.Snapshots().Delete(ctx, srcRD, snapName) + if err != nil && !errors.Is(err, ErrNotFound) { + return keepSnapshot(ctx, st, srcRD, snapName, mark, + fmt.Errorf("delete snapshot %s of %s: %w", snapName, srcRD, err)) + } + + return nil +} + +// releaseRestoreSource ends the dependency of every definition restored from +// srcRD:snapName, or refuses with ErrRestoreSourceInUse; see DeleteSnapshot. +// The definitions are listed past the cache: one created a moment ago is +// exactly the one a cache misses. +func releaseRestoreSource(ctx context.Context, st Store, srcRD, snapName string) error { marker := srcRD + ":" + snapName - rds, err := st.ResourceDefinitions().List(ctx) + rds, err := st.ResourceDefinitions().ListUncached(ctx) if err != nil { return fmt.Errorf("list the definitions restored from %s: %w", marker, err) } @@ -74,6 +155,70 @@ func ReleaseRestoreSource(ctx context.Context, st Store, srcRD, snapName string) return nil } +// keepSnapshot takes this delete's mark back off a snapshot it decided to +// keep, so restores from it are not refused, and returns why it was kept. +func keepSnapshot(ctx context.Context, st Store, srcRD, snapName, mark string, why error) error { + err := dropDeletingMark(ctx, st, srcRD, snapName, mark) + if err != nil && !errors.Is(err, ErrNotFound) { + return fmt.Errorf("%w; the deleting mark stayed on it, so restores from it are refused "+ + "for up to %s: %w", why, snapshotDeletingMarkTTL, err) + } + + return why +} + +// setDeletingMark writes mark as the deleting mark on snap. The write is a +// patch of the props the API server holds, so a mark another writer put on +// the snapshot after the caller read it, a reap's say, is kept. +func setDeletingMark(ctx context.Context, st Store, snap *apiv1.Snapshot, mark string) error { + //nolint:wrapcheck // the caller wraps with the snapshot it names + return st.Snapshots().PatchSnapshotProps(ctx, snap.ResourceName, snap.Name, func(props map[string]string) error { + props[SnapshotDeletingProp] = mark + + return nil + }) +} + +// errDeletingMarkNotMine stops a removal that finds a mark other than its own. +var errDeletingMarkNotMine = errors.New("the deleting mark is not this delete's") + +// dropDeletingMark removes the deleting mark when it is still mark, deciding +// and writing in one patch. A snapshot that had no props before the mark +// gets none back: a restore falls back to the source's props only for a +// snapshot without any. +func dropDeletingMark(ctx context.Context, st Store, srcRD, snapName, mark string) error { + err := st.Snapshots().PatchSnapshotProps(ctx, srcRD, snapName, func(props map[string]string) error { + if props[SnapshotDeletingProp] != mark { + return errDeletingMarkNotMine + } + + delete(props, SnapshotDeletingProp) + + return nil + }) + if errors.Is(err, errDeletingMarkNotMine) { + return nil + } + + return err //nolint:wrapcheck // the caller wraps with the snapshot it names +} + +// deletingMarkIsLive reports whether a deleting mark belongs to a delete that +// can still issue one. A mark whose time cannot be read is not. +func deletingMarkIsLive(mark string, now time.Time) bool { + at := strings.LastIndex(mark, "@") + if at < 0 { + return false + } + + written, err := time.Parse(time.RFC3339, mark[at+1:]) + if err != nil { + return false + } + + return now.Sub(written) < snapshotDeletingMarkTTL +} + func releaseRestoreMarker(ctx context.Context, st Store, rdName, marker string) error { refusal := RestoreMarkerClearRefusal(ctx, st, rdName) if errors.Is(refusal, ErrRestoreUnfinished) { diff --git a/pkg/store/snapshot_delete_reap_mark_test.go b/pkg/store/snapshot_delete_reap_mark_test.go new file mode 100644 index 00000000..c256cf7b --- /dev/null +++ b/pkg/store/snapshot_delete_reap_mark_test.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package store + +import ( + "context" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" +) + +// racingSnaps runs inject once, right before the delete's write numbered at +// (from 1): the delete writes its mark, then takes it back off. A writer +// that comes in there comes after whatever read the delete decided that +// write on. +type racingSnaps struct { + SnapshotStore + + at int + writes int + inject func() +} + +func (s *racingSnaps) Update(ctx context.Context, snap *apiv1.Snapshot) error { + s.beforeWrite() + + return s.SnapshotStore.Update(ctx, snap) //nolint:wrapcheck // a pass-through +} + +func (s *racingSnaps) PatchSnapshotProps( + ctx context.Context, rd, snap string, mutate func(map[string]string) error, +) error { + s.beforeWrite() + + return s.SnapshotStore.PatchSnapshotProps(ctx, rd, snap, mutate) //nolint:wrapcheck // a pass-through +} + +func (s *racingSnaps) beforeWrite() { + s.writes++ + if s.writes == s.at && s.inject != nil { + s.inject() + } +} + +type racingStore struct { + Store + + snaps *racingSnaps +} + +func (s *racingStore) Snapshots() SnapshotStore { return s.snaps } + +// A delete marks the snapshot as being deleted, then keeps it under an +// unfinished restore and takes its mark back off. A reap's mark written +// right before either write must survive it: it is what makes a restore from the snapshot stand +// down while the reap, which has already listed dependents, deletes it. +func TestDeleteKeepsAReapMarkWrittenInBetween(t *testing.T) { + t.Parallel() + + for name, at := range map[string]int{"before the mark is set": 1, "before the mark is taken off": 2} { + t.Run(name, func(t *testing.T) { + t.Parallel() + + inner := NewInMemory() + seedRestoreSource(t, inner, "src", "snap") + seedRestoredFrom(t, inner, "dst", "src:snap") // unfinished: the delete keeps the snapshot + + reap := newReapingMark("clone1", time.Now()) + st := &racingStore{Store: inner, snaps: &racingSnaps{SnapshotStore: inner.Snapshots(), at: at}} + st.snaps.inject = func() { + if err := setReapingMark(t.Context(), inner, "src", "snap", reap); err != nil { + t.Errorf("write the reap mark: %v", err) + } + } + + _ = DeleteSnapshot(t.Context(), st, "src", "snap") + + snap, err := inner.Snapshots().Get(t.Context(), "src", "snap") + if err != nil { + t.Fatalf("the kept snapshot is gone: %v", err) + } + + if snap.Props[CloneSnapshotReapingProp] != reap { + t.Fatalf("reap mark lost: props=%v", snap.Props) + } + + if mark, ok := snap.Props[SnapshotDeletingProp]; ok { + t.Errorf("the kept snapshot still carries the deleting mark %q", mark) + } + }) + } +} + +// Taking the deleting mark off a snapshot that had no props leaves it with +// none, so a restore from it still falls back to its source's props. +func TestDroppingTheDeletingMarkLeavesNoProps(t *testing.T) { + t.Parallel() + + st := NewInMemory() + seedRestoreSource(t, st, "src-bare", "snap-bare") + seedRestoredFrom(t, st, "dst-bare", "src-bare:snap-bare") + + _ = DeleteSnapshot(t.Context(), st, "src-bare", "snap-bare") + + snap, err := st.Snapshots().Get(t.Context(), "src-bare", "snap-bare") + if err != nil { + t.Fatalf("the kept snapshot is gone: %v", err) + } + + if snap.Props != nil { + t.Errorf("props = %#v, want none", snap.Props) + } +} + +// Two deletes of one snapshot can overlap: one that keeps the snapshot +// takes off only its own deleting mark. Another delete's mark, written +// right before the drop, stays, so a restore reading back in that gap +// still stands down while the other delete goes on. +func TestDeleteTakesOffOnlyItsOwnDeletingMark(t *testing.T) { + t.Parallel() + + inner := NewInMemory() + seedRestoreSource(t, inner, "src-two", "snap-two") + seedRestoredFrom(t, inner, "dst-two", "src-two:snap-two") // unfinished: the delete keeps the snapshot + + other := "other-delete@" + time.Now().UTC().Format(time.RFC3339) + st := &racingStore{Store: inner, snaps: &racingSnaps{SnapshotStore: inner.Snapshots(), at: 2}} + st.snaps.inject = func() { + err := inner.Snapshots().PatchSnapshotProps(t.Context(), "src-two", "snap-two", func(props map[string]string) error { + props[SnapshotDeletingProp] = other + + return nil + }) + if err != nil { + t.Errorf("write the other delete's mark: %v", err) + } + } + + _ = DeleteSnapshot(t.Context(), st, "src-two", "snap-two") + + snap, err := inner.Snapshots().Get(t.Context(), "src-two", "snap-two") + if err != nil { + t.Fatalf("the kept snapshot is gone: %v", err) + } + + if got := snap.Props[SnapshotDeletingProp]; got != other { + t.Fatalf("deleting mark = %q, want the other delete's %q", got, other) + } +} diff --git a/pkg/store/snapshot_delete_test.go b/pkg/store/snapshot_delete_test.go new file mode 100644 index 00000000..2b62965c --- /dev/null +++ b/pkg/store/snapshot_delete_test.go @@ -0,0 +1,259 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package store + +import ( + "context" + "errors" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" +) + +func seedRestoreSource(t *testing.T, st Store, srcRD, snapName string) { + t.Helper() + + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{Name: srcRD}); err != nil { + t.Fatalf("seed source: %v", err) + } + + if err := st.Snapshots().Create(t.Context(), &apiv1.Snapshot{Name: snapName, ResourceName: srcRD}); err != nil { + t.Fatalf("seed snapshot: %v", err) + } +} + +func seedRestoredFrom(t *testing.T, st Store, rdName, marker string, flags ...string) { + t.Helper() + + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: rdName, Flags: flags, Props: map[string]string{RestoreFromSnapshotProp: marker}, + }); err != nil { + t.Fatalf("seed restored definition: %v", err) + } +} + +// A delete of a snapshot that is not there holds nothing: a definition whose +// marker still names it, with a restore that can never finish, used to turn +// the idempotent answer into a refusal every retry met. +func TestDeleteSnapshotOfAMissingSnapshotRefusesNothing(t *testing.T) { + t.Parallel() + + st := NewInMemory() + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{Name: "src-gone"}); err != nil { + t.Fatalf("seed source: %v", err) + } + + seedRestoredFrom(t, st, "dst-gone", "src-gone:snap-gone") + + err := DeleteSnapshot(t.Context(), st, "src-gone", "snap-gone") + if !errors.Is(err, ErrNotFound) || errors.Is(err, ErrRestoreSourceInUse) { + t.Fatalf("delete of a missing snapshot = %v, want ErrNotFound alone", err) + } +} + +// A definition being deleted is not waited for. +func TestDeleteSnapshotSkipsADefinitionBeingDeleted(t *testing.T) { + t.Parallel() + + st := NewInMemory() + seedRestoreSource(t, st, "src-del", "snap-del") + seedRestoredFrom(t, st, "dst-del", "src-del:snap-del", apiv1.ResourceFlagDelete) + + if err := DeleteSnapshot(t.Context(), st, "src-del", "snap-del"); err != nil { + t.Fatalf("delete under a definition being deleted = %v, want nil", err) + } + + if _, err := st.Snapshots().Get(t.Context(), "src-del", "snap-del"); !errors.Is(err, ErrNotFound) { + t.Fatalf("the snapshot is still there: %v", err) + } +} + +// A delete the restore refuses takes its mark back off, so restores from the +// kept snapshot are not refused for the mark's lifetime. +func TestDeleteSnapshotTakesItsMarkOffASnapshotItKeeps(t *testing.T) { + t.Parallel() + + st := NewInMemory() + seedRestoreSource(t, st, "src-keep", "snap-keep") + seedRestoredFrom(t, st, "dst-keep", "src-keep:snap-keep") + + err := DeleteSnapshot(t.Context(), st, "src-keep", "snap-keep") + if !errors.Is(err, ErrRestoreSourceInUse) { + t.Fatalf("delete under an unfinished restore = %v, want ErrRestoreSourceInUse", err) + } + + snap, err := st.Snapshots().Get(t.Context(), "src-keep", "snap-keep") + if err != nil { + t.Fatalf("the refused delete took the snapshot: %v", err) + } + + if mark, ok := snap.Props[SnapshotDeletingProp]; ok { + t.Errorf("the kept snapshot still carries the deleting mark %q", mark) + } + + if err := RestoreSourceWithdrawn(t.Context(), st, "src-keep", "snap-keep"); err != nil { + t.Errorf("a restore from the kept snapshot is withdrawn: %v", err) + } +} + +// raceOnList runs before every uncached definition list: what a restore does +// between the delete's mark and its list. +type raceOnList struct { + Store + + before func(ctx context.Context) +} + +type raceOnListRDs struct { + ResourceDefinitionStore + + before func(ctx context.Context) +} + +func (r raceOnList) ResourceDefinitions() ResourceDefinitionStore { + return raceOnListRDs{ResourceDefinitionStore: r.Store.ResourceDefinitions(), before: r.before} +} + +func (r raceOnListRDs) ListUncached(ctx context.Context) ([]apiv1.ResourceDefinition, error) { + r.before(ctx) + + return r.ResourceDefinitionStore.ListUncached(ctx) //nolint:wrapcheck // pass-through test double +} + +// A restore whose definition lands after the delete's mark and before its +// list is seen by both halves: the restore reads the mark back and withdraws, +// and the delete lists the definition and keeps the snapshot. Neither ends +// with a definition naming a snapshot that is gone. +func TestDeleteSnapshotAndARacingRestoreSeeEachOther(t *testing.T) { + t.Parallel() + + backend := NewInMemory() + seedRestoreSource(t, backend, "src-race", "snap-race") + + var withdrawn error + + st := raceOnList{Store: backend, before: func(ctx context.Context) { + seedRestoredFrom(t, backend, "dst-race", "src-race:snap-race") //nolint:contextcheck // the helper seeds on t's own context + withdrawn = RestoreSourceWithdrawn(ctx, backend, "src-race", "snap-race") + }} + + err := DeleteSnapshot(t.Context(), st, "src-race", "snap-race") + if !errors.Is(err, ErrRestoreSourceInUse) { + t.Errorf("the delete did not see the restore that raced it: %v", err) + } + + if !errors.Is(withdrawn, ErrRestoreSourceWithdrawn) || !errors.Is(withdrawn, ErrRestoreSourceDeleting) { + t.Errorf("the restore did not see the delete that raced it: %v", withdrawn) + } +} + +func TestRestoreSourceWithdrawn(t *testing.T) { + t.Parallel() + + now := time.Now().UTC() + + for _, tc := range []struct { + name string + props map[string]string + missing bool + wantErrs []error + }{ + {name: "plain"}, + {name: "gone", missing: true, wantErrs: []error{ErrRestoreSourceWithdrawn, ErrNotFound}}, + { + name: "live-mark", + props: map[string]string{SnapshotDeletingProp: "tok@" + now.Format(time.RFC3339)}, + wantErrs: []error{ErrRestoreSourceWithdrawn, ErrRestoreSourceDeleting}, + }, + { + name: "expired-mark", + props: map[string]string{SnapshotDeletingProp: "tok@" + now.Add(-time.Hour).Format(time.RFC3339)}, + }, + {name: "unreadable-mark", props: map[string]string{SnapshotDeletingProp: "tok"}}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + st := NewInMemory() + if !tc.missing { + if err := st.Snapshots().Create(t.Context(), &apiv1.Snapshot{ + Name: "snap", ResourceName: "src", Props: tc.props, + }); err != nil { + t.Fatalf("seed: %v", err) + } + } + + err := RestoreSourceWithdrawn(t.Context(), st, "src", "snap") + if len(tc.wantErrs) == 0 && err != nil { + t.Fatalf("withdrawn = %v, want nil", err) + } + + for _, want := range tc.wantErrs { + if !errors.Is(err, want) { + t.Errorf("withdrawn = %v, want it to wrap %v", err, want) + } + } + }) + } +} + +// The mark describes the snapshot it is on: a definition restored from the +// snapshot while it was marked does not carry it. +func TestTravellingPropsLeaveTheDeletingMarkBehind(t *testing.T) { + t.Parallel() + + out := TravellingProps(map[string]string{SnapshotDeletingProp: "tok@now", "Aux/keep": "v"}) + if _, ok := out[SnapshotDeletingProp]; ok || out["Aux/keep"] != "v" { + t.Fatalf("TravellingProps = %v", out) + } +} + +// cacheBlindToDefinitions answers the cached definition list from a cache +// that has not seen any definition yet. +type cacheBlindToDefinitions struct { + Store +} + +type cacheBlindRDs struct { + ResourceDefinitionStore +} + +func (c cacheBlindToDefinitions) ResourceDefinitions() ResourceDefinitionStore { + return cacheBlindRDs{ResourceDefinitionStore: c.Store.ResourceDefinitions()} +} + +func (cacheBlindRDs) List(context.Context) ([]apiv1.ResourceDefinition, error) { + return nil, nil +} + +// The dependent most likely to be missing from a cache is the one created a +// moment ago, so the delete lists past it. +func TestDeleteSnapshotListsDependentsPastTheCache(t *testing.T) { + t.Parallel() + + backend := NewInMemory() + seedRestoreSource(t, backend, "src-cache", "snap-cache") + seedRestoredFrom(t, backend, "dst-cache", "src-cache:snap-cache") + + err := DeleteSnapshot(t.Context(), cacheBlindToDefinitions{Store: backend}, "src-cache", "snap-cache") + if !errors.Is(err, ErrRestoreSourceInUse) { + t.Fatalf("delete with a cache blind to the dependent = %v, want ErrRestoreSourceInUse", err) + } +} diff --git a/tests/e2e/cli-matrix/README.md b/tests/e2e/cli-matrix/README.md index ee4116cd..a2245a51 100644 --- a/tests/e2e/cli-matrix/README.md +++ b/tests/e2e/cli-matrix/README.md @@ -44,6 +44,7 @@ A cell counts as **FAIL** if either leg times out, the `linstor` CLI exits non-z | `rd-clone-vd-data-plane.sh` | 020 | `linstor rd clone ` on a VD-bearing source (plain CLI body, no `use_zfs_clone`) AND the raw-REST `use_zfs_clone=true` body linstor-csi sends both materialise a real clone: 2 replicas UpToDate, marker bytes from the source present on EVERY clone replica (promote each in turn), clone status COMPLETE, internal `clone-` snapshot visible on the source. Pre-fix: 400 on `use_zfs_clone`, 501 on VD-bearing sources (Bug 114 gate). | | `rd-clone-retry-semantics.sh` | parity | The clone endpoint's retry contract. `linstor rd clone ` run twice answers the same way twice, and still does after `vd set-size` grows the source: a finished clone is a copy of a point-in-time and owes the source nothing, and judging the replay against the live source would make an ordinary resize refuse every later retry permanently (the snapshot outlives the clone by design, so deleting it is no way out). Also pins a clone body carrying `delete_namespaces`, driven over raw REST because the verb has no flag for it — it is a wire field from golinstor's `GenericPropsModify`. | | `snap-restore-retry-semantics.sh` | parity | The resource restore's retry contract. LINSTOR's documented restore sequence, `rd c`, `s vd restore`, then `s rsc restore`, is the one linstor-csi issues, and the resource restore takes the definition the first two steps made as its own target. Run again with the same argv, the retry CSI issues when a response is lost, it answers success and places nothing more. | +| `snap-d-restore-source.sh` | 197 | A snapshot a restore still reads from is kept until the restore is done. `s d` of the source of a bare `s rsc restore`, whose target has no replica yet, is refused and the snapshot stays; once a replica of the target is UpToDate, `s d` succeeds and takes the restore marker off, a repeated `s d` of the snapshot that is gone still exits 0, and a later replica of the target syncs from its peer. Pre-fix `s d` deleted the snapshot at once, and the next replica came up blank. | | `encryption-passphrase-luks-rd.sh` | 023 | Secret-only LUKS flow: `linstor encryption create-passphrase` alone (legacy `DrbdOptions/EncryptPassphrase` controller prop asserted ABSENT throughout, and `controller set-property DrbdOptions/EncryptPassphrase` asserted REFUSED) unlocks `rd c -l drbd,luks,storage` + autoplace to UpToDate, and the Secret-backed passphrase actually opens the LUKS header on each replica's backing device. Requires the Bug-023 fix (PR #143); pre-fix the rd-create is rejected with "LUKS layer requires DrbdOptions/EncryptPassphrase to be set first". | | `vd-modify-preserves-drbd-minor.sh` | 433 | A legal in-bounds VD-scoped modify (`vd set-size` grow / `vd set-property`) must NOT change the per-volume DRBDMinor — the /dev/drbd device identity. Creates rd-a (lowest minor Ma) + rd-b (next minor Mb), frees Ma by deleting rd-a, then modifies rd-b: its DRBDMinor AND device path must stay Mb over a settle window. Pre-fix `wireToCRDVD` dropped the minor on the VD-scoped write-back and the allocator re-stamped the freed lower Ma — a permanent device-identity change on a live resized volume. | | `rg-modify-invalid-layer-rejected.sh` | 434 | `rg modify --layer-list storage,drbd` (STORAGE-before-DRBD, the ordering the create path refuses) must be rejected, the RG must keep its valid `[DRBD,STORAGE]` stack, and an RD inheriting from the RG must get the valid stack — never `[STORAGE,DRBD]`. Pre-fix `rg modify` gated only place_count and `handleRDCreate` validated the explicit layer_list BEFORE the RG inherit, so an invalid stack reached a persisted RD via the modify→inherit chain. | diff --git a/tests/e2e/cli-matrix/snap-d-restore-source.sh b/tests/e2e/cli-matrix/snap-d-restore-source.sh new file mode 100755 index 00000000..6827e8f3 --- /dev/null +++ b/tests/e2e/cli-matrix/snap-d-restore-source.sh @@ -0,0 +1,116 @@ +#!/usr/bin/env bash +# +# usage: snap-d-restore-source.sh WORK_DIR +# +# L6 cli-matrix cell: a snapshot a restore still reads from is kept until +# the restore is done, and a delete of a snapshot that is gone stays +# idempotent (cozystack/blockstor#197). +# +# A restored definition keeps its restore marker, and while the marker is +# there every new replica is brought up from the snapshot. Deleting the +# snapshot under an unfinished restore left the next replica blank and +# never UpToDate. +# +# Contract: +# 1. source RD with one UpToDate diskful replica, snapshot created. +# 2. bare s rsc restore: the target has its volumes and no replica. +# 3. s d: non-zero, the snapshot is still listed. +# 4. r c the target on node 1: UpToDate. +# 5. s d: exit 0, and the target no longer carries the restore marker. +# 6. s d again: exit 0 over a snapshot that is gone. +# 7. r c the target on node 2: UpToDate, synced from node 1. +# +# Unit pins: pkg/rest/restore_source_lifetime_test.go, +# pkg/rest/snapshot_delete_restore_race_test.go (REST door), +# internal/cli/snapshot_delete_restore_source_test.go (CLI door). L7 +# companion: tests/operator-harness/replay/snap-d-restore-source.yaml. + +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 2 + +linstor_cli_setup + +SRC=cli-matrix-sdsrc-src +TGT=cli-matrix-sdsrc-tgt +SNAP=sdsrc +POOL=${POOL:-stand} + +N1=$WORKER_1 +N2=$WORKER_2 + +# The target goes first, then the snapshot, then the source. +cleanup() { + local rc=$? clean=0 rd + set +e + "${LCTL[@]}" resource-definition delete "$TGT" >/dev/null 2>&1 + "${LCTL[@]}" snapshot delete "$SRC" "$SNAP" >/dev/null 2>&1 + strict_cleanup_on_exit "${_STRICT_CLEANUP_RDS[@]}" || clean=1 + for rd in "${_STRICT_CLEANUP_RDS[@]}"; do + assert_no_orphans "$rd" || clean=1 + done + linstor_cli_teardown + if [[ $rc -eq 0 && $clean -ne 0 ]]; then + echo "FAIL: the cluster could not be cleaned after the cell; PASS demoted to FAIL" >&2 + rc=1 + fi + exit "$rc" +} +register_strict_cleanup "$TGT" "$SRC" +trap cleanup EXIT + +echo ">> source RD: 1 diskful replica on $POOL, then a snapshot" +"${LCTL[@]}" resource-definition create "$SRC" >/dev/null +"${LCTL[@]}" volume-definition create "$SRC" 64M >/dev/null +"${LCTL[@]}" resource create "$N1" "$SRC" --storage-pool="$POOL" >/dev/null +wait_uptodate "$SRC" "$N1" +"${LCTL[@]}" snapshot create "$SRC" "$SNAP" >/dev/null + +echo ">> bare restore: the target has its volumes and no replica yet" +if ! out=$("${LCTL[@]}" snapshot resource restore \ + --from-resource "$SRC" --from-snapshot "$SNAP" --to-resource "$TGT" 2>&1); then + echo "FAIL: bare s rsc restore: $out" >&2 + exit 1 +fi + +echo ">> s d while the restore is unfinished must be refused" +if out=$("${LCTL[@]}" snapshot delete "$SRC" "$SNAP" 2>&1); then + echo "FAIL: s d took the source of an unfinished restore: $out" >&2 + exit 1 +fi +if ! "${LCTL[@]}" snapshot list 2>/dev/null | grep -q "$SNAP"; then + echo "FAIL: the refused s d took the snapshot anyway" >&2 + exit 1 +fi + +echo ">> place the restored replica, then s d succeeds" +"${LCTL[@]}" resource create "$N1" "$TGT" --storage-pool="$POOL" >/dev/null +wait_uptodate "$TGT" "$N1" +if ! out=$("${LCTL[@]}" snapshot delete "$SRC" "$SNAP" 2>&1); then + echo "FAIL: s d after the restore finished: $out" >&2 + exit 1 +fi +if "${LCTL[@]}" resource-definition list-properties "$TGT" 2>/dev/null \ + | grep -q BlockstorRestoreFromSnapshot; then + echo "FAIL: $TGT still names the deleted snapshot as its restore source" >&2 + exit 1 +fi + +echo ">> s d again: the snapshot is gone, the delete is still a success" +if ! out=$("${LCTL[@]}" snapshot delete "$SRC" "$SNAP" 2>&1); then + echo "FAIL: s d of a snapshot that is gone exited non-zero: $out" >&2 + exit 1 +fi + +echo ">> a later replica syncs from its peer" +"${LCTL[@]}" resource create "$N2" "$TGT" --storage-pool="$POOL" >/dev/null +wait_uptodate "$TGT" "$N1" "$N2" + +echo ">> snap-d-restore-source OK (source kept while needed, released after, delete idempotent)" diff --git a/tests/operator-harness/replay/snap-d-restore-source.yaml b/tests/operator-harness/replay/snap-d-restore-source.yaml new file mode 100644 index 00000000..6cb9fc02 --- /dev/null +++ b/tests/operator-harness/replay/snap-d-restore-source.yaml @@ -0,0 +1,83 @@ +name: snap-d-restore-source +description: | + A snapshot a restore still reads from is kept until the restore is + done, and a delete of a snapshot that is gone stays idempotent + (cozystack/blockstor#197). + + A restored definition keeps its restore marker, and while the marker + is there every new replica is brought up from the snapshot. Deleting + the snapshot under an unfinished restore left the next replica blank. + Once the restore is done, the delete takes the marker off, and later + replicas sync from their peers. + + Steps: + 1. source: rd create + vd create + one diskful replica, UpToDate + 2. snapshot create, then a bare s rsc restore: the target has its + volumes and no replica, so its restore is not finished + 3. s d of the snapshot: refused (exit 10), the snapshot stays + 4. r c the target on node1: the replica restores, UpToDate + 5. s d of the snapshot: exit 0 + 6. s d again, the CSI retry: exit 0 over a snapshot that is gone + 7. r c the target on node2: it syncs from node1, UpToDate + +prerequisites: + min_nodes: 2 + storage_pool: stand + +vars: + sp: stand + rd: replay-sdsrc + +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 + await: + kind: all_uptodate + rd: "{{rd}}" + timeout_s: 240 + - name: snapshot-create + cmd: ["snapshot", "create", "{{rd}}", "sdsrc"] + expect_exit: 0 + - name: bare-resource-restore + cmd: ["snapshot", "resource", "restore", + "--from-resource", "{{rd}}", "--from-snapshot", "sdsrc", + "--to-resource", "{{rd}}-t"] + expect_exit: 0 + - name: s-d-refused-while-the-restore-is-unfinished + cmd: ["snapshot", "delete", "{{rd}}", "sdsrc"] + expect_exit: 10 + - name: place-the-restored-replica + cmd: ["resource", "create", "{{node1}}", "{{rd}}-t", "--storage-pool={{sp}}"] + expect_exit: 0 + await: + kind: all_uptodate + rd: "{{rd}}-t" + timeout_s: 240 + - name: s-d-once-the-restore-is-done + cmd: ["snapshot", "delete", "{{rd}}", "sdsrc"] + expect_exit: 0 + - name: s-d-again-is-idempotent + cmd: ["snapshot", "delete", "{{rd}}", "sdsrc"] + expect_exit: 0 + - name: a-later-replica-syncs-from-its-peer + cmd: ["resource", "create", "{{node2}}", "{{rd}}-t", "--storage-pool={{sp}}"] + expect_exit: 0 + await: + kind: all_uptodate + rd: "{{rd}}-t" + timeout_s: 240 + +teardown: + - cmd: ["resource-definition", "delete", "{{rd}}-t"] + - cmd: ["snapshot", "delete", "{{rd}}", "sdsrc"] + - cmd: ["resource-definition", "delete", "{{rd}}"] + +invariants: + - no_orphans From d8645c4ebcf3bd32553e0c70e58eee2ee61fd472 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 18:50:49 +0100 Subject: [PATCH 5/8] test(rollback): pin the time on the marks the rollback writers put The takeover of a stale in-progress mark reads the time written next to it, and every takeover test seeded that mark by hand. Writers that dropped the time, or a time left behind when the mark was replaced or removed, passed the whole suite while no real mark could ever go stale. The REST rollback and the store calls the CLI rollback runs on now have tests that drive them to the in-progress mark and check its time, and check that the step mark replacing it and a stand-down removing it take the time with them. An operator setting the time by hand is refused like any other server-owned prop. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/rest/rd_modify_server_owned_props_test.go | 5 + pkg/rest/rollback_mark_time_test.go | 162 ++++++++++++++++++ pkg/store/rollback_mark_time_test.go | 114 ++++++++++++ 3 files changed, 281 insertions(+) create mode 100644 pkg/rest/rollback_mark_time_test.go create mode 100644 pkg/store/rollback_mark_time_test.go diff --git a/pkg/rest/rd_modify_server_owned_props_test.go b/pkg/rest/rd_modify_server_owned_props_test.go index 643f758a..b7e878de 100644 --- a/pkg/rest/rd_modify_server_owned_props_test.go +++ b/pkg/rest/rd_modify_server_owned_props_test.go @@ -38,6 +38,11 @@ func TestRDModifyRefusesEditsOfServerOwnedProps(t *testing.T) { {name: "set-rollback-mark", body: map[string]any{ "override_props": map[string]string{store.RollbackAbandonedProp: store.RollbackInProgress}, }}, + // The time is what a retry takes over a live mark on: set by hand, it + // hands a running rollback's definition to the next retry. + {name: "set-rollback-since", body: map[string]any{ + "override_props": map[string]string{store.RollbackSinceProp: "2000-01-01T00:00:00Z"}, + }}, } { t.Run(tc.name, func(t *testing.T) { t.Parallel() diff --git a/pkg/rest/rollback_mark_time_test.go b/pkg/rest/rollback_mark_time_test.go new file mode 100644 index 00000000..e516f219 --- /dev/null +++ b/pkg/rest/rollback_mark_time_test.go @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package rest + +import ( + "context" + "maps" + "sync" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// recordMarks keeps the props every successful definition patch left. +type recordMarks struct { + store.Store + + mu *sync.Mutex + props *[]map[string]string +} + +type recordMarksRDs struct { + store.ResourceDefinitionStore + + outer recordMarks +} + +func (r recordMarks) ResourceDefinitions() store.ResourceDefinitionStore { + return recordMarksRDs{ResourceDefinitionStore: r.Store.ResourceDefinitions(), outer: r} +} + +func (r recordMarksRDs) PatchResourceDefinitionSpec( + ctx context.Context, name string, mutate func(*apiv1.ResourceDefinition) error, +) error { + err := r.ResourceDefinitionStore.PatchResourceDefinitionSpec(ctx, name, mutate) + if err != nil { + return err //nolint:wrapcheck // pass-through test double + } + + rd, getErr := r.GetUncached(ctx, name) + if getErr == nil { + r.outer.mu.Lock() + *r.outer.props = append(*r.outer.props, maps.Clone(rd.Props)) + r.outer.mu.Unlock() + } + + return nil +} + +// The takeover of a stale mark reads the time on it, so the writer has to put +// it there: a REST rollback's in-progress mark carries the time it was +// written, and the step mark that replaces it carries none. +func TestTheRESTRollbackStampsItsInProgressMarkWithTheTime(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + snapshot bool + adopted bool + wantStep string + }{ + {name: "completes", wantStep: rollbackStepNames[rollbackStepReapReplicas]}, + {name: "stops-at-snapshots", snapshot: true, wantStep: rollbackStepNames[rollbackStepSnapshots]}, + // A rollback that yields takes its mark off, and the time with it. + {name: "yields", adopted: true}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + rdName := "dst-since-" + tc.name + seedRolledBackDefinition(t, backend, rdName) + + if tc.snapshot { + if err := backend.Snapshots().Create(t.Context(), &apiv1.Snapshot{Name: "snap", ResourceName: rdName}); err != nil { + t.Fatalf("seed snapshot: %v", err) + } + } + + if tc.adopted { + if err := backend.ResourceDefinitions().PatchResourceDefinitionSpec(t.Context(), rdName, + func(rd *apiv1.ResourceDefinition) error { + rd.Props = map[string]string{store.RestoreAdoptedProp: "claim"} + + return nil + }); err != nil { + t.Fatalf("seed the adoption: %v", err) + } + } + + var recorded []map[string]string + + srv := &Server{Store: recordMarks{Store: backend, mu: &sync.Mutex{}, props: &recorded}} + before := time.Now().Add(-time.Second) + + _ = srv.rollBackCompensating(t.Context(), rdName, []string{"node-a"}, rollbackEvenIfFinished) + + assertMarksCarryTheirTime(t, recorded, before, tc.wantStep) + }) + } +} + +// assertMarksCarryTheirTime checks that every in-progress mark among props +// carries a time no earlier than before, that one was written at all, and +// that the step mark wantStep was written without one. +func assertMarksCarryTheirTime(t *testing.T, recorded []map[string]string, before time.Time, wantStep string) { + t.Helper() + + sawInProgress, sawStep := false, false + + for _, props := range recorded { + switch props[store.RollbackAbandonedProp] { + case store.RollbackInProgress: + sawInProgress = true + + since, err := time.Parse(time.RFC3339, props[store.RollbackSinceProp]) + if err != nil || since.Before(before.Truncate(time.Second)) || since.After(time.Now()) { + t.Errorf("in-progress mark carries time %q (%v), want one written now", props[store.RollbackSinceProp], err) + } + case wantStep: + sawStep = true + + if v, ok := props[store.RollbackSinceProp]; ok { + t.Errorf("step mark %q carries a time %q: a step stands until an operator acts", wantStep, v) + } + case "": + if v, ok := props[store.RollbackSinceProp]; ok { + t.Errorf("the mark was taken off, but its time %q stayed", v) + } + } + } + + if !sawInProgress { + t.Errorf("no in-progress mark was written: %v", recorded) + } + + if wantStep != "" && !sawStep { + t.Errorf("no %q step mark was written: %v", wantStep, recorded) + } + + if wantStep == "" && (len(recorded) == 0 || recorded[len(recorded)-1][store.RollbackAbandonedProp] != "") { + t.Errorf("the mark was not taken off: %v", recorded) + } +} diff --git a/pkg/store/rollback_mark_time_test.go b/pkg/store/rollback_mark_time_test.go new file mode 100644 index 00000000..41967369 --- /dev/null +++ b/pkg/store/rollback_mark_time_test.go @@ -0,0 +1,114 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package store + +import ( + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" +) + +// The CLI's rollback writes its marks through HoldRollback, +// EnterDestructiveRollback and the stand-downs. The takeover of a stale mark +// reads the time on it, so the in-progress mark has to carry it, and every +// write that replaces or removes that mark has to take the time with it. +func TestTheCLIRollbackWritersKeepTheTimeOnTheInProgressMarkOnly(t *testing.T) { + t.Parallel() + + ctx := t.Context() + st := NewInMemory() + + for _, name := range []string{"dst-hold", "dst-yield"} { + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: name}); err != nil { + t.Fatalf("seed %s: %v", name, err) + } + } + + props := func(name string) map[string]string { + rd, err := st.ResourceDefinitions().GetUncached(ctx, name) + if err != nil { + t.Fatalf("read %s: %v", name, err) + } + + return rd.Props + } + + before := time.Now().Add(-time.Second).Truncate(time.Second) + + if err := HoldRollback(ctx, st, "dst-hold", true); err != nil { + t.Fatalf("HoldRollback: %v", err) + } + + held := props("dst-hold") + + since, err := time.Parse(time.RFC3339, held[RollbackSinceProp]) + if held[RollbackAbandonedProp] != RollbackInProgress || err != nil || since.Before(before) { + t.Fatalf("held mark = %v (%v), want in-progress with the time it was written", held, err) + } + + if err := EnterDestructiveRollback(ctx, st, "dst-hold", RollbackStepReapReplicas); err != nil { + t.Fatalf("EnterDestructiveRollback: %v", err) + } + + if step := props("dst-hold"); step[RollbackAbandonedProp] != RollbackStepReapReplicas || step[RollbackSinceProp] != "" { + t.Errorf("step mark = %v, want the step with no time", step) + } + + // A hold that yields stands down: the mark goes, and its time with it. + if err := st.ResourceDefinitions().PatchResourceDefinitionSpec(ctx, "dst-yield", + func(rd *apiv1.ResourceDefinition) error { + rd.Props = map[string]string{RestoreAdoptedProp: "claim"} + + return nil + }); err != nil { + t.Fatalf("seed the adoption: %v", err) + } + + if err := HoldRollback(ctx, st, "dst-yield", true); err == nil { + t.Fatal("HoldRollback over an adopted definition did not yield") + } + + if left := props("dst-yield"); left[RollbackAbandonedProp] != "" || left[RollbackSinceProp] != "" { + t.Errorf("after the stand-down = %v, want no mark and no time", left) + } +} + +func TestMarkAndUnmarkRollbackMoveTheTimeWithTheMark(t *testing.T) { + t.Parallel() + + now := time.Date(2026, 10, 27, 12, 0, 0, 0, time.UTC) + + props := MarkRollback(nil, RollbackInProgress, now) + if props[RollbackSinceProp] != now.Format(time.RFC3339) { + t.Fatalf("in-progress mark = %v, want the time on it", props) + } + + props = MarkRollback(props, RollbackStepReapReplicas, now) + if _, ok := props[RollbackSinceProp]; ok { + t.Errorf("step mark = %v, want no time", props) + } + + props = MarkRollback(props, RollbackInProgress, now) + UnmarkRollback(props) + + if len(props) != 0 { + t.Errorf("after UnmarkRollback = %v, want nothing", props) + } +} From 6b953dc6b499851bf28a4fca3c09494ed403a223 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Tue, 27 Oct 2026 19:00:55 +0100 Subject: [PATCH 6/8] docs(parity): record that a snapshot a restore still reads is kept Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/cli-parity-known-deltas.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/cli-parity-known-deltas.md b/docs/cli-parity-known-deltas.md index 879639d8..0c5d0772 100644 --- a/docs/cli-parity-known-deltas.md +++ b/docs/cli-parity-known-deltas.md @@ -64,6 +64,7 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see | 07 | `rd l --resource-definitions ` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions ` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | | 33 | `s d ` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `SUCCESS: snapshot already absent: ` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition of resource not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). | +| 93 | `s d ` while a definition is still being restored from it | BEHAVIOR | permanent | BLOCKSTOR_STRICTER. Upstream deletes the snapshot. blockstor keeps a snapshot a definition still restores from: a definition whose restore is finished has its marker taken off and the delete goes on, and one whose restore is still running makes the delete answer a retryable 409 naming it, so its next replica does not come up blank. A delete also marks the snapshot first, so a restore that races it withdraws instead of answering over a snapshot that is gone. Pinned by the unit tests in `pkg/store` and `pkg/rest`; the L6 cell `cli-matrix/snap-d-restore-source.sh` and the L7 replay `replay/snap-d-restore-source.yaml` cover it on a stand and have not run there yet. | | 42 | `r d ` (idempotent resource delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a resource placement on a node that holds none returns `SUCCESS: resource already absent: on `; upstream LINSTOR returns `WARNING: Node: , Resource: not found`. Both exit 0. Deliberate: a CSI `DeleteVolume` / operator retry on an already-removed placement MUST be idempotent. Same family as the snapshot-delete idempotency in row 33. Pinned by the resource-delete REST handler's already-absent path (`pkg/rest/resources.go`). | | 40 | `n c --node-type Satellite` (node-create success envelope) | WIRE_SHAPE | permanent | BLOCKSTOR envelope shape on satellite node registration. BS emits `node created: ` + a `SUCCESS: No active connection to satellite ''` line; upstream emits `New node '' registered.` (with a UUID detail) + a `WARNING: No active connection to satellite ''` line whose Details explain the controller will (re-)establish the connection. Both register the node and exit 0; the operator-visible outcome (node exists, awaiting satellite handshake) is identical. The "no active connection" notice is INFO/SUCCESS-class in BS vs WARNING-class upstream, and BS does not surface the volatile node UUID inline. Envelope-shape only; no behavioural divergence. | | 16 | `ps l` (physical-storage list — `size` field) | WIRE_SHAPE | 2026-12-31 | BS `/v1/physical-storage` omits the per-device `size` key that python-linstor's `show_physical_storage` reads unconditionally (`linstor/responses.py` `devices.size`), so `linstor ps l` raises `KeyError: 'size'` and exits 2 against BS where upstream renders the table and exits 0. `ps l` is a hardware-discovery convenience surface; linstor-csi / piraeus-operator never call it, and the device-pool creation path (`ps create-device-pool`, cli-matrix `ps-cdp-*`) is unaffected. Tracked as a missing wire field to populate on the physical-storage DTO; whitelisted until then. | From 9b1f47bc2ee8705d0efe8d1f12ba589ac3fdeb71 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 20:28:47 +0100 Subject: [PATCH 7/8] docs(site): say a restore source cannot be deleted mid-restore Both snapshot examples delete the snapshot right after restoring from it. That delete is refused until the restore has an UpToDate replica: the blockstor CLI fails with "a restore from this snapshot is not finished", and linstor snapshot delete gets a 409. Say so next to it. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- site/content/en/docs/cli.md | 2 ++ site/content/en/docs/operations.md | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/site/content/en/docs/cli.md b/site/content/en/docs/cli.md index df1a4e02..f5143883 100644 --- a/site/content/en/docs/cli.md +++ b/site/content/en/docs/cli.md @@ -132,6 +132,8 @@ blockstor snapshot resource restore \ In-place rollback is not available on purpose: it destroys every snapshot newer than the one you roll back to. Restore into a new volume instead. +A snapshot that a restore reads from cannot be deleted until that restore has a replica with every volume `UpToDate`: until then `blockstor snapshot delete` fails with `a restore from this snapshot is not finished` (`linstor snapshot delete` gets a 409), and you retry it later. + ```sh blockstor snapshot delete my-volume before-upgrade ``` diff --git a/site/content/en/docs/operations.md b/site/content/en/docs/operations.md index ca50ca60..6fdf5b02 100644 --- a/site/content/en/docs/operations.md +++ b/site/content/en/docs/operations.md @@ -171,7 +171,7 @@ blockstor snapshot resource restore \ --from-resource my-volume --from-snapshot before-upgrade --to-resource my-volume-restored ``` -In-place rollback is not available on purpose. A definition cannot be deleted while it still has snapshots, so delete them first: +In-place rollback is not available on purpose. A definition cannot be deleted while it still has snapshots, so delete them first. A snapshot that a restore reads from cannot be deleted until that restore has a replica with every volume `UpToDate`: until then `blockstor snapshot delete` fails with `a restore from this snapshot is not finished` (`linstor snapshot delete` gets a 409), and you retry it later. ```sh blockstor snapshot delete my-volume before-upgrade From 1150d36c640ea69015368d75a240250ff6219677 Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 22:51:18 +0100 Subject: [PATCH 8/8] test(store): pin the stored spelling a restore source is read under Assisted-by: LLM Signed-off-by: Andrei Kvapil --- pkg/store/k8s/restore_source_spelling_test.go | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 pkg/store/k8s/restore_source_spelling_test.go diff --git a/pkg/store/k8s/restore_source_spelling_test.go b/pkg/store/k8s/restore_source_spelling_test.go new file mode 100644 index 00000000..ee6cca63 --- /dev/null +++ b/pkg/store/k8s/restore_source_spelling_test.go @@ -0,0 +1,55 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package k8s_test + +import ( + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// The uncached snapshot read is scoped by the definition name in the +// snapshot's spec, which the API server compares verbatim. A restore that +// names its source in another case than the definition was registered in +// still finds the snapshot: the read asks under the stored spelling. Asked +// as given, it would answer "deleted" and withdraw every such restore. +func TestRestoreSourceIsFoundUnderAnotherSpellingOfItsDefinition(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + ctx := t.Context() + st := k8s.NewWithAPIReader(fixture.client, fixture.client) + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "Pvc-Src"}); err != nil { + t.Fatalf("create definition: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{Name: "snap", ResourceName: "Pvc-Src"}); err != nil { + t.Fatalf("create snapshot: %v", err) + } + + if err := store.RestoreSourceWithdrawn(ctx, st, "PVC-SRC", "snap"); err != nil { + t.Fatalf("a restore from PVC-SRC/snap is withdrawn: %v", err) + } +}