fix(rest): unbreak CSI clone-from-volume and make snapshot restore idempotent

cmd/apiserver/main.go

@@ -198,7 +198,10 @@ func registerProbesAndREST(mgr manager.Manager, st store.Store, flags *apiserver
 		Store:     st,
 		Client:    mgr.GetClient(),
 		Namespace: namespace,
-		OnReady:   ready.MarkBound,
+		// The passphrase Secret is read past the cache, so no informer
+		// watches every Secret in the cluster for it.
+		SecretReader: mgr.GetAPIReader(),
+		OnReady:      ready.MarkBound,
 	})
 	if err != nil {
 		return errors.Wrap(err, "add rest server")

cmd/controller/main.go

@@ -314,7 +314,9 @@ func main() {
 			Store:     st,
 			Client:    mgr.GetClient(),
 			Namespace: controllerNamespace,
-			OnReady:   ready.MarkBound,
+			// As in cmd/apiserver: the passphrase Secret is read past the cache.
+			SecretReader: mgr.GetAPIReader(),
+			OnReady:      ready.MarkBound,
 		}); err != nil {
 			setupLog.Error(err, "Failed to register REST API server")
 			os.Exit(1)

config/manager/manager.yaml

@@ -107,4 +107,4 @@ spec:
       # 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
+      terminationGracePeriodSeconds: 21

internal/cli/group_server_owned_props_test.go

@@ -0,0 +1,60 @@
+// SPDX-License-Identifier: Apache-2.0
+
+package cli_test
+
+import (
+	"context"
+	"strings"
+	"testing"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+// A group's props are copied onto every definition spawned from it, so the CLI
+// door refuses a prop blockstor reads back off a definition the way `rg c` and
+// `rg modify` refuse it on the REST door, and still sets an ordinary one and
+// still deletes a server-owned one a group already carries.
+func TestResourceGroupSetPropertyRefusesServerOwnedProps(t *testing.T) {
+	t.Parallel()
+
+	app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) {
+		_ = backend.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{
+			Name:  "rg-owned",
+			Props: map[string]string{store.RestoreAdoptedProp: "left-from-before"},
+		})
+	})
+
+	if got := app.Run(t.Context(), []string{"rg", "set-property", "rg-owned", "Aux/team", "storage"}); got != 0 {
+		t.Fatalf("setting an ordinary prop exit = %d (stderr: %s)", got, errBuf.String())
+	}
+
+	if got := app.Run(t.Context(), []string{"rg", "set-property", "rg-owned", store.RestoreFromSnapshotProp, "src:snap"}); got == 0 {
+		t.Fatal("a group took a restore marker every spawned definition would inherit")
+	}
+
+	if !strings.Contains(errBuf.String(), "blockstor sets this property itself") {
+		t.Errorf("stderr %q does not say why", errBuf.String())
+	}
+
+	if got := app.Run(t.Context(), []string{"rg", "delete-property", "rg-owned", store.RestoreAdoptedProp}); got != 0 {
+		t.Fatalf("deleting a server-owned prop from a group exit = %d (stderr: %s)", got, errBuf.String())
+	}
+
+	rg, err := appStore(t, app).ResourceGroups().Get(t.Context(), "rg-owned")
+	if err != nil {
+		t.Fatalf("read back: %v", err)
+	}
+
+	if rg.Props["Aux/team"] != "storage" {
+		t.Errorf("the ordinary prop was not set: %v", rg.Props)
+	}
+
+	if _, ok := rg.Props[store.RestoreFromSnapshotProp]; ok {
+		t.Errorf("the refused marker reached the group: %v", rg.Props)
+	}
+
+	if _, ok := rg.Props[store.RestoreAdoptedProp]; ok {
+		t.Errorf("the server-owned prop survived its delete: %v", rg.Props)
+	}
+}

internal/cli/props.go

@@ -88,20 +88,42 @@ var storagePoolProps = withNamedKeyGuard(objectProps("storage pool", 2, // (node
 	// The REST path has refused these since it was written; this CLI edits
 	// the same bag directly, so the rule has to hold here too.
 	validate.StoragePoolPropEdit,
-), validate.StoragePoolPropNamed)
+), func(key string, _ bool) error { return validate.StoragePoolPropNamed(key) })
 
-// resourceGroupProps accesses a group's property bag.
+// resourceGroupProps accesses a group's property bag. A group's props are
+// copied onto every definition spawned from it, so the props blockstor reads
+// back off a definition are refused here as on the REST door; deleting one
+// stays allowed, which is how a group written before the refusal is cleaned.
 //
 //nolint:gochecknoglobals,dupl // static accessor table; the parallel shape is the point
-var resourceGroupProps = objectProps("resource group", 1,
+var resourceGroupProps = withNamedKeyGuard(objectProps("resource group", 1,
 	func(ctx context.Context, st store.Store, ident []string) (apiv1.ResourceGroup, error) {
 		return st.ResourceGroups().Get(ctx, ident[0])
 	},
 	func(group *apiv1.ResourceGroup) *map[string]string { return &group.Props },
 	func(ctx context.Context, st store.Store, ident []string, mutate func(*apiv1.ResourceGroup) error) error {
 		return st.ResourceGroups().PatchResourceGroup(ctx, ident[0], mutate)
 	},
-)
+), refuseServerOwnedGroupProp)
+
+// aSetValue stands for the value of a named-key edit that sets a key: the
+// server-owned checks judge only that a value is set, and an empty one is read
+// as a delete.
+const aSetValue = "set"
+
+// refuseServerOwnedGroupProp is the named-key guard for resourceGroupProps.
+func refuseServerOwnedGroupProp(key string, deleting bool) error {
+	if deleting {
+		return nil
+	}
+
+	if store.ServerOwnedPropEdit(map[string]string{key: aSetValue}, nil, nil) != "" {
+		return fmt.Errorf("%w: %s records what blockstor did to a resource definition, and a group's "+
+			"props are copied onto every definition spawned from it", errServerOwnedProp, key)
+	}
+
+	return nil
+}
 
 // volumeDefinitionProps accesses a volume definition's property bag,
 // identified as `<resource-definition> <volume-number>`.

internal/cli/rollback_gone_test.go

@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+Copyright 2026 Cozystack contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cli
+
+import (
+	"context"
+	"errors"
+	"sync/atomic"
+	"testing"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+var errPlacementFailed = errors.New("placement failed")
+
+// markFindsNothing answers the gone-th definition patch NotFound, while the
+// backend holds a definition under the name: the one the rollback meant went,
+// and another writer put a new one there.
+type markFindsNothing struct {
+	store.ResourceDefinitionStore
+
+	patches *atomic.Int32
+	gone    int32
+}
+
+func (r markFindsNothing) PatchResourceDefinitionSpec(
+	ctx context.Context, name string, mutate func(*apiv1.ResourceDefinition) error,
+) error {
+	if r.patches.Add(1) == r.gone {
+		return store.ErrNotFound
+	}
+
+	return r.ResourceDefinitionStore.PatchResourceDefinitionSpec(ctx, name, mutate) //nolint:wrapcheck // test double
+}
+
+type markFindsNothingStore struct {
+	store.Store
+
+	patches *atomic.Int32
+	gone    int32
+}
+
+func (s markFindsNothingStore) ResourceDefinitions() store.ResourceDefinitionStore {
+	return markFindsNothing{ResourceDefinitionStore: s.Store.ResourceDefinitions(), patches: s.patches, gone: s.gone}
+}
+
+// A restore rollback whose definition is gone deletes nothing by name, and
+// answers with the restore's own failure alone, since there was nothing left
+// to roll back: whether it finds the definition gone on the mark that starts
+// the rollback or on the one that takes it apart.
+func TestRestoreRollbackWhoseDefinitionGoesBeforeItsDeleteDeletesNothing(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		name string
+		gone int32
+	}{
+		{name: "gone when the rollback starts", gone: 1},
+		{name: "gone before the first delete", gone: 2},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			backend := store.NewInMemory()
+			ctx := t.Context()
+
+			if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-regone"}); err != nil {
+				t.Fatalf("seed: %v", err)
+			}
+
+			// The flags the rollback's advice reads, so a regression fails on
+			// this case's own assertions rather than a nil dereference that
+			// takes the sibling case down with it.
+			run := &runContext{
+				Store: markFindsNothingStore{Store: backend, patches: new(atomic.Int32), gone: tc.gone},
+				Flags: &flagSet{Values: map[string]string{}},
+			}
+
+			err := rollbackRestore(ctx, run, "pvc-regone", nil, errPlacementFailed)
+			if err == nil || err.Error() != errPlacementFailed.Error() {
+				t.Errorf("rollback = %v, want the restore's own failure and nothing else", err)
+			}
+
+			if _, err := backend.ResourceDefinitions().Get(ctx, "pvc-regone"); err != nil {
+				t.Errorf("the definition now under the name was deleted: %v", err)
+			}
+		})
+	}
+}

internal/cli/rollback_mark_stuck_test.go

@@ -0,0 +1,115 @@
+// SPDX-License-Identifier: Apache-2.0
+
+/*
+Copyright 2026 Cozystack contributors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cli
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"sync/atomic"
+	"testing"
+
+	apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
+	"github.com/cozystack/blockstor/pkg/store"
+)
+
+var errMarkPatchRefused = errors.New("patch refused")
+
+// A rollback that yields to an adopter and cannot take its mark back off
+// leaves a definition every re-run is refused over. The answer says so and
+// names the command that clears the mark, rather than only the re-run.
+func TestRestoreRollbackThatYieldsWithAStuckMarkNamesTheClear(t *testing.T) {
+	t.Parallel()
+
+	ctx := t.Context()
+	backend := store.NewInMemory()
+
+	if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{
+		Name: "pvc-yield", Props: map[string]string{store.RestoreAdoptedProp: "token"},
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	// The first patch, the in-progress mark, lands; the one that takes it
+	// back off fails.
+	run := &runContext{
+		Store: patchesFailAfterStore{Store: backend, patches: new(atomic.Int32), after: 1},
+		Flags: &flagSet{Values: map[string]string{
+			"from-resource": "src", "from-snapshot": "snap", "to-resource": "pvc-yield",
+		}},
+	}
+
+	err := rollbackRestore(ctx, run, "pvc-yield", nil, errPlacementFailed)
+	if !errors.Is(err, errPlacementFailed) {
+		t.Fatalf("rollback = %v, want the restore's own failure first", err)
+	}
+
+	if !strings.Contains(err.Error(), "delete-property pvc-yield "+store.RollbackAbandonedProp) {
+		t.Errorf("answer %q does not name the command that clears the stuck mark", err)
+	}
+}
+
+// patchesFailAfterStore lets the first after definition patches through and
+// fails every one after them.
+type patchesFailAfterStore struct {
+	store.Store
+
+	patches *atomic.Int32
+	after   int32
+}
+
+type patchesFailAfter struct {
+	store.ResourceDefinitionStore
+
+	patches *atomic.Int32
+	after   int32
+}
+
+func (s patchesFailAfterStore) ResourceDefinitions() store.ResourceDefinitionStore {
+	return patchesFailAfter{ResourceDefinitionStore: s.Store.ResourceDefinitions(), patches: s.patches, after: s.after}
+}
+
+func (r patchesFailAfter) PatchResourceDefinitionSpec(
+	ctx context.Context, name string, mutate func(*apiv1.ResourceDefinition) error,
+) error {
+	if r.patches.Add(1) > r.after {
+		return errMarkPatchRefused
+	}
+
+	return r.ResourceDefinitionStore.PatchResourceDefinitionSpec(ctx, name, mutate) //nolint:wrapcheck // test double
+}
+
+// A replica the plan put on a pool of its own is named with that pool: without
+// it `resource create` takes the pool of the first diskful sibling.
+func TestPlaceCommandsNameThePoolThePlanChose(t *testing.T) {
+	t.Parallel()
+
+	planned := []apiv1.Resource{
+		{Name: "pvc-x", NodeName: "node-a", Props: map[string]string{storPoolNameProp: "fast"}},
+		{Name: "pvc-x", NodeName: "node-b", Props: map[string]string{storPoolNameProp: "slow"}},
+		{Name: "pvc-x", NodeName: "node-c"},
+	}
+
+	got := placeCommands("pvc-x", planned, []string{"node-b", "node-c"})
+
+	want := "run `blockstor resource create node-b pvc-x --storage-pool slow`, `blockstor resource create node-c pvc-x`"
+	if got != want {
+		t.Errorf("commands = %q, want %q", got, want)
+	}
+}