diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index bec86987..27adba42 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -104,4 +104,7 @@ spec: volumeMounts: [] volumes: [] serviceAccountName: controller-manager - terminationGracePeriodSeconds: 10 + # The REST server's graceful-shutdown window is derived from the + # detached rollback budget (pkg/rest/rg_deleted_race.go); this has to + # outlive it, or a SIGTERM kills a compensation mid-cascade. + terminationGracePeriodSeconds: 20 diff --git a/internal/cli/local_props_test.go b/internal/cli/local_props_test.go new file mode 100644 index 00000000..ba2eada8 --- /dev/null +++ b/internal/cli/local_props_test.go @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: Apache-2.0 + +package cli_test + +import ( + "context" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +func markPVCX(ctx context.Context, backend store.Store) { + def, err := backend.ResourceDefinitions().Get(ctx, "pvc-x") + if err != nil { + return + } + + if def.Props == nil { + def.Props = map[string]string{} + } + + def.Props[store.RollbackAbandonedProp] = "snapshots" + _ = backend.ResourceDefinitions().Update(ctx, &def) +} + +// The CLI copies a definition's props onward on the same two paths the REST +// door does, and the abandoned-rollback mark is about the definition it sits +// on, never about a copy of it. +func TestCLIDoesNotCarryTheAbandonedRollbackMarkOnward(t *testing.T) { + t.Parallel() + + t.Run("snapshot-create", func(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + markPVCX(ctx, backend) + }) + + if got := app.Run(t.Context(), []string{"s", "c", "pvc-x", "snap-m"}); got != 0 { + t.Fatalf("create exit = %d (stderr: %s)", got, errBuf.String()) + } + + snap, err := appStore(t, app).Snapshots().Get(t.Context(), "pvc-x", "snap-m") + if err != nil { + t.Fatalf("get snapshot: %v", err) + } + + if step, ok := snap.Props[store.RollbackAbandonedProp]; ok { + t.Errorf("the snapshot carries the source's mark %q", step) + } + }) + + for _, tc := range []struct { + name string + seed func(context.Context, store.Store) + }{ + {name: "restore-from-a-snapshot-carrying-it", seed: func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + _ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-m", ResourceName: "pvc-x", Nodes: []string{"node-1", "node-2"}, + Props: map[string]string{store.RollbackAbandonedProp: "snapshots"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}}, + }) + }}, + {name: "restore-falling-back-to-the-source-props", seed: func(ctx context.Context, backend store.Store) { + seedSnapshotSource(ctx, backend) + markPVCX(ctx, backend) + _ = backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-m", ResourceName: "pvc-x", Nodes: []string{"node-1", "node-2"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 1 << 20}}, + }) + }}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, tc.seed) + + argv := []string{ + "s", "resource", "restore", + "--from-resource", "pvc-x", "--from-snapshot", "snap-m", "--to-resource", "pvc-m", + } + + if got := app.Run(t.Context(), argv); got != 0 { + t.Fatalf("restore exit = %d (stderr: %s)", got, errBuf.String()) + } + + def, err := appStore(t, app).ResourceDefinitions().Get(t.Context(), "pvc-m") + if err != nil { + t.Fatalf("get restored definition: %v", err) + } + + if step, ok := def.Props[store.RollbackAbandonedProp]; ok { + t.Errorf("the restored definition carries the mark %q", step) + } + }) + } +} diff --git a/internal/cli/snapshot.go b/internal/cli/snapshot.go index 2da55aa9..a1b940d8 100644 --- a/internal/cli/snapshot.go +++ b/internal/cli/snapshot.go @@ -22,7 +22,6 @@ import ( "context" "errors" "fmt" - "maps" "slices" "strings" @@ -235,7 +234,7 @@ func hydrateSnapshot(ctx context.Context, run *runContext, snap *apiv1.Snapshot) } if snap.Props == nil { - snap.Props = def.Props + snap.Props = store.TravellingProps(def.Props) } if snap.SnapshotDefinitionProps == nil { @@ -327,11 +326,11 @@ func snapshotRestoreResource(ctx context.Context, run *runContext) error { // group breaks the restore-then-list workflow. ResourceGroupName: src.ResourceGroupName, LayerStack: src.LayerStack, - Props: maps.Clone(snap.Props), + Props: store.TravellingProps(snap.Props), } if def.Props == nil { - def.Props = maps.Clone(src.Props) + def.Props = store.TravellingProps(src.Props) } if def.Props == nil { diff --git a/pkg/rest/cache_invalidation_bug_124_test.go b/pkg/rest/cache_invalidation_bug_124_test.go index 78df29a2..77f6b066 100644 --- a/pkg/rest/cache_invalidation_bug_124_test.go +++ b/pkg/rest/cache_invalidation_bug_124_test.go @@ -155,6 +155,10 @@ func (l *laggingRDs) Get(ctx context.Context, name string) (apiv1.ResourceDefini return l.inner.Get(ctx, name) //nolint:wrapcheck // test helper } +func (l *laggingRDs) GetUncached(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { + return l.inner.GetUncached(ctx, name) //nolint:wrapcheck // test helper +} + func (l *laggingRDs) Create(ctx context.Context, rd *apiv1.ResourceDefinition) error { return l.inner.Create(ctx, rd) //nolint:wrapcheck // test helper } diff --git a/pkg/rest/rd_clone.go b/pkg/rest/rd_clone.go index 0c25e426..c7e105f0 100644 --- a/pkg/rest/rd_clone.go +++ b/pkg/rest/rd_clone.go @@ -23,11 +23,14 @@ import ( "maps" "net/http" "strings" + "time" "github.com/LINBIT/golinstor/client" "github.com/LINBIT/golinstor/clonestatus" + "github.com/cockroachdb/errors" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/store" + "sigs.k8s.io/controller-runtime/pkg/log" ) // rdCloneRequest is the body for `resource-definition clone`. Only the @@ -230,13 +233,17 @@ func (s *Server) cloneWithData(w http.ResponseWriter, r *http.Request, src *apiv // operation with no follow-up autoplace, so the clone replicas must // materialise on the snapshot-holding nodes in the source pool here // (same backend by construction — Bug 038). - _, err := s.materializeRestoredRD(ctx, src.Name, restoreReq, snap, true) + made, err := s.materializeRestoredRD(ctx, src.Name, restoreReq, snap, true) if err != nil { - writeCloneRefused(w, http.StatusInternalServerError, src.Name, req.Name, &apiv1.APICallRc{ - RetCode: apiCallRcError, - Message: "clone of resource definition '" + src.Name + "' failed: " + err.Error(), - }) + writeCloneRefused(w, http.StatusInternalServerError, src.Name, req.Name, + s.failedMaterialiseRefusal(ctx, "clone of resource definition '"+src.Name+"' failed: "+err.Error(), + "clone", req.Name, made.Placed, err)) + + return + } + uncheckedRG, ok := s.cloneParentRGSurvived(ctx, w, src, req.Name, made) + if !ok { return } @@ -251,15 +258,113 @@ func (s *Server) cloneWithData(w http.ResponseWriter, r *http.Request, src *apiv return } + writeCloneStarted(w, src.Name, req.Name, "resource definition cloned: "+req.Name, uncheckedRG) +} + +// correcRecreateGroupThenClone is the one wording both rollback doors on this +// path give the operator. +const correcRecreateGroupThenClone = "re-create the resource group, then clone again" + +// writeCloneStarted emits the envelope golinstor's Clone decoder expects, with +// any warning the post-write checks want to ride back alongside the result. +func writeCloneStarted(w http.ResponseWriter, srcName, cloneName, message string, warn *apiv1.APICallRc) { + messages := []apiv1.APICallRc{{ + RetCode: maskInfo, + Message: message, + }} + + if warn != nil { + messages = append(messages, *warn) + } + writeJSON(w, http.StatusCreated, cloneStartedResponse{ - Location: "/v1/resource-definitions/" + src.Name + "/clone/" + req.Name, - SourceName: src.Name, - CloneName: req.Name, - Messages: &[]apiv1.APICallRc{{ - RetCode: maskInfo, - Message: "resource definition cloned: " + req.Name, - }}, + Location: "/v1/resource-definitions/" + srcName + "/clone/" + cloneName, + SourceName: srcName, + CloneName: cloneName, + Messages: &messages, + }) +} + +// cloneParentRGSurvived is the post-write half of the Bug 174 guard on the +// clone path: the target inherits the source's resource group, and a `rg d` +// that lands between the check the create did and the definition this wrote +// leaves the clone parented to a group that is gone. False means the clone has +// been rolled back and a refusal written. +func (s *Server) cloneParentRGSurvived( + ctx context.Context, w http.ResponseWriter, + src *apiv1.ResourceDefinition, cloneName string, made materialisedRD, +) (*apiv1.APICallRc, bool) { + stampedRG := made.StampedRG + + ctx, cancel := detachedCompensation(ctx) + defer cancel() + + survived, err := s.parentRGSurvived(ctx, stampedRG) + if err != nil { + // The check failed, not the clone. See restoreParentRGSurvived for + // why an inconclusive safety net must not undo work that succeeded — + // and why the caller is told it went unverified rather than left to + // find out from an apiserver log. + log.FromContext(ctx).Info("could not re-check the clone's parent group", + "resourceDefinition", cloneName, "resourceGroup", stampedRG, "reason", err.Error()) + + return uncheckedCloneGroupWarning(cloneName, stampedRG, err), true + } + + if survived { + return nil, true + } + + if !made.createdHere() { + writeCloneRefused(w, http.StatusConflict, src.Name, cloneName, + adoptedOverDeletedGroupRefusal("clone", cloneName, stampedRG, correcRecreateGroupThenClone)) + + return nil, false + } + + rollbackErr := s.rollBackCompensating(ctx, cloneName, made.Placed) + if rollbackErr != nil { + cause, correc := rollbackFailureAdvice(rollbackErr, cloneName) + + writeCloneRefused(w, http.StatusInternalServerError, src.Name, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone of resource definition '" + src.Name + "': " + + rollbackFailedMessage(cloneName, stampedRG, rollbackErr), + Cause: cause, + Correc: correc, + }) + + return nil, false + } + + writeCloneRefused(w, http.StatusNotFound, src.Name, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone of resource definition '" + src.Name + "' rolled back: " + + rgDeletedRaceCorrection(stampedRG), + Cause: "the clone inherits its parent group from the source, and that group was " + + "deleted while the clone was being materialised; a definition pointing at a " + + "group that is gone lists fine and places badly", + Correc: correcRecreateGroupThenClone, }) + + return nil, false +} + +// uncheckedCloneGroupWarning is what both halves of the clone's post-write +// group check ride back when the check itself could not be made. +func uncheckedCloneGroupWarning(cloneName, rgName string, err error) *apiv1.APICallRc { + return &apiv1.APICallRc{ + RetCode: maskWarn, + Message: "resource group '" + rgName + "' could not be re-checked after the " + + "clone: " + err.Error(), + Cause: "the clone itself succeeded; only the safety net over it could not be " + + "inspected, so a group deleted during the clone would not have been caught", + Correc: "confirm resource group '" + rgName + "' still exists", + ObjRefs: map[string]string{ + objRefRscDfn: cloneName, + objRefRscGrp: rgName, + }, + } } // cloneSnapshotName derives the internal snapshot name backing a @@ -291,6 +396,10 @@ func (s *Server) cloneTargetPreexists(ctx context.Context, w http.ResponseWriter } if existing.Props["BlockstorRestoreFromSnapshot"] == srcName+":"+cloneSnapshotName(cloneName) { + if !s.cloneLeftoverIsUsable(ctx, w, srcName, cloneName, &existing) { + return true + } + writeJSON(w, http.StatusCreated, cloneStartedResponse{ Location: "/v1/resource-definitions/" + srcName + "/clone/" + cloneName, SourceName: srcName, @@ -313,6 +422,359 @@ func (s *Server) cloneTargetPreexists(ctx context.Context, w http.ResponseWriter return true } +// cloneLeftover is what a replay found under a marker-bearing definition. +type cloneLeftover int + +const ( + // cloneLeftoverWhole holds volumes and at least one replica that is not + // being deleted: the least a replay may answer 201 over. + cloneLeftoverWhole cloneLeftover = iota + // cloneLeftoverTearingDown still lists replicas, but every one of them is + // already accepted for deletion. + cloneLeftoverTearingDown + // cloneLeftoverEmpty has neither volumes nor replicas. + cloneLeftoverEmpty + // cloneLeftoverNoVolumes has replicas and no volumes. + cloneLeftoverNoVolumes + // cloneLeftoverNoReplicas has volumes and no replica at all. + cloneLeftoverNoReplicas +) + +// assessCloneLeftover reads a marker-bearing definition until it is whole or +// the cache-retry budget the parent-group read carries is spent. +// +// Both reads are informer-cache served, and the informer that matched the +// marker is not the one answering the replica listing, so a definition seen +// with its replicas not yet seen is the ordinary skew that budget exists for. +// Deciding on the first read told a complete clone to delete itself. A NotFound +// counts as "not seen yet" for the same reason; any other read error is +// returned at once, and the caller refuses on it. +func (s *Server) assessCloneLeftover(ctx context.Context, cloneName string) (cloneLeftover, error) { + var state cloneLeftover + + for attempt := range cacheRetryAttempts { + var err error + + state, err = s.readCloneLeftover(ctx, cloneName) + if err != nil || state == cloneLeftoverWhole || attempt == cacheRetryAttempts-1 { + return state, err + } + + select { + case <-ctx.Done(): + return state, errors.Wrapf(ctx.Err(), "re-read the leftover %q", cloneName) + case <-time.After(cacheRetryDelay): + } + } + + return state, nil +} + +// readCloneLeftover is one read of a marker-bearing definition's volumes and +// replicas. A replica stamped for deletion is not counted as holding the +// clone: its satellite finalizer may keep it listed for as long as the owning +// node is down, and a replay answered 201 over it binds a volume to a clone +// that is going away. +func (s *Server) readCloneLeftover(ctx context.Context, cloneName string) (cloneLeftover, error) { + vds, err := s.Store.VolumeDefinitions().List(ctx, cloneName) + if err != nil && !errors.Is(err, store.ErrNotFound) { + return cloneLeftoverEmpty, errors.Wrapf(err, "list the volumes of %q", cloneName) + } + + replicas, err := s.Store.Resources().ListByDefinition(ctx, cloneName) + if err != nil && !errors.Is(err, store.ErrNotFound) { + return cloneLeftoverEmpty, errors.Wrapf(err, "list the replicas of %q", cloneName) + } + + live := 0 + + for i := range replicas { + if !replicaAcceptedForDeletion(&replicas[i]) { + live++ + } + } + + switch { + case len(vds) > 0 && live > 0: + return cloneLeftoverWhole, nil + case len(replicas) > 0 && live == 0: + return cloneLeftoverTearingDown, nil + case len(vds) == 0 && len(replicas) == 0: + return cloneLeftoverEmpty, nil + case len(vds) == 0: + return cloneLeftoverNoVolumes, nil + default: + return cloneLeftoverNoReplicas, nil + } +} + +// cloneShellParentRGSurvived is the vol-less half of the same guard. +// +// handleRDClone splits on the source's volume count. The branch above copies a +// bare definition, carrying the source's resource group over verbatim, and had +// no check on either side of its write — so a `rg d` landing while it runs +// leaves exactly the definition the guard exists to prevent, on the cheaper of +// the two branches. +// +// The compensation is the one both other post-write doors run, on the same +// detached context. What this branch created is a bare definition, with no +// volumes hydrated and no replicas stamped, so a single Delete would undo what +// it wrote — but the rollback is needed exactly when the caller is already +// gone, and a Delete on the request's own context fails on its first call +// then. The shared rollback earns its other steps by not assuming the +// definition is bare, which is what an auto-tiebreaker stamped underneath it +// in the meantime would make false. +func (s *Server) cloneShellParentRGSurvived( + ctx context.Context, w http.ResponseWriter, srcName, cloneName, stampedRG string, +) (*apiv1.APICallRc, bool) { + if stampedRG == "" { + return nil, true + } + + ctx, cancel := detachedCompensation(ctx) + defer cancel() + + survived, err := s.parentRGSurvived(ctx, stampedRG) + if err != nil { + // The check failed, not the clone. Same stance as the data-plane + // half, and the same report: an inconclusive safety net must not + // undo work that succeeded, and the caller is told it went + // unverified rather than left to find out from an apiserver log. + log.FromContext(ctx).Info("could not re-check the cloned shell's parent group", + "resourceDefinition", cloneName, "resourceGroup", stampedRG, "reason", err.Error()) + + return uncheckedCloneGroupWarning(cloneName, stampedRG, err), true + } + + if survived { + return nil, true + } + + // Checked, unlike refuseRDCreateOnRGDeletedRace: the 404 below tells the + // caller the clone was rolled back, and a delete that failed leaves the + // shell exactly where it was, parented to a group that is gone. The data + // path refuses to make that claim over a failed compensation, and the two + // halves of one guard should not answer the same question differently. + err = s.rollBackCompensating(ctx, cloneName, nil) + if err != nil { + // The advice the shared rollback's other doors give, for the step + // that failed: a snapshot on the shell makes "delete it by hand" a + // dead end, since `rd d` refuses a definition that has snapshots. + cause, correc := rollbackFailureAdvice(err, cloneName) + + writeCloneRefused(w, http.StatusInternalServerError, srcName, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone of resource definition '" + srcName + "': " + + rollbackFailedMessage(cloneName, stampedRG, err), + Cause: cause, + Correc: correc, + }) + + return nil, false + } + + writeCloneRefused(w, http.StatusNotFound, srcName, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone of resource definition '" + srcName + "' rolled back: " + + rgDeletedRaceCorrection(stampedRG), + Cause: "the clone inherits its parent group from the source, and that group was " + + "deleted while the clone was being created; a definition pointing at a group " + + "that is gone lists fine and places badly", + Correc: correcRecreateGroupThenClone, + }) + + return nil, false +} + +// cloneLeftoverIsUsable decides whether a definition that carries this clone's +// marker may be answered as an idempotent replay. +// +// The marker is stamped at RD-create, before the volumes are hydrated and +// before any replica exists, so it says "an attempt at this clone got this +// far" and nothing more. When a rollback fails, the definition is deliberately +// kept and the operator is told to delete it — but the CSI target name is +// deterministic and linstor-csi retries CreateVolume on any error, so the very +// next call meets that leftover, matches the marker and is answered 201 +// "already cloned" for a definition still parented to a group that is gone. +// The advice in the 500 never reaches a human, because the machine turns the +// failure into a success first. +// +// So a replay is answered 201 only over a leftover that is whole and whose +// parent group still resolves. An inconclusive read of either refuses: unlike +// the post-write check, which guards a clone this request has just made, this +// gate would otherwise report a clone nobody verified, and the CSI retry makes +// a refusal cheap. +func (s *Server) cloneLeftoverIsUsable( + ctx context.Context, w http.ResponseWriter, srcName, cloneName string, existing *apiv1.ResourceDefinition, +) bool { + stampedRG := existing.ResourceGroupName + // The group resolving is not enough on its own. A failed rollback may + // have reaped every replica and still kept the definition, and both of + // this guard's corrections tell the operator to re-create the group — + // so the moment they do, a group-only gate answers 201 for a clone that + // exists on no node. The leftover has to be whole. + state, err := s.assessCloneLeftover(ctx, cloneName) + if err != nil { + writeCloneRefused(w, http.StatusInternalServerError, srcName, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone target '" + cloneName + "' exists, but reading it back failed: " + err.Error(), + Cause: "without its volumes and replicas the replay cannot tell a finished clone " + + "from an unfinished one, and answering 201 over the second binds a volume to " + + "a clone that may exist on no node", + Correc: "retry the clone", + }) + + return false + } + + if state != cloneLeftoverWhole { + writeCloneRefused(w, http.StatusConflict, srcName, cloneName, cloneLeftoverRefusal(cloneName, state)) + + return false + } + + if stampedRG == "" { + return !s.cloneRollbackWasAbandoned(ctx, w, srcName, cloneName) + } + + // An unreadable group is refused rather than waved through. Refusing + // costs nothing here, since the CSI retry is self-healing, while a false + // 201 binds a PV to a definition parented to nothing. + // + // But it is refused as unreadable, not as deleted. The two readings send + // the operator to opposite actions: "the group is gone, delete the clone" + // over an apiserver blip destroys a working volume for a reason that is + // not true, where the honest answer is to try again. + survived, err := s.parentRGSurvived(ctx, stampedRG) + if err != nil { + writeCloneRefused(w, http.StatusInternalServerError, srcName, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone target '" + cloneName + "' exists, but its parent resource group '" + + stampedRG + "' could not be read: " + err.Error(), + Cause: "the replay only answers for a clone whose parent group resolves, and this " + + "read failed rather than saying the group is gone", + Correc: "retry the clone", + }) + + return false + } + + if survived { + return !s.cloneRollbackWasAbandoned(ctx, w, srcName, cloneName) + } + + writeCloneRefused(w, http.StatusConflict, srcName, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone target '" + cloneName + "' exists but is parented to resource group '" + + stampedRG + "', which no longer exists", + Cause: "an earlier attempt at this clone could not be rolled back after its parent " + + "group was deleted, so the definition was left in place rather than orphaning " + + "its replicas; answering this retry as an idempotent replay would report a " + + "clone that is not usable", + Correc: "delete '" + cloneName + "' by hand, re-create resource group '" + stampedRG + + "', then clone again", + }) + + return false +} + +// cloneRollbackWasAbandoned refuses, as the last word before a replay would +// answer 201, a leftover whose rollback gave up. True means the refusal has +// been written. +// +// It comes last on purpose. Every earlier refusal is more precise about the +// same leftover (still being torn down, parented to a group that is gone), and +// this one only has to catch what they all let through: a leftover that looks +// whole because the placement the rollback stopped in the middle of left a +// live replica and the volumes, when the clone intended more replicas than +// that. The rollback is the one party that knows; see rollbackAbandonedKey. +// +// The definition is read again here, from the API server. The props the gate +// started from are one cache-served read taken before the two gates ahead of +// this one waited out their own cache lag, and the mark is written through the +// API server by a rollback that may have given up moments before this replay +// arrived, which is exactly when the cache has not caught up with it. A read +// that fails refuses: the replay cannot vouch for a clone it could not check. +func (s *Server) cloneRollbackWasAbandoned( + ctx context.Context, w http.ResponseWriter, srcName, cloneName string, +) bool { + existing, err := s.Store.ResourceDefinitions().GetUncached(ctx, cloneName) + if err != nil { + writeCloneRefused(w, http.StatusInternalServerError, srcName, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone target '" + cloneName + "' exists, but reading it back to check " + + "for an abandoned rollback failed: " + err.Error(), + Cause: "an earlier attempt whose rollback gave up leaves a definition that looks " + + "whole, and only the mark it carries tells the two apart", + Correc: "retry the clone", + }) + + return true + } + + spelled := existing.Props[rollbackAbandonedKey] + if spelled == "" { + return false + } + + step, known := rollbackStepByName(spelled) + cause, correc := rollbackStepAdvice(step, known, cloneName) + + writeCloneRefused(w, http.StatusConflict, srcName, cloneName, &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone target '" + cloneName + "' is what an earlier attempt left when its " + + "rollback gave up (" + spelled + ")", + Cause: "an earlier attempt at this clone failed and could not be rolled back, so the " + + "definition may hold less than the clone intended; " + cause, + Correc: correc, + }) + + return true +} + +// cloneLeftoverRefusal words the refusal for a leftover that is not whole, +// naming what is missing. +// +// None of these states proves the attempt behind it is dead. The marker lands +// before the volumes and the volumes before the replicas, so a first attempt +// still running looks exactly like an attempt that stopped, and the correction +// has to hold for both: an operator reading "delete it" must not be pointed at +// a clone that is about to finish. +func cloneLeftoverRefusal(cloneName string, state cloneLeftover) *apiv1.APICallRc { + const replayWouldLie = "; answering this retry as an idempotent replay would bind a volume " + + "to a clone that exists on no node" + + correcIfStopped := "if no clone under that name is still running, delete '" + cloneName + + "' by hand, then clone again" + + refusal := &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: "clone target '" + cloneName + "' exists but is not a whole clone", + Correc: correcIfStopped, + } + + switch state { + case cloneLeftoverTearingDown: + refusal.Message = "clone target '" + cloneName + "' exists but is still being torn down" + refusal.Cause = "the definition under that name carries this clone's marker, and every " + + "replica of it is already accepted for deletion" + replayWouldLie + refusal.Correc = "wait until the replicas of '" + cloneName + "' are gone, then clone again" + case cloneLeftoverEmpty: + refusal.Cause = "the definition under that name carries this clone's marker but has no " + + "volumes and no replicas: an attempt at this clone stopped before creating any " + + "volume, or has not reached that step yet" + replayWouldLie + case cloneLeftoverNoVolumes: + refusal.Cause = "the definition under that name carries this clone's marker and has " + + "replicas but no volumes" + replayWouldLie + case cloneLeftoverNoReplicas, cloneLeftoverWhole: + refusal.Cause = "the definition under that name carries this clone's marker and has " + + "volumes but no replicas: an attempt at this clone has not placed any yet, or a " + + "rollback removed them and could not remove the definition" + replayWouldLie + } + + return refusal +} + // 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. @@ -513,7 +975,7 @@ func (s *Server) cloneEmptyRDShell(w http.ResponseWriter, r *http.Request, if src.Props != nil || len(req.OverrideProps) > 0 { clone.Props = make(map[string]string, len(src.Props)+len(req.OverrideProps)) - maps.Copy(clone.Props, src.Props) + maps.Copy(clone.Props, store.TravellingProps(src.Props)) } maps.Copy(clone.Props, req.OverrideProps) @@ -529,6 +991,11 @@ func (s *Server) cloneEmptyRDShell(w http.ResponseWriter, r *http.Request, return } + uncheckedRG, ok := s.cloneShellParentRGSurvived(r.Context(), w, src.Name, clone.Name, clone.ResourceGroupName) + if !ok { + return + } + // golinstor's ResourceDefinitionService.Clone decodes into // `ResourceDefinitionCloneStarted` (an object), NOT // `[]ApiCallRc`. Returning the bare ApiCallRc array breaks the @@ -536,15 +1003,7 @@ func (s *Server) cloneEmptyRDShell(w http.ResponseWriter, r *http.Request, // client.ResourceDefinitionCloneStarted" — surfaced as a // CSI CreateVolume-from-source failure in csi-sanity. Emit the // envelope shape upstream specifies. - writeJSON(w, http.StatusCreated, cloneStartedResponse{ - Location: "/v1/resource-definitions/" + src.Name + "/clone/" + clone.Name, - SourceName: src.Name, - CloneName: clone.Name, - Messages: &[]apiv1.APICallRc{{ - RetCode: maskInfo, - Message: "resource definition cloned: " + clone.Name, - }}, - }) + writeCloneStarted(w, src.Name, clone.Name, "resource definition cloned: "+clone.Name, uncheckedRG) } // handleRDCloneStatus answers golinstor's `CloneStatus` poll. The diff --git a/pkg/rest/rg_deleted_race.go b/pkg/rest/rg_deleted_race.go new file mode 100644 index 00000000..7a99fc3a --- /dev/null +++ b/pkg/rest/rg_deleted_race.go @@ -0,0 +1,674 @@ +// 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" + "fmt" + "slices" + "strings" + "time" + + "github.com/cockroachdb/errors" + "sigs.k8s.io/controller-runtime/pkg/log" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// parentRGSurvived re-reads the resource group a freshly materialised +// definition was parented to, and reports whether it is still there. +// +// This is the post-write half of the Bug 174 guard. `POST +// /v1/resource-definitions` runs it twice — once before the write and once +// after, rolling the definition back when a concurrent `rg d` won the race — +// because a definition left pointing at a group that no longer exists lists +// fine and places badly: the placer's Controller→RG→RD prop-inheritance walk +// drops the RG tier without a word, taking auto-place, auto-diskful, +// place_count observability and rebalance scheduling with it. +// +// Clone and snapshot-restore create definitions the same way and inherit the +// group the same way, and had NEITHER half: refuseRDCreateOnRGDeletedRace has +// exactly one caller, and neither rd_clone.go nor snapshot_restore.go read +// ResourceGroups() at all. +// +// They are not the whole class. `rg spawn` (spawnCreate) also creates a +// definition parented to a group, on the path linstor-csi takes for every +// ordinary CreateVolume, and has neither half either. It is left out of this +// change on purpose: the doors here are the ones this fix set out to close, +// and spawn's compensation is a different shape (it rolls back through +// rollbackSpawn, not rollBackMaterialisedRD), so it needs its own change +// rather than a line added here. +// +// That distinction reaches the operator. A refusal derived from this check +// that always blames a concurrent delete sends someone whose group never +// existed — from adoption, or from data that predates Bug 134 — hunting a race +// that never happened, so the wording covers both. +// +// The read carries the standard cache-retry budget for the same reason +// refuseRDCreateOnRGDeletedRace does: on the CreateVolume hot path the group +// may have been created moments ago and the informer cache may still trail +// it, and mistaking that lag for a delete race would roll back a perfectly +// good clone (see pkg/rest/cache_retry.go). A real `rg d` still trips it once +// the budget is spent. +func (s *Server) parentRGSurvived(ctx context.Context, rgName string) (bool, error) { + if rgName == "" { + return true, nil + } + + _, err := getRGWithCacheRetry(ctx, s.Store, rgName) + if err == nil { + return true, nil + } + + if errors.Is(err, store.ErrNotFound) { + return false, nil + } + + return false, err +} + +// detachedRollbackBudget bounds the compensation a post-write door runs after +// the request it belongs to may have ended: the re-read of the parent group +// that decides whether to roll back, and the rollback itself. +// +// It is the first term of one chain, and every term below it is derived so the +// process outlives a compensation it started: +// +// groupRecheckBudget 0.6s parentRGSurvived's cache-retry +// + 2 * cacheConvergeBudget 10s the rollback's two convergence waits +// + rollbackWriteBudget 2s the rollback's own writes +// + 2 * markWriteBudget 2s the abandoned-rollback mark, before and after +// = detachedRollbackBudget 14.6s +// + shutdownMargin 2s the rest of a graceful shutdown +// = gracefulShutdownWindow 16.6s how long Shutdown waits for in-flight handlers +// + terminationGraceMargin 3s +// <= terminationGracePeriodSeconds in every manifest that serves REST +// +// The compensation runs inside the handler on a context Shutdown cannot +// cancel, so a window shorter than the budget means a SIGTERM during a rolling +// restart cuts a cascade in half and kills the connection that would have said +// so, which is the state WithoutCancel was added to prevent one failure mode +// over. Cutting the budget instead is not the trade: its two waits are what +// keep the definition from going over replicas that were never stamped. +// +// TestRollbackBudgetFitsTheShutdownWindow holds the chain, manifests included. +const ( + groupRecheckBudget = cacheRetryAttempts * cacheRetryDelay + rollbackWriteBudget = 2 * time.Second + markWriteBudget = time.Second + detachedRollbackBudget = groupRecheckBudget + 2*cacheConvergeBudget + rollbackWriteBudget + 2*markWriteBudget + shutdownMargin = 2 * time.Second + terminationGraceMargin = 3 * time.Second +) + +// detachedCompensation is the context a compensation runs on: the request +// cannot end it, and detachedRollbackBudget bounds it. +// +// A post-write door takes one for the group re-read as well as for the +// rollback, because the read is what decides whether to roll back. On the +// request's context a caller that has gone, or a SIGTERM (the server hands +// every request the runnable's own context as its base), turns that read into +// a cancelled one, which parentRGSurvived can only report as "could not +// check", and the door then answers success over a group that is gone. A +// cancelled caller is not an inconclusive answer about the group. +func detachedCompensation(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(context.WithoutCancel(ctx), detachedRollbackBudget) +} + +// rollBackCompensating runs rollBackMaterialisedRD on ctx, which has to be a +// detachedCompensation context already: every door detaches exactly once and +// hands the rollback what is left of that one budget. Detaching again here +// would restart it, because WithoutCancel drops the deadline along with the +// cancellation, and the chain the manifests' termination grace is derived +// from would then be the re-read plus a whole second budget. +// +// Every compensation on these paths is most likely to be needed when the +// caller has already gone: a CSI caller times out mid-clone, and the RG-deleted +// rollback itself waits out two cache-convergence budgets. A compensation that +// inherits the request's context fails on its first call once that happens and +// leaves exactly what it exists to remove, a definition every later retry is +// refused over until an operator deletes it. +// +// The abandoned-rollback mark is written before anything is touched, and a +// rollback that completes takes it away with the definition. Written after a +// failure instead, it could not land in the two cases that leave a genuinely +// half-torn leftover: the budget running out mid-cascade leaves no context to +// write it on, and a killed process runs nothing at all. A failure then only +// refines the mark to the step it stopped at, best-effort. +func (s *Server) rollBackCompensating(ctx context.Context, rdName string, placed []string) error { + s.markRollbackAbandoned(ctx, rdName, rollbackInProgress) + + err := s.rollBackMaterialisedRD(ctx, rdName, placed) + if err != nil { + s.markRollbackAbandoned(ctx, rdName, rollbackStepName(err)) + } + + return err +} + +// rollbackInProgress is the mark a rollback carries until it either completes, +// taking the definition and the mark with it, or names the step it stopped at. +// Read back, it is a rollback that never reported how it ended. +const rollbackInProgress = "in-progress" + +// rollbackAbandonedKey marks a definition whose compensation gave up, with +// the step it stopped at. +// +// The replay gate needs it. A rollback that stops after placement succeeded +// on one node and failed on another leaves a definition holding fewer +// replicas than the operation intended, and the 500 advising a manual delete +// goes to a caller that retries under the same deterministic name long before +// anyone reads it: the retry finds volumes and a live replica and would answer +// 201 over it. Comparing the replicas with the snapshot's nodes cannot tell +// that apart from a finished clone that was evacuated or scaled down since, +// and the rollback is the one party that knows it gave up. +const rollbackAbandonedKey = store.RollbackAbandonedProp + +// markRollbackAbandoned records on the definition that its rollback started, +// or where it stopped. Best-effort: a mark that does not land leaves the +// replay gate where it was before the mark existed. +// +// Each write gets markWriteBudget of its own and is not waited on past it: a +// second covers a get and a patch against a loaded API server, where a fraction +// of one would leave the first mark missing exactly when the cascade after it +// is cut short. +// The patch retries on conflict with a backoff that sleeps without a context, +// sized for heavy contention, so under a reconciler bumping the definition's +// resourceVersion a mark could otherwise spend the convergence waits the +// cascade after it is budgeted for. A write abandoned at its deadline cannot +// land later: every call it would still make runs on the expired context. +func (s *Server) markRollbackAbandoned(ctx context.Context, rdName, step string) { + markCtx, cancel := context.WithTimeout(ctx, markWriteBudget) + defer cancel() + + done := make(chan error, 1) + + 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 + + return nil + }) + }() + + var err error + + select { + case err = <-done: + case <-markCtx.Done(): + err = markCtx.Err() + } + + switch { + case err == nil: + case errors.Is(err, store.ErrNotFound): + // The definition is gone, so there is nothing left to mark. Kept + // at V(1) so a mark that did not land can still be told apart + // from one that was never needed. + log.FromContext(ctx).V(1).Info("no definition to mark an abandoned rollback on", + "resourceDefinition", rdName, "step", step) + default: + log.FromContext(ctx).Info("could not mark an abandoned rollback on its definition", + "resourceDefinition", rdName, "step", step, "reason", err.Error()) + } +} + +// failedMaterialiseRefusal rolls back what a failed materialisation left, when +// that is this request's own partial work, and words the refusal. noun names +// the operation for the operator ("clone", "restore"). +// +// The marker is stamped at RD-create, so a failure after the create leaves a +// definition every retry matches, and nothing but an operator would ever remove +// it. Only a failure materializeRestoredRD reports as after its own create is +// rolled back; any other leaves whatever was there before the call, which may +// belong to another attempt that is still running, and is never touched. +func (s *Server) failedMaterialiseRefusal( + ctx context.Context, message, noun, rdName string, placed []string, err error, +) *apiv1.APICallRc { + var partial *materialiseAfterCreateError + if !errors.As(err, &partial) { + return &apiv1.APICallRc{RetCode: apiCallRcError, Message: message} + } + + rollbackCtx, cancel := detachedCompensation(ctx) + defer cancel() + + rollbackErr := s.rollBackCompensating(rollbackCtx, rdName, placed) + if rollbackErr != nil { + cause, correc := rollbackFailureAdvice(rollbackErr, rdName) + + return &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: message + "; rolling the partial " + noun + " back failed too: " + rollbackErr.Error() + + "; '" + rdName + "' is still there", + Cause: cause, + Correc: correc, + } + } + + return &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: message + "; the partial " + noun + " '" + rdName + "' was rolled back", + Correc: "retry the " + noun, + } +} + +// adoptedOverDeletedGroupRefusal is the RG-deleted refusal over a definition +// this request did not create. It is left in place: the leftover belongs to an +// earlier attempt at the same operation, which may still be running, and +// reaping it would delete that attempt's work rather than this one's. +func adoptedOverDeletedGroupRefusal(noun, rdName, rgName, correc string) *apiv1.APICallRc { + return &apiv1.APICallRc{ + RetCode: apiCallRcError, + Message: noun + " target '" + rdName + "' is parented to resource group '" + rgName + + "', which no longer exists", + Cause: "the definition was not created by this request, so it is left in place " + + "rather than rolled back", + Correc: correc, + } +} + +// correcRecreateGroupThenRestore is the restore door's twin of +// correcRecreateGroupThenClone. +const correcRecreateGroupThenRestore = "re-create the resource group, then restore again" + +// errReplicasNotStamped is the rollback's own refusal: replicas that are still +// there and were never accepted for deletion, which is the one shape the +// parent must not be dropped over. +var errReplicasNotStamped = errors.New("replica(s) were not accepted for deletion") + +// errSnapshotsOnTarget refuses to drop a definition that carries snapshots, +// the refusal handleRDDelete makes before the sweep it shares with this path. +var errSnapshotsOnTarget = errors.New("snapshot(s) exist on the definition") + +// rollbackStep names where the compensation stopped, because the operator is +// pointed at a different object in each case: a replica that would not go, a +// read that could not confirm, a snapshot that is somebody's data, or a +// definition whose own delete failed after everything under it went. +type rollbackStep int + +const ( + rollbackStepReapReplicas rollbackStep = iota + rollbackStepRereadReplicas + rollbackStepSnapshots + rollbackStepDeleteDefinition + rollbackStepReadSnapshots +) + +type rollbackStepError struct { + step rollbackStep + err error +} + +func (f *rollbackStepError) Error() string { return f.err.Error() } + +func (f *rollbackStepError) Unwrap() error { return f.err } + +func newRollbackError(step rollbackStep, err error) error { + return &rollbackStepError{step: step, err: err} +} + +// rollbackStepNames spells each step for the abandoned-rollback mark. +var rollbackStepNames = map[rollbackStep]string{ //nolint:gochecknoglobals // a fixed table, read-only + rollbackStepReapReplicas: "reap-replicas", + rollbackStepRereadReplicas: "reread-replicas", + rollbackStepSnapshots: "snapshots", + rollbackStepDeleteDefinition: "delete-definition", + rollbackStepReadSnapshots: "read-snapshots", +} + +// rollbackStepName spells the step a compensation stopped at, or "unknown". +func rollbackStepName(err error) string { + var failure *rollbackStepError + if errors.As(err, &failure) { + return rollbackStepNames[failure.step] + } + + return "unknown" +} + +// rollbackStepByName reads a step back from its spelling. +func rollbackStepByName(name string) (rollbackStep, bool) { + for step, spelled := range rollbackStepNames { + if spelled == name { + return step, true + } + } + + return rollbackStepReapReplicas, false +} + +// rollbackFailureAdvice is the Cause and Correc for a failed compensation, +// written for the step that failed rather than once for all of them. +func rollbackFailureAdvice(err error, rdName string) (string, string) { + var failure *rollbackStepError + if !errors.As(err, &failure) { + return rollbackStepAdvice(rollbackStepReapReplicas, false, rdName) + } + + return rollbackStepAdvice(failure.step, true, rdName) +} + +// rollbackStepAdvice is rollbackFailureAdvice for a step already known, which +// is what the replay gate has when it reads an abandoned-rollback mark. +func rollbackStepAdvice(step rollbackStep, known bool, rdName string) (string, string) { + if !known { + return "the compensation could not complete", "delete '" + rdName + "' by hand" + } + + switch step { + case rollbackStepRereadReplicas: + return "the replicas were told to go, but reading them back to confirm failed, " + + "so the definition was left in place rather than dropped over replicas " + + "nobody could see", + "check the replicas of '" + rdName + "', then delete it by hand" + case rollbackStepSnapshots: + return "a snapshot exists on the definition, and the rollback does not destroy " + + "a snapshot the way `rd d` refuses to", + "delete the snapshot(s) of '" + rdName + "' if they are not needed, then delete '" + + rdName + "' by hand" + case rollbackStepReadSnapshots: + return "the snapshots of the definition could not be read, and the rollback does " + + "not delete a definition it cannot show has none", + "check whether '" + rdName + "' has snapshots (`linstor s l`), delete any that are " + + "not needed, then delete '" + rdName + "' by hand" + case rollbackStepDeleteDefinition: + return "every replica went, but deleting the definition itself failed", + "delete '" + rdName + "' by hand" + case rollbackStepReapReplicas: + } + + return "the replicas could not all be reaped, so the definition was left in place " + + "rather than orphaning them", + "delete '" + rdName + "' by hand once the replicas can be removed" +} + +// rollBackMaterialisedRD undoes a clone or restore whose parent group was +// deleted underneath it. +// +// RD-create can compensate with a single Delete because the definition it +// rolls back is bare. These paths cannot: by the time the group can vanish the +// target has volumes hydrated from the snapshot and replicas stamped on the +// nodes that hold it, so the compensation is the cascade `rd d` performs — +// replicas first, then the definition, which carries its inline volumes with +// it. +// +// The internal snapshot a clone took is deliberately left behind. It may be +// the only copy of something, and deleting one is the operator's decision, not +// this endpoint's — the same stance the clone's snapshot reuse takes. +// +// The order is not merely tidy: the definition goes ONLY if the replicas +// went. CascadeDeleteResources stops at the first replica it cannot delete and +// leaves the rest untried, and dropping the parent anyway produces precisely +// the orphan this rollback exists to avoid — a Resource whose RD vanished +// never gets a DeletionTimestamp, so the satellite's finalizer never runs, +// `drbdadm down` never happens, and the DRBD minor, port and peer entries stay +// live on every satellite until the next create with that name collides with +// them. On the CSI path the target name is deterministic, so the retry IS that +// collision. Both other doors that perform this teardown — handleRDDelete and +// the CLI's `rd d` — refuse to proceed on a failed cascade for the same +// reason, and a satellite writing status on the very replicas being reaped +// makes a conflict there an ordinary outcome rather than a rare one. +// +// So this returns an error, and a caller that gets one must not report a +// rollback. What is left behind is a definition parented to a group that is +// gone, which is the state the operator has to be told about, with its name. +func (s *Server) rollBackMaterialisedRD(ctx context.Context, rdName string, placed []string) error { + // Nothing is touched until the snapshot refusal has run, for the reason + // handleRDDelete gives for its own: once the replicas are reaped, a + // refused definition delete leaves the target half torn down, with its + // children going and its parent kept, which no retry reconciles. A refusal + // whose correction is "drop the snapshots and retry" has to arrive while + // there is still something to retry over. + err := s.refuseRollbackOverSnapshots(ctx, rdName) + if err != nil { + return err + } + + // The replicas this request placed are deleted by name first. A write goes + // to the API server whatever the cache has seen, so this is the one step + // that does not depend on a listing having caught up with the placement + // that happened a moment ago. Without it, a cache that has not yet seen + // the stamps lists nothing, the cascade deletes nothing, the check below + // finds nothing stranded, and the definition goes over live replicas that + // will never be stamped — the orphan this rollback exists to prevent. + for _, node := range placed { + err = s.Store.Resources().Delete(ctx, rdName, node) + if err != nil && !errors.Is(err, store.ErrNotFound) { + return newRollbackError(rollbackStepReapReplicas, + errors.Wrapf(err, "delete the replica of %q on %q", rdName, node)) + } + } + + // And the cascade for anything else under the definition: an + // auto-tiebreaker the controller stamped in the meantime is not in + // `placed`. + err = store.CascadeDeleteResources(ctx, s.Store, rdName) + if err != nil { + return newRollbackError(rollbackStepReapReplicas, + errors.Wrapf(err, "cascade the replicas of %q", rdName)) + } + + err = s.waitForReplicasAcceptedForDeletion(ctx, rdName) + if err != nil { + return err + } + + err = s.Store.ResourceDefinitions().Delete(ctx, rdName) + if err != nil && !errors.Is(err, store.ErrNotFound) { + return newRollbackError(rollbackStepDeleteDefinition, + errors.Wrapf(err, "delete %q", rdName)) + } + + // The same two companions handleRDDelete runs after its own delete, for + // the same two reasons. + // + // The convergence wait, because reads here are informer-cache backed and a + // delete lags them: a retry landing inside that window reads the + // pre-delete definition, matches the clone marker and is answered 201 for + // a definition that is genuinely gone — the same false success as + // answering over a leftover, by a different route. + // + // The sweep, because a snapshot create can land between the refusal above + // and the delete, and the row it leaves has no parent to address it. + s.waitForRDDeletionVisible(ctx, rdName) + s.sweepOrphanSnapshotsAfterRDDelete(ctx, rdName) + + return nil +} + +// refuseRollbackOverSnapshots is the refusal handleRDDelete makes before its +// cascade. The sweep that follows the definition delete removes every Snapshot +// row under the definition, which is safe there only because the handler +// refuses outright when snapshots exist, so the sweep can only ever see rows +// that raced in. A snapshot taken on the target inside the rollback window is +// somebody's data, not a race. +func (s *Server) refuseRollbackOverSnapshots(ctx context.Context, rdName string) error { + // A listing that failed is not a snapshot that exists: the operator is + // told which of the two it was, since only one of them points at an + // object to delete. + snaps, err := s.Store.Snapshots().ListByDefinition(ctx, rdName) + if err != nil && !errors.Is(err, store.ErrNotFound) { + return newRollbackError(rollbackStepReadSnapshots, + errors.Wrapf(err, "list the snapshots of %q", rdName)) + } + + if len(snaps) == 0 { + return nil + } + + names := make([]string, 0, len(snaps)) + for i := range snaps { + names = append(names, snaps[i].Name) + } + + return newRollbackError(rollbackStepSnapshots, + fmt.Errorf("%q: %w: %s", rdName, errSnapshotsOnTarget, strings.Join(names, ", "))) +} + +// waitForReplicasAcceptedForDeletion decides whether any replica is stranded +// on a read that has had time to see the deletes this request just issued. +// +// One read is wrong in both directions at the exact moment it would run. The +// resources listing is informer-cache backed and the cascade sleeps nowhere +// across its passes, so the read outruns the cache by construction: a replica +// that was deleted still lists, unstamped, and the rollback refuses over +// replicas that are going, leaving the definition parented to a group that is +// gone with nothing to retry it. So the decision waits, on the same budget the +// RD delete's convergence wait uses, and only a replica still unstamped when +// that budget runs out counts as stranded. +// +// The wait also deletes. A replica can become visible only now: an +// auto-tiebreaker the controller stamps moments after placement is not in +// `placed`, and the cascade's passes run back to back, so it typically +// surfaces during this wait, when nothing else would issue a delete for it. +// Every unstamped replica a read shows is told to go once. Once, because a +// replica the cascade already deleted also lists unstamped while the cache +// trails, and the one extra delete that costs is cheap where one per poll is +// not. +func (s *Server) waitForReplicasAcceptedForDeletion(ctx context.Context, rdName string) error { + deadline := time.Now().Add(cacheConvergeBudget) + told := map[string]struct{}{} + + for { + stranded, err := replicasNotAcceptedForDeletion(ctx, s.Store, rdName) + if err != nil { + return newRollbackError(rollbackStepRereadReplicas, + errors.Wrapf(err, "re-read the replicas of %q", rdName)) + } + + if len(stranded) == 0 { + return nil + } + + err = s.deleteReplicasNotYetTold(ctx, rdName, stranded, told) + if err != nil { + return err + } + + if time.Now().After(deadline) { + return newRollbackError(rollbackStepReapReplicas, + fmt.Errorf("%q: %w: %s", rdName, errReplicasNotStamped, strings.Join(stranded, ", "))) + } + + select { + case <-ctx.Done(): + return newRollbackError(rollbackStepRereadReplicas, + errors.Wrapf(ctx.Err(), "wait for the replicas of %q", rdName)) + case <-time.After(cacheConvergePollInterval): + } + } +} + +// deleteReplicasNotYetTold issues one delete per replica node the wait has not +// already told to go, and records it. +func (s *Server) deleteReplicasNotYetTold( + ctx context.Context, rdName string, nodes []string, told map[string]struct{}, +) error { + for _, node := range nodes { + if _, done := told[node]; done { + continue + } + + told[node] = struct{}{} + + err := s.Store.Resources().Delete(ctx, rdName, node) + if err != nil && !errors.Is(err, store.ErrNotFound) { + return newRollbackError(rollbackStepReapReplicas, + errors.Wrapf(err, "delete the replica of %q on %q", rdName, node)) + } + } + + return nil +} + +// replicasNotAcceptedForDeletion names the replicas that are still there and +// carry no deletion stamp, which is the only shape the parent must not be +// dropped over. +// +// CascadeDeleteResources answers nil in two different situations: every +// replica went, and its pass budget ran out with replicas still listed. That +// is the right contract for `rd d`, whose caller asked for the definition to +// go and who gets a convergence wait behind it. It is the wrong one to build a +// compensation on, and the difference is not an edge case in a cluster: every +// Resource carries the satellite's finalizer, an apiserver DELETE on a +// finalizer-held object is accepted with no error, and the listing does not +// filter what is Terminating — so the ordinary path through the cascade is +// "accepted, still listed", five passes, nil. +// +// A replica already stamped for deletion is not stranded: the stamp is what +// makes the satellite's finalizer run, and it runs whether or not the parent +// outlives it. A replica with no stamp is the orphan this rollback exists to +// avoid, because nothing will ever give it one once the definition is gone. +func replicasNotAcceptedForDeletion(ctx context.Context, st store.Store, rdName string) ([]string, error) { + replicas, err := st.Resources().ListByDefinition(ctx, rdName) + if err != nil { + if errors.Is(err, store.ErrNotFound) { + return nil, nil + } + + return nil, errors.Wrapf(err, "list replicas of %q", rdName) + } + + var stranded []string + + for i := range replicas { + if replicaAcceptedForDeletion(&replicas[i]) { + continue + } + + stranded = append(stranded, replicas[i].NodeName) + } + + return stranded, nil +} + +// replicaAcceptedForDeletion is the one reading of the deletion stamp, shared +// by the rollback, which may drop a parent over a stamped replica, and the +// replay, which may not answer 201 over one. +func replicaAcceptedForDeletion(replica *apiv1.Resource) bool { + return slices.Contains(replica.Flags, apiv1.ResourceFlagDelete) +} + +// rollbackFailedMessage is what the operator is told when the compensation +// could not complete: naming the definition that is still there matters more +// than the refusal itself, because nothing else will name it. +// +// It offers both readings the success path offers. parentRGSurvived cannot +// tell a group deleted while the operation ran from one that was never there, +// which adoption and data predating Bug 134 both produce, and asserting the +// race sends the operator hunting one that may never have happened. +func rollbackFailedMessage(rdName, rgName string, cause error) string { + return "resource group '" + rgName + "' does not exist (it was deleted while the " + + "operation ran, or it was never there) AND rolling '" + rdName + "' back failed: " + + cause.Error() + "; '" + rdName + "' is still there, parented to a group that does not exist" +} + +// rgDeletedRaceCorrection is the one wording for the refusal, so an operator +// reads the same correction whichever endpoint lost the race. +func rgDeletedRaceCorrection(rgName string) string { + return "resource group '" + rgName + "' does not exist — it was deleted while the " + + "operation ran, or it was never there: retry after creating the resource group" +} diff --git a/pkg/rest/rg_deleted_race_round10_props_test.go b/pkg/rest/rg_deleted_race_round10_props_test.go new file mode 100644 index 00000000..d1e6af5d --- /dev/null +++ b/pkg/rest/rg_deleted_race_round10_props_test.go @@ -0,0 +1,182 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "encoding/json" + "net/http" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +func markDefinition(t *testing.T, st store.Store, rdName, step string) { + t.Helper() + + rd, err := st.ResourceDefinitions().Get(t.Context(), rdName) + if err != nil { + t.Fatalf("read %s: %v", rdName, err) + } + + if rd.Props == nil { + rd.Props = map[string]string{} + } + + rd.Props[rollbackAbandonedKey] = step + + if err := st.ResourceDefinitions().Update(t.Context(), &rd); err != nil { + t.Fatalf("mark %s: %v", rdName, err) + } +} + +func assertUnmarked(t *testing.T, props map[string]string, what string) { + t.Helper() + + if step, ok := props[rollbackAbandonedKey]; ok { + t.Errorf("%s carries the abandoned-rollback mark %q it was never given", what, step) + } +} + +// A definition's props travel onto every clone and restore taken from it, and +// the abandoned-rollback mark travelled with them: a healthy clone of a marked +// source was refused on the replay linstor-csi sends. +func TestRDCloneReplayOfACloneOfAMarkedSourceIsStillAReplay(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-marked10", "grp-marked10", true) + markDefinition(t, backend, "src-marked10", "snapshots") + + base, stop := startServerWithStore(t, backend) + defer stop() + + for attempt := 1; attempt <= 2; attempt++ { + resp := postClone(t, base, "src-marked10", map[string]any{"name": "dst-marked10", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("attempt %d = %d, want 201: the clone is whole and its group is live", attempt, resp.StatusCode) + } + } + + clone, err := backend.ResourceDefinitions().Get(t.Context(), "dst-marked10") + if err != nil { + t.Fatalf("read the clone: %v", err) + } + + assertUnmarked(t, clone.Props, "the clone") +} + +// Each place a definition's props are copied onward strips the mark itself. +// The clone path passes through two of them, so each gets a case where it is +// the only one on the way. +func TestTheAbandonedRollbackMarkDoesNotTravel(t *testing.T) { + t.Parallel() + + t.Run("snapshot-of-a-marked-definition", func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedDeployedCloneSource(t, backend, "src-snap10") + markDefinition(t, backend, "src-snap10", "snapshots") + + base, stop := startServerWithStore(t, backend) + defer stop() + + body, _ := json.Marshal(map[string]any{"name": "snap10", "resource_name": "src-snap10"}) + + resp := httpPost(t, base+"/v1/resource-definitions/src-snap10/snapshots", body) + _ = resp.Body.Close() + + snap, err := backend.Snapshots().Get(t.Context(), "src-snap10", "snap10") + if err != nil { + t.Fatalf("snapshot create = %d, read back: %v", resp.StatusCode, err) + } + + assertUnmarked(t, snap.Props, "the snapshot") + }) + + t.Run("restore-from-a-snapshot-carrying-it", func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedDeployedCloneSource(t, backend, "src-rs10") + + // A snapshot taken before the mark stopped travelling. + if err := backend.Snapshots().Create(t.Context(), &apiv1.Snapshot{ + Name: "snap-rs10", ResourceName: "src-rs10", Nodes: []string{"node-a"}, + Props: map[string]string{rollbackAbandonedKey: "snapshots"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 64 * 1024}}, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + restoreInto(t, backend, "src-rs10", "snap-rs10", "dst-rs10") + }) + + t.Run("restore-falling-back-to-the-source-props", func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedDeployedCloneSource(t, backend, "src-rf10") + markDefinition(t, backend, "src-rf10", "snapshots") + + if err := backend.Snapshots().Create(t.Context(), &apiv1.Snapshot{ + Name: "snap-rf10", ResourceName: "src-rf10", Nodes: []string{"node-a"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{{VolumeNumber: 0, SizeKib: 64 * 1024}}, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + restoreInto(t, backend, "src-rf10", "snap-rf10", "dst-rf10") + }) + + t.Run("volume-less-clone", func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + + if err := backend.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: "shell-src10", Props: map[string]string{rollbackAbandonedKey: "snapshots", "Aux/keep": "1"}, + }); err != nil { + t.Fatalf("seed the volume-less source: %v", err) + } + + base, stop := startServerWithStore(t, backend) + defer stop() + + resp := postClone(t, base, "shell-src10", map[string]any{"name": "shell-dst10"}) + _ = resp.Body.Close() + + clone, err := backend.ResourceDefinitions().Get(t.Context(), "shell-dst10") + if err != nil { + t.Fatalf("clone = %d, read back: %v", resp.StatusCode, err) + } + + assertUnmarked(t, clone.Props, "the volume-less clone") + + if clone.Props["Aux/keep"] != "1" { + t.Errorf("the clone lost the source's ordinary props: %v", clone.Props) + } + }) +} + +func restoreInto(t *testing.T, backend store.Store, src, snap, dst string) { + t.Helper() + + base, stop := startServerWithStore(t, backend) + defer stop() + + body, _ := json.Marshal(map[string]any{"to_resource": dst}) + + resp := httpPost(t, base+"/v1/resource-definitions/"+src+"/snapshot-restore-resource/"+snap, body) + _ = resp.Body.Close() + + restored, err := backend.ResourceDefinitions().Get(t.Context(), dst) + if err != nil { + t.Fatalf("restore = %d, read back: %v", resp.StatusCode, err) + } + + assertUnmarked(t, restored.Props, "the restored definition") +} diff --git a/pkg/rest/rg_deleted_race_round10_test.go b/pkg/rest/rg_deleted_race_round10_test.go new file mode 100644 index 00000000..345fa64c --- /dev/null +++ b/pkg/rest/rg_deleted_race_round10_test.go @@ -0,0 +1,201 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// Every door stamps a group, so the grouped branch of the abandoned-rollback +// gate is the mainline one, and the round-9 fixture reached only the +// ungrouped branch. +func TestRDCloneReplayRefusesAGroupedLeftoverWhoseRollbackGaveUp(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedTwoNodeSource(t, backend, "src-half10") + + src, err := backend.ResourceDefinitions().Get(t.Context(), "src-half10") + if err != nil { + t.Fatalf("read the source: %v", err) + } + + src.ResourceGroupName = "grp-half10" + if err := backend.ResourceDefinitions().Update(t.Context(), &src); err != nil { + t.Fatalf("parent the source: %v", err) + } + + if err := backend.ResourceGroups().Create(t.Context(), &apiv1.ResourceGroup{Name: "grp-half10"}); err != nil { + t.Fatalf("seed the group: %v", err) + } + + base, stop := startServerWithStore(t, halfPlacingStore{Store: backend, target: "dst-half10"}) + defer stop() + + first := postClone(t, base, "src-half10", map[string]any{"name": "dst-half10", "use_zfs_clone": true}) + _ = first.Body.Close() + + if first.StatusCode != http.StatusInternalServerError { + t.Fatalf("first attempt = %d, want 500: placement failed and the rollback gave up", first.StatusCode) + } + + leftover, err := backend.ResourceDefinitions().Get(t.Context(), "dst-half10") + if err != nil || leftover.ResourceGroupName != "grp-half10" { + t.Fatalf("fixture: want a grouped leftover, got group %q (err=%v)", leftover.ResourceGroupName, err) + } + + retry := postClone(t, base, "src-half10", map[string]any{"name": "dst-half10", "use_zfs_clone": true}) + defer func() { _ = retry.Body.Close() }() + + if retry.StatusCode == http.StatusCreated { + t.Fatal("the retry answered 201 over a grouped leftover whose rollback gave up half-placed") + } + + if rc := decodeCloneMessage(t, retry); !strings.Contains(rc.Message, "rollback gave up") { + t.Errorf("refusal %q does not say an earlier rollback gave up", rc.Message) + } +} + +// deadlineResources blocks a replica delete until its context ends, the way a +// call to a slow apiserver runs out the budget mid-cascade. +type deadlineResources struct{ store.ResourceStore } + +func (d deadlineResources) Delete(ctx context.Context, _, _ string) error { + <-ctx.Done() + + return ctx.Err() //nolint:wrapcheck // the context's own error is the point +} + +// deadlineRDs refuses a write on a context that has already ended, which the +// Kubernetes store does and the in-memory one does not. +type deadlineRDs struct{ store.ResourceDefinitionStore } + +func (d deadlineRDs) PatchResourceDefinitionSpec( + ctx context.Context, name string, mutate func(*apiv1.ResourceDefinition) error, +) error { + if err := ctx.Err(); err != nil { + return err //nolint:wrapcheck // the context's own error is the point + } + + return d.ResourceDefinitionStore.PatchResourceDefinitionSpec(ctx, name, mutate) //nolint:wrapcheck // pass-through +} + +type deadlineStore struct{ store.Store } + +func (d deadlineStore) Resources() store.ResourceStore { + return deadlineResources{d.Store.Resources()} +} + +func (d deadlineStore) ResourceDefinitions() store.ResourceDefinitionStore { + return deadlineRDs{d.Store.ResourceDefinitions()} +} + +// The mark used to be written after the rollback had failed, on the context +// the rollback had just run out. When the budget expires mid-cascade that +// write cannot land, and neither can anything after a kill, which are exactly +// the leftovers that are half torn down. +func TestAnAbandonedRollbackIsMarkedEvenWhenItsBudgetRunsOut(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, backend, "dst-budget10") + + s := &Server{Store: deadlineStore{backend}} + + rollbackCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 300*time.Millisecond) + defer cancel() + + if err := s.rollBackCompensating(rollbackCtx, "dst-budget10", []string{"node-a"}); err == nil { + t.Fatal("fixture: the rollback was supposed to run out of budget") + } + + leftover, err := backend.ResourceDefinitions().Get(ctx, "dst-budget10") + if err != nil { + t.Fatalf("read the leftover: %v", err) + } + + if leftover.Props[rollbackAbandonedKey] == "" { + t.Fatal("a rollback that ran out of budget mid-cascade left no mark, so the replay would answer 201 over it") + } + + w := httptest.NewRecorder() + if !(&Server{Store: backend}).cloneRollbackWasAbandoned(ctx, w, "src", "dst-budget10") { + t.Error("the replay gate did not refuse over the mark") + } +} + +// deadlineRecorder remembers the deadline of the first context a rollback +// hands the store. +type deadlineRecorder struct { + store.SnapshotStore + + mu *sync.Mutex + seen *time.Time +} + +func (d deadlineRecorder) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + d.mu.Lock() + + if d.seen.IsZero() { + if deadline, ok := ctx.Deadline(); ok { + *d.seen = deadline + } + } + + d.mu.Unlock() + + return d.SnapshotStore.ListByDefinition(ctx, rdName) //nolint:wrapcheck // pass-through +} + +type deadlineRecorderStore struct { + store.Store + + rec deadlineRecorder +} + +func (d deadlineRecorderStore) Snapshots() store.SnapshotStore { + d.rec.SnapshotStore = d.Store.Snapshots() + + return d.rec +} + +// A door detaches once, reads the group on that context, and hands the +// rollback what is left of the same budget. Detaching again inside the +// rollback restarted it, since WithoutCancel drops the deadline, so the chain +// the termination grace was derived from was not the one the code ran. +func TestTheRollbackRunsOnWhatIsLeftOfTheDoorsBudget(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedDeployedCloneSource(t, backend, "dst-chain10") + + var seen time.Time + + s := &Server{Store: deadlineRecorderStore{Store: backend, rec: deadlineRecorder{mu: &sync.Mutex{}, seen: &seen}}} + + doorCtx, cancel := context.WithTimeout(context.WithoutCancel(t.Context()), time.Second) + defer cancel() + + doorDeadline, _ := doorCtx.Deadline() + + _ = s.rollBackCompensating(doorCtx, "dst-chain10", nil) + + if seen.IsZero() { + t.Fatal("fixture: the rollback's context carried no deadline at all") + } + + if seen.After(doorDeadline) { + t.Errorf("the rollback ran to %s past the door's deadline: it restarted the budget", + seen.Sub(doorDeadline).Round(time.Millisecond)) + } +} diff --git a/pkg/rest/rg_deleted_race_round11_props_test.go b/pkg/rest/rg_deleted_race_round11_props_test.go new file mode 100644 index 00000000..c3318061 --- /dev/null +++ b/pkg/rest/rg_deleted_race_round11_props_test.go @@ -0,0 +1,112 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "maps" + "net/http" + "strings" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// The restore marker records where one definition's data came from. A +// volume-less clone of a restored definition inherited it, so the first volume +// added to the shell later was restored from the source's snapshot, placed on +// that snapshot's nodes. +func TestRDCloneOfAVolumeLessRestoredDefinitionDropsItsMarker(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + + if err := backend.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: "shell-src11", + Props: map[string]string{ + "BlockstorRestoreFromSnapshot": "other11:snap11", + "Aux/keep": "1", + }, + }); err != nil { + t.Fatalf("seed the volume-less source: %v", err) + } + + base, stop := startServerWithStore(t, backend) + defer stop() + + resp := postClone(t, base, "shell-src11", map[string]any{"name": "shell-dst11"}) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("clone = %d, want 201", resp.StatusCode) + } + + clone, err := backend.ResourceDefinitions().Get(t.Context(), "shell-dst11") + if err != nil { + t.Fatalf("read the clone: %v", err) + } + + if marker, ok := clone.Props["BlockstorRestoreFromSnapshot"]; ok { + t.Errorf("the shell inherited the source's restore marker %q", marker) + } + + if clone.Props["Aux/keep"] != "1" { + t.Errorf("the shell lost the source's ordinary props: %v", clone.Props) + } +} + +// markCacheTrails serves Get from a cache that has not seen the abandoned- +// rollback mark yet; GetUncached reads the backend. +type markCacheTrails struct{ store.ResourceDefinitionStore } + +func (m markCacheTrails) Get(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { + rd, err := m.ResourceDefinitionStore.Get(ctx, name) + if err == nil { + rd.Props = maps.Clone(rd.Props) + delete(rd.Props, rollbackAbandonedKey) + } + + return rd, err //nolint:wrapcheck // pass-through test double +} + +type markCacheTrailsStore struct{ store.Store } + +func (m markCacheTrailsStore) ResourceDefinitions() store.ResourceDefinitionStore { + return markCacheTrails{m.Store.ResourceDefinitions()} +} + +// The mark is written through the API server by a rollback that gave up +// moments before linstor-csi's retry arrives, so the props the replay gate +// started from, one cache-served read taken before two other gates waited, did +// not carry it yet, and the replay answered 201 over a leftover the rollback +// had abandoned. +func TestRDCloneReplaySeesAnAbandonedRollbackTheCacheHasNotCaughtUpWith(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedDeployedCloneSource(t, backend, "src-lag11") + + base, stop := startServerWithStore(t, markCacheTrailsStore{backend}) + defer stop() + + first := postClone(t, base, "src-lag11", map[string]any{"name": "dst-lag11", "use_zfs_clone": true}) + _ = first.Body.Close() + + if first.StatusCode != http.StatusCreated { + t.Fatalf("fixture: first attempt = %d, want 201", first.StatusCode) + } + + markDefinition(t, backend, "dst-lag11", "placement") + + retry := postClone(t, base, "src-lag11", map[string]any{"name": "dst-lag11", "use_zfs_clone": true}) + defer func() { _ = retry.Body.Close() }() + + if retry.StatusCode == http.StatusCreated { + t.Fatal("the replay answered 201 over a leftover whose rollback gave up, read through a cache that trailed the mark") + } + + if rc := decodeCloneMessage(t, retry); !strings.Contains(rc.Message, "rollback gave up") { + t.Errorf("refusal %q does not say an earlier rollback gave up", rc.Message) + } +} diff --git a/pkg/rest/rg_deleted_race_round11_test.go b/pkg/rest/rg_deleted_race_round11_test.go new file mode 100644 index 00000000..022be189 --- /dev/null +++ b/pkg/rest/rg_deleted_race_round11_test.go @@ -0,0 +1,94 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +var errSnapshotListingDown = errors.New("snapshot listing timed out") + +type snapshotListingFails struct{ store.SnapshotStore } + +func (snapshotListingFails) ListByDefinition(context.Context, string) ([]apiv1.Snapshot, error) { + return nil, errSnapshotListingDown +} + +type snapshotListingFailsStore struct{ store.Store } + +func (s snapshotListingFailsStore) Snapshots() store.SnapshotStore { + return snapshotListingFails{s.Store.Snapshots()} +} + +// A timeout, a 403 and a decode failure on the snapshot listing all reached the +// advice for a snapshot that exists, which tells the operator to delete +// snapshots that may not be there, and the same words went into the mark the +// replay gate repeats. +func TestARollbackThatCouldNotReadTheSnapshotsSaysSo(t *testing.T) { + t.Parallel() + + s := &Server{Store: snapshotListingFailsStore{store.NewInMemory()}} + + err := s.refuseRollbackOverSnapshots(t.Context(), "dst-read11") + if err == nil { + t.Fatal("fixture: a failed snapshot listing was supposed to stop the rollback") + } + + if got := rollbackStepName(err); got != "read-snapshots" { + t.Errorf("step = %q, want read-snapshots", got) + } + + cause, correc := rollbackFailureAdvice(err, "dst-read11") + if strings.Contains(cause, "a snapshot exists") { + t.Errorf("cause %q states as fact a snapshot nobody could read", cause) + } + + if !strings.Contains(cause, "could not be read") || !strings.Contains(correc, "dst-read11") { + t.Errorf("advice %q / %q does not say the snapshots could not be read", cause, correc) + } + + step, known := rollbackStepByName(rollbackStepName(err)) + if replayCause, _ := rollbackStepAdvice(step, known, "dst-read11"); replayCause != cause { + t.Errorf("the replay gate reads the mark back as %q, not %q", replayCause, cause) + } +} + +// patchSleepsThroughItsContext stands for the conflict backoff, which sleeps +// without looking at the context. +type patchSleepsThroughItsContext struct{ store.ResourceDefinitionStore } + +func (patchSleepsThroughItsContext) PatchResourceDefinitionSpec( + context.Context, string, func(*apiv1.ResourceDefinition) error, +) error { + time.Sleep(2 * time.Second) + + return nil +} + +type patchSleepsStore struct{ store.Store } + +func (p patchSleepsStore) ResourceDefinitions() store.ResourceDefinitionStore { + return patchSleepsThroughItsContext{p.Store.ResourceDefinitions()} +} + +// A mark retrying on conflict could spend the convergence waits the cascade +// after it is budgeted for, and the chain set nothing aside for it. +func TestAMarkWriteStopsAtItsOwnBudget(t *testing.T) { + t.Parallel() + + s := &Server{Store: patchSleepsStore{store.NewInMemory()}} + + start := time.Now() + s.markRollbackAbandoned(t.Context(), "dst-mark11", rollbackInProgress) + + if took := time.Since(start); took > markWriteBudget+300*time.Millisecond { + t.Errorf("the mark write held the compensation for %s, past its budget of %s", took, markWriteBudget) + } +} diff --git a/pkg/rest/rg_deleted_race_round5_test.go b/pkg/rest/rg_deleted_race_round5_test.go new file mode 100644 index 00000000..1ad1ab09 --- /dev/null +++ b/pkg/rest/rg_deleted_race_round5_test.go @@ -0,0 +1,414 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "encoding/json" + "net/http" + "slices" + "strings" + "testing" + + "github.com/cockroachdb/errors" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +func decodeCloneMessage(t *testing.T, resp *http.Response) apiv1.APICallRc { + t.Helper() + + var envelope cloneStartedResponse + if err := json.NewDecoder(resp.Body).Decode(&envelope); err != nil { + t.Fatalf("decode the envelope: %v", err) + } + + if envelope.Messages == nil || len(*envelope.Messages) == 0 { + t.Fatal("empty envelope") + } + + return (*envelope.Messages)[0] +} + +// The resources listing trails the deletes the rollback just issued, and the +// cascade sleeps nowhere across its passes, so one read outruns the cache by +// construction. Deciding on that read refused a rollback whose replicas were +// going, and left the definition parented to a group that is gone. +func TestRDCloneRollbackWaitsForTheCacheToSeeItsDeletes(t *testing.T) { + st := newLaggingStore(laggingDuration) + ctx := t.Context() + seedGroupedCloneSource(t, st, "src-lag", "grp-lag-gone", false) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-lag", map[string]any{"name": "dst-lag", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusNotFound { + t.Fatalf("status = %d, want 404 — the replicas were deleted, the cache only trailed it", + resp.StatusCode) + } + + if _, err := st.inner.ResourceDefinitions().Get(ctx, "dst-lag"); err == nil { + t.Error("the definition survived a rollback whose replicas all went") + } +} + +// unlistedPlacements is the other direction of the same trail: a cache that has +// not yet seen the replicas this request placed a moment ago. It lists nothing +// for the target while every write still reaches the backend. +type unlistedPlacements struct { + store.ResourceStore + + hidden string +} + +func (u unlistedPlacements) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Resource, error) { + if rdName == u.hidden { + return nil, nil + } + + replicas, err := u.ResourceStore.ListByDefinition(ctx, rdName) + + return replicas, errors.Wrap(err, "list through the unlisted-placement double") +} + +type unlistedPlacementsStore struct { + store.Store + + hidden string +} + +func (u unlistedPlacementsStore) Resources() store.ResourceStore { + return unlistedPlacements{ResourceStore: u.Store.Resources(), hidden: u.hidden} +} + +// A cache that has not yet listed the placements makes the cascade delete +// nothing and the stranded check find nothing, so the definition used to go +// over live replicas that were never stamped — the orphan the rollback exists +// to prevent. Deleting what this request placed by name does not depend on +// any listing having caught up. +func TestRDCloneRollbackReapsReplicasTheCacheHasNotListedYet(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-unlisted", "grp-unlisted-gone", false) + + base, stop := startServerWithStore(t, unlistedPlacementsStore{Store: backend, hidden: "dst-unlisted"}) + defer stop() + + resp := postClone(t, base, "src-unlisted", map[string]any{"name": "dst-unlisted", "use_zfs_clone": true}) + _ = resp.Body.Close() + + replicas, err := backend.Resources().ListByDefinition(ctx, "dst-unlisted") + if err != nil { + t.Fatalf("list the backend's replicas: %v", err) + } + + if len(replicas) != 0 { + t.Errorf("%d replica(s) left on the backend after the rollback; the cache had not "+ + "listed them, so nothing reaped them", len(replicas)) + } +} + +// stampedButListedDeletes is the ordinary cascade outcome in a cluster: the +// DELETE is accepted, the object stays behind its finalizer, and the listing +// shows it carrying the deletion stamp. +type stampedButListedDeletes struct { + store.ResourceStore +} + +func (s stampedButListedDeletes) Delete(ctx context.Context, rdName, node string) error { + replica, err := s.Get(ctx, rdName, node) + if err != nil { + return errors.Wrap(err, "read the replica to stamp") + } + + if !slices.Contains(replica.Flags, apiv1.ResourceFlagDelete) { + replica.Flags = append(replica.Flags, apiv1.ResourceFlagDelete) + } + + return errors.Wrap(s.Update(ctx, &replica), "stamp the replica") +} + +type stampedButListedStore struct{ store.Store } + +func (s stampedButListedStore) Resources() store.ResourceStore { + return stampedButListedDeletes{s.Store.Resources()} +} + +// A replica already stamped for deletion is not stranded: the stamp is what +// makes the finalizer run, whether or not the parent outlives it. No fixture +// presented one, so the term the stranded check turns on was unpinned. +func TestRDCloneRollbackProceedsOverReplicasAlreadyStampedForDeletion(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-stamped", "grp-stamped-gone", false) + + base, stop := startServerWithStore(t, stampedButListedStore{backend}) + defer stop() + + resp := postClone(t, base, "src-stamped", map[string]any{"name": "dst-stamped", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusNotFound { + t.Fatalf("status = %d, want 404 — every replica is stamped for deletion", resp.StatusCode) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-stamped"); err == nil { + t.Error("the definition survived although every replica was accepted for deletion") + } +} + +// snapshotsOnTarget reports a snapshot on the clone target, the shape of one +// taken on it inside the rollback window. +type snapshotsOnTarget struct { + store.SnapshotStore + + target string +} + +func (s snapshotsOnTarget) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + if rdName == s.target { + return []apiv1.Snapshot{{Name: "snap-in-window", ResourceName: rdName}}, nil + } + + snaps, err := s.SnapshotStore.ListByDefinition(ctx, rdName) + + return snaps, errors.Wrap(err, "list through the snapshots-on-target double") +} + +type snapshotsOnTargetStore struct { + store.Store + + target string +} + +func (s snapshotsOnTargetStore) Snapshots() store.SnapshotStore { + return snapshotsOnTarget{SnapshotStore: s.Store.Snapshots(), target: s.target} +} + +// The rollback borrowed rd d's sweep without rd d's refusal. The sweep deletes +// every snapshot row under the definition, which is safe in the handler only +// because it refuses outright when snapshots exist. +func TestRDCloneRollbackRefusesOverASnapshotOnTheTarget(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-snapwin", "grp-snapwin-gone", false) + + base, stop := startServerWithStore(t, snapshotsOnTargetStore{Store: backend, target: "dst-snapwin"}) + defer stop() + + resp := postClone(t, base, "src-snapwin", map[string]any{"name": "dst-snapwin", "use_zfs_clone": true}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusNotFound { + t.Fatal("status = 404 — the rollback went ahead over a snapshot on the target") + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-snapwin"); err != nil { + t.Errorf("the definition was dropped over a snapshot: %v", err) + } + + // And nothing under it was touched. The refusal's correction is to drop the + // snapshots and retry, which only makes sense while the replicas are still + // there: refusing after the reap leaves the target half torn down. + replicas, err := backend.Resources().ListByDefinition(ctx, "dst-snapwin") + if err != nil { + t.Fatalf("list the target's replicas: %v", err) + } + + if len(replicas) == 0 { + t.Error("the replicas were reaped before the snapshot refusal fired") + } + + for i := range replicas { + if slices.Contains(replicas[i].Flags, apiv1.ResourceFlagDelete) { + t.Errorf("replica on %s was stamped for deletion before the refusal", replicas[i].NodeName) + } + } + + if rc := decodeCloneMessage(t, resp); !strings.Contains(strings.ToLower(rc.Cause), "snapshot") { + t.Errorf("cause = %q, want it to point at the snapshot", rc.Cause) + } +} + +// A failed rollback may reap every replica and still keep the definition, and +// both corrections tell the operator to re-create the group. The moment they +// do, a gate asking only about the group answered 201 for a clone on no node. +func TestRDCloneReplayRefusesALeftoverWhoseReplicasWereReaped(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, st, "src-reaped", "grp-reaped", true) + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "dst-reaped", + ResourceGroupName: "grp-reaped", + Props: map[string]string{ + "BlockstorRestoreFromSnapshot": "src-reaped:" + cloneSnapshotName("dst-reaped"), + }, + }); err != nil { + t.Fatalf("seed the leftover: %v", err) + } + + if err := st.VolumeDefinitions().Create(ctx, "dst-reaped", + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 64 * 1024}); err != nil { + t.Fatalf("seed the leftover's volume: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-reaped", map[string]any{"name": "dst-reaped", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if resp.StatusCode == http.StatusCreated { + t.Error("replay answered 201 for a clone whose replicas were all reaped") + } +} + +// Refusing on an unreadable group costs nothing, since the CSI retry heals +// itself; a 201 binds a PV to a definition that may be parented to nothing. +func TestRDCloneReplayRefusesWhenTheParentGroupCannotBeRead(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-rgread", "grp-rgread", true) + + base, stop := startServerWithStore(t, backend) + + first := postClone(t, base, "src-rgread", map[string]any{"name": "dst-rgread", "use_zfs_clone": true}) + _ = first.Body.Close() + + stop() + + if first.StatusCode != http.StatusCreated { + t.Fatalf("first clone = %d, want 201", first.StatusCode) + } + + base2, stop2 := startServerWithStore(t, failingRGReadStore{backend}) + defer stop2() + + replay := postClone(t, base2, "src-rgread", map[string]any{"name": "dst-rgread", "use_zfs_clone": true}) + defer func() { _ = replay.Body.Close() }() + + if replay.StatusCode != http.StatusInternalServerError { + t.Fatalf("replay with the parent group unreadable = %d, want 500", replay.StatusCode) + } + + // Refused as unreadable, not as deleted: the deleted wording tells the + // operator to delete a working clone over a read that failed. + rc := decodeCloneMessage(t, replay) + said := rc.Message + " " + rc.Cause + " " + rc.Correc + + if !strings.Contains(rc.Message, "could not be read") { + t.Errorf("message = %q, want it to say the group could not be read", rc.Message) + } + + if rc.Correc != "retry the clone" { + t.Errorf("correction = %q, want %q", rc.Correc, "retry the clone") + } + + for _, wrong := range []string{"no longer exists", "delete"} { + if strings.Contains(said, wrong) { + t.Errorf("refusal over an unreadable group says %q: %q", wrong, said) + } + } +} + +var errRDDeleteFailed = errors.New("delete the resource definition failed") + +type failingRDDeletes struct{ store.ResourceDefinitionStore } + +func (failingRDDeletes) Delete(context.Context, string) error { return errRDDeleteFailed } + +type failingRDDeleteStore struct{ store.Store } + +func (f failingRDDeleteStore) ResourceDefinitions() store.ResourceDefinitionStore { + return failingRDDeletes{f.Store.ResourceDefinitions()} +} + +// The shell rollback threw its Delete error away and told the caller the clone +// was rolled back, over a shell still parented to a group that is gone. +func TestRDCloneOfAVolumelessSourceReportsAFailedShellDelete(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "src-shell-del", + ResourceGroupName: "grp-shell-del-gone", + }); err != nil { + t.Fatalf("seed the volume-less source: %v", err) + } + + base, stop := startServerWithStore(t, failingRDDeleteStore{backend}) + defer stop() + + resp := postClone(t, base, "src-shell-del", map[string]any{"name": "dst-shell-del"}) + _ = resp.Body.Close() + + if resp.StatusCode == http.StatusNotFound { + t.Error("status = 404 \"rolled back\" although the shell's delete failed") + } +} + +// One Cause used to be written for every rollback failure, pointing the +// operator at replicas when it was the definition's own delete that failed. +func TestRDCloneRollbackNamesTheStepThatFailed(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-rddel", "grp-rddel-gone", false) + + base, stop := startServerWithStore(t, failingRDDeleteStore{backend}) + defer stop() + + resp := postClone(t, base, "src-rddel", map[string]any{"name": "dst-rddel", "use_zfs_clone": true}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", resp.StatusCode) + } + + rc := decodeCloneMessage(t, resp) + if strings.Contains(rc.Cause, "replicas could not all be reaped") { + t.Errorf("cause = %q, which blames replicas for a failed definition delete", rc.Cause) + } +} + +// The convergence wait after the rollback's own delete: without it a retry +// landing in the cache-lag window reads the pre-delete definition. Nothing +// pinned it, so removing it left the suite green. +func TestRDCloneRollbackWaitsForItsDefinitionDeleteToBeVisible(t *testing.T) { + st := newLaggingStore(laggingDuration) + seedGroupedCloneSource(t, st, "src-rdlag", "grp-rdlag-gone", false) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-rdlag", map[string]any{"name": "dst-rdlag", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusNotFound { + t.Fatalf("status = %d, want 404", resp.StatusCode) + } + + get := httpGet(t, base+"/v1/resource-definitions/dst-rdlag") + _ = get.Body.Close() + + if get.StatusCode != http.StatusNotFound { + t.Errorf("GET right after the rollback = %d, want 404 — the reply went out before "+ + "its own delete was visible", get.StatusCode) + } +} diff --git a/pkg/rest/rg_deleted_race_round6_test.go b/pkg/rest/rg_deleted_race_round6_test.go new file mode 100644 index 00000000..62782777 --- /dev/null +++ b/pkg/rest/rg_deleted_race_round6_test.go @@ -0,0 +1,616 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "slices" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/cockroachdb/errors" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// seedCloneLeftover writes a definition carrying the clone marker of +// src→dst straight into the store, with a volume when withVolume is set and +// one replica on node-a when replicaFlags is non-nil. +func seedCloneLeftover(t *testing.T, st store.Store, src, dst string, withVolume bool, replicaFlags []string) { + t.Helper() + + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: dst, + Props: map[string]string{"BlockstorRestoreFromSnapshot": src + ":" + cloneSnapshotName(dst)}, + }); err != nil { + t.Fatalf("seed the leftover: %v", err) + } + + if withVolume { + if err := st.VolumeDefinitions().Create(ctx, dst, + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 64 * 1024}); err != nil { + t.Fatalf("seed the leftover's volume: %v", err) + } + } + + if replicaFlags != nil { + if err := st.Resources().Create(ctx, &apiv1.Resource{ + Name: dst, + NodeName: "node-a", + Flags: replicaFlags, + }); err != nil { + t.Fatalf("seed the leftover's replica: %v", err) + } + } +} + +// Both rollback steps that keep a definition used to run after the replicas +// were accepted for deletion, so their leftover's replicas all carry the stamp, +// for as long as the satellite finalizer holds them. Counting those as live +// answered a CSI retry 201 over a clone being torn down. +func TestRDCloneReplayRefusesALeftoverWhoseReplicasAreAllBeingDeleted(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-term", "grp-term", false) + + base, stop := startServerWithStore(t, failingRDDeleteStore{stampedButListedStore{backend}}) + defer stop() + + first := postClone(t, base, "src-term", map[string]any{"name": "dst-term", "use_zfs_clone": true}) + _ = first.Body.Close() + + if first.StatusCode != http.StatusInternalServerError { + t.Fatalf("first attempt = %d, want 500 — the rollback's definition delete failed", first.StatusCode) + } + + replicas, err := backend.Resources().ListByDefinition(ctx, "dst-term") + if err != nil { + t.Fatalf("list the leftover's replicas: %v", err) + } + + if len(replicas) == 0 || slices.ContainsFunc(replicas, func(r apiv1.Resource) bool { + return !slices.Contains(r.Flags, apiv1.ResourceFlagDelete) + }) { + t.Fatalf("the fixture must leave replicas that all carry DELETE, got %+v", replicas) + } + + // The operator follows the correction and re-creates the group. + if err := backend.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{Name: "grp-term"}); err != nil { + t.Fatalf("re-create the group: %v", err) + } + + replay := postClone(t, base, "src-term", map[string]any{"name": "dst-term", "use_zfs_clone": true}) + defer func() { _ = replay.Body.Close() }() + + if replay.StatusCode == http.StatusCreated { + t.Fatal("replay = 201 over a clone whose every replica is being deleted") + } + + rc := decodeCloneMessage(t, replay) + if !strings.Contains(rc.Cause, "accepted for deletion") { + t.Errorf("cause = %q, want it to say the replicas are being deleted", rc.Cause) + } + + if strings.Contains(rc.Correc, "by hand") { + t.Errorf("correc = %q, which tells the operator to delete what is already going", rc.Correc) + } +} + +// The stamp is the only thing that separates the two leftovers below, so the +// refusal above is about the stamp and not about the gate as such. +func TestRDCloneReplayOverASeededLeftoverTurnsOnTheDeletionStamp(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + flags []string + replay bool + dstName string + }{ + {name: "stamped", flags: []string{apiv1.ResourceFlagDelete}, replay: false, dstName: "dst-seed-stamped"}, + {name: "unstamped", flags: []string{}, replay: true, dstName: "dst-seed-live"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedDeployedCloneSource(t, st, "src-seed") + seedCloneLeftover(t, st, "src-seed", tc.dstName, true, tc.flags) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-seed", map[string]any{"name": tc.dstName, "use_zfs_clone": true}) + _ = resp.Body.Close() + + if got := resp.StatusCode == http.StatusCreated; got != tc.replay { + t.Errorf("status = %d, replay answered = %v, want %v", resp.StatusCode, got, tc.replay) + } + }) + } +} + +// Only the replicas term of the wholeness check had a fixture. A leftover with +// a replica and no volume isolates the volumes term. +func TestRDCloneReplayRefusesALeftoverWithReplicasButNoVolumes(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedDeployedCloneSource(t, st, "src-novol") + seedCloneLeftover(t, st, "src-novol", "dst-novol", false, []string{}) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-novol", map[string]any{"name": "dst-novol", "use_zfs_clone": true}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusCreated { + t.Fatal("replay = 201 over a leftover with no volumes") + } + + if rc := decodeCloneMessage(t, resp); !strings.Contains(rc.Cause, "no volumes") { + t.Errorf("cause = %q, want it to name the missing volumes", rc.Cause) + } +} + +// A leftover that stopped before creating any volume is not rollback debris, +// and the refusal must not tell a first attempt still running to delete itself. +func TestRDCloneReplayWordsAMarkerOnlyLeftoverForBothWaysItArises(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedDeployedCloneSource(t, st, "src-bare") + seedCloneLeftover(t, st, "src-bare", "dst-bare", false, nil) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-bare", map[string]any{"name": "dst-bare", "use_zfs_clone": true}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusCreated { + t.Fatal("replay = 201 over a definition with no volumes and no replicas") + } + + rc := decodeCloneMessage(t, resp) + if strings.Contains(rc.Cause, "rollback") { + t.Errorf("cause = %q, which blames a rollback that never ran", rc.Cause) + } + + if !strings.Contains(rc.Correc, "still running") { + t.Errorf("correc = %q, want it conditioned on no attempt still running", rc.Correc) + } +} + +// trailingReplicaListing hides the target's replicas from the first few +// listings, the skew between the definition informer that matched the marker +// and the resource informer answering the listing. +type trailingReplicaListing struct { + store.ResourceStore + + target string + hides *atomic.Int32 +} + +func (l trailingReplicaListing) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Resource, error) { + if rdName == l.target && l.hides.Add(-1) >= 0 { + return nil, nil + } + + replicas, err := l.ResourceStore.ListByDefinition(ctx, rdName) + + return replicas, errors.Wrap(err, "list through the trailing double") +} + +type trailingReplicaListingStore struct { + store.Store + + target string + hides *atomic.Int32 +} + +func (s trailingReplicaListingStore) Resources() store.ResourceStore { + return trailingReplicaListing{ResourceStore: s.Store.Resources(), target: s.target, hides: s.hides} +} + +// The wholeness reads carried no cache-retry budget while the group read four +// lines below did, so a complete clone whose replica listing trailed was told +// to delete itself. +func TestRDCloneReplayWaitsForAReplicaListingThatTrails(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-trail", "grp-trail", true) + + base, stop := startServerWithStore(t, backend) + + first := postClone(t, base, "src-trail", map[string]any{"name": "dst-trail", "use_zfs_clone": true}) + _ = first.Body.Close() + + stop() + + if first.StatusCode != http.StatusCreated { + t.Fatalf("first clone = %d, want 201", first.StatusCode) + } + + hides := &atomic.Int32{} + hides.Store(cacheRetryAttempts - 1) + + base2, stop2 := startServerWithStore(t, trailingReplicaListingStore{ + Store: backend, target: "dst-trail", hides: hides, + }) + defer stop2() + + replay := postClone(t, base2, "src-trail", map[string]any{"name": "dst-trail", "use_zfs_clone": true}) + defer func() { _ = replay.Body.Close() }() + + if replay.StatusCode != http.StatusCreated { + t.Errorf("replay = %d, want 201 — the clone is complete, its listing only trailed; message: %+v", + replay.StatusCode, decodeCloneMessage(t, replay)) + } +} + +// lateWitness lands an unstamped replica on the target on the first listing +// after the rollback's deletes have come back empty: an auto-tiebreaker the +// controller stamped moments after placement, which the cascade's back-to-back +// passes miss and the wait is the first to see. +type lateWitness struct { + store.ResourceStore + + target string + deleted *atomic.Bool + emptied *atomic.Bool + landed *atomic.Bool +} + +func (l lateWitness) Delete(ctx context.Context, rdName, node string) error { + if rdName == l.target { + l.deleted.Store(true) + } + + return errors.Wrap(l.ResourceStore.Delete(ctx, rdName, node), "delete through the late-witness double") +} + +func (l lateWitness) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Resource, error) { + replicas, err := l.ResourceStore.ListByDefinition(ctx, rdName) + if err != nil || rdName != l.target || !l.deleted.Load() { + return replicas, errors.Wrap(err, "list through the late-witness double") + } + + if len(replicas) == 0 && l.emptied.CompareAndSwap(false, true) { + return replicas, nil + } + + if l.emptied.Load() && l.landed.CompareAndSwap(false, true) { + if err := l.Create(ctx, &apiv1.Resource{Name: rdName, NodeName: "node-witness"}); err != nil { + return nil, errors.Wrap(err, "land the witness") + } + + replicas, err = l.ResourceStore.ListByDefinition(ctx, rdName) + } + + return replicas, errors.Wrap(err, "list through the late-witness double") +} + +type lateWitnessStore struct { + store.Store + + target string + deleted *atomic.Bool + emptied *atomic.Bool + landed *atomic.Bool +} + +func (s lateWitnessStore) Resources() store.ResourceStore { + return lateWitness{ + ResourceStore: s.Store.Resources(), target: s.target, + deleted: s.deleted, emptied: s.emptied, landed: s.landed, + } +} + +// The wait only re-read, so a replica that became visible after the cascade +// was watched for the whole budget and never told to go, and the rollback gave +// up over a replica one delete would have removed. +func TestRDCloneRollbackReapsAReplicaThatLandsDuringTheWait(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-witness", "grp-witness-gone", false) + + st := lateWitnessStore{ + Store: backend, target: "dst-witness", + deleted: &atomic.Bool{}, emptied: &atomic.Bool{}, landed: &atomic.Bool{}, + } + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-witness", map[string]any{"name": "dst-witness", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if !st.landed.Load() { + t.Fatal("the witness never landed, so this test proves nothing") + } + + if resp.StatusCode != http.StatusNotFound { + t.Fatalf("status = %d, want 404 — one delete removes the witness and finishes the rollback", + resp.StatusCode) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-witness"); err == nil { + t.Error("the definition survived a rollback that could have finished") + } +} + +var errVolumeCreateFailed = errors.New("probe: volume create failed") + +// failingTargetVolumeCreates fails hydration of one definition, the failure +// after the marker-bearing definition already exists. +type failingTargetVolumeCreates struct { + store.VolumeDefinitionStore + + target string +} + +func (f failingTargetVolumeCreates) Create(ctx context.Context, rdName string, vd *apiv1.VolumeDefinition) error { + if rdName == f.target { + return errVolumeCreateFailed + } + + return errors.Wrap(f.VolumeDefinitionStore.Create(ctx, rdName, vd), "create through the failing double") +} + +type failingTargetVolumeCreateStore struct { + store.Store + + target string +} + +func (f failingTargetVolumeCreateStore) VolumeDefinitions() store.VolumeDefinitionStore { + return failingTargetVolumeCreates{VolumeDefinitionStore: f.Store.VolumeDefinitions(), target: f.target} +} + +// The marker is stamped at RD-create and the error branch wrote a 500 without +// undoing anything, so every retry matched the marker, failed wholeness and was +// told to delete by hand a definition that was provably this clone's own debris. +func TestRDCloneRollsBackItsOwnPartialWorkWhenHydrationFails(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, backend, "src-hydrate") + + base, stop := startServerWithStore(t, failingTargetVolumeCreateStore{Store: backend, target: "dst-hydrate"}) + defer stop() + + resp := postClone(t, base, "src-hydrate", map[string]any{"name": "dst-hydrate", "use_zfs_clone": true}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", resp.StatusCode) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-hydrate"); err == nil { + t.Error("the half-made definition was left for every retry to trip over") + } + + if rc := decodeCloneMessage(t, resp); !strings.Contains(rc.Message, "rolled back") { + t.Errorf("message = %q, want it to say the partial clone was rolled back", rc.Message) + } +} + +// blockingTargetVolumeCreates holds hydration of one definition until the +// request's context ends, the shape of a CSI caller timing out mid-clone. +type blockingTargetVolumeCreates struct { + store.VolumeDefinitionStore + + target string +} + +func (b blockingTargetVolumeCreates) Create(ctx context.Context, rdName string, vd *apiv1.VolumeDefinition) error { + if rdName == b.target { + <-ctx.Done() + + return errors.Wrap(ctx.Err(), "hydrate through the blocking double") + } + + return errors.Wrap(b.VolumeDefinitionStore.Create(ctx, rdName, vd), "create through the blocking double") +} + +// contextHonouringRDDeletes refuses a delete on an ended context, as a real +// API client does; the in-memory store ignores the context altogether. +type contextHonouringRDDeletes struct { + store.ResourceDefinitionStore +} + +func (c contextHonouringRDDeletes) Delete(ctx context.Context, name string) error { + if err := ctx.Err(); err != nil { + return errors.Wrap(err, "delete on an ended context") + } + + return errors.Wrap(c.ResourceDefinitionStore.Delete(ctx, name), "delete through the context double") +} + +type abandonedHydrationStore struct { + store.Store + + target string +} + +func (a abandonedHydrationStore) VolumeDefinitions() store.VolumeDefinitionStore { + return blockingTargetVolumeCreates{VolumeDefinitionStore: a.Store.VolumeDefinitions(), target: a.target} +} + +func (a abandonedHydrationStore) ResourceDefinitions() store.ResourceDefinitionStore { + return contextHonouringRDDeletes{a.Store.ResourceDefinitions()} +} + +// The likeliest way hydration fails is the caller going away, and a rollback on +// the request's own context fails on its first call for the same reason. +func TestRDCloneRollbackOfPartialWorkOutlivesTheRequest(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedDeployedCloneSource(t, backend, "src-abandon") + + base, stop := startServerWithStore(t, abandonedHydrationStore{Store: backend, target: "dst-abandon"}) + defer stop() + + raw, err := json.Marshal(map[string]any{"name": "dst-abandon", "use_zfs_clone": true}) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + reqCtx, cancel := context.WithTimeout(t.Context(), 500*time.Millisecond) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, + base+"/v1/resource-definitions/src-abandon/clone", bytes.NewReader(raw)) + if err != nil { + t.Fatalf("build the request: %v", err) + } + + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + if err == nil { + _ = resp.Body.Close() + + t.Fatalf("the request finished with %d; the fixture needs it abandoned", resp.StatusCode) + } + + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + if _, err := backend.ResourceDefinitions().Get(t.Context(), "dst-abandon"); errors.Is(err, store.ErrNotFound) { + return + } + + time.Sleep(50 * time.Millisecond) + } + + t.Error("the half-made definition outlived an abandoned request") +} + +var errTargetReadBlip = errors.New("probe: transient failure reading the target") + +// blippingTargetRDReads fails the first read of one definition, which the +// pre-existence check treats as absent and proceeds past. +type blippingTargetRDReads struct { + store.ResourceDefinitionStore + + target string + blips *atomic.Int32 +} + +func (b blippingTargetRDReads) Get(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { + if name == b.target && b.blips.Add(-1) >= 0 { + return apiv1.ResourceDefinition{}, errTargetReadBlip + } + + rd, err := b.ResourceDefinitionStore.Get(ctx, name) + + return rd, errors.Wrap(err, "get through the blipping double") +} + +type blippingTargetRDReadStore struct { + store.Store + + target string + blips *atomic.Int32 +} + +func (b blippingTargetRDReadStore) ResourceDefinitions() store.ResourceDefinitionStore { + return blippingTargetRDReads{ResourceDefinitionStore: b.Store.ResourceDefinitions(), target: b.target, blips: b.blips} +} + +// The rollback in the error branch undoes only what this request created. A +// definition that was there before it, another attempt's still running, must +// survive a request that failed on its create. +func TestRDCloneFailureDoesNotRollBackADefinitionItDidNotCreate(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedDeployedCloneSource(t, backend, "src-other") + seedCloneLeftover(t, backend, "src-other", "dst-other", false, nil) + + blips := &atomic.Int32{} + blips.Store(1) + + base, stop := startServerWithStore(t, blippingTargetRDReadStore{Store: backend, target: "dst-other", blips: blips}) + defer stop() + + resp := postClone(t, base, "src-other", map[string]any{"name": "dst-other", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if resp.StatusCode == http.StatusCreated { + t.Fatalf("status = 201, but the create under an existing name cannot have succeeded") + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-other"); err != nil { + t.Errorf("a definition this request did not create was deleted: %v", err) + } +} + +// countedRetainedDeletes accepts every delete, keeps the replica listed and +// unstamped, and counts the calls: a cache that never catches up. +type countedRetainedDeletes struct { + store.ResourceStore + + calls *atomic.Int32 +} + +func (c countedRetainedDeletes) Delete(context.Context, string, string) error { + c.calls.Add(1) + + return nil +} + +type countedRetainedStore struct { + store.Store + + calls *atomic.Int32 +} + +func (c countedRetainedStore) Resources() store.ResourceStore { + return countedRetainedDeletes{ResourceStore: c.Store.Resources(), calls: c.calls} +} + +// The wait deletes what it sees, but a replica already deleted also lists +// unstamped while the cache trails. Deleting on every poll would send the API +// server a hundred deletes per replica over one budget. +func TestRDCloneRollbackWaitTellsEachReplicaToGoOnce(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-once", "grp-once-gone", false) + + calls := &atomic.Int32{} + + base, stop := startServerWithStore(t, countedRetainedStore{Store: backend, calls: calls}) + defer stop() + + resp := postClone(t, base, "src-once", map[string]any{"name": "dst-once", "use_zfs_clone": true}) + _ = resp.Body.Close() + + if resp.StatusCode == http.StatusNotFound { + t.Fatal("status = 404 over replicas that were never stamped") + } + + // One replica: the delete by name, one per cascade pass, one from the wait. + if limit := int32(1 + store.CascadeDeleteMaxPasses + 1); calls.Load() > limit { + t.Errorf("%d deletes for one replica, want at most %d", calls.Load(), limit) + } +} diff --git a/pkg/rest/rg_deleted_race_round7_test.go b/pkg/rest/rg_deleted_race_round7_test.go new file mode 100644 index 00000000..b4eac2ba --- /dev/null +++ b/pkg/rest/rg_deleted_race_round7_test.go @@ -0,0 +1,437 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "bytes" + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "strings" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/cockroachdb/errors" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// rollbackGate holds the RG-deleted rollback at its first step, the snapshot +// listing of the target, until the test has abandoned the request. It then +// gives an ended context time to show before letting the rollback go on, and +// refuses on one the way a real API client does. +type rollbackGate struct { + store.SnapshotStore + + target string + reached chan struct{} + release chan struct{} + once *sync.Once +} + +func (g rollbackGate) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + if rdName == g.target { + g.once.Do(func() { close(g.reached) }) + + <-g.release + + select { + case <-ctx.Done(): + return nil, errors.Wrap(ctx.Err(), "list snapshots on an ended context") + case <-time.After(500 * time.Millisecond): + } + } + + snaps, err := g.SnapshotStore.ListByDefinition(ctx, rdName) + + return snaps, errors.Wrap(err, "list snapshots through the gate") +} + +type rollbackGateStore struct { + store.Store + + gate rollbackGate +} + +func (s rollbackGateStore) Snapshots() store.SnapshotStore { + gate := s.gate + gate.SnapshotStore = s.Store.Snapshots() + + return gate +} + +func newRollbackGateStore(backend store.Store, target string) rollbackGateStore { + return rollbackGateStore{Store: backend, gate: rollbackGate{ + target: target, + reached: make(chan struct{}), + release: make(chan struct{}), + once: &sync.Once{}, + }} +} + +// abandonAtTheRollback posts body to url, abandons the request once the +// rollback has started, and lets the rollback go on. +func abandonAtTheRollback(t *testing.T, url string, body any, gated rollbackGateStore) { + t.Helper() + + abandonAtTheGate(t, url, body, gated.gate.reached, gated.gate.release) +} + +// abandonAtTheGate is the same, over whichever step of a rollback the fixture +// gated: it waits for reached, abandons the request, and closes release. +func abandonAtTheGate(t *testing.T, url string, body any, reached, release chan struct{}) { + t.Helper() + + raw, err := json.Marshal(body) + if err != nil { + t.Fatalf("marshal: %v", err) + } + + reqCtx, cancel := context.WithCancel(t.Context()) + defer cancel() + + req, err := http.NewRequestWithContext(reqCtx, http.MethodPost, url, bytes.NewReader(raw)) + if err != nil { + t.Fatalf("build the request: %v", err) + } + + req.Header.Set("Content-Type", "application/json") + + done := make(chan error, 1) + + go func() { + resp, err := http.DefaultClient.Do(req) + if err == nil { + _ = resp.Body.Close() + } + + done <- err + }() + + select { + case <-reached: + case err := <-done: + t.Fatalf("the request finished (err=%v) before the rollback started; the fixture needs it abandoned", err) + case <-time.After(20 * time.Second): + t.Fatal("the rollback never started") + } + + cancel() + + if err := <-done; err == nil { + t.Fatal("the request finished; the fixture needs it abandoned") + } + + close(release) +} + +func waitForDefinitionGone(t *testing.T, backend store.Store, name string) { + t.Helper() + + deadline := time.Now().Add(15 * time.Second) + for time.Now().Before(deadline) { + if _, err := backend.ResourceDefinitions().Get(t.Context(), name); errors.Is(err, store.ErrNotFound) { + return + } + + time.Sleep(50 * time.Millisecond) + } + + t.Errorf("definition %q outlived an abandoned request, parented to a group that is gone", name) +} + +// The RG-deleted rollback is the long one, waiting out two convergence +// budgets, so a CSI caller that gives up inside it is the ordinary case. On the +// request's context the compensation died with the caller, left the definition +// parented to a group that is gone, and the replay gate refused every retry. +func TestRDCloneRGDeletedRollbackOutlivesTheRequest(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-rg-abandon", "grp-rg-abandon-gone", false) + + gated := newRollbackGateStore(backend, "dst-rg-abandon") + + base, stop := startServerWithStore(t, gated) + defer stop() + + abandonAtTheRollback(t, base+"/v1/resource-definitions/src-rg-abandon/clone", + map[string]any{"name": "dst-rg-abandon", "use_zfs_clone": true}, gated) + + waitForDefinitionGone(t, backend, "dst-rg-abandon") +} + +// The restore door has the same rollback and had the same context. +func TestSnapshotRestoreRGDeletedRollbackOutlivesTheRequest(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedRestoreSource(t, backend, "restore-abandon-src", "grp-restore-abandon-gone", "snap-abandon") + + gated := newRollbackGateStore(backend, "restore-abandon-dst") + + base, stop := startServerWithStore(t, gated) + defer stop() + + abandonAtTheRollback(t, base+"/v1/resource-definitions/restore-abandon-src/snapshot-restore-resource", + map[string]string{"to_resource": "restore-abandon-dst", "from_snapshot": "snap-abandon"}, gated) + + waitForDefinitionGone(t, backend, "restore-abandon-dst") +} + +// seedGroupedRestoreSource seeds a source parented to rgName, which is not +// created, and a one-volume snapshot of it. +func seedGroupedRestoreSource(t *testing.T, st store.Store, src, rgName, snap string) { + t.Helper() + + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: src, + ResourceGroupName: rgName, + }); err != nil { + t.Fatalf("seed the source: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: snap, + ResourceName: src, + Nodes: []string{"n1"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 1024 * 1024}, + }, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } +} + +// seedAdoptedTarget is a definition an earlier attempt left, with a replica, +// parented to a group that is gone. +func seedAdoptedTarget(t *testing.T, st store.Store, name, rgName string) { + t.Helper() + + if err := st.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: name, + ResourceGroupName: rgName, + }); err != nil { + t.Fatalf("seed the adopted target: %v", err) + } + + if err := st.Resources().Create(t.Context(), &apiv1.Resource{Name: name, NodeName: "node-a"}); err != nil { + t.Fatalf("seed its replica: %v", err) + } +} + +// Once a door tolerates a leftover, "materialise succeeded" can mean "adopted a +// definition another attempt created", and the RG-deleted rollback cascades +// every replica under the name, not only this request's. Over a definition +// this request did not create the door refuses and leaves it in place; over +// one it did, it rolls back. Both doors, both ways. +func TestPostWriteGroupCheckLeavesAnAdoptedDefinitionInPlace(t *testing.T) { + t.Parallel() + + doors := map[string]func(s *Server, rec *httptest.ResponseRecorder, made materialisedRD) bool{ + "clone": func(s *Server, rec *httptest.ResponseRecorder, made materialisedRD) bool { + _, ok := s.cloneParentRGSurvived(t.Context(), rec, + &apiv1.ResourceDefinition{Name: "adopt-src", ResourceGroupName: made.StampedRG}, made.Name, made) + + return ok + }, + "restore": func(s *Server, rec *httptest.ResponseRecorder, made materialisedRD) bool { + _, ok := s.restoreParentRGSurvived(t.Context(), rec, made) + + return ok + }, + } + + for door, check := range doors { + for _, created := range []bool{false, true} { + t.Run(door+"/created="+map[bool]string{false: "false", true: "true"}[created], func(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedAdoptedTarget(t, st, "adopt-dst", "grp-adopt-gone") + + rec := httptest.NewRecorder() + + made := adoptedRD("adopt-dst", "grp-adopt-gone") + if created { + made = createdRD("adopt-dst", "grp-adopt-gone") + } + + made.Placed = []string{"node-a"} + + if check(&Server{Store: st}, rec, made) { + t.Fatal("the check passed over a parent group that does not exist") + } + + _, getErr := st.ResourceDefinitions().Get(t.Context(), "adopt-dst") + replicas, listErr := st.Resources().ListByDefinition(t.Context(), "adopt-dst") + + if listErr != nil && !errors.Is(listErr, store.ErrNotFound) { + t.Fatalf("list the target's replicas: %v", listErr) + } + + if created { + if getErr == nil { + t.Error("this request's own definition was not rolled back") + } + + return + } + + if getErr != nil || len(replicas) != 1 { + t.Errorf("an adopted definition was reaped: get err=%v, %d replica(s) left", getErr, len(replicas)) + } + + if rec.Code != http.StatusConflict { + t.Errorf("status = %d, want 409", rec.Code) + } + + if !strings.Contains(rec.Body.String(), "not created by this request") { + t.Errorf("body %s does not say the definition was left because this request did not create it", + rec.Body.String()) + } + }) + } + } +} + +var errRestoreHydrateBlip = errors.New("probe: one volume create failed") + +// failOnceTargetVolumeCreates fails the first hydration of one definition and +// lets every later one through: a transient failure. +type failOnceTargetVolumeCreates struct { + store.VolumeDefinitionStore + + target string + failed *atomic.Bool +} + +func (f failOnceTargetVolumeCreates) Create(ctx context.Context, rdName string, vd *apiv1.VolumeDefinition) error { + if rdName == f.target && f.failed.CompareAndSwap(false, true) { + return errRestoreHydrateBlip + } + + return errors.Wrap(f.VolumeDefinitionStore.Create(ctx, rdName, vd), "create through the fail-once double") +} + +type failOnceTargetVolumeCreateStore struct { + store.Store + + target string + failed *atomic.Bool +} + +func (f failOnceTargetVolumeCreateStore) VolumeDefinitions() store.VolumeDefinitionStore { + return failOnceTargetVolumeCreates{VolumeDefinitionStore: f.Store.VolumeDefinitions(), target: f.target, failed: f.failed} +} + +// The restore endpoint has no idempotent-replay gate, and its marker-bearing +// definition was left behind after a hydrate failure, so one transient failure +// turned every retry under the deterministic CSI target name into a 409. +func TestSnapshotRestoreRetrySucceedsAfterAHydrateFailure(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{Name: "grp-hydrate-restore"}); err != nil { + t.Fatalf("seed RG: %v", err) + } + + seedGroupedRestoreSource(t, backend, "hydrate-restore-src", "grp-hydrate-restore", "snap-hydrate") + + base, stop := startServerWithStore(t, failOnceTargetVolumeCreateStore{ + Store: backend, target: "hydrate-restore-dst", failed: &atomic.Bool{}, + }) + defer stop() + + body, _ := json.Marshal(map[string]string{ + "to_resource": "hydrate-restore-dst", + "from_snapshot": "snap-hydrate", + }) + + url := base + "/v1/resource-definitions/hydrate-restore-src/snapshot-restore-resource" + + first := httpPost(t, url, body) + defer func() { _ = first.Body.Close() }() + + if first.StatusCode != http.StatusInternalServerError { + t.Fatalf("first restore = %d, want 500", first.StatusCode) + } + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(first.Body).Decode(&rcs); err != nil || len(rcs) == 0 { + t.Fatalf("decode the restore's envelope: %v (%d entries)", err, len(rcs)) + } + + if !strings.Contains(rcs[0].Message, "rolled back") { + t.Errorf("message = %q, want it to say the partial restore was rolled back", rcs[0].Message) + } + + retry := httpPost(t, url, body) + _ = retry.Body.Close() + + if retry.StatusCode != http.StatusCreated { + t.Errorf("retry after one transient hydrate failure = %d, want 201", retry.StatusCode) + } +} + +// Both halves of the clone's post-write group check proceed on an inconclusive +// read. The data half told the caller; the volume-less half left it in a log. +func TestRDCloneOfAVolumelessSourceWarnsWhenTheGroupCannotBeRechecked(t *testing.T) { + t.Parallel() + + for _, unreadable := range []bool{true, false} { + name := map[bool]string{true: "unreadable", false: "readable"}[unreadable] + + t.Run(name, func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + src, dst, rg := "shell-warn-src-"+name, "shell-warn-dst-"+name, "grp-shell-warn-"+name + + if err := backend.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{Name: rg}); err != nil { + t.Fatalf("seed RG: %v", err) + } + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: src, ResourceGroupName: rg, + }); err != nil { + t.Fatalf("seed the volume-less source: %v", err) + } + + var served store.Store = backend + if unreadable { + served = failingRGReadStore{backend} + } + + base, stop := startServerWithStore(t, served) + defer stop() + + resp := postClone(t, base, src, map[string]any{"name": dst}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status = %d, want 201", resp.StatusCode) + } + + var envelope cloneStartedResponse + if err := json.NewDecoder(resp.Body).Decode(&envelope); err != nil || envelope.Messages == nil { + t.Fatalf("decode the envelope: %v", err) + } + + warned := envelopeWarnsAbout(*envelope.Messages, rg) + if warned != unreadable { + t.Errorf("warned about the unverified group = %v, want %v; messages %+v", + warned, unreadable, *envelope.Messages) + } + }) + } +} diff --git a/pkg/rest/rg_deleted_race_round8_test.go b/pkg/rest/rg_deleted_race_round8_test.go new file mode 100644 index 00000000..b456025c --- /dev/null +++ b/pkg/rest/rg_deleted_race_round8_test.go @@ -0,0 +1,218 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "net/http/httptest" + "os" + "path/filepath" + "regexp" + "strconv" + "sync" + "testing" + "time" + + "github.com/cockroachdb/errors" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// rdDeleteGate holds a rollback at the step both the bare delete and the +// shared rollback end on, the definition delete, until the test has abandoned +// the request. It then refuses on an ended context the way a real API client +// does; the in-memory store ignores the context entirely. +type rdDeleteGate struct { + store.ResourceDefinitionStore + + target string + reached chan struct{} + release chan struct{} + once *sync.Once +} + +func (g rdDeleteGate) Delete(ctx context.Context, name string) error { + if name != g.target { + return g.ResourceDefinitionStore.Delete(ctx, name) //nolint:wrapcheck // pass-through in a fixture + } + + g.once.Do(func() { close(g.reached) }) + + <-g.release + + // An abandoned request reaches the handler's context asynchronously, so + // give it the moment the round-7 gate gives it before deciding. + select { + case <-ctx.Done(): + return errors.Wrap(ctx.Err(), "delete on an ended context") + case <-time.After(500 * time.Millisecond): + } + + return g.ResourceDefinitionStore.Delete(ctx, name) //nolint:wrapcheck // pass-through in a fixture +} + +type rdDeleteGateStore struct { + store.Store + + gate rdDeleteGate +} + +func (s rdDeleteGateStore) ResourceDefinitions() store.ResourceDefinitionStore { + gate := s.gate + gate.ResourceDefinitionStore = s.Store.ResourceDefinitions() + + return gate +} + +func newRDDeleteGateStore(backend store.Store, target string) rdDeleteGateStore { + return rdDeleteGateStore{Store: backend, gate: rdDeleteGate{ + target: target, + reached: make(chan struct{}), + release: make(chan struct{}), + once: &sync.Once{}, + }} +} + +// The vol-less clone is the third post-write door. Its compensation ran on the +// request's own context, so a caller that gave up while the group check was +// still inside its NotFound budget left the shell behind, parented to a group +// that is gone. The shell has no marker and no replay gate, so every retry +// then meets AlreadyExists and 409s until someone deletes it by hand, and the +// 500 that would have said so went to a connection that was already closed. +func TestRDCloneShellRollbackOutlivesTheRequest(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + + if err := backend.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{ + Name: "shell-abandon-src", + ResourceGroupName: "grp-shell-abandon-gone", + }); err != nil { + t.Fatalf("seed the vol-less source: %v", err) + } + + gated := newRDDeleteGateStore(backend, "shell-abandon-dst") + + base, stop := startServerWithStore(t, gated) + defer stop() + + abandonAtTheGate(t, base+"/v1/resource-definitions/shell-abandon-src/clone", + map[string]any{"name": "shell-abandon-dst"}, gated.gate.reached, gated.gate.release) + + waitForDefinitionGone(t, backend, "shell-abandon-dst") +} + +// The rollback runs inside the handler on a context Shutdown cannot cancel, so +// the process has to outlive it: the budget under the shutdown window, the +// window under the termination grace of every manifest that serves REST. A +// SIGTERM landing between those numbers cuts a cascade in half and kills the +// connection that would have named what was left. +func TestRollbackBudgetFitsTheShutdownWindow(t *testing.T) { + t.Parallel() + + if detachedRollbackBudget+shutdownMargin > gracefulShutdownWindow { + t.Errorf("rollback budget %s plus margin %s does not fit the shutdown window %s", + detachedRollbackBudget, shutdownMargin, gracefulShutdownWindow) + } + + // The two mark writes are budgeted on top of everything the cascade is, + // not carved out of its waits. + if cascade := groupRecheckBudget + 2*cacheConvergeBudget + rollbackWriteBudget; detachedRollbackBudget < cascade+2*markWriteBudget { + t.Errorf("rollback budget %s leaves the two abandoned-rollback marks (%s each) nothing on top "+ + "of the cascade's %s", detachedRollbackBudget, markWriteBudget, cascade) + } + + // Both convergence waits the rollback can spend in full, back to back. + if detachedRollbackBudget < 2*cacheConvergeBudget { + t.Errorf("rollback budget %s is under its own two waits of %s; a rollback would be "+ + "cut where those waits are what keep the definition off unstamped replicas", + detachedRollbackBudget, cacheConvergeBudget) + } + + // Every manifest whose pod runs a process that serves REST: both server + // binaries call rest.Server, and the satellite does not. + for _, manifest := range []string{ + filepath.Join("..", "..", "config", "manager", "manager.yaml"), + filepath.Join("..", "..", "stand", "blockstor-deploy.yaml"), + filepath.Join("..", "..", "stand", "blockstor-apiserver-deploy.yaml"), + } { + grace := terminationGraceOf(t, manifest) + + if gracefulShutdownWindow+terminationGraceMargin > grace { + t.Errorf("%s gives the pod %s; the shutdown window %s plus margin %s needs more", + manifest, grace, gracefulShutdownWindow, terminationGraceMargin) + } + } +} + +// terminationGraceOf reads terminationGracePeriodSeconds out of a manifest, +// falling back to the Kubernetes default when the field is absent. +func terminationGraceOf(t *testing.T, path string) time.Duration { + t.Helper() + + const kubeletDefaultGrace = 30 * time.Second + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + + match := regexp.MustCompile(`terminationGracePeriodSeconds:\s*(\d+)`).FindSubmatch(raw) + if match == nil { + return kubeletDefaultGrace + } + + seconds, err := strconv.Atoi(string(match[1])) + if err != nil { + t.Fatalf("parse the grace period in %s: %v", path, err) + } + + return time.Duration(seconds) * time.Second +} + +// A materialisedRD that states no origin is not a claim of ownership. The +// producers go through the constructors; a literal that skips them, which is +// what a door adopting a leftover would add, must not authorise a cascade over +// a definition nobody said this request created. +func TestAMaterialisedRDWithNoStatedOriginIsNotThisRequestsToReap(t *testing.T) { + t.Parallel() + + zero := materialisedRD{Name: "x", StampedRG: "g"} + + if zero.origin != rdOriginUnstated { + t.Errorf("the zero value states origin %d; the unstated one is what a literal leaves", zero.origin) + } + + if zero.createdHere() { + t.Error("a materialisedRD with no stated origin authorised a rollback over its definition") + } + + if adoptedRD("x", "g").createdHere() { + t.Error("an adopted definition authorised a rollback over itself") + } + + if !createdRD("x", "g").createdHere() { + t.Error("a definition this request created was not its own to roll back") + } +} + +// And the doors read it the same way: an unstated origin is refused and left in +// place, exactly as an adopted one is. +func TestPostWriteGroupCheckLeavesADefinitionWithNoStatedOriginInPlace(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedAdoptedTarget(t, st, "unstated-dst", "grp-unstated-gone") + + rec := httptest.NewRecorder() + made := materialisedRD{Name: "unstated-dst", StampedRG: "grp-unstated-gone", Placed: []string{"node-a"}} + + if _, ok := (&Server{Store: st}).restoreParentRGSurvived(t.Context(), rec, made); ok { + t.Fatal("the check passed over a parent group that does not exist") + } + + if _, err := st.ResourceDefinitions().Get(t.Context(), "unstated-dst"); err != nil { + t.Errorf("a definition with no stated origin was rolled back: %v", err) + } +} diff --git a/pkg/rest/rg_deleted_race_round9_test.go b/pkg/rest/rg_deleted_race_round9_test.go new file mode 100644 index 00000000..ec116c83 --- /dev/null +++ b/pkg/rest/rg_deleted_race_round9_test.go @@ -0,0 +1,444 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "sync" + "testing" + "time" + + "github.com/cockroachdb/errors" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// A cancelled request context is what both an abandoned caller and a SIGTERM +// hand a post-write door: the server gives every request the runnable's own +// context as its base, and the manager cancels that on shutdown. On that +// context the group re-read came back cancelled, which parentRGSurvived can +// only call "could not check", and the door answered success over a group +// that is gone. Every door, driven directly with a context that is already +// cancelled. +func TestPostWriteGroupCheckIsNotEndedByTheCaller(t *testing.T) { + t.Parallel() + + doors := map[string]func(ctx context.Context, s *Server, rec *httptest.ResponseRecorder, made materialisedRD) bool{ + "clone": func(ctx context.Context, s *Server, rec *httptest.ResponseRecorder, made materialisedRD) bool { + _, ok := s.cloneParentRGSurvived(ctx, rec, + &apiv1.ResourceDefinition{Name: "cancel-src", ResourceGroupName: made.StampedRG}, made.Name, made) + + return ok + }, + "restore": func(ctx context.Context, s *Server, rec *httptest.ResponseRecorder, made materialisedRD) bool { + _, ok := s.restoreParentRGSurvived(ctx, rec, made) + + return ok + }, + "volume-less": func(ctx context.Context, s *Server, rec *httptest.ResponseRecorder, made materialisedRD) bool { + _, ok := s.cloneShellParentRGSurvived(ctx, rec, "cancel-src", made.Name, made.StampedRG) + + return ok + }, + } + + for door, check := range doors { + t.Run(door, func(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + seedAdoptedTarget(t, st, "cancel-dst", "grp-cancel-gone") + + made := createdRD("cancel-dst", "grp-cancel-gone") + made.Placed = []string{"node-a"} + + ended, cancel := context.WithCancel(t.Context()) + cancel() + + rec := httptest.NewRecorder() + + if check(ended, &Server{Store: st}, rec, made) { + t.Fatalf("the door passed over a group that is gone because its caller had ended; body %s", + rec.Body.String()) + } + + if _, err := st.ResourceDefinitions().Get(t.Context(), "cancel-dst"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the definition was not rolled back: %v", err) + } + }) + } +} + +// rgGetGate holds the post-write group re-read, the first read of the group +// once the target exists, until the test has abandoned the request. It then +// refuses on an ended context the way a real API client does; the in-memory +// store ignores the context entirely. +type rgGetGate struct { + store.ResourceGroupStore + + backend store.Store + target string + reached chan struct{} + release chan struct{} + once *sync.Once +} + +func (g rgGetGate) Get(ctx context.Context, name string) (apiv1.ResourceGroup, error) { + if _, err := g.backend.ResourceDefinitions().Get(ctx, g.target); err == nil { + g.once.Do(func() { close(g.reached) }) + + <-g.release + + select { + case <-ctx.Done(): + return apiv1.ResourceGroup{}, errors.Wrap(ctx.Err(), "get the group on an ended context") + case <-time.After(500 * time.Millisecond): + } + } + + return g.ResourceGroupStore.Get(ctx, name) //nolint:wrapcheck // pass-through in a fixture +} + +type rgGetGateStore struct { + store.Store + + gate rgGetGate +} + +func (s rgGetGateStore) ResourceGroups() store.ResourceGroupStore { + gate := s.gate + gate.ResourceGroupStore = s.Store.ResourceGroups() + + return gate +} + +// Ivan's shape on the data path: the caller gives up while the post-write +// check is reading the group. The read has to finish on its own context and +// roll the clone back; on the request's context it came back cancelled, the +// clone answered 201 to nobody, and the definition stayed parented to a group +// that is gone. +func TestRDCloneGroupCheckOutlivesTheRequest(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-rgget", "grp-rgget-gone", false) + + gated := rgGetGateStore{Store: backend, gate: rgGetGate{ + backend: backend, + target: "dst-rgget", + reached: make(chan struct{}), + release: make(chan struct{}), + once: &sync.Once{}, + }} + + base, stop := startServerWithStore(t, gated) + defer stop() + + abandonAtTheGate(t, base+"/v1/resource-definitions/src-rgget/clone", + map[string]any{"name": "dst-rgget", "use_zfs_clone": true}, gated.gate.reached, gated.gate.release) + + waitForDefinitionGone(t, backend, "dst-rgget") +} + +// deadlineRecordingRDs records what context the spawn rollback's delete ran on. +type deadlineRecordingRDs struct { + store.ResourceDefinitionStore + + sawDeadline *time.Duration + sawEnded *bool +} + +func (d deadlineRecordingRDs) Delete(ctx context.Context, name string) error { + if deadline, ok := ctx.Deadline(); ok { + *d.sawDeadline = time.Until(deadline) + } + + *d.sawEnded = ctx.Err() != nil + + return d.ResourceDefinitionStore.Delete(ctx, name) //nolint:wrapcheck // pass-through in a fixture +} + +type deadlineRecordingStore struct { + store.Store + + rds deadlineRecordingRDs +} + +func (s deadlineRecordingStore) ResourceDefinitions() store.ResourceDefinitionStore { + rds := s.rds + rds.ResourceDefinitionStore = s.Store.ResourceDefinitions() + + return rds +} + +// rollbackSpawn detached from the caller with no bound, so the graceful +// shutdown window derived from the rollback budget did not cover it. It runs +// on the same budget now. +func TestSpawnRollbackIsBoundedLikeTheOtherCompensations(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + if err := backend.ResourceDefinitions().Create(t.Context(), &apiv1.ResourceDefinition{Name: "spawn-half"}); err != nil { + t.Fatalf("seed the half-spawned definition: %v", err) + } + + var ( + deadline time.Duration + ended bool + ) + + st := deadlineRecordingStore{Store: backend, rds: deadlineRecordingRDs{sawDeadline: &deadline, sawEnded: &ended}} + + caller, cancel := context.WithCancel(t.Context()) + cancel() + + rollbackSpawn(caller, st, "spawn-half") + + if ended { + t.Error("the spawn rollback ran on the caller's ended context") + } + + if deadline <= 0 || deadline > detachedRollbackBudget { + t.Errorf("the spawn rollback ran with %s left, want a deadline within %s", deadline, detachedRollbackBudget) + } + + if _, err := backend.ResourceDefinitions().Get(t.Context(), "spawn-half"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the half-spawned definition survived its rollback: %v", err) + } +} + +// The volume-less door kept one hardcoded cause and "delete it by hand" after +// it gained the shared four-step rollback. With a snapshot on the shell that +// advice is a dead end, because `rd d` refuses a definition that has +// snapshots; the step-correct advice names the snapshot. +func TestRDCloneOfAVolumelessSourceAdvisesForTheStepThatFailed(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "src-shell-snap", + ResourceGroupName: "grp-shell-snap-gone", + }); err != nil { + t.Fatalf("seed the volume-less source: %v", err) + } + + // A snapshot row under the target name, which is what the rollback's + // snapshot refusal finds on the shell. + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-on-shell", ResourceName: "dst-shell-snap", + }); err != nil { + t.Fatalf("seed the snapshot on the target: %v", err) + } + + base, stop := startServerWithStore(t, backend) + defer stop() + + resp := postClone(t, base, "src-shell-snap", map[string]any{"name": "dst-shell-snap"}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500: the rollback refuses over a snapshot", resp.StatusCode) + } + + rc := decodeCloneMessage(t, resp) + if !strings.Contains(rc.Correc, "snapshot") { + t.Errorf("correction %q does not name the snapshot that stopped the rollback", rc.Correc) + } + + if !strings.Contains(rc.Message, "or it was never there") { + t.Errorf("message %q asserts a concurrent delete without the other reading", rc.Message) + } +} + +// Every door's failed-rollback message offers both readings of a group that +// does not exist, as the success path does: deleted while the operation ran, +// or never there. The fixture here never created the group at all. +func TestRDCloneFailedRollbackDoesNotAssertARace(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-noracs", "grp-never-there", false) + + base, stop := startServerWithStore(t, failingRDDeleteStore{backend}) + defer stop() + + resp := postClone(t, base, "src-noracs", map[string]any{"name": "dst-noracs", "use_zfs_clone": true}) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", resp.StatusCode) + } + + rc := decodeCloneMessage(t, resp) + + if strings.Contains(rc.Message, "deleted concurrently") { + t.Errorf("message %q asserts a concurrent delete", rc.Message) + } + + if !strings.Contains(rc.Message, "or it was never there") { + t.Errorf("message %q does not offer the never-existed reading", rc.Message) + } +} + +// racingSnapshots lets the rollback's snapshot refusal see an empty listing and +// then lands a snapshot on the target, the snapshot create that slips between +// the refusal and the definition delete. +type racingSnapshots struct { + store.SnapshotStore + + target string + once *sync.Once +} + +func (r racingSnapshots) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + snaps, err := r.SnapshotStore.ListByDefinition(ctx, rdName) + + if rdName == r.target { + r.once.Do(func() { + _ = r.Create(ctx, &apiv1.Snapshot{Name: "snap-raced", ResourceName: r.target}) + }) + } + + return snaps, err //nolint:wrapcheck // pass-through in a fixture +} + +type racingSnapshotStore struct { + store.Store + + target string + once *sync.Once +} + +func (s racingSnapshotStore) Snapshots() store.SnapshotStore { + return racingSnapshots{SnapshotStore: s.Store.Snapshots(), target: s.target, once: s.once} +} + +// The sweep after the rollback's own definition delete is what mops up a +// snapshot row that raced in behind the refusal, and nothing held it: deleting +// the sweep alone left the whole package green. +func TestRollbackSweepsASnapshotThatRacedInBehindTheRefusal(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedAdoptedTarget(t, backend, "sweep-dst", "grp-sweep") + + st := racingSnapshotStore{Store: backend, target: "sweep-dst", once: &sync.Once{}} + + if err := (&Server{Store: st}).rollBackMaterialisedRD(t.Context(), "sweep-dst", []string{"node-a"}); err != nil { + t.Fatalf("rollback: %v", err) + } + + if _, err := backend.Snapshots().Get(t.Context(), "sweep-dst", "snap-raced"); !errors.Is(err, store.ErrNotFound) { + t.Errorf("the snapshot that raced in outlived the rollback, parented to nothing: %v", err) + } +} + +// seedTwoNodeSource is seedDeployedCloneSource with a second diskful replica, +// so a clone places two and "placed on one, failed on the other" can happen. +func seedTwoNodeSource(t *testing.T, st store.Store, rdName string) { + t.Helper() + + ctx := t.Context() + seedDeployedCloneSource(t, st, rdName) + + if err := st.Nodes().Create(ctx, &apiv1.Node{Name: "node-b", ConnectionStatus: "ONLINE"}); err != nil { + t.Fatalf("seed node-b: %v", err) + } + + if err := st.StoragePools().Create(ctx, &apiv1.StoragePool{ + StoragePoolName: "zfs-thin", NodeName: "node-b", ProviderKind: "ZFS_THIN", SupportsSnapshot: true, + }); err != nil { + t.Fatalf("seed the pool on node-b: %v", err) + } + + if err := st.Resources().Create(ctx, &apiv1.Resource{ + Name: rdName, NodeName: "node-b", Props: map[string]string{"StorPoolName": "zfs-thin"}, + }); err != nil { + t.Fatalf("seed the replica on node-b: %v", err) + } +} + +var ( + errPlacementFailed = errors.New("place the replica failed") + errReapConflict = errors.New("the object has been modified") +) + +// halfPlacingResources places the clone on node-a, fails it on node-b, and +// then refuses to reap node-a: the satellite conflict the rollback treats as +// an ordinary outcome. +type halfPlacingResources struct { + store.ResourceStore + + target string +} + +func (h halfPlacingResources) Create(ctx context.Context, r *apiv1.Resource) error { + if r.Name == h.target && r.NodeName == "node-b" { + return errPlacementFailed + } + + return h.ResourceStore.Create(ctx, r) //nolint:wrapcheck // pass-through in a fixture +} + +func (h halfPlacingResources) Delete(ctx context.Context, rdName, node string) error { + if rdName == h.target { + return errReapConflict + } + + return h.ResourceStore.Delete(ctx, rdName, node) //nolint:wrapcheck // pass-through in a fixture +} + +type halfPlacingStore struct { + store.Store + + target string +} + +func (s halfPlacingStore) Resources() store.ResourceStore { + return halfPlacingResources{ResourceStore: s.Store.Resources(), target: s.target} +} + +// Placement succeeds on one node and fails on the other, the rollback gives up +// on a conflict, and linstor-csi retries under the same name long before +// anyone reads the 500. The leftover has its volumes and a live replica, so +// the wholeness gate answered 201 "already cloned" over a clone with fewer +// replicas than intended. The rollback records that it gave up, and the replay +// refuses on that record. +func TestRDCloneReplayRefusesALeftoverWhoseRollbackGaveUp(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedTwoNodeSource(t, backend, "src-half9") + + base, stop := startServerWithStore(t, halfPlacingStore{Store: backend, target: "dst-half9"}) + defer stop() + + first := postClone(t, base, "src-half9", map[string]any{"name": "dst-half9", "use_zfs_clone": true}) + _ = first.Body.Close() + + if first.StatusCode != http.StatusInternalServerError { + t.Fatalf("first attempt = %d, want 500: placement failed and the rollback gave up", first.StatusCode) + } + + replicas, err := backend.Resources().ListByDefinition(t.Context(), "dst-half9") + if err != nil || len(replicas) != 1 { + t.Fatalf("fixture: want the half-placed leftover with one replica, got %d (err=%v)", len(replicas), err) + } + + retry := postClone(t, base, "src-half9", map[string]any{"name": "dst-half9", "use_zfs_clone": true}) + defer func() { _ = retry.Body.Close() }() + + if retry.StatusCode == http.StatusCreated { + t.Fatal("the retry answered 201 over a leftover whose rollback gave up half-placed") + } + + if rc := decodeCloneMessage(t, retry); !strings.Contains(rc.Message, "rollback gave up") { + t.Errorf("refusal %q does not say an earlier rollback gave up", rc.Message) + } +} diff --git a/pkg/rest/rg_deleted_race_test.go b/pkg/rest/rg_deleted_race_test.go new file mode 100644 index 00000000..104acaaf --- /dev/null +++ b/pkg/rest/rg_deleted_race_test.go @@ -0,0 +1,772 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// seedGroupedCloneSource is seedDeployedCloneSource with the source parented +// to a resource group, which is the shape every definition linstor-csi creates +// has. +func seedGroupedCloneSource(t *testing.T, st store.Store, rdName, rgName string, createGroup bool) { + t.Helper() + + seedDeployedCloneSource(t, st, rdName) + + src, err := st.ResourceDefinitions().Get(t.Context(), rdName) + if err != nil { + t.Fatalf("read the seeded source: %v", err) + } + + src.ResourceGroupName = rgName + + if err := st.ResourceDefinitions().Update(t.Context(), &src); err != nil { + t.Fatalf("parent the source: %v", err) + } + + if createGroup { + if err := st.ResourceGroups().Create(t.Context(), + &apiv1.ResourceGroup{Name: rgName}); err != nil { + t.Fatalf("seed RG: %v", err) + } + } +} + +// `POST /v1/resource-definitions` checks the resource group twice — before the +// write and again after it, rolling the definition back when a concurrent +// `rg d` won the race. A definition left pointing at a group that is gone +// lists fine and places badly: the placer's Controller→RG→RD walk drops the RG +// tier without a word, taking auto-place, auto-diskful, place_count and +// rebalance with it. +// +// Clone creates a definition the same way and inherits the group the same way, +// and had neither half. +func TestRDCloneRollsBackWhenTheParentGroupIsGone(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, st, "src-rg-race", "grp-gone", false) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-rg-race", map[string]any{ + "name": "dst-rg-race", + "use_zfs_clone": true, + }) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusNotFound { + t.Fatalf("status = %d, want 404 — the parent group is gone", resp.StatusCode) + } + + if _, err := st.ResourceDefinitions().Get(ctx, "dst-rg-race"); err == nil { + t.Error("the definition survived, parented to a group that does not exist") + } + + replicas, err := st.Resources().ListByDefinition(ctx, "dst-rg-race") + if err != nil { + t.Fatalf("list the target's replicas: %v", err) + } + + // Non-vacuous because its control, TestRDCloneKeepsGoingWhenTheParentGroupIsThere, + // asserts this same fixture places one when the group is there. + if len(replicas) != 0 { + t.Errorf("%d replica(s) left behind pointing at a definition that was rolled back", + len(replicas)) + } + + // The source is untouched, and so is the snapshot the clone took: it may + // be the only copy of something, and deleting one is the operator's call. + if _, err := st.ResourceDefinitions().Get(ctx, "src-rg-race"); err != nil { + t.Errorf("the rollback took the source with it: %v", err) + } + + if _, err := st.Snapshots().Get(ctx, "src-rg-race", + cloneSnapshotName("dst-rg-race")); err != nil { + t.Errorf("the rollback deleted the internal snapshot: %v", err) + } +} + +// The positive control: the same clone with the group where it should be. +func TestRDCloneKeepsGoingWhenTheParentGroupIsThere(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, st, "src-rg-ok", "grp-there", true) + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-rg-ok", map[string]any{ + "name": "dst-rg-ok", + "use_zfs_clone": true, + }) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status = %d, want 201", resp.StatusCode) + } + + if _, err := st.ResourceDefinitions().Get(ctx, "dst-rg-ok"); err != nil { + t.Errorf("target RD not persisted: %v", err) + } + + // This fixture places a replica, which is what makes the rolled-back + // twin's "no replicas left behind" assertion mean something. Without it + // that assertion passes over a clone that never placed one. + replicas, err := st.Resources().ListByDefinition(ctx, "dst-rg-ok") + if err != nil { + t.Fatalf("list the target's replicas: %v", err) + } + + if len(replicas) == 0 { + t.Error("the fixture placed no replicas, so the rollback twin proves nothing") + } +} + +// The restore path inherits the same group the same way, and had the same gap. +func TestSnapshotRestoreRollsBackWhenTheParentGroupIsGone(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "restore-src", + ResourceGroupName: "grp-gone-restore", + }); err != nil { + t.Fatalf("seed the source: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-rg", + ResourceName: "restore-src", + Nodes: []string{"n1"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 1024 * 1024}, + }, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + body, _ := json.Marshal(map[string]string{ + "to_resource": "restore-dst", + "from_snapshot": "snap-rg", + }) + + resp := httpPost(t, base+"/v1/resource-definitions/restore-src/snapshot-restore-resource", body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusNotFound { + t.Fatalf("status = %d, want 404 — the parent group is gone", resp.StatusCode) + } + + if _, err := st.ResourceDefinitions().Get(ctx, "restore-dst"); err == nil { + t.Error("the definition survived, parented to a group that does not exist") + } +} + +// The positive control for the restore path. +func TestSnapshotRestoreKeepsGoingWhenTheParentGroupIsThere(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + + if err := st.ResourceGroups().Create(ctx, + &apiv1.ResourceGroup{Name: "grp-there-restore"}); err != nil { + t.Fatalf("seed RG: %v", err) + } + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "restore-src-ok", + ResourceGroupName: "grp-there-restore", + }); err != nil { + t.Fatalf("seed the source: %v", err) + } + + if err := st.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-rg-ok", + ResourceName: "restore-src-ok", + Nodes: []string{"n1"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 1024 * 1024}, + }, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + base, stop := startServerWithStore(t, st) + defer stop() + + body, _ := json.Marshal(map[string]string{ + "to_resource": "restore-dst-ok", + "from_snapshot": "snap-rg-ok", + }) + + resp := httpPost(t, base+"/v1/resource-definitions/restore-src-ok/snapshot-restore-resource", body) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status = %d, want 201", resp.StatusCode) + } + + if _, err := st.ResourceDefinitions().Get(ctx, "restore-dst-ok"); err != nil { + t.Errorf("target RD not persisted: %v", err) + } +} + +// errReplicaDeleteFailed stands in for what a satellite writing status on the +// very replicas being reaped produces: a conflict, a timeout, an RBAC gap. +var errReplicaDeleteFailed = errors.New("probe: replica delete failed") + +// failingReplicaDeletes is a store whose replica deletes fail and whose +// everything else works. +type failingReplicaDeletes struct { + store.ResourceStore +} + +func (f failingReplicaDeletes) Delete(context.Context, string, string) error { + return errReplicaDeleteFailed +} + +type failingCascadeStore struct { + store.Store +} + +func (f failingCascadeStore) Resources() store.ResourceStore { + return failingReplicaDeletes{f.Store.Resources()} +} + +// The rollback drops the definition ONLY if the replicas went with it. +// CascadeDeleteResources stops at the first replica it cannot delete and +// leaves the rest untried, so deleting the parent anyway produces the orphan +// this rollback exists to avoid: a Resource whose RD vanished never gets a +// DeletionTimestamp, its satellite's finalizer never runs, and the DRBD minor, +// port and peer entries stay live until the next create with that name +// collides with them — which on the CSI path is the retry, because the target +// name is deterministic. +// +// Both other doors that perform this teardown refuse to proceed on a failed +// cascade. This one does too, and says what it left behind. +func TestRDCloneRollbackKeepsTheDefinitionWhenTheCascadeFails(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-orphan", "grp-orphan-gone", false) + + st := failingCascadeStore{backend} + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := postClone(t, base, "src-orphan", map[string]any{ + "name": "dst-orphan", + "use_zfs_clone": true, + }) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusNotFound { + t.Fatalf("status = 404, the code the successful rollback uses — a failed cascade " + + "must not be reported as a rollback") + } + + // The definition stays, because its replicas are still there. + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-orphan"); err != nil { + t.Fatalf("the definition was deleted over replicas that could not be: %v", err) + } + + replicas, err := backend.Resources().ListByDefinition(ctx, "dst-orphan") + if err != nil { + t.Fatalf("list the replicas: %v", err) + } + + if len(replicas) == 0 { + t.Fatal("the fixture left no replicas, so this test proves nothing") + } + + // And the operator is told which definition is now parented to nothing. + var envelope cloneStartedResponse + if err := json.NewDecoder(resp.Body).Decode(&envelope); err != nil { + t.Fatalf("decode the envelope: %v", err) + } + + if envelope.Messages == nil || len(*envelope.Messages) == 0 { + t.Fatal("empty envelope") + } + + msg := (*envelope.Messages)[0].Message + if !strings.Contains(msg, "dst-orphan") || !strings.Contains(msg, "still there") { + t.Errorf("message = %q, want it to name the definition left behind", msg) + } +} + +// errRGReadFailed stands in for what a re-check can hit that says nothing +// about the group: apiserver unavailability, a timeout, a decode failure, a +// cancelled request context. getRGWithCacheRetry returns immediately on +// anything that is not NotFound, so all of them arrive here. +var errRGReadFailed = errors.New("probe: transient failure reading the resource group") + +type failingRGReads struct { + store.ResourceGroupStore +} + +func (f failingRGReads) Get(context.Context, string) (apiv1.ResourceGroup, error) { + return apiv1.ResourceGroup{}, errRGReadFailed +} + +type failingRGReadStore struct { + store.Store +} + +func (f failingRGReadStore) ResourceGroups() store.ResourceGroupStore { + return failingRGReads{f.Store.ResourceGroups()} +} + +// The post-write check is a safety net over a restore that already succeeded. +// When the net itself cannot be inspected, undoing the restore trades a rare +// dangling parent group for a certain lost restore — and a worse one, because +// this endpoint has no idempotent-replay gate: the definition stays behind and +// every later attempt under that name meets AlreadyExists and answers 409 from +// then on. A blip in a check that has nothing to do with whether the restore +// worked must not be able to do that. +func TestSnapshotRestoreSurvivesAFailedParentGroupRecheck(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.ResourceGroups().Create(ctx, + &apiv1.ResourceGroup{Name: "grp-flaky"}); err != nil { + t.Fatalf("seed RG: %v", err) + } + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "flaky-src", + ResourceGroupName: "grp-flaky", + }); err != nil { + t.Fatalf("seed the source: %v", err) + } + + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-flaky", + ResourceName: "flaky-src", + Nodes: []string{"n1"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 1024 * 1024}, + }, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + base, stop := startServerWithStore(t, failingRGReadStore{backend}) + defer stop() + + body, _ := json.Marshal(map[string]string{ + "to_resource": "flaky-dst", + "from_snapshot": "snap-flaky", + }) + + resp := httpPost(t, base+"/v1/resource-definitions/flaky-src/snapshot-restore-resource", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status = %d, want 201 — the restore succeeded; only the re-check failed", + resp.StatusCode) + } + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil { + t.Fatalf("decode the envelope: %v", err) + } + + if !envelopeWarnsAbout(rcs, "grp-flaky") { + t.Errorf("nothing in %+v warns that the parent group went unverified", rcs) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "flaky-dst"); err != nil { + t.Errorf("the restored definition was rolled back over a failed check: %v", err) + } + + vds, err := backend.VolumeDefinitions().List(ctx, "flaky-dst") + if err != nil { + t.Fatalf("list the restored volumes: %v", err) + } + + if len(vds) != 1 { + t.Errorf("restored definition has %d volume(s), want 1", len(vds)) + } +} + +// acceptedButRetainedDeletes is the shape a cluster actually has. Every +// Resource carries the satellite's finalizer, so an apiserver DELETE on one is +// ACCEPTED with no error and the object stays until the finalizer clears, and +// the listing does not filter what is Terminating. The in-memory store deletes +// synchronously, which is why the suite could not see it. +// +// This double accepts the delete and keeps the replica listed, unstamped: +// the ordinary outcome of a cascade in a cluster, and the one that used to +// walk straight past "the definition goes only if the replicas went". +type acceptedButRetainedDeletes struct { + store.ResourceStore +} + +func (acceptedButRetainedDeletes) Delete(context.Context, string, string) error { + return nil +} + +type acceptedButRetainedStore struct { + store.Store +} + +func (f acceptedButRetainedStore) Resources() store.ResourceStore { + return acceptedButRetainedDeletes{f.Store.Resources()} +} + +// CascadeDeleteResources answers nil twice over: when every replica went, and +// when its pass budget ran out with replicas still listed. The rollback read +// that nil as the first, so on the shape above it dropped the parent over +// replicas that were never stamped for deletion — the orphan it exists to +// avoid, since nothing gives a Resource a DeletionTimestamp once its +// definition is gone. +func TestRDCloneRollbackKeepsTheDefinitionWhenTheCascadeOnlyAcceptedTheDeletes(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-retained", "grp-retained-gone", false) + + base, stop := startServerWithStore(t, acceptedButRetainedStore{backend}) + defer stop() + + resp := postClone(t, base, "src-retained", map[string]any{ + "name": "dst-retained", + "use_zfs_clone": true, + }) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusNotFound { + t.Fatalf("status = 404, the code a completed rollback uses — the replicas are " + + "still there and were never stamped for deletion") + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-retained"); err != nil { + t.Fatalf("the definition was dropped over replicas that are still there: %v", err) + } + + replicas, err := backend.Resources().ListByDefinition(ctx, "dst-retained") + if err != nil { + t.Fatalf("list the replicas: %v", err) + } + + if len(replicas) == 0 { + t.Fatal("the fixture left no replicas, so this test proves nothing") + } + + var envelope cloneStartedResponse + if err := json.NewDecoder(resp.Body).Decode(&envelope); err != nil { + t.Fatalf("decode the envelope: %v", err) + } + + if envelope.Messages == nil || len(*envelope.Messages) == 0 { + t.Fatal("empty envelope") + } + + msg := (*envelope.Messages)[0].Message + if !strings.Contains(msg, "dst-retained") || !strings.Contains(msg, "still there") { + t.Errorf("message = %q, want it to name the definition left behind", msg) + } +} + +// The clone twin of TestSnapshotRestoreSurvivesAFailedParentGroupRecheck. The +// post-write check is a safety net over a clone that already succeeded, so a +// read failure that says nothing about the group must not undo it. +func TestRDCloneSurvivesAFailedParentGroupRecheck(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-flaky-rg", "grp-flaky-clone", true) + + base, stop := startServerWithStore(t, failingRGReadStore{backend}) + defer stop() + + resp := postClone(t, base, "src-flaky-rg", map[string]any{ + "name": "dst-flaky-rg", + "use_zfs_clone": true, + }) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status = %d, want 201 — the clone succeeded; only the re-check failed", + resp.StatusCode) + } + + // Proceeding is right; being silent about it is not. The caller is told + // the clone worked, and has to be told the group behind it went + // unverified, which is the one thing that would make them look. + var envelope cloneStartedResponse + if err := json.NewDecoder(resp.Body).Decode(&envelope); err != nil { + t.Fatalf("decode the envelope: %v", err) + } + + if envelope.Messages == nil { + t.Fatal("empty envelope") + } + + if !envelopeWarnsAbout(*envelope.Messages, "grp-flaky-clone") { + t.Errorf("nothing in %+v warns that the parent group went unverified", + *envelope.Messages) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-flaky-rg"); err != nil { + t.Errorf("the clone was rolled back over a failed check: %v", err) + } + + vds, err := backend.VolumeDefinitions().List(ctx, "dst-flaky-rg") + if err != nil { + t.Fatalf("list the cloned volumes: %v", err) + } + + if len(vds) == 0 { + t.Error("the clone was undone: the target has no volumes") + } +} + +// The restore twin of TestRDCloneRollbackKeepsTheDefinitionWhenTheCascadeFails. +// Both endpoints materialise the same way and compensate the same way, so a +// failed cascade has to leave the same trace on both. +func TestSnapshotRestoreRollbackKeepsTheDefinitionWhenTheCascadeFails(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + // A deployed source, so the restore places a replica the cascade then has + // to fail on. Without one the rollback has nothing to do and answers the + // success code whatever the cascade would have done. + seedGroupedCloneSource(t, backend, "restore-orphan-src", "grp-restore-orphan-gone", false) + + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-restore-orphan", + ResourceName: "restore-orphan-src", + Nodes: []string{"node-a"}, + VolumeDefinitions: []apiv1.SnapshotVolumeDef{ + {VolumeNumber: 0, SizeKib: 64 * 1024}, + }, + }); err != nil { + t.Fatalf("seed the snapshot: %v", err) + } + + base, stop := startServerWithStore(t, failingCascadeStore{backend}) + defer stop() + + // node_names, so the restore places the replica whose reaping then fails. + body, _ := json.Marshal(map[string]any{ + "to_resource": "restore-orphan-dst", + "from_snapshot": "snap-restore-orphan", + "node_names": []string{"node-a"}, + }) + + resp := httpPost(t, base+"/v1/resource-definitions/restore-orphan-src/snapshot-restore-resource", body) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode == http.StatusNotFound { + t.Fatalf("status = 404, the code a completed rollback uses — the cascade failed") + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "restore-orphan-dst"); err != nil { + t.Fatalf("the definition was deleted over replicas that could not be: %v", err) + } + + var rcs []apiv1.APICallRc + if err := json.NewDecoder(resp.Body).Decode(&rcs); err != nil { + t.Fatalf("decode the envelope: %v", err) + } + + if len(rcs) == 0 { + t.Fatal("empty envelope") + } + + if !strings.Contains(rcs[0].Message, "restore-orphan-dst") || + !strings.Contains(rcs[0].Message, "still there") { + t.Errorf("message = %q, want it to name the definition left behind", rcs[0].Message) + } +} + +// The CSI target name is deterministic and linstor-csi retries CreateVolume on +// any error, so the retry after a failed rollback meets the definition that was +// deliberately left behind, matches its marker and used to be answered 201 +// "already cloned" — for a definition parented to a group that is gone. The +// 500 telling the operator to delete it never reaches a human, because the +// machine turns the failure into a success first. +func TestRDCloneRetryAfterAFailedRollbackIsNotReportedAsDone(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + seedGroupedCloneSource(t, backend, "src-retry", "grp-retry-gone", false) + + st := failingCascadeStore{backend} + + base, stop := startServerWithStore(t, st) + defer stop() + + first := postClone(t, base, "src-retry", map[string]any{ + "name": "dst-retry", + "use_zfs_clone": true, + }) + _ = first.Body.Close() + + if first.StatusCode != http.StatusInternalServerError { + t.Fatalf("first attempt = %d, want 500 — the rollback failed", first.StatusCode) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-retry"); err != nil { + t.Fatalf("the definition was not left behind, so the retry has nothing to meet: %v", err) + } + + second := postClone(t, base, "src-retry", map[string]any{ + "name": "dst-retry", + "use_zfs_clone": true, + }) + defer func() { _ = second.Body.Close() }() + + if second.StatusCode == http.StatusCreated { + t.Fatalf("retry = 201 — a definition parented to a group that is gone was " + + "reported as an already-finished clone") + } + + var envelope cloneStartedResponse + if err := json.NewDecoder(second.Body).Decode(&envelope); err != nil { + t.Fatalf("decode the envelope: %v", err) + } + + if envelope.Messages == nil || len(*envelope.Messages) == 0 { + t.Fatal("empty envelope") + } + + msg := (*envelope.Messages)[0].Message + if !strings.Contains(msg, "grp-retry-gone") { + t.Errorf("message = %q, want it to name the group that is gone", msg) + } +} + +// The control Ivan asked for: with the group alive the same replay is a +// legitimate idempotent 201, so this is not a complaint about the replay gate +// in general. +func TestRDCloneReplayWithALiveGroupIsStillAnIdempotentSuccess(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + seedGroupedCloneSource(t, backend, "src-replay-ok", "grp-replay-ok", true) + + base, stop := startServerWithStore(t, backend) + defer stop() + + for attempt := 1; attempt <= 2; attempt++ { + resp := postClone(t, base, "src-replay-ok", map[string]any{ + "name": "dst-replay-ok", + "use_zfs_clone": true, + }) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("attempt %d = %d, want 201", attempt, resp.StatusCode) + } + } +} + +// handleRDClone splits on the source's volume count, and the branch that copies +// a bare definition carried the source's resource group over verbatim with no +// check on either side of its write. +func TestRDCloneOfAVolumelessSourceRollsBackWhenTheParentGroupIsGone(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "src-shell", + ResourceGroupName: "grp-shell-gone", + }); err != nil { + t.Fatalf("seed the volume-less source: %v", err) + } + + base, stop := startServerWithStore(t, backend) + defer stop() + + resp := postClone(t, base, "src-shell", map[string]any{"name": "dst-shell"}) + _ = resp.Body.Close() + + if resp.StatusCode == http.StatusCreated { + t.Fatalf("status = 201 — the shell was cloned into a group that does not exist") + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-shell"); err == nil { + t.Error("the cloned shell survived, parented to a group that does not exist") + } +} + +// Its control: the same volume-less clone with the group where it should be. +func TestRDCloneOfAVolumelessSourceKeepsGoingWhenTheParentGroupIsThere(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.ResourceGroups().Create(ctx, + &apiv1.ResourceGroup{Name: "grp-shell-ok"}); err != nil { + t.Fatalf("seed RG: %v", err) + } + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "src-shell-ok", + ResourceGroupName: "grp-shell-ok", + }); err != nil { + t.Fatalf("seed the volume-less source: %v", err) + } + + base, stop := startServerWithStore(t, backend) + defer stop() + + resp := postClone(t, base, "src-shell-ok", map[string]any{"name": "dst-shell-ok"}) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusCreated { + t.Fatalf("status = %d, want 201", resp.StatusCode) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "dst-shell-ok"); err != nil { + t.Errorf("the cloned shell was not persisted: %v", err) + } +} + +// envelopeWarnsAbout reports whether any entry rides back in the warn band and +// names the group, which is what tells the operator the safety net did not run +// rather than that it passed. +func envelopeWarnsAbout(rcs []apiv1.APICallRc, rgName string) bool { + for i := range rcs { + if rcs[i].RetCode&maskWarn != 0 && strings.Contains(rcs[i].Message, rgName) { + return true + } + } + + return false +} diff --git a/pkg/rest/server.go b/pkg/rest/server.go index 6d593c9d..654b6155 100644 --- a/pkg/rest/server.go +++ b/pkg/rest/server.go @@ -366,6 +366,15 @@ func isSSEPath(p string) bool { return p == "/v1/events/drbd/promotion" || p == "/v1/events/nodes" } +// gracefulShutdownWindow is how long Shutdown waits for handlers that are +// still running. It is derived from detachedRollbackBudget, which bounds every +// context a handler detaches from its caller (detachedCompensation: the +// post-write group re-read, the rollbacks, rollbackSpawn), so it is the +// longest a handler can still be working after its caller has gone. See that +// constant for the whole chain, down to the termination grace the manifests +// give the pod. +const gracefulShutdownWindow = detachedRollbackBudget + shutdownMargin + // waitAndShutdown blocks until ctx is cancelled or any listener // reports a fatal error, then gracefully shuts down every server. func waitAndShutdown(ctx context.Context, servers []*http.Server, errCh <-chan error) error { @@ -383,7 +392,7 @@ func waitAndShutdown(ctx context.Context, servers []*http.Server, errCh <-chan e // one of them reported a fatal serve error — otherwise the surviving // server's goroutine leaks and keeps its port bound, blocking a clean // restart. - shutCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 10*time.Second) + shutCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), gracefulShutdownWindow) defer cancel() var shutErr error diff --git a/pkg/rest/snapshot_restore.go b/pkg/rest/snapshot_restore.go index f4a2bf47..3fc71418 100644 --- a/pkg/rest/snapshot_restore.go +++ b/pkg/rest/snapshot_restore.go @@ -20,12 +20,14 @@ package rest import ( "context" - "maps" "net/http" "slices" "strconv" "strings" + "github.com/cockroachdb/errors" + "sigs.k8s.io/controller-runtime/pkg/log" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/store" "github.com/cozystack/blockstor/pkg/validate" @@ -313,17 +315,59 @@ func (s *Server) handleSnapshotRestore(w http.ResponseWriter, r *http.Request) { // target is left an empty shell for the operator / linstor-csi to // place (restore-then-scale-out); an explicit node list is still // stamped verbatim inside materializeRestoredRD. - newRDName, err := s.materializeRestoredRD(r.Context(), srcRD, &req, &snap, false) + made, err := s.materializeRestoredRD(r.Context(), srcRD, &req, &snap, false) if err != nil { + s.writeRestoreMaterialiseFailed(r.Context(), w, snapName, req.ToResource, made, err) + + return + } + + // The group validated is the one that was WRITTEN, not the source's read + // back a second time: re-reading answers a different question, and the + // extra read was itself a way to fail a restore that had already worked. + uncheckedRG, ok := s.restoreParentRGSurvived(r.Context(), w, made) + if !ok { + return + } + + writeRestoreDone(w, "snapshot restored: "+snapName+" → "+made.Name, uncheckedRG) +} + +// writeRestoreMaterialiseFailed answers a restore whose materialisation failed. +// +// A failure after the create is this request's own partial work, and this +// endpoint has no idempotent-replay gate: left in place, the marker-bearing +// definition turns every retry under the deterministic CSI target name into +// AlreadyExists, for good. So that work is rolled back before the answer goes +// out. Anything else is answered as the store error it is. +func (s *Server) writeRestoreMaterialiseFailed( + ctx context.Context, w http.ResponseWriter, snapName, rdName string, made materialisedRD, err error, +) { + var partial *materialiseAfterCreateError + if !errors.As(err, &partial) { writeStoreError(w, err) return } - writeJSON(w, http.StatusCreated, []apiv1.APICallRc{{ + writeJSON(w, http.StatusInternalServerError, []apiv1.APICallRc{*s.failedMaterialiseRefusal(ctx, + "snapshot restore of '"+snapName+"' into '"+rdName+"' failed: "+err.Error(), + "restore", rdName, made.Placed, err)}) +} + +// writeRestoreDone emits the restore's success envelope, with any warning the +// post-write checks want to ride back alongside it. +func writeRestoreDone(w http.ResponseWriter, message string, warn *apiv1.APICallRc) { + rcs := []apiv1.APICallRc{{ RetCode: maskInfo, - Message: "snapshot restored: " + snapName + " → " + newRDName, - }}) + Message: message, + }} + + if warn != nil { + rcs = append(rcs, *warn) + } + + writeJSON(w, http.StatusCreated, rcs) } // validateRestoreNodesHoldSnapshot is the Bug 397 input-validation guard @@ -387,6 +431,99 @@ func resolveSnapshotName(r *http.Request, req *snapshotRestoreRequest) string { return req.SnapshotName } +// uncheckedRestoreGroupWarning tells the caller the restore worked and that +// the group behind it went unverified, which is the one piece of information +// that would make them look. +func uncheckedRestoreGroupWarning(rdName, rgName string, err error) *apiv1.APICallRc { + return &apiv1.APICallRc{ + RetCode: maskWarn, + Message: "resource group '" + rgName + "' could not be re-checked after the " + + "restore: " + err.Error(), + Cause: "the restore itself succeeded; only the safety net over it could not be " + + "inspected, so a group deleted during the restore would not have been caught", + Correc: "confirm resource group '" + rgName + "' still exists", + ObjRefs: map[string]string{ + objRefRscDfn: rdName, + objRefRscGrp: rgName, + }, + } +} + +// restoreParentRGSurvived is the post-write half of the Bug 174 guard on the +// restore path. The restored definition inherits the source's resource group, +// so a `rg d` landing while it materialises leaves it parented to a group that +// is gone. False means the restore has been rolled back and a refusal written. +func (s *Server) restoreParentRGSurvived( + ctx context.Context, w http.ResponseWriter, made materialisedRD, +) (*apiv1.APICallRc, bool) { + newRDName, stampedRG := made.Name, made.StampedRG + + ctx, cancel := detachedCompensation(ctx) + defer cancel() + + survived, err := s.parentRGSurvived(ctx, stampedRG) + if err != nil { + // The CHECK failed, which says nothing about the restore: that + // already succeeded, and this is the safety net over it. Undoing a + // completed restore because the net could not be inspected trades a + // rare dangling group for a certain lost restore — and a much worse + // one, since this endpoint has no idempotent-replay gate, so the + // definition stays behind and every retry under that name meets + // AlreadyExists and answers 409 from then on. + // + // getRGWithCacheRetry returns immediately on anything that is not + // NotFound, so this branch is apiserver unavailability, a timeout or + // a decode failure, none of them a statement about the group. A + // cancelled request is not among them: the read runs on the detached + // context above, so a caller that has gone does not get its restore + // waved through over a group that is gone. + log.FromContext(ctx).Info("could not re-check the restored definition's parent group", + "resourceDefinition", newRDName, "resourceGroup", stampedRG, "reason", err.Error()) + + // And say so to the caller. Proceeding is right; leaving the only + // trace in an apiserver log is not. + return uncheckedRestoreGroupWarning(newRDName, stampedRG, err), true + } + + if survived { + return nil, true + } + + if !made.createdHere() { + writeJSON(w, http.StatusConflict, []apiv1.APICallRc{ + *adoptedOverDeletedGroupRefusal("restore", newRDName, stampedRG, correcRecreateGroupThenRestore), + }) + + return nil, false + } + + rollbackErr := s.rollBackCompensating(ctx, newRDName, made.Placed) + if rollbackErr != nil { + cause, correc := rollbackFailureAdvice(rollbackErr, newRDName) + + writeJSON(w, http.StatusInternalServerError, []apiv1.APICallRc{{ + RetCode: apiCallRcError, + Message: "snapshot restore: " + + rollbackFailedMessage(newRDName, stampedRG, rollbackErr), + Cause: cause, + Correc: correc, + }}) + + return nil, false + } + + writeJSON(w, http.StatusNotFound, []apiv1.APICallRc{{ + RetCode: apiCallRcError, + Message: "snapshot restore rolled back: " + rgDeletedRaceCorrection(stampedRG), + Cause: "the restored definition inherits its parent group from the source, and that " + + "group was deleted while the restore was being materialised; a definition " + + "pointing at a group that is gone lists fine and places badly", + Correc: correcRecreateGroupThenRestore, + }}) + + return nil, false +} + // materializeRestoredRD creates the target RD inheriting the source // RD's LayerStack + Props (snapshot Props win when set) and hydrates // its VolumeDefinitions from the snapshot's recorded volume layout. @@ -413,10 +550,10 @@ func resolveSnapshotName(r *http.Request, req *snapshotRestoreRequest) string { // // An explicit caller node list is always stamped verbatim, regardless of // eagerPlace. -func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *snapshotRestoreRequest, snap *apiv1.Snapshot, eagerPlace bool) (string, error) { +func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *snapshotRestoreRequest, snap *apiv1.Snapshot, eagerPlace bool) (materialisedRD, error) { srcRDObj, err := s.Store.ResourceDefinitions().Get(ctx, srcRD) if err != nil { - return "", err //nolint:wrapcheck // surfaced via writeStoreError + return materialisedRD{}, err //nolint:wrapcheck // surfaced via writeStoreError } newRD := apiv1.ResourceDefinition{ @@ -429,12 +566,12 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s // LINSTOR ties together (the parent RG drives subsequent // auto-placement and prop inheritance). ResourceGroupName: srcRDObj.ResourceGroupName, - Props: maps.Clone(snap.Props), + Props: store.TravellingProps(snap.Props), LayerStack: srcRDObj.LayerStack, } if newRD.Props == nil { - newRD.Props = maps.Clone(srcRDObj.Props) + newRD.Props = store.TravellingProps(srcRDObj.Props) } // Stamp the clone-source so the dispatcher's buildVolumes (called @@ -452,12 +589,14 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s err = s.Store.ResourceDefinitions().Create(ctx, &newRD) if err != nil { - return "", err //nolint:wrapcheck // surfaced via writeStoreError + return materialisedRD{}, err //nolint:wrapcheck // surfaced via writeStoreError } + made := createdRD(newRD.Name, newRD.ResourceGroupName) + err = hydrateVolumesFromSnapshot(ctx, s, newRD.Name, snap) if err != nil { - return "", err + return made, &materialiseAfterCreateError{err: err} } // Bug 354: stamp per-node Resource CRDs so satellites have something @@ -466,14 +605,80 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s // observed a Resource for the new RD, so the BlockstorRestoreFromSnapshot // prop marker on the RD was dead code and the restored RD stayed an // empty shell. Mirrors upstream CtrlSnapshotRestoreApiCallHandler. - err = s.placeRestoredResources(ctx, srcRD, &newRD, req, snap, eagerPlace) + made.Placed, err = s.placeRestoredResources(ctx, srcRD, &newRD, req, snap, eagerPlace) if err != nil { - return "", err + return made, &materialiseAfterCreateError{err: err} } - return newRD.Name, nil + return made, nil +} + +// materialisedRD is what a materialisation wrote, and whether the definition +// under that name is this call's to undo. +// +// Created-or-adopted is the line every compensation on these paths draws. A +// definition this call created is its own work, and a rollback may take it with +// everything under it. A definition that was already there, adopted as the +// leftover of an earlier attempt at the same operation, is not: that attempt +// may still be running, and reaping it deletes someone else's clone. +// +// So the answer is not a field a producer can forget. It is carried in an +// unexported origin whose zero value says nothing, and the two constructors are +// the only way to state it; a literal that skips them leaves the origin +// unstated, and createdHere answers false for it, which is the safe side of the +// line. Every materialisation on this branch creates, so only createdRD is +// called here; the shape exists so a door that starts tolerating a leftover +// cannot hand a compensation someone else's definition by omission. +type materialisedRD struct { + // Name is the target definition. + Name string + // StampedRG is the resource group the definition was written with. + StampedRG string + // Placed are the nodes this call stamped a replica on. + Placed []string + + origin rdOrigin +} + +// rdOrigin says who the definition under the target name belongs to. +type rdOrigin uint8 + +const ( + // rdOriginUnstated is the zero value, and never a claim of ownership. + rdOriginUnstated rdOrigin = iota + // rdOriginCreated is this call's own create of the definition. + rdOriginCreated + // rdOriginAdopted is a definition that was already there. + rdOriginAdopted +) + +// createdRD records a definition this call created. +func createdRD(name, stampedRG string) materialisedRD { + return materialisedRD{Name: name, StampedRG: stampedRG, origin: rdOriginCreated} +} + +// adoptedRD records a definition that was already there when this call ran. +func adoptedRD(name, stampedRG string) materialisedRD { + return materialisedRD{Name: name, StampedRG: stampedRG, origin: rdOriginAdopted} } +// createdHere reports whether a compensation may reap this definition. +func (m materialisedRD) createdHere() bool { + return m.origin == rdOriginCreated +} + +// materialiseAfterCreateError is a materialisation that failed after this call +// created the target definition. What stands under the name is then this +// call's own partial work, and a caller may undo it; any other failure leaves +// whatever was there before the call, which may be another attempt's. +type materialiseAfterCreateError struct { + err error +} + +func (e *materialiseAfterCreateError) Error() string { return e.err.Error() } + +func (e *materialiseAfterCreateError) Unwrap() error { return e.err } + // placeRestoredResources stamps the Resource CRDs that materialise the // restored RD on the cluster. Two branches mirror upstream LINSTOR's // snapshot-restore handler: @@ -504,7 +709,7 @@ func (s *Server) materializeRestoredRD(ctx context.Context, srcRD string, req *s // // The Nodes / NodeNames request fields are aliased — callers may use // either; we normalise to one canonical list before iterating. -func (s *Server) placeRestoredResources(ctx context.Context, srcRDName string, newRD *apiv1.ResourceDefinition, req *snapshotRestoreRequest, snap *apiv1.Snapshot, eagerPlace bool) error { +func (s *Server) placeRestoredResources(ctx context.Context, srcRDName string, newRD *apiv1.ResourceDefinition, req *snapshotRestoreRequest, snap *apiv1.Snapshot, eagerPlace bool) ([]string, error) { nodes := canonicalRestoreNodeList(req) if len(nodes) == 0 { @@ -518,7 +723,7 @@ func (s *Server) placeRestoredResources(ctx context.Context, srcRDName string, n // snap keeps the signature uniform with the eager branch. _ = snap - return nil + return nil, nil } // Clone path (eager): stamp one replica on every snapshot node @@ -554,10 +759,11 @@ func (s *Server) placeRestoredResources(ctx context.Context, srcRDName string, n // pool — pool names are cluster-wide in LINSTOR, so the fallback only // matters when the source replica on that node is already gone). // Idempotent on duplicates in the list (one Create per unique node). -func (s *Server) stampRestoredResourcesOnNodes(ctx context.Context, srcRDName, newRDName string, nodes []string) error { +func (s *Server) stampRestoredResourcesOnNodes(ctx context.Context, srcRDName, newRDName string, nodes []string) ([]string, error) { poolByNode, fallbackPool := storPoolsByNodeFromSourceRD(ctx, s.Store, srcRDName) seen := make(map[string]struct{}, len(nodes)) + placed := make([]string, 0, len(nodes)) for _, node := range nodes { if node == "" { @@ -586,11 +792,13 @@ func (s *Server) stampRestoredResourcesOnNodes(ctx context.Context, srcRDName, n err := s.Store.Resources().Create(ctx, &res) if err != nil { - return err //nolint:wrapcheck // surfaced via writeStoreError + return placed, err //nolint:wrapcheck // wrapped as materialiseAfterCreateError by the caller } + + placed = append(placed, node) } - return nil + return placed, nil } // canonicalRestoreNodeList collapses the request's two node-list @@ -661,7 +869,7 @@ func hydrateVolumesFromSnapshot(ctx context.Context, s *Server, rdName string, s err := s.Store.VolumeDefinitions().Create(ctx, rdName, &vd) if err != nil { - return err //nolint:wrapcheck // surfaced via writeStoreError + return err //nolint:wrapcheck // wrapped as materialiseAfterCreateError by the caller } } diff --git a/pkg/rest/snapshots.go b/pkg/rest/snapshots.go index 0cce3a55..f6f48715 100644 --- a/pkg/rest/snapshots.go +++ b/pkg/rest/snapshots.go @@ -951,7 +951,7 @@ func (s *Server) hydrateSnapshotFromRD(ctx context.Context, snap *apiv1.Snapshot } if snap.Props == nil { - snap.Props = srcRD.Props + snap.Props = store.TravellingProps(srcRD.Props) } if len(snap.Nodes) == 0 { diff --git a/pkg/rest/spawn.go b/pkg/rest/spawn.go index 9b35000e..89a6889b 100644 --- a/pkg/rest/spawn.go +++ b/pkg/rest/spawn.go @@ -304,8 +304,13 @@ func copyVolumeGroupProps(vgs []apiv1.VolumeGroup, volNumber int32) map[string]s // rollbackSpawn best-effort cleans up a half-spawned RD. Errors are // swallowed because we are already on an error path; the controller's // reconciler will sweep stale RDs on next pass. +// +// Detached like every other compensation, and bounded by the same budget, so +// the graceful-shutdown window derived from that budget covers it too: an +// unbounded delete against an apiserver that stopped answering would outlive +// the window and be cut off by the kill instead. func rollbackSpawn(ctx context.Context, st store.Store, rdName string) { - deleteCtx, cancel := context.WithCancel(context.WithoutCancel(ctx)) + deleteCtx, cancel := detachedCompensation(ctx) defer cancel() err := st.ResourceDefinitions().Delete(deleteCtx, rdName) diff --git a/pkg/store/inmemory_resource_definition.go b/pkg/store/inmemory_resource_definition.go index b6c6df6e..06234153 100644 --- a/pkg/store/inmemory_resource_definition.go +++ b/pkg/store/inmemory_resource_definition.go @@ -59,6 +59,11 @@ func (s *inMemoryResourceDefinitions) Get(_ context.Context, name string) (apiv1 return rd, nil } +// GetUncached is Get: the in-memory store has no cache to trail. +func (s *inMemoryResourceDefinitions) GetUncached(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { + return s.Get(ctx, name) +} + func (s *inMemoryResourceDefinitions) Create(_ context.Context, rd *apiv1.ResourceDefinition) error { if rd == nil { return errors.New("nil ResourceDefinition") diff --git a/pkg/store/k8s/rd_patch_cache_miss_test.go b/pkg/store/k8s/rd_patch_cache_miss_test.go new file mode 100644 index 00000000..a21563fc --- /dev/null +++ b/pkg/store/k8s/rd_patch_cache_miss_test.go @@ -0,0 +1,126 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s_test + +import ( + "context" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// definitionCacheTrails is a client whose cache has not seen any definition +// yet: every Get of one answers NotFound, the way an informer does right after +// another client created it. Writes go through. +type definitionCacheTrails struct{ ctrlclient.Client } + +func (d definitionCacheTrails) Get( + ctx context.Context, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption, +) error { + if _, ok := obj.(*crdv1alpha1.ResourceDefinition); ok { + return apierrors.NewNotFound(schema.GroupResource{Resource: "resourcedefinitions"}, key.Name) + } + + return d.Client.Get(ctx, key, obj, opts...) //nolint:wrapcheck // pass-through test double +} + +// A patch written moments after the definition was created reads it at the +// peak of the informer's lag. Taking the cache's NotFound there dropped the +// write as if the definition were gone: the abandoned-rollback mark, written +// first thing in a compensation, did not land, and a rollback that then died +// left a partial clone the replay answered 201 over. +func TestResourceDefinitionPatchLandsOnADefinitionTheCacheHasNotSeen(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + ctx := t.Context() + seed := k8s.New(fixture.client) + + if err := seed.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-patch-lag"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + t.Cleanup(func() { _ = seed.ResourceDefinitions().Delete(context.Background(), "pvc-patch-lag") }) + + st := k8s.NewWithAPIReader(definitionCacheTrails{fixture.client}, fixture.client) + + err := st.ResourceDefinitions().PatchResourceDefinitionSpec(ctx, "pvc-patch-lag", + func(rd *apiv1.ResourceDefinition) error { + if rd.Props == nil { + rd.Props = map[string]string{} + } + + rd.Props["Aux/patched"] = "yes" + + return nil + }) + if err != nil { + t.Fatalf("patch of a definition the cache has not seen: %v", err) + } + + got, err := seed.ResourceDefinitions().Get(ctx, "pvc-patch-lag") + if err != nil { + t.Fatalf("read back: %v", err) + } + + if got.Props["Aux/patched"] != "yes" { + t.Errorf("the patch did not land: props %v", got.Props) + } +} + +// definitionCacheIsStale holds every definition as it was before its props +// were written. +type definitionCacheIsStale struct{ ctrlclient.Client } + +func (d definitionCacheIsStale) Get( + ctx context.Context, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption, +) error { + err := d.Client.Get(ctx, key, obj, opts...) + if rd, ok := obj.(*crdv1alpha1.ResourceDefinition); ok && err == nil { + rd.Spec = crdv1alpha1.ResourceDefinitionSpec{} + rd.Annotations = nil + } + + return err //nolint:wrapcheck // pass-through test double +} + +// GetUncached exists for the decision a cache that holds the object stale gets +// wrong: Get falls back to the API server only when the cache has nothing. +func TestResourceDefinitionGetUncachedReadsPastAStaleCache(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + ctx := t.Context() + seed := k8s.New(fixture.client) + + if err := seed.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{ + Name: "pvc-stale-cache", Props: map[string]string{"Aux/fresh": "yes"}, + }); err != nil { + t.Fatalf("seed definition: %v", err) + } + + t.Cleanup(func() { _ = seed.ResourceDefinitions().Delete(context.Background(), "pvc-stale-cache") }) + + st := k8s.NewWithAPIReader(definitionCacheIsStale{fixture.client}, fixture.client) + + if cached, err := st.ResourceDefinitions().Get(ctx, "pvc-stale-cache"); err != nil || cached.Props["Aux/fresh"] != "" { + t.Fatalf("fixture: the cached read was supposed to be stale: %v %v", cached.Props, err) + } + + live, err := st.ResourceDefinitions().GetUncached(ctx, "pvc-stale-cache") + if err != nil { + t.Fatalf("GetUncached: %v", err) + } + + if live.Props["Aux/fresh"] != "yes" { + t.Errorf("GetUncached answered from the stale cache: props %v", live.Props) + } +} diff --git a/pkg/store/k8s/resource_definitions.go b/pkg/store/k8s/resource_definitions.go index 9e19e57a..1b8a9984 100644 --- a/pkg/store/k8s/resource_definitions.go +++ b/pkg/store/k8s/resource_definitions.go @@ -86,6 +86,16 @@ func (s *resourceDefinitions) Get(ctx context.Context, name string) (apiv1.Resou return s.getUncached(ctx, name) } +// GetUncached reads the definition from the API server when the store has a +// direct reader, and through the client otherwise. +func (s *resourceDefinitions) GetUncached(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { + if s.apiReader == nil { + return s.Get(ctx, name) + } + + return s.getUncached(ctx, name) +} + func (s *resourceDefinitions) Create(ctx context.Context, in *apiv1.ResourceDefinition) error { if in == nil { return errors.New("nil ResourceDefinition") @@ -242,22 +252,16 @@ func (s *resourceDefinitions) PatchResourceDefinitionSpec(ctx context.Context, n } return errors.Wrapf(retry.RetryOnConflict(patchRetryBackoff(), func() error { - var existing crdv1alpha1.ResourceDefinition - - err := s.c.Get(ctx, types.NamespacedName{Name: Name(name)}, &existing) + existing, err := s.getForPatch(ctx, name) if err != nil { - if apierrors.IsNotFound(err) { - return errors.Wrapf(store.ErrNotFound, "resource definition %q", name) - } - - return errors.Wrapf(err, "get ResourceDefinition %q", name) + return err } base := existing.DeepCopy() // Surface as wire shape so the closure runs in the same // vocabulary as REST handlers. - wire := crdToWireRD(&existing) + wire := crdToWireRD(existing) err = mutate(&wire) if err != nil { @@ -284,7 +288,7 @@ func (s *resourceDefinitions) PatchResourceDefinitionSpec(ctx context.Context, n mergeUserAnnotationsInto(&existing.ObjectMeta, wire.Annotations) - return s.c.Patch(ctx, &existing, ctrlclient.MergeFromWithOptions(base, ctrlclient.MergeFromWithOptimisticLock{})) + return s.c.Patch(ctx, existing, ctrlclient.MergeFromWithOptions(base, ctrlclient.MergeFromWithOptimisticLock{})) }), "patch ResourceDefinition %q", name) } @@ -303,6 +307,40 @@ func (s *resourceDefinitions) Delete(ctx context.Context, name string) error { return nil } +// getForPatch reads the object a patch starts from. A cache NotFound is +// re-read live, the way Get does: a caller that patches a definition it has +// just created reads it at the peak of the informer's lag, and taking the +// cache's word there drops the write as if the definition were gone. A stale +// object the cache does hold costs nothing, since the optimistic lock refuses +// the patch and the retry reads again. +func (s *resourceDefinitions) getForPatch(ctx context.Context, name string) (*crdv1alpha1.ResourceDefinition, error) { + var existing crdv1alpha1.ResourceDefinition + + err := s.c.Get(ctx, types.NamespacedName{Name: Name(name)}, &existing) + if err == nil { + return &existing, nil + } + + if !apierrors.IsNotFound(err) { + return nil, errors.Wrapf(err, "get ResourceDefinition %q", name) + } + + if s.apiReader == nil { + return nil, errors.Wrapf(store.ErrNotFound, "resource definition %q", name) + } + + err = s.apiReader.Get(ctx, types.NamespacedName{Name: Name(name)}, &existing) + if err == nil { + return &existing, nil + } + + if apierrors.IsNotFound(err) { + return nil, errors.Wrapf(store.ErrNotFound, "resource definition %q", name) + } + + return nil, errors.Wrapf(err, "get ResourceDefinition %q live", name) +} + // getUncached resolves a cache-miss RD Get against the direct API reader. // nil apiReader (unit/in-memory stores) keeps the cached NotFound. func (s *resourceDefinitions) getUncached(ctx context.Context, name string) (apiv1.ResourceDefinition, error) { diff --git a/pkg/store/local_props.go b/pkg/store/local_props.go new file mode 100644 index 00000000..6ed6b321 --- /dev/null +++ b/pkg/store/local_props.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 store + +import "maps" + +// RollbackAbandonedProp marks a resource definition whose compensation started +// and did not report finishing, with the step it stopped at. The replay gate of +// the clone path refuses over it. +const RollbackAbandonedProp = "BlockstorRollbackAbandoned" + +// restoreFromSnapshotMarker is the `:` a restored or cloned +// definition carries; see TravellingProps for why it never travels. +const restoreFromSnapshotMarker = "BlockstorRestoreFromSnapshot" + +// TravellingProps copies a props bag for use on another object, leaving out +// the props that describe the object they were read from and nothing derived +// from it. Nil stays nil. +// +// A definition's props are copied forward wholesale: onto every snapshot taken +// of it, and from a snapshot, or from the source when the snapshot has none, +// onto every definition restored or cloned from it. A mark that records +// something about THIS definition, read on a copy, says it about a definition +// it was never true of: a healthy clone of a marked source was refused on the +// replay linstor-csi sends, and the correction said to delete it. +// +// The restore marker is the other member of the class: it records where THIS +// definition's data came from, and the dispatcher, the placer and autoplace +// all read it off whatever definition carries it. A volume-less clone of a +// restored definition kept the source's marker, so the first volume added to +// it later was restored from somebody else's snapshot, on that snapshot's +// nodes. Every restore door stamps its own marker after the copy, and nothing +// reads the marker off a snapshot. +func TravellingProps(props map[string]string) map[string]string { + out := maps.Clone(props) + delete(out, RollbackAbandonedProp) + delete(out, restoreFromSnapshotMarker) + + return out +} diff --git a/pkg/store/store.go b/pkg/store/store.go index 8bbe196d..f43d5732 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -182,6 +182,12 @@ type ResourceDefinitionStore interface { Update(ctx context.Context, rd *apiv1.ResourceDefinition) error Delete(ctx context.Context, name string) error + // GetUncached answers the same question as Get from the API server when + // the store has a direct reader. Get falls back to it only on a cache + // NotFound, so a definition the cache holds stale comes back as is; a + // decision made on a prop another request has just written needs this. + GetUncached(ctx context.Context, name string) (apiv1.ResourceDefinition, error) + // PatchResourceDefinitionSpec runs `mutate` against the freshly-fetched // ResourceDefinition wire value and persists the result. On 409 the // fetch+mutate+patch cycle re-runs against fresh state — so disjoint diff --git a/stand/blockstor-apiserver-deploy.yaml b/stand/blockstor-apiserver-deploy.yaml index a592c465..75a05c92 100644 --- a/stand/blockstor-apiserver-deploy.yaml +++ b/stand/blockstor-apiserver-deploy.yaml @@ -74,6 +74,10 @@ spec: metadata: {labels: {app: blockstor-apiserver}} spec: serviceAccountName: blockstor-apiserver + # The REST server's graceful-shutdown window is derived from the + # detached rollback budget (pkg/rest/rg_deleted_race.go); this has to + # outlive it, or a SIGTERM kills a compensation mid-cascade. + terminationGracePeriodSeconds: 20 # Spread replicas across nodes when possible so a single # node loss doesn't take all apiserver pods at once. topologySpreadConstraints: diff --git a/stand/blockstor-deploy.yaml b/stand/blockstor-deploy.yaml index 7f5bffc0..6c7472c5 100644 --- a/stand/blockstor-deploy.yaml +++ b/stand/blockstor-deploy.yaml @@ -71,6 +71,10 @@ spec: metadata: {labels: {app: blockstor-controller}} spec: serviceAccountName: blockstor-controller + # The REST server's graceful-shutdown window is derived from the + # detached rollback budget (pkg/rest/rg_deleted_race.go); this has to + # outlive it, or a SIGTERM kills a compensation mid-cascade. + terminationGracePeriodSeconds: 20 containers: - name: manager image: __REGISTRY__/blockstor-controller:dev