diff --git a/api/v1alpha1/resource_types.go b/api/v1alpha1/resource_types.go index bbf5dfbd..2ecb8558 100644 --- a/api/v1alpha1/resource_types.go +++ b/api/v1alpha1/resource_types.go @@ -523,6 +523,12 @@ type ResourceVolumeStatus struct { // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + '.' + self.spec.nodeName).lowerAscii()",message="metadata.name must equal . (case-insensitive)",optionalOldSelf=true +// Server-side field selectors. Without these the API server refuses a +// selector on these paths outright, which is the failure mode we want: a +// selector that silently returned a subset is the Bug 038 shape, where a +// partial-but-correct answer hid the replicas an operator applied by hand. +// +kubebuilder:selectablefield:JSONPath=`.spec.nodeName` +// +kubebuilder:selectablefield:JSONPath=`.spec.resourceDefinitionName` // +kubebuilder:printcolumn:name="Definition",type=string,JSONPath=`.spec.resourceDefinitionName` // +kubebuilder:printcolumn:name="Node",type=string,JSONPath=`.spec.nodeName` // +kubebuilder:printcolumn:name="Pool",type=string,JSONPath=`.spec.storagePool` diff --git a/api/v1alpha1/snapshot_types.go b/api/v1alpha1/snapshot_types.go index 69152d47..4994d631 100644 --- a/api/v1alpha1/snapshot_types.go +++ b/api/v1alpha1/snapshot_types.go @@ -274,6 +274,12 @@ type SnapshotPerNodeStatus struct { // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + '.' + self.spec.snapshotName).lowerAscii()",message="metadata.name must equal . (case-insensitive)",optionalOldSelf=true +// spec.resourceDefinitionName is selectable so a definition-scoped snapshot +// read is a definition-scoped query. A field, not the label the objects +// usually carry: a Snapshot adopted from a LINSTOR dump has no labels, and a +// label selector would answer partial-but-correct on the read that refuses +// `rd d` and sweeps the leftovers after it. +// +kubebuilder:selectablefield:JSONPath=`.spec.resourceDefinitionName` // +kubebuilder:printcolumn:name="Definition",type=string,JSONPath=`.spec.resourceDefinitionName` // +kubebuilder:printcolumn:name="Snapshot",type=string,JSONPath=`.spec.snapshotName` // +kubebuilder:printcolumn:name="Nodes",type=string,JSONPath=`.spec.nodes` diff --git a/api/v1alpha1/storagepool_types.go b/api/v1alpha1/storagepool_types.go index 50ebb9ea..86ddb3d8 100644 --- a/api/v1alpha1/storagepool_types.go +++ b/api/v1alpha1/storagepool_types.go @@ -112,6 +112,14 @@ type StoragePoolStatus struct { // +kubebuilder:subresource:status // +kubebuilder:resource:scope=Cluster // +kubebuilder:validation:XValidation:rule="oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.poolName + '.' + self.spec.nodeName).lowerAscii()",message="metadata.name must equal . (case-insensitive)",optionalOldSelf=true +// spec.nodeName is selectable so a node-scoped read is a node-scoped query. +// The pools on a node are what a `node delete` is refused on and what the +// cascade removes, and answering that by listing every pool in the cluster is +// the read this replaces. A field, not the label the objects usually carry: a +// pool created by an operator (piraeus writes them with `kubectl apply`) has +// no label, and a label selector would answer partial-but-correct — which on +// the refusal path means deleting a node the cluster still has pools on. +// +kubebuilder:selectablefield:JSONPath=`.spec.nodeName` // +kubebuilder:printcolumn:name="Node",type=string,JSONPath=`.spec.nodeName` // +kubebuilder:printcolumn:name="Pool",type=string,JSONPath=`.spec.poolName` // +kubebuilder:printcolumn:name="Provider",type=string,JSONPath=`.spec.providerKind` diff --git a/cmd/apiserver/main.go b/cmd/apiserver/main.go index 1ca7246e..a3a70389 100644 --- a/cmd/apiserver/main.go +++ b/cmd/apiserver/main.go @@ -138,8 +138,8 @@ func newScheme() *runtime.Scheme { // election off — every apiserver replica serves reads // independently. Caches still warm up so the REST server's // cached-client reads are cheap. -func buildManager(flags *apiserverFlags) (manager.Manager, error) { - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ +func buildManager(flags *apiserverFlags) (manager.Manager, *storek8s.Store, error) { + mgr, st, err := storek8s.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: newScheme(), Metrics: metricsserver.Options{ BindAddress: flags.metricsAddr, @@ -150,10 +150,10 @@ func buildManager(flags *apiserverFlags) (manager.Manager, error) { LeaderElection: false, }) if err != nil { - return nil, errors.Wrap(err, "new manager") + return nil, nil, errors.Wrap(err, "new manager") } - return mgr, nil + return mgr, st, nil } // resolveNamespace mirrors the controller's namespace-resolution @@ -248,24 +248,12 @@ func main() { flags := parseFlags() namespace := resolveNamespace(flags.controllerNamespace) - mgr, err := buildManager(flags) + mgr, st, err := buildManager(flags) if err != nil { setupLog.Error(err, "Failed to start manager") os.Exit(1) } - // CRD-backed store is the only supported persistence layer - // post-Phase-11 — the apiserver/controller split made - // in-process state pointless across replicas. - // - // BUG-048: pass the manager's direct (uncached) API reader so the - // atomic VolumeNumber allocation re-reads live RD state on each - // conflict-retry. With only the informer-cached client, two - // concurrent `vd c` against one RD both retry against a stale cache, - // re-derive the same number, exhaust the retry budget, and silently - // drop the second volume. - st := storek8s.NewWithAPIReader(mgr.GetClient(), mgr.GetAPIReader()) - ready := newReadyState() // Bug 219: `ctrl.SetupSignalHandler` is one-shot — a second call diff --git a/cmd/blockstor/main.go b/cmd/blockstor/main.go index d0ac6fa0..72492524 100644 --- a/cmd/blockstor/main.go +++ b/cmd/blockstor/main.go @@ -41,8 +41,10 @@ import ( "os" "strings" + ctrl "sigs.k8s.io/controller-runtime" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/config" + "sigs.k8s.io/controller-runtime/pkg/log/zap" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/runtime" @@ -55,6 +57,25 @@ import ( ) func main() { + // The store logs when a scoped read falls back to reading everything. Only + // a cluster whose CRD predates the selectable fields takes that branch, + // and both uncached readers reach it there: this client, and the servers' + // node-scoped reads, which go to the manager's API reader and are refused + // on the wire by the same API server. What the servers do not reach is the + // cache half of it, since they register the indexes. + // + // Without a root logger controller-runtime buffers the line, then after + // thirty seconds promotes to a null sink and prints its own "SetLogger + // was never called" stack trace into operator-facing stderr instead. So + // set one: quiet at the default level, and BLOCKSTOR_DEBUG turns the + // V(1) lines on for the operator who is asking why a large cluster + // crawls. + logOpts := zap.Options{ + Development: os.Getenv("BLOCKSTOR_DEBUG") != "", + DestWriter: os.Stderr, + } + ctrl.SetLogger(zap.New(zap.UseFlagOptions(&logOpts))) + app := &cli.App{ Out: os.Stdout, Err: os.Stderr, diff --git a/cmd/controller/main.go b/cmd/controller/main.go index 8eaa5d21..07a60692 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -166,7 +166,7 @@ func main() { metricsServerOptions.KeyName = metricsCertKey } - mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ + mgr, st, err := storek8s.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{ Scheme: scheme, Metrics: metricsServerOptions, WebhookServer: webhookServer, @@ -190,12 +190,11 @@ func main() { os.Exit(1) } - // Construct the store before reconciler wiring so the - // NodeReconciler can drive eviction-triggered migration via the - // shared placer. CRD-backed is the only supported persistence - // layer since Phase 11.x — the apiserver/controller split makes - // in-process state pointless across replicas. - st := storek8s.New(mgr.GetClient()) + // The store came back with the manager, before reconciler wiring, so the + // NodeReconciler can drive eviction-triggered migration via the shared + // placer. CRD-backed is the only supported persistence layer since Phase + // 11.x — the apiserver/controller split makes in-process state pointless + // across replicas. if err := (&controller.NodeReconciler{ Client: mgr.GetClient(), diff --git a/config/crd/bases/blockstor.cozystack.io_resources.yaml b/config/crd/bases/blockstor.cozystack.io_resources.yaml index 2a3317be..bd8063a7 100644 --- a/config/crd/bases/blockstor.cozystack.io_resources.yaml +++ b/config/crd/bases/blockstor.cozystack.io_resources.yaml @@ -781,6 +781,9 @@ spec: optionalOldSelf: true rule: oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + '.' + self.spec.nodeName).lowerAscii() + selectableFields: + - jsonPath: .spec.nodeName + - jsonPath: .spec.resourceDefinitionName served: true storage: true subresources: diff --git a/config/crd/bases/blockstor.cozystack.io_snapshots.yaml b/config/crd/bases/blockstor.cozystack.io_snapshots.yaml index 822e044a..c3d74e2e 100644 --- a/config/crd/bases/blockstor.cozystack.io_snapshots.yaml +++ b/config/crd/bases/blockstor.cozystack.io_snapshots.yaml @@ -322,6 +322,8 @@ spec: optionalOldSelf: true rule: oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.resourceDefinitionName + '.' + self.spec.snapshotName).lowerAscii() + selectableFields: + - jsonPath: .spec.resourceDefinitionName served: true storage: true subresources: diff --git a/config/crd/bases/blockstor.cozystack.io_storagepools.yaml b/config/crd/bases/blockstor.cozystack.io_storagepools.yaml index 7789eb0d..d9c39125 100644 --- a/config/crd/bases/blockstor.cozystack.io_storagepools.yaml +++ b/config/crd/bases/blockstor.cozystack.io_storagepools.yaml @@ -223,6 +223,8 @@ spec: optionalOldSelf: true rule: oldSelf.hasValue() || self.metadata.name.lowerAscii() == (self.spec.poolName + '.' + self.spec.nodeName).lowerAscii() + selectableFields: + - jsonPath: .spec.nodeName served: true storage: true subresources: diff --git a/internal/cli/handlers.go b/internal/cli/handlers.go index d93d5e41..65fe1fe1 100644 --- a/internal/cli/handlers.go +++ b/internal/cli/handlers.go @@ -22,6 +22,8 @@ import ( "context" "fmt" + "github.com/cockroachdb/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" @@ -307,6 +309,16 @@ func volumeDefinitionList(ctx context.Context, run *runContext) error { // the percentage silently disappeared during exactly the resync an // operator is watching, while the design doc promised it and the // colour classifier went to the trouble of stripping it. +// +// The replicas are read whole and filtered in process, `-n` and `-r` +// included. The filter compares the way LINSTOR does, case-insensitively, +// and a replica's spec keeps whatever case its writer used: an adoption +// run or linstor-csi can store `NODE-1` for a node the operator types as +// `node-1`. The node- and definition-scoped reads compare the stored +// spelling verbatim (see store.FoldName), so answering a narrowed listing +// with them returned fewer rows than this filter finds, with exit status +// zero. Narrowing the read without that loss needs the folded selectable +// field FoldName describes; until it exists, the listing stays whole. func resourceList(ctx context.Context, run *runContext) error { resources, err := fetchResources(ctx, run.Store) if err != nil { @@ -334,39 +346,193 @@ func resourceList(ctx context.Context, run *runContext) error { }), "State", "Conns") } -// volumeSizesFor collects the per-volume sizes of the definitions in a -// listing, keyed the way the view expects. A definition whose sizes -// cannot be read is skipped rather than failing the listing: a missing -// percentage is a cosmetic loss, an unreadable `resource list` during -// an incident is not. +// volumeSizesBulkCutoff is where reading the definitions one at a time stops +// being the cheaper of the two reads. Below it a listing narrowed by `-r`, +// `-n` or `--limit` pays that many GETs; above it, one request for the lot. +// +// It counts the definitions the rendered rows cover, not the definitions in +// the cluster. The replica read behind those rows is whole whatever the +// filter (see resourceList), so this is the one read a narrowing makes +// cheaper, and the choice is between two costs rather than a guess about +// the cluster: up to the cutoff, that many sequential GETs; past it, one +// LIST that carries every definition, those without a replica included. +// So `-n` on a node holding more definitions than the cutoff takes the +// LIST to render its rows. That is the trade, not an oversight, and +// removing it means making the per-definition read concurrent rather than +// moving this number. +const volumeSizesBulkCutoff = 16 + +// volumeSizesFor builds the per-volume sizes the sync-percentage column needs. +// +// Two reads, chosen on how much of the cluster the listing actually covers. +// Reading them one definition at a time is what this used to do +// unconditionally, and on the Kubernetes store each of those is a GET of one +// ResourceDefinition against an uncached client — so `resource list` on a +// cluster with a thousand definitions was one LIST plus a thousand sequential +// round trips, in the command an operator runs while watching a resync. +// Reading them all in one request fixes that and breaks the opposite case: +// `resource list -r one-volume` asked about one definition and would pull +// every definition in the cluster back to answer it. +// +// So the listing picks. Either side of the cutoff the answer is identical; +// only the number of requests and the size of them differ. +// +// A read that fails leaves that definition out rather than failing the +// listing: the column degrades to a bare state, which is what the +// per-definition version did when one of its reads failed. A missing +// percentage is a cosmetic loss; an unreadable `resource list` during an +// incident is not. func volumeSizesFor(ctx context.Context, run *runContext, resources []apiv1.Resource) map[string]map[int32]int64 { + names := distinctDefinitionNames(resources) + + if len(names) <= volumeSizesBulkCutoff { + return volumeSizesPerDefinition(ctx, run, names) + } + + sizes, err := volumeSizesInOneRequest(ctx, run, names) + if err == nil { + return sizes + } + + if !perDefinitionCanAnswer(ctx, err) { + warnSyncColumnUnavailable(run, err) + + return nil + } + + return volumeSizesPerDefinition(ctx, run, names) +} + +// perDefinitionCanAnswer says whether reading the definitions one at a time +// can answer what the single read could not. +// +// SelectorUnsupported (pkg/store/k8s) draws the same line one layer down, and +// for the same reason: a fallback is worth taking when the first read failed +// on its shape, not when it ran out of the budget the second read spends +// again. A cancelled context and a refusal aimed at the caller rather than at +// the request repeat identically once per definition, so the retry buys the +// same answer at N times the cost, in the command an operator is running +// because something is already wrong. +// +// A server that asked the caller to slow down is the same case as a refusal, +// by the same rule: 429 and 503 say the budget is the problem, and answering +// them with one narrow request per definition sends N requests to the server +// that just asked for fewer. +// +// A timeout is deliberately on the other side. One request covering every +// definition in the cluster is the read most likely to exceed a deadline, and +// the narrow ones after it are each small enough to land. +func perDefinitionCanAnswer(ctx context.Context, err error) bool { + if ctx.Err() != nil { + return false + } + + if apierrors.IsTooManyRequests(err) || apierrors.IsServiceUnavailable(err) { + return false + } + + return !apierrors.IsForbidden(err) && !apierrors.IsUnauthorized(err) +} + +// warnSyncColumnUnavailable tells the operator why the percentages are gone. +// +// The listing still prints: an unreadable `resource list` during an incident +// is worse than one without a percentage. But a column that emptied because +// every read was refused looks exactly like a cluster with nothing to sync, +// and that is the one reading an operator must not take away from it. Stderr, +// so the table on stdout stays the contract `awk -F'|'` parses. +func warnSyncColumnUnavailable(run *runContext, err error) { + if run.Err == nil { + return + } + + fmt.Fprintf(run.Err, "warning: sync percentages unavailable: %v\n", err) +} + +// distinctDefinitionNames is the set of definitions a listing covers, in +// first-seen order — every replica of one definition asks the same question. +func distinctDefinitionNames(resources []apiv1.Resource) []string { seen := make(map[string]struct{}, len(resources)) - sizes := make(map[string]map[int32]int64, len(resources)) + names := make([]string, 0, len(resources)) for i := range resources { name := resources[i].Name - if _, done := seen[name]; done { + if _, dup := seen[name]; dup { continue } seen[name] = struct{}{} + names = append(names, name) + } + + return names +} + +func volumeSizesPerDefinition(ctx context.Context, run *runContext, names []string) map[string]map[int32]int64 { + sizes := make(map[string]map[int32]int64, len(names)) + + var firstErr error + + for _, name := range names { vds, err := run.Store.VolumeDefinitions().List(ctx, name) if err != nil { + if firstErr == nil { + firstErr = err + } + continue } - perVolume := make(map[int32]int64, len(vds)) - for j := range vds { - perVolume[vds[j].VolumeNumber] = vds[j].SizeKib - } + sizes[name] = perVolumeSizes(vds) + } - sizes[name] = perVolume + // One definition that could not be read is the degradation this path is + // documented to accept. None of them read is not a degradation, it is a + // failure that reached the operator as an empty column. + if len(sizes) == 0 && firstErr != nil { + warnSyncColumnUnavailable(run, firstErr) } return sizes } +func volumeSizesInOneRequest( + ctx context.Context, run *runContext, names []string, +) (map[string]map[int32]int64, error) { + all, err := run.Store.VolumeDefinitions().ListAll(ctx) + if err != nil { + // One read, so one failure costs every row its percentage — where + // the per-definition path loses only the definition it could not + // read. The caller decides whether that path can do better, since + // it cannot for a failure that was never about the read's shape. + return nil, errors.Wrap(err, "read every definition's volumes") + } + + sizes := make(map[string]map[int32]int64, len(names)) + + for _, name := range names { + vds, ok := all[store.FoldName(name)] + if !ok { + continue + } + + sizes[name] = perVolumeSizes(vds) + } + + return sizes, nil +} + +// perVolumeSizes keys one definition's volumes the way the view reads them. +func perVolumeSizes(vds []apiv1.VolumeDefinition) map[int32]int64 { + out := make(map[int32]int64, len(vds)) + for i := range vds { + out[vds[i].VolumeNumber] = vds[i].SizeKib + } + + return out +} + // machineOut writes the machine-readable envelope. func machineOut[T any](run *runContext, items []T) error { err := output.MachineList(run.Out, items) diff --git a/internal/cli/node.go b/internal/cli/node.go index e3a8e9d1..c83c1b34 100644 --- a/internal/cli/node.go +++ b/internal/cli/node.go @@ -28,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" "github.com/cozystack/blockstor/internal/cli/command" "github.com/cozystack/blockstor/internal/cli/view" @@ -153,51 +154,31 @@ func checkNodeLostAllowed(ctx context.Context, run *runContext, name string) err // cascadeNodeObjects removes the replicas and pools that can never be // reconciled again. +// +// The same tear-down the REST node-delete runs, through the same function: +// two spellings of "remove everything pointing at this node" drift, and this +// one had drifted already — it read every replica in the cluster to find the +// node's, which is the question ListByNode answers with one scoped read. func cascadeNodeObjects(ctx context.Context, run *runContext, name string) error { - resources, err := run.Store.Resources().List(ctx) - if err != nil { - return fmt.Errorf("list resources: %w", err) - } - - for i := range resources { - if resources[i].NodeName != name { - continue - } - - err = run.Store.Resources().Delete(ctx, resources[i].Name, name) - if err != nil && !isNotFound(err) { - return fmt.Errorf("delete resource %s on %s: %w", resources[i].Name, name, err) - } - } - - pools, err := run.Store.StoragePools().ListByNode(ctx, name) - if err != nil { - return fmt.Errorf("list storage pools on %s: %w", name, err) - } - - for i := range pools { - err = run.Store.StoragePools().Delete(ctx, name, pools[i].StoragePoolName) - if err != nil && !isNotFound(err) { - return fmt.Errorf("delete storage pool %s on %s: %w", pools[i].StoragePoolName, name, err) - } - } - - return nil + //nolint:wrapcheck // the caller names the node and the operation + return store.CascadeOrphansForLostNode(ctx, run.Store, name) } // resourcesInUseOn names the replicas a consumer currently holds // Primary on the node, sorted so the message is stable. func resourcesInUseOn(ctx context.Context, run *runContext, name string) ([]string, error) { - resources, err := run.Store.Resources().List(ctx) + // The evacuate refusal is a node-fate gate like node delete's, so it + // asks in every spelling the node's replicas can carry. + resources, err := store.ReplicasOnNode(ctx, run.Store, name) if err != nil { - return nil, fmt.Errorf("list resources: %w", err) + return nil, fmt.Errorf("list replicas on %s: %w", name, err) } var inUse []string for i := range resources { res := &resources[i] - if res.NodeName == name && res.State.InUse != nil && *res.State.InUse { + if res.State.InUse != nil && *res.State.InUse { inUse = append(inUse, res.Name) } } diff --git a/internal/cli/node_scoped_reads_test.go b/internal/cli/node_scoped_reads_test.go new file mode 100644 index 00000000..08af64d3 --- /dev/null +++ b/internal/cli/node_scoped_reads_test.go @@ -0,0 +1,166 @@ +// SPDX-License-Identifier: Apache-2.0 + +package cli_test + +import ( + "bytes" + "context" + "strconv" + "sync/atomic" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" + + "github.com/cozystack/blockstor/internal/cli" +) + +// countingResources separates the node-scoped read from the whole-cluster one. +type countingResources struct { + store.ResourceStore + + wholeCluster atomic.Int64 + byNode atomic.Int64 +} + +func (c *countingResources) List(ctx context.Context) ([]apiv1.Resource, error) { + c.wholeCluster.Add(1) + + return c.ResourceStore.List(ctx) //nolint:wrapcheck // test decorator +} + +func (c *countingResources) ListByNode(ctx context.Context, node string) ([]apiv1.Resource, error) { + c.byNode.Add(1) + + return c.ResourceStore.ListByNode(ctx, node) //nolint:wrapcheck // test decorator +} + +type countingResourceStore struct { + store.Store + + resources *countingResources +} + +func (c *countingResourceStore) Resources() store.ResourceStore { return c.resources } + +// `node lost` and `node evacuate` both ask about one node — which replicas it +// holds, and whether any of them is in use — and both answered by listing +// every replica in the cluster and filtering here. That is the read +// ListByNode was added to replace, and on the two commands an operator runs +// during a node failure it was still being taken. +func TestNodeCommandsReadOnlyTheirNode(t *testing.T) { + t.Parallel() + + for name, argv := range map[string][]string{ + "node lost": {"node", "lost", "node-1"}, + "node evacuate": {"node", "evacuate", "node-1"}, + // The command #187 is written about. It reaches the replicas through + // ReferencesOnNode on the refusal path and through the cascade under + // --force, and both used to list the cluster. + "node delete --force": {"node", "delete", "node-1", "--force"}, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + counted := seedTwoNodeCluster(t) + + var out, errBuf bytes.Buffer + + app := &cli.App{ + Out: &out, + Err: &errBuf, + StoreFor: func(context.Context) (store.Store, error) { + return counted, nil + }, + } + + if got := app.Run(t.Context(), argv); got != 0 { + t.Fatalf("exit = %d (stderr: %s)", got, errBuf.String()) + } + + if n := counted.resources.wholeCluster.Load(); n != 0 { + t.Errorf("%d whole-cluster reads, want none — the question is about one node", n) + } + + if n := counted.resources.byNode.Load(); n == 0 { + t.Error("no node-scoped reads at all; the command answered from somewhere else") + } + }) + } +} + +// seedTwoNodeCluster puts replicas and a pool on each of two nodes, behind +// counters on the replica reads. +func seedTwoNodeCluster(t *testing.T) *countingResourceStore { + t.Helper() + + backend := store.NewInMemory() + ctx := t.Context() + + for _, node := range []string{"node-1", "node-2"} { + if err := backend.Nodes().Create(ctx, + &apiv1.Node{Name: node, Type: "SATELLITE"}); err != nil { + t.Fatalf("seed node %s: %v", node, err) + } + + if err := backend.StoragePools().Create(ctx, &apiv1.StoragePool{ + StoragePoolName: "pool-1", + NodeName: node, + ProviderKind: "LVM_THIN", + }); err != nil { + t.Fatalf("seed pool on %s: %v", node, err) + } + + for i := range 3 { + rd := "pvc-" + node + "-" + strconv.Itoa(i) + + if err := backend.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: rd}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + if err := backend.Resources().Create(ctx, + &apiv1.Resource{Name: rd, NodeName: node}); err != nil { + t.Fatalf("seed replica: %v", err) + } + } + } + + return &countingResourceStore{ + Store: backend, + resources: &countingResources{ResourceStore: backend.Resources()}, + } +} + +// The refusal path of the same command, which is the one an operator hits +// first: `node delete` without --force asks whether anything still references +// the node, and #187's acceptance is the number of requests that costs. +func TestNodeDeleteRefusalReadsOnlyItsNode(t *testing.T) { + t.Parallel() + + counted := seedTwoNodeCluster(t) + + var out, errBuf bytes.Buffer + + app := &cli.App{ + Out: &out, + Err: &errBuf, + StoreFor: func(context.Context) (store.Store, error) { + return counted, nil + }, + } + + // Refused, because node-1 still carries replicas and a pool. That is the + // answer under test; what it cost to reach it is what is counted. + if got := app.Run(t.Context(), []string{"node", "delete", "node-1"}); got == 0 { + t.Fatalf("exit = 0, want a refusal — the node still carries replicas") + } + + if n := counted.resources.wholeCluster.Load(); n != 0 { + t.Errorf("%d whole-cluster reads, want none — the question is about one node", n) + } + + if n := counted.resources.byNode.Load(); n == 0 { + t.Error("no node-scoped reads at all; the refusal was answered from somewhere else") + } +} diff --git a/internal/cli/node_test.go b/internal/cli/node_test.go index 5d111129..9849cf37 100644 --- a/internal/cli/node_test.go +++ b/internal/cli/node_test.go @@ -98,6 +98,30 @@ func TestNodeEvacuateRefusesInUse(t *testing.T) { } } +// The refusal is a node-fate gate, so it asks in every spelling the node's +// replicas carry. A node registered in upper case, which is how adoption from +// LINSTOR writes it, holds its replicas under that spelling, and asking only +// as typed let `n evacuate node-1` past a mounted volume. +func TestNodeEvacuateRefusesInUseUnderTheRegisteredSpelling(t *testing.T) { + t.Parallel() + + app, _, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { + _ = backend.Nodes().Create(ctx, &apiv1.Node{Name: "NODE-1"}) + _ = backend.Resources().Create(ctx, &apiv1.Resource{ + Name: "pvc-x", NodeName: "NODE-1", + State: apiv1.ResourceState{InUse: boolPtr(true)}, + }) + }) + + if got := app.Run(t.Context(), []string{"n", "evacuate", "node-1"}); got == 0 { + t.Fatal("evacuating a node with an in-use resource succeeded") + } + + if !strings.Contains(errBuf.String(), "in use") || !strings.Contains(errBuf.String(), "pvc-x") { + t.Errorf("the refusal does not name the blocking resource:\n%s", errBuf.String()) + } +} + // A replica whose satellite has not reported yet has in_use unset. // That is "unknown", not "in use" — refusing there would block an // operator draining a node that never came up. diff --git a/internal/cli/resource_list_requests_test.go b/internal/cli/resource_list_requests_test.go new file mode 100644 index 00000000..f082d560 --- /dev/null +++ b/internal/cli/resource_list_requests_test.go @@ -0,0 +1,212 @@ +// SPDX-License-Identifier: Apache-2.0 + +package cli_test + +import ( + "bytes" + "context" + "strconv" + "sync/atomic" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" + + "github.com/cozystack/blockstor/internal/cli" +) + +// countingVDs records how the command reaches the volume sizes. Embedding the +// real store keeps every other method behaviourally identical, so the only +// difference between this and the plain in-memory store is the counters. +type countingVDs struct { + store.VolumeDefinitionStore + + perDefinition atomic.Int64 + wholeCluster atomic.Int64 +} + +func (c *countingVDs) List(ctx context.Context, rdName string) ([]apiv1.VolumeDefinition, error) { + c.perDefinition.Add(1) + + return c.VolumeDefinitionStore.List(ctx, rdName) //nolint:wrapcheck // test helper +} + +func (c *countingVDs) ListAll(ctx context.Context) (map[string][]apiv1.VolumeDefinition, error) { + c.wholeCluster.Add(1) + + return c.VolumeDefinitionStore.ListAll(ctx) //nolint:wrapcheck // test helper +} + +type countingStore struct { + store.Store + + vds *countingVDs +} + +func (c *countingStore) VolumeDefinitions() store.VolumeDefinitionStore { return c.vds } + +// seedCountedCluster builds a cluster of `definitions` single-volume +// definitions, each with one replica, behind counters on the volume reads. +func seedCountedCluster(t *testing.T, definitions int) *countingStore { + t.Helper() + + backend := store.NewInMemory() + ctx := t.Context() + + for i := range definitions { + name := "pvc-" + strconv.Itoa(i) + + if err := backend.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: name}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + if err := backend.VolumeDefinitions().Create(ctx, name, + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 1 << 20}); err != nil { + t.Fatalf("seed volume: %v", err) + } + + if err := backend.Resources().Create(ctx, + &apiv1.Resource{Name: name, NodeName: "node-1"}); err != nil { + t.Fatalf("seed replica: %v", err) + } + } + + return &countingStore{ + Store: backend, + vds: &countingVDs{VolumeDefinitionStore: backend.VolumeDefinitions()}, + } +} + +// runCountedList runs the command against the counted store and fails on a +// non-zero exit. +func runCountedList(t *testing.T, counted *countingStore, args ...string) { + t.Helper() + + var out, errBuf bytes.Buffer + + app := &cli.App{ + Out: &out, + Err: &errBuf, + StoreFor: func(context.Context) (store.Store, error) { + return counted, nil + }, + } + + if got := app.Run(t.Context(), args); got != 0 { + t.Fatalf("%v: exit = %d (stderr: %s)", args, got, errBuf.String()) + } +} + +// `resource list` fills its sync-percentage column from the volume sizes, and +// it used to read them one definition at a time. On the Kubernetes store each +// of those is a GET of one ResourceDefinition against an uncached client, so +// the command an operator runs while watching a resync cost one LIST plus one +// round trip per definition. +// +// The acceptance is a request count that does not grow with the number of +// definitions, so the test asserts it across two cluster sizes rather than +// against a fixed number: a per-definition read would make the count track the +// seed size, and no single expected value could catch that. +func TestResourceListDoesNotReadPerDefinition(t *testing.T) { + t.Parallel() + + for _, definitions := range []int{40, 200} { + counted := seedCountedCluster(t, definitions) + + runCountedList(t, counted, "resource", "list") + + if n := counted.vds.perDefinition.Load(); n != 0 { + t.Errorf("%d definitions: %d per-definition reads, want none — the count would "+ + "track the cluster size", definitions, n) + } + + if n := counted.vds.wholeCluster.Load(); n != 1 { + t.Errorf("%d definitions: %d whole-cluster reads, want exactly 1", definitions, n) + } + } +} + +// The other side of the same trade. A listing narrowed to one definition asked +// about one definition, and answering it by reading every definition in the +// cluster is the cost the wide case was optimised out of, moved onto the +// operator who was already being specific. +func TestResourceListNarrowedDoesNotReadTheWholeCluster(t *testing.T) { + t.Parallel() + + counted := seedCountedCluster(t, 200) + + runCountedList(t, counted, "resource", "list", "-r", "pvc-7") + + if n := counted.vds.wholeCluster.Load(); n != 0 { + t.Errorf("%d whole-cluster reads for a one-definition listing, want none", n) + } + + if n := counted.vds.perDefinition.Load(); n != 1 { + t.Errorf("%d per-definition reads, want exactly 1 — the listing covers one definition", n) + } +} + +// A narrowed listing returns every row the filter matches. The filter compares +// case-insensitively, as LINSTOR does, while a replica's spec keeps the case its +// writer used: an adoption run or linstor-csi can store `NODE-1` for a node the +// operator types as `node-1`. Answering `-n` and `-r` with the scoped reads, +// which compare the stored spelling, printed an empty table for those replicas +// and exited zero, during the incident the narrowing is typed for. +// +// Both directions are held: stored in upper case and typed in lower, and the +// mirror. +func TestResourceListNarrowedFindsEveryStoredSpelling(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + for _, rep := range []apiv1.Resource{ + {Name: "PVC-ADOPTED", NodeName: "NODE-1"}, + {Name: "pvc-written", NodeName: "node-2"}, + } { + if err := backend.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: rep.Name}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + if err := backend.Resources().Create(ctx, &rep); err != nil { + t.Fatalf("seed replica: %v", err) + } + } + + for _, tc := range []struct { + name string + args []string + want string + }{ + {"node stored upper, typed lower", []string{"-n", "node-1"}, "PVC-ADOPTED"}, + {"definition stored upper, typed lower", []string{"-r", "pvc-adopted"}, "PVC-ADOPTED"}, + {"node stored lower, typed upper", []string{"-n", "NODE-2"}, "pvc-written"}, + {"definition stored lower, typed upper", []string{"-r", "PVC-WRITTEN"}, "pvc-written"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + var out, errBuf bytes.Buffer + + app := &cli.App{ + Out: &out, + Err: &errBuf, + StoreFor: func(context.Context) (store.Store, error) { + return backend, nil + }, + } + + args := append([]string{"resource", "list", "-m"}, tc.args...) + if got := app.Run(t.Context(), args); got != 0 { + t.Fatalf("%v: exit = %d (stderr: %s)", args, got, errBuf.String()) + } + + if !bytes.Contains(out.Bytes(), []byte(tc.want)) { + t.Errorf("%v lost the replica of %s; output = %s", args, tc.want, out.String()) + } + }) + } +} diff --git a/internal/cli/volume_sizes_test.go b/internal/cli/volume_sizes_test.go new file mode 100644 index 00000000..ba0a7ccf --- /dev/null +++ b/internal/cli/volume_sizes_test.go @@ -0,0 +1,395 @@ +// SPDX-License-Identifier: Apache-2.0 + +package cli + +import ( + "bytes" + "context" + "strconv" + "strings" + "testing" + + "github.com/cockroachdb/errors" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// A replica names its definition in whatever case it was written with, and the +// definition is stored in whatever case IT was written with. LINSTOR treats the +// two as one object; a map does not. +// +// The per-definition read hands the name to the store, which resolves it the +// way it resolves every other name. The whole-cluster read gets a map back and +// does the lookup here, so this is the one place the equality has to be +// spelled out — and keyed raw it misses silently, rendering a definition as +// though it had no volumes and dropping the sync percentage during exactly the +// resync an operator is watching. +func TestVolumeSizesFindMixedCaseDefinitionsInTheBulkRead(t *testing.T) { + t.Parallel() + + st := store.NewInMemory() + ctx := t.Context() + run := &runContext{Store: st} + + // Enough definitions that volumeSizesFor takes the whole-cluster read. + definitions := volumeSizesBulkCutoff + 1 + resources := make([]apiv1.Resource, 0, definitions) + + for i := range definitions { + // The definition is stored one way and the replica names it another: + // wireToCRDResourceSpec keeps Spec.ResourceDefinitionName verbatim, + // so a replica really does carry a spelling its definition is not + // stored under. Folding the replica's name here instead would hand + // both sides the same key and the lookup-side fold could never be + // the discriminator. + stored := "pvc-fold-" + strconv.Itoa(i) + spelled := "PVC-Fold-" + strconv.Itoa(i) + + if err := st.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: stored}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + if err := st.VolumeDefinitions().Create(ctx, stored, + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 4096}); err != nil { + t.Fatalf("seed volume: %v", err) + } + + resources = append(resources, apiv1.Resource{Name: spelled, NodeName: "node-1"}) + } + + sizes := volumeSizesFor(ctx, run, resources) + + for i := range resources { + perVolume, ok := sizes[resources[i].Name] + if !ok { + t.Fatalf("no sizes for %q — the lookup missed the definition it was stored under", + resources[i].Name) + } + + if perVolume[0] != 4096 { + t.Errorf("%q volume 0 = %d KiB, want 4096", resources[i].Name, perVolume[0]) + } + } +} + +// failingListAll is a store whose whole-cluster read is broken and whose +// per-definition read is not — a partial outage, an RBAC gap on list, a +// request too large for the apiserver. +type failingListAll struct { + store.VolumeDefinitionStore +} + +// errBulkReadFailed stands in for whatever breaks a whole-cluster read: a +// partial outage, an RBAC gap on list, a response too large. +var errBulkReadFailed = errors.New("list every definition failed") + +func (f failingListAll) ListAll(context.Context) (map[string][]apiv1.VolumeDefinition, error) { + return nil, errBulkReadFailed +} + +type failingListAllStore struct { + store.Store +} + +func (f failingListAllStore) VolumeDefinitions() store.VolumeDefinitionStore { + return failingListAll{f.Store.VolumeDefinitions()} +} + +// One read means one failure costs every row its percentage, where the +// per-definition path loses only the definition it could not read. The two +// sides are supposed to answer the same and degrade the same, and the doc +// comment says so — so a broken bulk read falls through rather than blanking +// the column for the whole listing. +func TestVolumeSizesDegradePerDefinitionWhenTheBulkReadFails(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + run := &runContext{Store: failingListAllStore{backend}} + + definitions := volumeSizesBulkCutoff + 1 + resources := make([]apiv1.Resource, 0, definitions) + + for i := range definitions { + name := "pvc-degrade-" + strconv.Itoa(i) + + if err := backend.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: name}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + if err := backend.VolumeDefinitions().Create(ctx, name, + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 4096}); err != nil { + t.Fatalf("seed volume: %v", err) + } + + resources = append(resources, apiv1.Resource{Name: name, NodeName: "node-1"}) + } + + sizes := volumeSizesFor(ctx, run, resources) + + if len(sizes) != definitions { + t.Fatalf("sizes for %d of %d definitions — one failed read blanked the column "+ + "for the whole listing", len(sizes), definitions) + } +} + +// countingLists records how many per-definition reads a fallback actually +// issued, so a test can assert the retry did not happen rather than assert on +// its result — which is the same either way when both reads fail. +type countingLists struct { + store.VolumeDefinitionStore + + bulkErr error + calls *int +} + +func (c countingLists) ListAll(context.Context) (map[string][]apiv1.VolumeDefinition, error) { + return nil, c.bulkErr +} + +func (c countingLists) List(ctx context.Context, rdName string) ([]apiv1.VolumeDefinition, error) { + *c.calls++ + + vds, err := c.VolumeDefinitionStore.List(ctx, rdName) + + return vds, errors.Wrap(err, "counted per-definition read") +} + +type countingStore struct { + store.Store + + bulkErr error + calls *int +} + +func (c countingStore) VolumeDefinitions() store.VolumeDefinitionStore { + return countingLists{ + VolumeDefinitionStore: c.Store.VolumeDefinitions(), + bulkErr: c.bulkErr, + calls: c.calls, + } +} + +func seedDefinitionsForSizes(t *testing.T, backend store.Store, prefix string, n int) []apiv1.Resource { + t.Helper() + + ctx := t.Context() + resources := make([]apiv1.Resource, 0, n) + + for i := range n { + name := prefix + strconv.Itoa(i) + + if err := backend.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: name}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + if err := backend.VolumeDefinitions().Create(ctx, name, + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 4096}); err != nil { + t.Fatalf("seed volume: %v", err) + } + + resources = append(resources, apiv1.Resource{Name: name, NodeName: "node-1"}) + } + + return resources +} + +// A refusal aimed at the caller is not a statement about the read's shape, so +// answering it with one narrow request per definition spends N round trips to +// be refused N times — in the command an operator is running because something +// is already wrong. The listing still prints; the reason the column is empty +// reaches stderr instead of looking like a cluster with nothing to sync. +func TestVolumeSizesDoNotRetryARefusalPerDefinition(t *testing.T) { + t.Parallel() + + definitions := schema.GroupResource{Group: "blockstor.cozystack.io", Resource: "resourcedefinitions"} + + for _, tc := range []struct { + name string + err error + }{ + {"forbidden", apierrors.NewForbidden(definitions, "", errors.New("no list permission"))}, + // An expired token on a CLI invocation: refused as surely as a + // missing permission, and just as identically on every retry. + {"unauthorized", apierrors.NewUnauthorized("token expired")}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + calls := 0 + warnings := &bytes.Buffer{} + run := &runContext{ + Store: countingStore{Store: backend, bulkErr: tc.err, calls: &calls}, + Err: warnings, + } + + resources := seedDefinitionsForSizes(t, backend, "pvc-"+tc.name+"-", volumeSizesBulkCutoff+1) + + sizes := volumeSizesFor(t.Context(), run, resources) + + if calls != 0 { + t.Errorf("the refused bulk read was retried as %d per-definition reads", calls) + } + + if len(sizes) != 0 { + t.Errorf("sizes for %d definitions after a refusal that reached no data", len(sizes)) + } + + if !strings.Contains(warnings.String(), "sync percentages unavailable") { + t.Errorf("nothing told the operator why the column is empty; stderr = %q", warnings.String()) + } + }) + } +} + +// A server that answered "slow down" is the budget case by the same rule the +// refusal is. Falling through turns one rejected request into one per +// definition against the server that just asked for fewer, which is the +// opposite of what it asked for and arrives while an operator is watching a +// cluster misbehave. +func TestVolumeSizesDoNotRetryAThrottledServer(t *testing.T) { + t.Parallel() + + for _, tc := range []struct { + name string + err error + }{ + {"tooManyRequests", apierrors.NewTooManyRequests("slow down", 1)}, + {"serviceUnavailable", apierrors.NewServiceUnavailable("overloaded")}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + calls := 0 + warnings := &bytes.Buffer{} + run := &runContext{ + Store: countingStore{Store: backend, bulkErr: tc.err, calls: &calls}, + Err: warnings, + } + + resources := seedDefinitionsForSizes(t, backend, "pvc-"+tc.name+"-", volumeSizesBulkCutoff+1) + + volumeSizesFor(t.Context(), run, resources) + + if calls != 0 { + t.Errorf("a throttled server got %d more requests", calls) + } + + if !strings.Contains(warnings.String(), "sync percentages unavailable") { + t.Errorf("nothing told the operator why the column is empty; stderr = %q", + warnings.String()) + } + }) + } +} + +// Same line, drawn on the budget rather than the permission: a cancelled or +// timed-out invocation has nothing left to spend on a larger retry, and every +// one of those reads would fail on the same expired context. +func TestVolumeSizesDoNotRetryAfterTheContextIsDone(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + calls := 0 + run := &runContext{ + Store: countingStore{Store: backend, bulkErr: context.Canceled, calls: &calls}, + Err: &bytes.Buffer{}, + } + + resources := seedDefinitionsForSizes(t, backend, "pvc-cancelled-", volumeSizesBulkCutoff+1) + + ctx, cancel := context.WithCancel(t.Context()) + cancel() + + volumeSizesFor(ctx, run, resources) + + if calls != 0 { + t.Errorf("a cancelled invocation issued %d more reads", calls) + } +} + +// The positive control for both: a bulk read that failed on its own shape, +// with the invocation still live and permitted, still falls through — and does +// so silently, because the column is filled. +func TestVolumeSizesStillFallThroughOnAnOrdinaryBulkFailure(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + calls := 0 + warnings := &bytes.Buffer{} + run := &runContext{ + Store: countingStore{Store: backend, bulkErr: errBulkReadFailed, calls: &calls}, + Err: warnings, + } + + definitions := volumeSizesBulkCutoff + 1 + resources := seedDefinitionsForSizes(t, backend, "pvc-fallthrough-", definitions) + + sizes := volumeSizesFor(t.Context(), run, resources) + + if calls != definitions { + t.Errorf("fallback issued %d per-definition reads, want %d", calls, definitions) + } + + if len(sizes) != definitions { + t.Errorf("sizes for %d of %d definitions", len(sizes), definitions) + } + + if warnings.Len() != 0 { + t.Errorf("warned about a column it went on to fill: %q", warnings.String()) + } +} + +// failingLists refuses every per-definition read, the way a listing under the +// bulk cutoff meets an API server that is refusing reads altogether. +type failingLists struct { + store.VolumeDefinitionStore +} + +var errPerDefinitionReadFailed = errors.New("read one definition failed") + +func (failingLists) List(context.Context, string) ([]apiv1.VolumeDefinition, error) { + return nil, errPerDefinitionReadFailed +} + +type failingListsStore struct { + store.Store +} + +func (f failingListsStore) VolumeDefinitions() store.VolumeDefinitionStore { + return failingLists{f.Store.VolumeDefinitions()} +} + +// Under the cutoff the listing never takes the bulk read, so the warning the +// bulk path gives is not the one that fires. One definition that could not be +// read is the degradation the per-definition path accepts; none of them read +// is a failure, and without its own warning it reaches the operator as a +// column that looks like a cluster with nothing to sync. +func TestVolumeSizesWarnWhenNoDefinitionCouldBeReadOneAtATime(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + warnings := &bytes.Buffer{} + run := &runContext{Store: failingListsStore{backend}, Err: warnings} + + resources := seedDefinitionsForSizes(t, backend, "pvc-unreadable-", volumeSizesBulkCutoff-1) + + sizes := volumeSizesFor(t.Context(), run, resources) + + if len(sizes) != 0 { + t.Errorf("sizes for %d definitions that could not be read", len(sizes)) + } + + if !strings.Contains(warnings.String(), "sync percentages unavailable") { + t.Errorf("every per-definition read failed and nothing said so; stderr = %q", warnings.String()) + } +} diff --git a/pkg/rest/bug_359_mid_delete_promote_test.go b/pkg/rest/bug_359_mid_delete_promote_test.go index 4b9ee465..b7568371 100644 --- a/pkg/rest/bug_359_mid_delete_promote_test.go +++ b/pkg/rest/bug_359_mid_delete_promote_test.go @@ -80,6 +80,10 @@ func (v *midDeleteTBResources) ListByDefinition(ctx context.Context, rdName stri return v.inner.ListByDefinition(ctx, rdName) //nolint:wrapcheck // test helper } +func (v *midDeleteTBResources) ListByNode(ctx context.Context, node string) ([]apiv1.Resource, error) { + return v.inner.ListByNode(ctx, node) //nolint:wrapcheck // test helper +} + func (v *midDeleteTBResources) Get(ctx context.Context, rdName, node string) (apiv1.Resource, error) { if [2]string{rdName, node} == v.raceKey && v.dyingPresent.Load() { return apiv1.Resource{ diff --git a/pkg/rest/bug_359_witness_collapse_race_test.go b/pkg/rest/bug_359_witness_collapse_race_test.go index d0f2a5eb..0614e2ac 100644 --- a/pkg/rest/bug_359_witness_collapse_race_test.go +++ b/pkg/rest/bug_359_witness_collapse_race_test.go @@ -73,6 +73,10 @@ func (v *vanishingTBResources) ListByDefinition(ctx context.Context, rdName stri return v.inner.ListByDefinition(ctx, rdName) //nolint:wrapcheck // test helper } +func (v *vanishingTBResources) ListByNode(ctx context.Context, node string) ([]apiv1.Resource, error) { + return v.inner.ListByNode(ctx, node) //nolint:wrapcheck // test helper +} + func (v *vanishingTBResources) Get(ctx context.Context, rdName, node string) (apiv1.Resource, error) { if [2]string{rdName, node} == v.raceKey { // Witness already finalized — Get sees the gap. @@ -400,6 +404,10 @@ func (v *alwaysVanishingTBResources) ListByDefinition(ctx context.Context, rdNam return v.inner.ListByDefinition(ctx, rdName) //nolint:wrapcheck // test helper } +func (v *alwaysVanishingTBResources) ListByNode(ctx context.Context, node string) ([]apiv1.Resource, error) { + return v.inner.ListByNode(ctx, node) //nolint:wrapcheck // test helper +} + func (v *alwaysVanishingTBResources) Get(ctx context.Context, rdName, node string) (apiv1.Resource, error) { if [2]string{rdName, node} == v.raceKey { return apiv1.Resource{}, errors.Wrapf(store.ErrNotFound, diff --git a/pkg/rest/cache_invalidation_bug_124_test.go b/pkg/rest/cache_invalidation_bug_124_test.go index 421ce7da..78df29a2 100644 --- a/pkg/rest/cache_invalidation_bug_124_test.go +++ b/pkg/rest/cache_invalidation_bug_124_test.go @@ -81,6 +81,10 @@ func (l *laggingResources) ListByDefinition(ctx context.Context, rdName string) return l.inner.ListByDefinition(ctx, rdName) //nolint:wrapcheck // test helper } +func (l *laggingResources) ListByNode(ctx context.Context, node string) ([]apiv1.Resource, error) { + return l.inner.ListByNode(ctx, node) //nolint:wrapcheck // test helper +} + func (l *laggingResources) Get(ctx context.Context, rdName, node string) (apiv1.Resource, error) { return l.inner.Get(ctx, rdName, node) //nolint:wrapcheck // test helper } diff --git a/pkg/rest/delete_toctou_polish_bug_177_178_test.go b/pkg/rest/delete_toctou_polish_bug_177_178_test.go index 84c7e737..6fafd5fb 100644 --- a/pkg/rest/delete_toctou_polish_bug_177_178_test.go +++ b/pkg/rest/delete_toctou_polish_bug_177_178_test.go @@ -193,6 +193,26 @@ func (t *twoPhaseResourceStore) List(_ context.Context) ([]apiv1.Resource, error return t.withRef, nil } +// ListByNode models the same two phases. The node-delete refusal asks the +// node-scoped question now, and a double that overrides only List would let +// the call fall through to the real store and lose the race this test exists +// to reproduce. +func (t *twoPhaseResourceStore) ListByNode(_ context.Context, node string) ([]apiv1.Resource, error) { + n := t.calls.Add(1) + if n == 1 { + return []apiv1.Resource{}, nil + } + + out := make([]apiv1.Resource, 0, len(t.withRef)) + for i := range t.withRef { + if t.withRef[i].NodeName == node { + out = append(out, t.withRef[i]) + } + } + + return out, nil +} + // bug178NodeStore composites the two flaky views with the real // InMemory backing for everything else. type bug178NodeStore struct { diff --git a/pkg/rest/n_d_registered_spelling_race_test.go b/pkg/rest/n_d_registered_spelling_race_test.go new file mode 100644 index 00000000..734aa69f --- /dev/null +++ b/pkg/rest/n_d_registered_spelling_race_test.go @@ -0,0 +1,115 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "net/http" + "testing" + + "github.com/cockroachdb/errors" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// foldingNodes resolves a node name the way the Kubernetes store does, through +// its folded form, and lands a replica under the registered spelling the moment +// the node row goes: the Bug 174 window, with a writer that spells the node +// the way it was registered. +type foldingNodes struct { + store.NodeStore + + onDelete func(ctx context.Context, registered string) error +} + +func (f foldingNodes) Get(ctx context.Context, name string) (apiv1.Node, error) { + registered, err := f.registered(ctx, name) + if err != nil { + return apiv1.Node{}, err + } + + return f.NodeStore.Get(ctx, registered) //nolint:wrapcheck // test double +} + +func (f foldingNodes) Delete(ctx context.Context, name string) error { + registered, err := f.registered(ctx, name) + if err != nil { + return err + } + + err = f.NodeStore.Delete(ctx, registered) + if err != nil { + return err //nolint:wrapcheck // test double + } + + return f.onDelete(ctx, registered) +} + +func (f foldingNodes) registered(ctx context.Context, name string) (string, error) { + all, err := f.List(ctx) + if err != nil { + return "", errors.Wrap(err, "list nodes") + } + + for i := range all { + if store.FoldName(all[i].Name) == store.FoldName(name) { + return all[i].Name, nil + } + } + + return name, nil +} + +type foldingNodeStore struct { + store.Store + + nodes foldingNodes +} + +func (f foldingNodeStore) Nodes() store.NodeStore { return f.nodes } + +// The Bug 174 re-walk runs after the node row is deleted, and the spelling a +// node is registered under is only known from that row. Re-deriving the +// spellings there asked the caller's spelling and its fold alone, so a replica +// that raced in under the registered spelling was invisible to the re-walk, the +// rollback never fired, and the node stayed deleted under a live replica. +func TestNodeDeleteRollsBackARaceUnderTheRegisteredSpelling(t *testing.T) { + t.Parallel() + + inner := store.NewInMemory() + ctx := t.Context() + + if err := inner.Nodes().Create(ctx, &apiv1.Node{Name: "NODE-X", Type: apiv1.NodeTypeSatellite}); err != nil { + t.Fatalf("seed node: %v", err) + } + + if err := inner.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "rd-race"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + st := foldingNodeStore{ + Store: inner, + nodes: foldingNodes{ + NodeStore: inner.Nodes(), + onDelete: func(ctx context.Context, registered string) error { + return inner.Resources().Create(ctx, &apiv1.Resource{Name: "rd-race", NodeName: registered}) + }, + }, + } + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := httpDelete(t, base+"/v1/nodes/node-x") + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusConflict { + t.Errorf("node delete over a replica that raced in under NODE-X = %d, want 409 from the rollback", + resp.StatusCode) + } + + if _, err := inner.Nodes().Get(ctx, "NODE-X"); err != nil { + t.Errorf("the node is gone with a replica still on it: %v", err) + } +} diff --git a/pkg/rest/node_lifecycle.go b/pkg/rest/node_lifecycle.go index 06bff73b..dc79160a 100644 --- a/pkg/rest/node_lifecycle.go +++ b/pkg/rest/node_lifecycle.go @@ -502,10 +502,16 @@ func (s *Server) handleNodeLost(w http.ResponseWriter, r *http.Request) { // idempotent contract (TestNodeLostUnknownIsIdempotent) stays // intact — the parent handler's cascade and Delete are both // NotFound-tolerant. +// +// The status is read uncached, for the same reason the node-scoped +// listings below it are: this decision is acted on immediately and +// nothing converges behind it. A cached ONLINE the satellite has +// already stopped sending refuses the cleanup a dead node needs, and +// a cached OFFLINE tears down a node that is answering. func (s *Server) checkNodeLostAllowed(w http.ResponseWriter, r *http.Request, name string) bool { ctx := r.Context() - node, err := s.Store.Nodes().Get(ctx, name) + node, err := s.Store.Nodes().GetUncached(ctx, name) if err != nil { if errors.Is(err, store.ErrNotFound) { return true diff --git a/pkg/rest/node_lost_stale_status_test.go b/pkg/rest/node_lost_stale_status_test.go new file mode 100644 index 00000000..c6e133ea --- /dev/null +++ b/pkg/rest/node_lost_stale_status_test.go @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "encoding/json" + "net/http" + "testing" + + "github.com/cockroachdb/errors" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// staleNodeCache is a store whose cached Get trails the API server. Get +// answers the status the node carried before the satellite stopped +// reporting; GetUncached answers what the API server actually holds. That is +// the shape a manager's informer produces under load, and the integration +// harness reproduced it: the satellite mock had stamped OFFLINE and the API +// server had it, while the REST server's cached client still read ONLINE and +// refused the cleanup. +type staleNodeCache struct { + store.NodeStore + + stale string +} + +func (s staleNodeCache) Get(ctx context.Context, name string) (apiv1.Node, error) { + node, err := s.NodeStore.Get(ctx, name) + if err != nil { + return node, errors.Wrap(err, "stale-cache node read") + } + + node.ConnectionStatus = s.stale + + return node, nil +} + +type staleNodeCacheStore struct { + store.Store + + stale string +} + +func (s staleNodeCacheStore) Nodes() store.NodeStore { + return staleNodeCache{NodeStore: s.Store.Nodes(), stale: s.stale} +} + +// The gate decides whether to unregister a node and cascade away its replicas +// on one field, and then acts. Read from a cache, a status one beat behind is +// not a slow answer but a wrong one: here the satellite is gone and the API +// server says so, while the cache still holds the last ONLINE heartbeat, and +// the cleanup the operator needs is refused with a reason that is no longer +// true. Nothing converges behind the refusal — the operator retries by hand. +func TestNodeLostReadsTheStatusFromTheAPIServerNotTheCache(t *testing.T) { + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.Nodes().Create(ctx, &apiv1.Node{Name: "n1"}); err != nil { + t.Fatalf("seed node: %v", err) + } + + if err := backend.Nodes().SetConnectionStatus(ctx, "n1", apiv1.NodeTypeOffline); err != nil { + t.Fatalf("seed connection status: %v", err) + } + + st := staleNodeCacheStore{Store: backend, stale: apiv1.NodeTypeOnline} + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := httpPost(t, base+"/v1/nodes/n1/lost", nil) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusOK { + var rcs []apiv1.APICallRc + _ = json.NewDecoder(resp.Body).Decode(&rcs) + + t.Fatalf("status: got %d, want 200 — the gate refused on a cached ONLINE the "+ + "API server had already moved past; envelope=%+v", resp.StatusCode, rcs) + } + + if _, err := backend.Nodes().Get(ctx, "n1"); err == nil { + t.Errorf("node still present after a legitimate `n lost`") + } +} + +// The other direction, and the dangerous one: the cache is behind on a node +// that has come back. Answering from it would unregister a satellite that is +// reporting and orphan the DRBD state on a live host, which is the whole +// reason the gate exists. +func TestNodeLostStillRefusesWhenOnlyTheCacheSaysOffline(t *testing.T) { + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.Nodes().Create(ctx, &apiv1.Node{Name: "n1"}); err != nil { + t.Fatalf("seed node: %v", err) + } + + if err := backend.Nodes().SetConnectionStatus(ctx, "n1", apiv1.NodeTypeOnline); err != nil { + t.Fatalf("seed connection status: %v", err) + } + + st := staleNodeCacheStore{Store: backend, stale: apiv1.NodeTypeOffline} + + base, stop := startServerWithStore(t, st) + defer stop() + + resp := httpPost(t, base+"/v1/nodes/n1/lost", nil) + defer func() { _ = resp.Body.Close() }() + + if resp.StatusCode != http.StatusConflict { + t.Fatalf("status: got %d, want 409 — a stale cached OFFLINE let `n lost` "+ + "through against a satellite that is reporting", resp.StatusCode) + } + + if _, err := backend.Nodes().Get(ctx, "n1"); err != nil { + t.Errorf("live node was removed anyway: %v", err) + } +} diff --git a/pkg/rest/nodes.go b/pkg/rest/nodes.go index 27283026..77e6dee0 100644 --- a/pkg/rest/nodes.go +++ b/pkg/rest/nodes.go @@ -1125,13 +1125,18 @@ func (s *Server) handleNodeDelete(w http.ResponseWriter, r *http.Request) { return } + spellings, ok := s.nodeDeleteSpellings(ctx, w, name, force) + if !ok { + return + } + (&deleteWithRollback[apiv1.Node]{ refuseIfReferenced: func() bool { if force { return false } - return s.refuseNodeDeleteIfReferenced(w, r, name) + return s.refuseNodeDeleteIfReferenced(w, r, name, spellings) }, capture: func() (apiv1.Node, bool) { return s.captureNode(ctx, name) @@ -1144,7 +1149,7 @@ func (s *Server) handleNodeDelete(w http.ResponseWriter, r *http.Request) { return false } - return s.rollbackNodeDeleteIfRaced(w, r, name, &captured) + return s.rollbackNodeDeleteIfRaced(w, r, name, spellings, &captured) }, writeWarn: func() { writeJSON(w, http.StatusOK, []apiv1.APICallRc{{ @@ -1161,6 +1166,27 @@ func (s *Server) handleNodeDelete(w http.ResponseWriter, r *http.Request) { }).run(w) } +// nodeDeleteSpellings resolves, once and while the node row still exists, the +// spellings both reference walks of a plain node delete ask in. The post-Delete +// re-walk runs after the row is gone, and re-deriving them then loses the +// registered spelling, which is the one a racing replica written under it is +// found by. A forced delete walks nothing and needs none. False means an error +// has been written. +func (s *Server) nodeDeleteSpellings(ctx context.Context, w http.ResponseWriter, name string, force bool) ([]string, bool) { + if force { + return nil, true + } + + spellings, err := store.NodeSpellings(ctx, s.Store, name) + if err != nil { + writeStoreError(w, err) + + return nil, false + } + + return spellings, true +} + // refuseNodeDeleteIfReferenced runs the pre-Delete Bug 92 / Bug 179 // walk. Returns true when the HTTP error has already been written // (the caller must stop processing) and false when the delete may @@ -1175,8 +1201,8 @@ func (s *Server) handleNodeDelete(w http.ResponseWriter, r *http.Request) { // autoplacer's free-space ranking then crashed on the nil-Node // lookup. Mirrors `n lost`'s cascadeOrphansForLostNode which // already walks both stores in lock-step. -func (s *Server) refuseNodeDeleteIfReferenced(w http.ResponseWriter, r *http.Request, name string) bool { - resourceRefs, spRefs, err := s.referencesOnNode(r.Context(), name) +func (s *Server) refuseNodeDeleteIfReferenced(w http.ResponseWriter, r *http.Request, name string, spellings []string) bool { + resourceRefs, spRefs, err := s.referencesOnNode(r.Context(), spellings) if err != nil { writeStoreError(w, err) @@ -1241,15 +1267,16 @@ func (s *Server) refuseNodeDeleteIfEvicted(ctx context.Context, w http.ResponseW // referencesOnNode bundles the two reference walks the Bug 92 / // Bug 179 gates run in lock-step (Resources + StoragePools on the -// target node). Returning both lists in one call keeps the -// pre-walk and the post-Delete re-walk byte-identical — drift -// between the two would let a racing dependent through the gate. -func (s *Server) referencesOnNode(ctx context.Context, name string) ([]string, []string, error) { +// target node). Returning both lists in one call, over spellings +// handleNodeDelete resolved once before the Delete, keeps the +// pre-walk and the post-Delete re-walk asking the same question — +// drift between the two would let a racing dependent through the gate. +func (s *Server) referencesOnNode(ctx context.Context, spellings []string) ([]string, []string, error) { // One implementation with the CLI, which refuses on the same question. // Keeping a second copy here is how the default-diskless-pool carve-out // came to exist on one door only. //nolint:wrapcheck // surfaced via writeStoreError - return store.ReferencesOnNode(ctx, s.Store, name) + return store.ReferencesUnderSpellings(ctx, s.Store, spellings) } // buildNodeDeleteRefusal assembles the 409 envelope for the Bug 92 @@ -1324,8 +1351,10 @@ func (s *Server) captureNode(ctx context.Context, name string) (apiv1.Node, bool // re-walk to ALSO catch a racing `sp c ` so an SP CRD // persisted during the TOCTOU window can't orphan into a deleted // Node either. -func (s *Server) rollbackNodeDeleteIfRaced(w http.ResponseWriter, r *http.Request, name string, captured *apiv1.Node) bool { - resourceRefs, spRefs, err := s.referencesOnNode(r.Context(), name) +func (s *Server) rollbackNodeDeleteIfRaced( + w http.ResponseWriter, r *http.Request, name string, spellings []string, captured *apiv1.Node, +) bool { + resourceRefs, spRefs, err := s.referencesOnNode(r.Context(), spellings) if err != nil { writeStoreError(w, err) @@ -1365,7 +1394,11 @@ func (s *Server) rollbackNodeDeleteIfRaced(w http.ResponseWriter, r *http.Reques // order on the K8s backend, and operators rerun `n d` to confirm // the refusal message after every replica drop). func (s *Server) resourcesOnNode(ctx context.Context, node string) ([]string, error) { - resources, err := s.Store.Resources().List(ctx) + // The node-scoped read, not the whole cluster filtered here: the + // refusal decision already moved to it, and a message helper answering + // the same one-node question with a full List was the read this change + // exists to remove. + resources, err := store.ReplicasOnNode(ctx, s.Store, node) if err != nil { return nil, errors.Wrap(err, "list resources") } @@ -1373,9 +1406,7 @@ func (s *Server) resourcesOnNode(ctx context.Context, node string) ([]string, er var refs []string for i := range resources { - if resources[i].NodeName == node { - refs = append(refs, resources[i].Name) - } + refs = append(refs, resources[i].Name) } sort.Strings(refs) diff --git a/pkg/rest/orphan_sweep_read_failure_test.go b/pkg/rest/orphan_sweep_read_failure_test.go new file mode 100644 index 00000000..bfd690a9 --- /dev/null +++ b/pkg/rest/orphan_sweep_read_failure_test.go @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/cockroachdb/errors" + "github.com/go-logr/logr/funcr" + ctrllog "sigs.k8s.io/controller-runtime/pkg/log" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// errSnapshotParentRead stands in for whatever breaks the read that finds the +// orphan: a timeout, a throttled API server, a selector the cluster cannot +// serve. +var errSnapshotParentRead = errors.New("list snapshots by definition failed") + +type failingSnapshotList struct { + store.SnapshotStore +} + +func (f failingSnapshotList) ListByDefinition(context.Context, string) ([]apiv1.Snapshot, error) { + return nil, errSnapshotParentRead +} + +// The sweep reads uncached, so the failure has to be on that read too. +func (f failingSnapshotList) ListByDefinitionUncached(context.Context, string) ([]apiv1.Snapshot, error) { + return nil, errSnapshotParentRead +} + +type failingSnapshotListStore struct { + store.Store +} + +func (f failingSnapshotListStore) Snapshots() store.SnapshotStore { + return failingSnapshotList{f.Store.Snapshots()} +} + +// sweepCaptureContext hands the sweep a logger that buffers every entry, so a +// test can assert on what reached the operator rather than on what the code +// meant to say. The package already declares a type named logr, so the logger +// is built inline rather than returned. +func sweepCaptureContext(t *testing.T, buf *bytes.Buffer) context.Context { + t.Helper() + + return ctrllog.IntoContext(t.Context(), funcr.New(func(prefix, args string) { + buf.WriteString(prefix) + buf.WriteString(" ") + buf.WriteString(args) + buf.WriteString("\n") + }, funcr.Options{Verbosity: 1})) +} + +// The sweep is best-effort by design, but the read it skips on is the one that +// finds the orphan. The operator has already been told the delete succeeded, +// so a silent return leaves the Bug 180 row alive with nothing anywhere saying +// the mop-up did not run. +func TestOrphanSweepSaysSoWhenItsReadFails(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + ctx := sweepCaptureContext(t, &buf) + srv := &Server{Store: failingSnapshotListStore{store.NewInMemory()}} + + srv.sweepOrphanSnapshotsAfterRDDelete(ctx, "pvc-swept") + + if !strings.Contains(buf.String(), "orphan-snapshot sweep skipped") { + t.Errorf("a failed parent read left no trace; log = %q", buf.String()) + } + + if !strings.Contains(buf.String(), "pvc-swept") { + t.Errorf("the log does not name the definition; log = %q", buf.String()) + } +} + +// The control: the ordinary path stays quiet. Without it the assertion above +// is satisfied by a function that logs on every call, which would bury the +// signal in the noise of every successful delete. +func TestOrphanSweepStaysQuietWhenItsReadSucceeds(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + + ctx := sweepCaptureContext(t, &buf) + srv := &Server{Store: store.NewInMemory()} + + srv.sweepOrphanSnapshotsAfterRDDelete(ctx, "pvc-clean") + + if buf.Len() != 0 { + t.Errorf("a clean sweep logged %q", buf.String()) + } +} diff --git a/pkg/rest/rd_delete_snapshot_read_test.go b/pkg/rest/rd_delete_snapshot_read_test.go new file mode 100644 index 00000000..ce08e111 --- /dev/null +++ b/pkg/rest/rd_delete_snapshot_read_test.go @@ -0,0 +1,125 @@ +// SPDX-License-Identifier: Apache-2.0 + +package rest + +import ( + "context" + "net/http" + "sync/atomic" + "testing" + + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" +) + +// cacheBlindSnapshots is a snapshot substore whose cached listing has not seen +// the row a concurrent create just wrote, while the API server has it. That is +// the moment rd d decides on: the snapshot most likely to be missing from the +// cache is exactly the one that raced the delete. +type cacheBlindSnapshots struct { + store.SnapshotStore +} + +func (cacheBlindSnapshots) ListByDefinition(context.Context, string) ([]apiv1.Snapshot, error) { + return nil, nil +} + +type cacheBlindSnapshotStore struct{ store.Store } + +func (c cacheBlindSnapshotStore) Snapshots() store.SnapshotStore { + return cacheBlindSnapshots{c.Store.Snapshots()} +} + +// rd d refuses over existing snapshots and then acts, so it is a destructive +// decision; this round applied "must not read a cache" to the node reads and +// not to this gate one file over. Deciding on the cached listing dropped a +// definition the API server knew still had a snapshot. +func TestRDDeleteRefusesOnASnapshotTheCacheHasNotSeen(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "rd-snapgate"}); err != nil { + t.Fatalf("seed RD: %v", err) + } + + if err := backend.Snapshots().Create(ctx, &apiv1.Snapshot{Name: "snap-raced", ResourceName: "rd-snapgate"}); err != nil { + t.Fatalf("seed snapshot: %v", err) + } + + base, stop := startServerWithStore(t, cacheBlindSnapshotStore{backend}) + defer stop() + + resp := httpDelete(t, base+"/v1/resource-definitions/rd-snapgate") + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusConflict { + t.Fatalf("status = %d, want 409 — the definition still has a snapshot", resp.StatusCode) + } + + if _, err := backend.ResourceDefinitions().Get(ctx, "rd-snapgate"); err != nil { + t.Errorf("the definition was deleted over its snapshot: %v", err) + } +} + +// fullListCounter counts whole-cluster replica listings. +type fullListCounter struct { + store.ResourceStore + + full *atomic.Int64 +} + +func (f fullListCounter) List(ctx context.Context) ([]apiv1.Resource, error) { + f.full.Add(1) + + return f.ResourceStore.List(ctx) //nolint:wrapcheck // test decorator +} + +type fullListCountingStore struct { + store.Store + + full *atomic.Int64 +} + +func (f fullListCountingStore) Resources() store.ResourceStore { + return fullListCounter{ResourceStore: f.Store.Resources(), full: f.full} +} + +// The refusal message named the node's replicas by listing every replica in +// the cluster, after the decision itself had already moved to the node-scoped +// read. A one-node question, answered with the read this change removes. +func TestNodeLostRefusalNamesReplicasWithoutAWholeClusterListing(t *testing.T) { + t.Parallel() + + backend := store.NewInMemory() + ctx := t.Context() + + if err := backend.Nodes().Create(ctx, &apiv1.Node{Name: "n-scoped"}); err != nil { + t.Fatalf("seed node: %v", err) + } + + if err := backend.Nodes().SetConnectionStatus(ctx, "n-scoped", apiv1.NodeTypeOnline); err != nil { + t.Fatalf("seed status: %v", err) + } + + if err := backend.Resources().Create(ctx, &apiv1.Resource{Name: "pvc-on-scoped", NodeName: "n-scoped"}); err != nil { + t.Fatalf("seed replica: %v", err) + } + + var full atomic.Int64 + + base, stop := startServerWithStore(t, fullListCountingStore{Store: backend, full: &full}) + defer stop() + + resp := httpPost(t, base+"/v1/nodes/n-scoped/lost", nil) + _ = resp.Body.Close() + + if resp.StatusCode != http.StatusConflict { + t.Fatalf("status = %d, want 409", resp.StatusCode) + } + + if n := full.Load(); n != 0 { + t.Errorf("%d whole-cluster replica listing(s) to name one node's replicas", n) + } +} diff --git a/pkg/rest/resource_definitions.go b/pkg/rest/resource_definitions.go index 74e65635..12efbc4f 100644 --- a/pkg/rest/resource_definitions.go +++ b/pkg/rest/resource_definitions.go @@ -23,6 +23,7 @@ import ( "net/http" "github.com/cockroachdb/errors" + "sigs.k8s.io/controller-runtime/pkg/log" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" "github.com/cozystack/blockstor/pkg/passphrase" @@ -1100,7 +1101,7 @@ func (s *Server) handleRDDelete(w http.ResponseWriter, r *http.Request) { // stamps DeletionTimestamp on every replica, a failed RD-delete // leaves the cluster half-torn-down (children gone, parent // kept, snapshots orphaned) which no retry can reconcile. - snaps, err := s.Store.Snapshots().ListByDefinition(r.Context(), name) + snaps, err := s.Store.Snapshots().ListByDefinitionUncached(r.Context(), name) if err != nil && !errors.Is(err, store.ErrNotFound) { writeStoreError(w, err) @@ -1213,8 +1214,21 @@ func (s *Server) handleRDDelete(w http.ResponseWriter, r *http.Request) { // primary": there's no RD to restore (rd-d's success was a deliberate // caller intent), so the right action is to mop up the orphan. func (s *Server) sweepOrphanSnapshotsAfterRDDelete(ctx context.Context, rdName string) { - leftovers, err := s.Store.Snapshots().ListByDefinition(ctx, rdName) - if err != nil || len(leftovers) == 0 { + leftovers, err := s.Store.Snapshots().ListByDefinitionUncached(ctx, rdName) + if err != nil { + // The sweep is best-effort, but silence here is not: the read that + // failed is the one that finds the orphan, so a transient failure + // leaves exactly the row this function exists to clear, and the + // operator has already been told the delete succeeded. Whether the + // mop-up ran is the one thing that would make them look. + log.FromContext(ctx).WithName("rest"). + Error(err, "orphan-snapshot sweep skipped: a snapshot that raced the delete may survive", + "resourceDefinition", rdName) + + return + } + + if len(leftovers) == 0 { return } diff --git a/pkg/rest/snap_d_wait_bug_193_test.go b/pkg/rest/snap_d_wait_bug_193_test.go index bd1af4ad..728e5f09 100644 --- a/pkg/rest/snap_d_wait_bug_193_test.go +++ b/pkg/rest/snap_d_wait_bug_193_test.go @@ -73,6 +73,10 @@ func (s *stuckSnapshots) ListByDefinition(ctx context.Context, rdName string) ([ return s.inner.ListByDefinition(ctx, rdName) //nolint:wrapcheck // test helper } +func (s *stuckSnapshots) ListByDefinitionUncached(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + return s.inner.ListByDefinitionUncached(ctx, rdName) //nolint:wrapcheck // test helper +} + func (s *stuckSnapshots) Get(ctx context.Context, rdName, snapName string) (apiv1.Snapshot, error) { return s.inner.Get(ctx, rdName, snapName) //nolint:wrapcheck // test helper } diff --git a/pkg/rest/snap_multi_scrub_bug_200_test.go b/pkg/rest/snap_multi_scrub_bug_200_test.go index 9e02fc1e..0f9981a4 100644 --- a/pkg/rest/snap_multi_scrub_bug_200_test.go +++ b/pkg/rest/snap_multi_scrub_bug_200_test.go @@ -361,3 +361,7 @@ func TestBug200SnapshotMultiPreservesLiteralMessages(t *testing.T) { }) } } + +func (s *errInjectingSnapshots) ListByDefinitionUncached(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + return s.ListByDefinition(ctx, rdName) +} diff --git a/pkg/rest/storage_pools.go b/pkg/rest/storage_pools.go index f2de8c73..20d30278 100644 --- a/pkg/rest/storage_pools.go +++ b/pkg/rest/storage_pools.go @@ -438,22 +438,16 @@ func (s *Server) handleStoragePoolsView(w http.ResponseWriter, r *http.Request) // handleNodeStoragePoolsList serves GET /v1/nodes/{node}/storage-pools. // // Implementation note: we deliberately go through the same List()+filter -// pipeline that /v1/view/storage-pools uses (rather than the store's -// ListByNode shortcut) for two reasons: +// pipeline that /v1/view/storage-pools uses rather than the store's +// ListByNode shortcut. Java LINSTOR matches node names case-insensitively on +// both paths, and ListByNode compares the stored spelling (see +// store.FoldName), so routing the per-node handler through the same +// matchAnyFold filter is what keeps the two endpoints in lockstep — a parity +// invariant the storage_pools_test.go MatchesViewFiltering test pins. // -// 1. The k8s backend's ListByNode relies on a label selector that is only -// populated when the CRD was created through our Create() path. Pools -// that land in the cluster via operator `kubectl apply -f` or a -// migration won't carry the label and would silently disappear from -// the per-node listing — but they show up correctly in the view. -// linstor-csi's autoplace probes /v1/nodes/{node}/storage-pools per -// node, so an empty per-node response means "no candidate nodes", -// leading to ResourceExhausted and stuck-Pending PVCs even though -// the pools are visible in the aggregate view. -// 2. Java LINSTOR matches node names case-insensitively in both paths. -// Routing the per-node handler through the same matchAnyFold filter -// keeps the two endpoints in lockstep — a parity invariant the -// storage_pools_test.go MatchesViewFiltering test pins. +// This used to give a second reason, that ListByNode ran a label selector a +// hand-applied pool would not match. It selects on spec.nodeName now, which +// every pool carries whoever wrote it, so that reason no longer holds. func (s *Server) handleNodeStoragePoolsList(w http.ResponseWriter, r *http.Request) { node := r.PathValue("node") diff --git a/pkg/store/cascade.go b/pkg/store/cascade.go index 210ed92b..c0bd32e2 100644 --- a/pkg/store/cascade.go +++ b/pkg/store/cascade.go @@ -22,6 +22,7 @@ import ( "context" "errors" "fmt" + "slices" "sort" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" @@ -88,33 +89,143 @@ func CascadeDeleteResources(ctx context.Context, st Store, rdName string) error return nil } +// ReplicasOnNode is the node-scoped replica read a node's fate is decided on, +// asked in every spelling the node's objects can carry. ReferencesOnNode and +// CascadeOrphansForLostNode ask the storage pools the same way. +// +// Nodes().Get and Delete fold the name, while spec.nodeName is stored +// verbatim, so a node addressed in a different case than its replicas were +// written with resolved for the delete and returned nothing for the gate: the +// refusal passed, the cascade reaped nothing, the node went, and the replicas +// still pointed at it. +// +// Three spellings are asked, see NodeSpellings. The one that remains out of +// reach is a spelling that is neither the caller's, nor the folded one, nor +// the node's registered one: a replica created by hand under yet another case. +// That is the boundary FoldName documents. The merge base asked only the +// caller's spelling, verbatim. +func ReplicasOnNode(ctx context.Context, st Store, node string) ([]apiv1.Resource, error) { + spellings, err := NodeSpellings(ctx, st, node) + if err != nil { + return nil, err + } + + return replicasUnder(ctx, st, spellings) +} + +// NodeSpellings is the set of spellings a node-scoped read has to be asked in: +// the caller's, the folded one, and the one the node is registered under. +// +// The registered spelling is the one that matters most and the one the other +// two missed. Adoption from LINSTOR and linstor-csi write a replica's node +// under the name the node was registered with, so a node registered as +// `NODE-1` carries replicas on `NODE-1`, and an operator typing the canonical +// `node-1` asked in one spelling, since that one is already folded, and found +// nothing. +// +// It is read from the node listing and compared with FoldName, rather than +// taken from Nodes().Get: the Kubernetes store's Get folds through the CRD +// slug, the in-memory one does not, and the listing gives both the same +// answer. A node that is not registered (already gone, or never was) +// contributes nothing, and the two remaining spellings are still asked. +// +// That makes the answer depend on whether the node row still exists, so a +// caller that re-asks after deleting the node resolves the spellings before the +// delete and passes them to ReferencesUnderSpellings for both walks. +func NodeSpellings(ctx context.Context, st Store, node string) ([]string, error) { + spellings := []string{node} + + add := func(spelling string) { + if !slices.Contains(spellings, spelling) { + spellings = append(spellings, spelling) + } + } + + add(FoldName(node)) + + nodes, err := st.Nodes().List(ctx) + if err != nil { + return nil, fmt.Errorf("list nodes to resolve %s: %w", node, err) + } + + for i := range nodes { + if FoldName(nodes[i].Name) == FoldName(node) { + add(nodes[i].Name) + } + } + + return spellings, nil +} + +func replicasUnder(ctx context.Context, st Store, spellings []string) ([]apiv1.Resource, error) { + return inEverySpelling(ctx, spellings, "replicas", st.Resources().ListByNode, + func(r *apiv1.Resource) string { return r.Name }) +} + +func poolsUnder(ctx context.Context, st Store, spellings []string) ([]apiv1.StoragePool, error) { + return inEverySpelling(ctx, spellings, "storage pools", st.StoragePools().ListByNode, + func(p *apiv1.StoragePool) string { return p.StoragePoolName }) +} + +// inEverySpelling runs a node-scoped read under each spelling, and returns +// every object once. +func inEverySpelling[T any]( + ctx context.Context, spellings []string, what string, + read func(context.Context, string) ([]T, error), name func(*T) string, +) ([]T, error) { + var out []T + + seen := map[string]struct{}{} + + for _, spelling := range spellings { + found, err := read(ctx, spelling) + if err != nil { + return nil, fmt.Errorf("list %s on %s: %w", what, spelling, err) + } + + for i := range found { + key := FoldName(name(&found[i])) + if _, dup := seen[key]; dup { + continue + } + + seen[key] = struct{}{} + + out = append(out, found[i]) + } + } + + return out, nil +} + // CascadeOrphansForLostNode deletes every Resource replica and StoragePool // that references the named node, which is what makes a forced node delete // leave nothing pointing at an object that is gone. func CascadeOrphansForLostNode(ctx context.Context, st Store, node string) error { - resources, err := st.Resources().List(ctx) + spellings, err := NodeSpellings(ctx, st, node) if err != nil { - return fmt.Errorf("list replicas: %w", err) + return err } - for i := range resources { - if resources[i].NodeName != node { - continue - } + resources, err := replicasUnder(ctx, st, spellings) + if err != nil { + return err + } - err = st.Resources().Delete(ctx, resources[i].Name, node) + for i := range resources { + err = st.Resources().Delete(ctx, resources[i].Name, resources[i].NodeName) if err != nil && !errors.Is(err, ErrNotFound) { return fmt.Errorf("delete replica %s on %s: %w", resources[i].Name, node, err) } } - pools, err := st.StoragePools().ListByNode(ctx, node) + pools, err := poolsUnder(ctx, st, spellings) if err != nil { - return fmt.Errorf("list storage pools on %s: %w", node, err) + return err } for i := range pools { - err = st.StoragePools().Delete(ctx, node, pools[i].StoragePoolName) + err = st.StoragePools().Delete(ctx, pools[i].NodeName, pools[i].StoragePoolName) if err != nil && !errors.Is(err, ErrNotFound) { return fmt.Errorf("delete storage pool %s on %s: %w", pools[i].StoragePoolName, node, err) } @@ -130,19 +241,28 @@ func CascadeOrphansForLostNode(ctx context.Context, st Store, node string) error // This is what a plain node delete is refused on: the operator either clears // the references or says explicitly that the node is gone. func ReferencesOnNode(ctx context.Context, st Store, node string) ([]string, []string, error) { - resources, err := st.Resources().List(ctx) + spellings, err := NodeSpellings(ctx, st, node) if err != nil { - return nil, nil, fmt.Errorf("list replicas: %w", err) + return nil, nil, err + } + + return ReferencesUnderSpellings(ctx, st, spellings) +} + +// ReferencesUnderSpellings is ReferencesOnNode over spellings the caller +// resolved with NodeSpellings. A caller that walks the references again after +// deleting the node has to use it: by then the node row is gone and a fresh +// NodeSpellings no longer knows the registered spelling. +func ReferencesUnderSpellings(ctx context.Context, st Store, spellings []string) ([]string, []string, error) { + resources, err := replicasUnder(ctx, st, spellings) + if err != nil { + return nil, nil, err } seen := map[string]struct{}{} rscRefs := make([]string, 0, len(resources)) for i := range resources { - if resources[i].NodeName != node { - continue - } - if _, dup := seen[resources[i].Name]; dup { continue } @@ -152,9 +272,9 @@ func ReferencesOnNode(ctx context.Context, st Store, node string) ([]string, []s rscRefs = append(rscRefs, resources[i].Name) } - pools, err := st.StoragePools().ListByNode(ctx, node) + pools, err := poolsUnder(ctx, st, spellings) if err != nil { - return nil, nil, fmt.Errorf("list storage pools on %s: %w", node, err) + return nil, nil, err } poolRefs := make([]string, 0, len(pools)) diff --git a/pkg/store/inmemory.go b/pkg/store/inmemory.go index 8b3bbd3d..c4dd60d0 100644 --- a/pkg/store/inmemory.go +++ b/pkg/store/inmemory.go @@ -317,6 +317,11 @@ func (s *inMemoryNodes) Get(_ context.Context, name string) (apiv1.Node, error) return decorateInMemoryNode(&n), nil } +// GetUncached has nothing to bypass here: this store is the API server. +func (s *inMemoryNodes) GetUncached(ctx context.Context, name string) (apiv1.Node, error) { + return s.Get(ctx, name) +} + // decorateInMemoryNode applies the wire-shape decoration both Get and // List run on every read: NetInterface upstream defaults // (port=3366 / type=PLAIN / first-iface IsActive=true) plus the F2 diff --git a/pkg/store/inmemory_resource.go b/pkg/store/inmemory_resource.go index 93dc0c9a..8d5e5f34 100644 --- a/pkg/store/inmemory_resource.go +++ b/pkg/store/inmemory_resource.go @@ -62,6 +62,23 @@ func (s *inMemoryResources) List(_ context.Context) ([]apiv1.Resource, error) { return out, nil } +func (s *inMemoryResources) ListByNode(_ context.Context, node string) ([]apiv1.Resource, error) { + s.mu.RLock() + defer s.mu.RUnlock() + + out := make([]apiv1.Resource, 0) + + for k := range s.m { + if s.m[k].NodeName == node { + out = append(out, s.m[k]) + } + } + + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + + return out, nil +} + func (s *inMemoryResources) ListByDefinition(_ context.Context, rdName string) ([]apiv1.Resource, error) { s.mu.RLock() defer s.mu.RUnlock() diff --git a/pkg/store/inmemory_snapshot.go b/pkg/store/inmemory_snapshot.go index cb06becc..0c3fa0be 100644 --- a/pkg/store/inmemory_snapshot.go +++ b/pkg/store/inmemory_snapshot.go @@ -75,6 +75,12 @@ func (s *inMemorySnapshots) ListByDefinition(_ context.Context, rdName string) ( return out, nil } +// ListByDefinitionUncached has nothing to bypass here: this store is the API +// server. +func (s *inMemorySnapshots) ListByDefinitionUncached(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + return s.ListByDefinition(ctx, rdName) +} + func (s *inMemorySnapshots) Get(_ context.Context, rdName, snapName string) (apiv1.Snapshot, error) { s.mu.RLock() defer s.mu.RUnlock() diff --git a/pkg/store/inmemory_volume_definition.go b/pkg/store/inmemory_volume_definition.go index a771762f..88627759 100644 --- a/pkg/store/inmemory_volume_definition.go +++ b/pkg/store/inmemory_volume_definition.go @@ -45,8 +45,14 @@ func (s *inMemoryVolumeDefinitions) List(_ context.Context, rdName string) ([]ap out := make([]apiv1.VolumeDefinition, 0) + // Folded, the way ListAll keys and the Kubernetes store resolves the name + // through the RD's folded metadata.name. Compared verbatim, a mixed-case + // lookup answered differently depending on which side of the CLI's + // bulk-read cutoff it landed on. + want := FoldName(rdName) + for k := range s.m { - if k.rd == rdName { + if FoldName(k.rd) == want { out = append(out, s.m[k]) } } @@ -56,6 +62,28 @@ func (s *inMemoryVolumeDefinitions) List(_ context.Context, rdName string) ([]ap return out, nil } +// ListAll groups every stored volume by its parent definition, keyed the way +// the Kubernetes store keys it: folded, because LINSTOR names are +// case-insensitive and the caller's spelling need not be the stored one. +func (s *inMemoryVolumeDefinitions) ListAll(_ context.Context) (map[string][]apiv1.VolumeDefinition, error) { + s.mu.RLock() + defer s.mu.RUnlock() + + out := make(map[string][]apiv1.VolumeDefinition) + + for k := range s.m { + key := FoldName(k.rd) + out[key] = append(out[key], s.m[k]) + } + + for rd := range out { + vds := out[rd] + sort.Slice(vds, func(i, j int) bool { return vds[i].VolumeNumber < vds[j].VolumeNumber }) + } + + return out, nil +} + func (s *inMemoryVolumeDefinitions) Get(_ context.Context, rdName string, volumeNumber int32) (apiv1.VolumeDefinition, error) { s.mu.RLock() defer s.mu.RUnlock() diff --git a/pkg/store/k8s/field_index.go b/pkg/store/k8s/field_index.go new file mode 100644 index 00000000..c2081f25 --- /dev/null +++ b/pkg/store/k8s/field_index.go @@ -0,0 +1,290 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package k8s + +import ( + "context" + "reflect" + "strings" + "time" + + "github.com/cockroachdb/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" +) + +// NewManager builds a manager and the store that serves from it, as one call. +// +// Three things have to hold together for a manager-backed store to answer the +// reads it issues, and each used to be a separate step a binary could skip. +// The cache needs the field indexes the store selects on, or every scoped read +// quietly lists the whole collection and filters in process — the read the +// scoped one exists to replace, and neither step fails loudly. And the store +// needs the manager's direct reader, or the reads that decide a node's fate +// answer from a cache that trails the API server. +// +// Registering the indexes separately is how both server binaries and the +// integration harness came to run on the fallback at once. Threading the +// reader separately is how the controller binary came to serve the LINSTOR +// surface from a cache-only store while the apiserver's had the reader. So +// there is no second constructor to leave out: the store comes back from the +// same call that built the manager, from that manager's own client and reader. +// +// Registering an index resolves its kind's REST mapping, which asks the API +// server, so construction waits for one that is briefly unreachable rather +// than failing on the first refused connection: both binaries exit when this +// returns an error, and an API server restarting while the pod starts would +// otherwise be a crash loop. The wait is bounded by indexRegistrationBudget, +// see there for why the bound is what it is. +// +//nolint:gocritic // ctrl.Options by value mirrors ctrl.NewManager, which this wraps +func NewManager(cfg *rest.Config, opts ctrl.Options) (ctrl.Manager, *Store, error) { + mgr, err := newIndexedManager(cfg, opts, indexRegistrationBudget) + if err != nil { + return nil, nil, err + } + + return mgr, NewWithAPIReader(mgr.GetClient(), mgr.GetAPIReader()), nil +} + +// indexRegistrationBudget is how long NewManager keeps retrying the index +// registration before it gives up. +// +// Nothing serves /healthz while it waits: the health endpoint comes up in +// mgr.Start, after construction returns. So every liveness probe fired during +// the wait fails, and the wait has to end before the kubelet's kill does, or +// the outage it rides out ends in a restart with nothing in the log saying +// why. The kubelet kills at initialDelaySeconds + (failureThreshold-1) * +// periodSeconds. The two deployments that ship leave the period and the +// threshold at their defaults of 10 and 3 after a 15s delay, which is 35s; +// config/manager/manager.yaml sets a 20s period, which is 55s. 20s leaves the +// earliest of those 15s for the process to start and build the manager before +// registration begins. TestIndexRegistrationBudgetEndsBeforeLivenessKills +// holds this against the manifests themselves. +// +// The bound is on the wait, not on an attempt: an attempt still in flight when +// the budget runs out is abandoned rather than awaited, since a connection +// into a dropped route can take client-go's 30s dial timeout to fail. +const indexRegistrationBudget = 20 * time.Second + +//nolint:gocritic // ctrl.Options by value mirrors ctrl.NewManager, which this wraps +func newIndexedManager(cfg *rest.Config, opts ctrl.Options, budget time.Duration) (ctrl.Manager, error) { + mgr, err := ctrl.NewManager(cfg, opts) + if err != nil { + return nil, errors.Wrap(err, "new manager") + } + + err = registerFieldIndexesWithin(budget, mgr.GetFieldIndexer()) + if err != nil { + return nil, err + } + + return mgr, nil +} + +// registerFieldIndexesWithin registers every index, retrying the ones that +// failed until the budget runs out. An index that registered is never asked +// again: the informer refuses a second indexer under the same name, so +// retrying the whole set after a partial success would fail on the part that +// worked. +func registerFieldIndexesWithin(budget time.Duration, indexer ctrlclient.FieldIndexer) error { + ctx, cancel := context.WithTimeout(context.Background(), budget) + defer cancel() + + pending := fieldIndexes() + delay := indexRetryFirstDelay + + var lastErr error + + for { + attempt := make(chan registrationAttempt, 1) + + go func(pending []fieldIndex) { + left, err := registerPending(ctx, indexer, pending) + attempt <- registrationAttempt{left: left, err: err} + }(pending) + + select { + case <-ctx.Done(): + return gaveUpRegistering(budget, lastErr, ctx.Err()) + case got := <-attempt: + if got.err == nil { + return nil + } + + pending, lastErr = got.left, got.err + } + + select { + case <-ctx.Done(): + return gaveUpRegistering(budget, lastErr, ctx.Err()) + case <-time.After(delay): + } + + delay = min(delay*2, indexRetryMaxDelay) + } +} + +type registrationAttempt struct { + left []fieldIndex + err error +} + +// gaveUpRegistering names the budget and the most telling error: the last +// attempt's when one finished, the deadline's when the first never did. +func gaveUpRegistering(budget time.Duration, lastErr, deadline error) error { + if lastErr == nil { + lastErr = deadline + } + + return errors.Wrapf(lastErr, "gave up registering the field indexes after %s", budget) +} + +const ( + indexRetryFirstDelay = 250 * time.Millisecond + indexRetryMaxDelay = 5 * time.Second +) + +// registerPending registers what it can and returns what is still left, with +// the first error it met. +func registerPending(ctx context.Context, indexer ctrlclient.FieldIndexer, pending []fieldIndex) ([]fieldIndex, error) { + var ( + left []fieldIndex + firstErr error + ) + + for _, index := range pending { + err := indexer.IndexField(ctx, index.object, index.field, index.extract) + if err != nil { + left = append(left, index) + + if firstErr == nil { + firstErr = errors.Wrapf(err, "index %s by %s", reflect.TypeOf(index.object).Elem().Name(), index.field) + } + } + } + + return left, firstErr +} + +// SelectorUnsupported reports whether an error means the server cannot answer +// that selector, as opposed to the read having failed. +// +// Only the first is safe to answer by reading everything instead. A timeout, +// an RBAC refusal or a cancelled context are not statements about the +// selector, and taking the whole-cluster read for them answers a failure that +// ran out of time or permission with a larger request against the same +// exhausted budget — and discards the error that said so. +// +// Three producers say it, two of them without a type to check. +// +// - the API server rejects a fieldSelector over an undeclared field with +// 400 "field label not supported", which is typed; +// - a manager's cache answers an unindexed field with `Index with name +// field: does not exist`; +// - controller-runtime's fake client, which the unit suites run on, words +// the same condition as `... no index with name has been registered +// for GroupVersionKind ...`. +// +// Both untyped wordings name the index, so that is what is matched. Matching +// either wording exactly is how the fake client's went unrecognised: a store +// built on it turned every scoped read into a 500 rather than the fallback. +func SelectorUnsupported(err error) bool { + if err == nil { + return false + } + + if apierrors.IsBadRequest(err) { + return true + } + + return strings.Contains(strings.ToLower(err.Error()), "index with name") +} + +// FieldResourceNodeName is the field a node-scoped Resource query selects on. +// The CRD declares it selectable, and the query only ever reaches an uncached +// reader: the CLI's client, or the API reader every manager-built store has +// (see resources.nodeScopedReader). Both turn it into a fieldSelector the API +// server answers, so the cache carries no index for it. +const FieldResourceNodeName = "spec.nodeName" + +// FieldResourceDefinitionName is the field a definition-scoped Resource query +// selects on. Unlike the label the objects usually carry, it is the spec value +// itself, so a replica applied by hand is found by it too — the Bug 038 shape, +// where a label selector answered with a partial-but-correct subset and the +// unlabelled replicas were invisible rather than an error. +const FieldResourceDefinitionName = "spec.resourceDefinitionName" + +// FieldStoragePoolNodeName is the same node field on StoragePool, read the same +// way and likewise not indexed. +const FieldStoragePoolNodeName = "spec.nodeName" + +// FieldSnapshotDefinitionName is the definition field a snapshot listing +// selects on, for the same reason its Resource sibling does not use a label: +// a Snapshot adopted from LINSTOR by pkg/linstormigrate carries none. +const FieldSnapshotDefinitionName = "spec.resourceDefinitionName" + +// fieldIndex is one index NewManager registers on the cache. +type fieldIndex struct { + object ctrlclient.Object + field string + extract ctrlclient.IndexerFunc +} + +// fieldIndexes are the fields the store selects on through a manager's cached +// client, which answers a field selector from a local index or not at all: an +// unindexed field is not a slow query but a failed one, "Index with name +// field:spec.resourceDefinitionName does not exist", and the store then falls +// back to listing every object, silently, on every call. +// +// Only the definition-scoped reads go through the cache (resources and +// snapshots ListByDefinition). The node-scoped ones go to the API reader, +// which sends the selector to the API server, so an index for spec.nodeName +// would be maintained over every Resource and StoragePool in the cache and +// read by nothing. +func fieldIndexes() []fieldIndex { + return []fieldIndex{ + { + object: &crdv1alpha1.Resource{}, field: FieldResourceDefinitionName, + extract: func(obj ctrlclient.Object) []string { + res, ok := obj.(*crdv1alpha1.Resource) + if !ok || res.Spec.ResourceDefinitionName == "" { + return nil + } + + return []string{res.Spec.ResourceDefinitionName} + }, + }, + { + object: &crdv1alpha1.Snapshot{}, field: FieldSnapshotDefinitionName, + extract: func(obj ctrlclient.Object) []string { + snap, ok := obj.(*crdv1alpha1.Snapshot) + if !ok || snap.Spec.ResourceDefinitionName == "" { + return nil + } + + return []string{snap.Spec.ResourceDefinitionName} + }, + }, + } +} diff --git a/pkg/store/k8s/field_index_retry_test.go b/pkg/store/k8s/field_index_retry_test.go new file mode 100644 index 00000000..854ba3e2 --- /dev/null +++ b/pkg/store/k8s/field_index_retry_test.go @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" +) + +var ( + errDiscoveryBlip = errors.New("failed to get server groups: connection refused") + errIndexerConflict = errors.New("indexer conflict") + errNeverRegistering = errors.New("still unreachable") +) + +// flakyIndexer refuses one field once, as a discovery blip does, and refuses a +// second indexer under a name it already holds, as an informer does. +type flakyIndexer struct { + failOnce string + registered map[string]int +} + +func (f *flakyIndexer) IndexField(_ context.Context, obj ctrlclient.Object, field string, _ ctrlclient.IndexerFunc) error { + key := fmt.Sprintf("%T/%s", obj, field) + + if key == f.failOnce { + f.failOnce = "" + + return errDiscoveryBlip + } + + if f.registered[key] > 0 { + return fmt.Errorf("%w: %s", errIndexerConflict, key) + } + + f.registered[key]++ + + return nil +} + +// A retry after a partial success must ask only for what failed. Retrying the +// whole set asks the informer for an index it already holds, which it refuses, +// so the construction that should have recovered spends its budget failing on +// the part that worked. +func TestIndexRegistrationRetriesOnlyWhatFailed(t *testing.T) { + t.Parallel() + + indexes := fieldIndexes() + last := indexes[len(indexes)-1] + + indexer := &flakyIndexer{ + failOnce: fmt.Sprintf("%T/%s", last.object, last.field), + registered: map[string]int{}, + } + + err := registerFieldIndexesWithin(10*time.Second, indexer) + if err != nil { + t.Fatalf("registration after one blip on the last index: %v", err) + } + + for _, index := range fieldIndexes() { + key := fmt.Sprintf("%T/%s", index.object, index.field) + if indexer.registered[key] != 1 { + t.Errorf("%s registered %d time(s), want 1", key, indexer.registered[key]) + } + } +} + +type deadIndexer struct{} + +func (deadIndexer) IndexField(context.Context, ctrlclient.Object, string, ctrlclient.IndexerFunc) error { + return errNeverRegistering +} + +func TestIndexRegistrationReportsTheLastErrorWhenTheBudgetRunsOut(t *testing.T) { + t.Parallel() + + err := registerFieldIndexesWithin(300*time.Millisecond, deadIndexer{}) + if !errors.Is(err, errNeverRegistering) { + t.Fatalf("err = %v, want it to carry the registration error", err) + } +} + +// hangingIndexer never answers, the way discovery does when its connection +// goes into a dropped route and waits out the dial timeout. +type hangingIndexer struct { + release chan struct{} +} + +func (h hangingIndexer) IndexField(context.Context, ctrlclient.Object, string, ctrlclient.IndexerFunc) error { + <-h.release + + return errNeverRegistering +} + +// The budget bounds the wait, not an attempt. An attempt that hangs has to be +// abandoned when the budget runs out, or the kill the budget was sized to beat +// arrives anyway, as long after it as the hung call takes to fail. +func TestIndexRegistrationAbandonsAnAttemptThatOutlivesTheBudget(t *testing.T) { + t.Parallel() + + indexer := hangingIndexer{release: make(chan struct{})} + t.Cleanup(func() { close(indexer.release) }) + + returned := make(chan error, 1) + + go func() { returned <- registerFieldIndexesWithin(200*time.Millisecond, indexer) }() + + select { + case err := <-returned: + if err == nil { + t.Fatal("registration over an indexer that never answered returned no error") + } + case <-time.After(5 * time.Second): + t.Fatal("registration was still waiting on a hung attempt 5s into a 200ms budget") + } +} diff --git a/pkg/store/k8s/field_index_test.go b/pkg/store/k8s/field_index_test.go new file mode 100644 index 00000000..981dc2a7 --- /dev/null +++ b/pkg/store/k8s/field_index_test.go @@ -0,0 +1,674 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s_test + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/manager" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + apiv1 "github.com/cozystack/blockstor/pkg/api/v1" + "github.com/cozystack/blockstor/pkg/store" + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// countingClient tells a scoped read from the exhaustive one. A List with no +// options is the whole collection; the store only issues one when a scoped +// read has failed and it fell back. +type countingClient struct { + ctrlclient.Client + + exhaustive atomic.Int64 +} + +func (c *countingClient) List(ctx context.Context, list ctrlclient.ObjectList, opts ...ctrlclient.ListOption) error { + if len(opts) == 0 { + c.exhaustive.Add(1) + } + + return c.Client.List(ctx, list, opts...) //nolint:wrapcheck // test decorator +} + +// A manager's client answers a field selector from a local index or not at +// all: an unindexed field is not a slow query, it is a failed one. The store +// falls back to reading every object when that happens, which is the +// whole-cluster read the scoped one exists to replace — taken silently, on +// every call, on both server binaries. +// +// The reads that reach the cache are the definition-scoped ones; the +// node-scoped reads go to the API reader, and +// TestNodeScopedReadsUseTheDirectReaderWhenThereIsOne holds that. So the +// acceptance here is not that the answer is right, since a fallback answers +// right too, but that each definition-scoped read was actually served from an +// index. +func TestRegisteredFieldIndexesServeTheScopedReads(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + seed := k8s.New(fixture.client) + ctx := t.Context() + + if err := seed.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-idx"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + for _, node := range []string{"node-a", "node-b"} { + if err := seed.Nodes().Create(ctx, &apiv1.Node{Name: node, Type: "SATELLITE"}); err != nil { + t.Fatalf("seed node %s: %v", node, err) + } + + if err := seed.Resources().Create(ctx, + &apiv1.Resource{Name: "pvc-idx", NodeName: node}); err != nil { + t.Fatalf("seed replica on %s: %v", node, err) + } + } + + if err := fixture.client.Create(ctx, &crdv1alpha1.Snapshot{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-idx.snap-idx"}, + Spec: crdv1alpha1.SnapshotSpec{ + ResourceDefinitionName: "pvc-idx", + SnapshotName: "snap-idx", + }, + }); err != nil { + t.Fatalf("seed snapshot: %v", err) + } + + counted := &countingClient{Client: startedCachedClient(t)} + cached := k8s.New(counted) + + // The cache trails the writes above, so the reads are retried until it + // has caught up. Every one of them is scoped: a fallback would show up in + // the counter whichever attempt took it. + waitFor(t, func() bool { + replicas, err := cached.Resources().ListByDefinition(t.Context(), "pvc-idx") + + return err == nil && len(replicas) == 2 + }, "the definition's replicas") + + waitFor(t, func() bool { + snaps, err := cached.Snapshots().ListByDefinition(t.Context(), "pvc-idx") + + return err == nil && len(snaps) == 1 + }, "the definition's snapshot") + + if n := counted.exhaustive.Load(); n != 0 { + t.Errorf("%d whole-collection reads, want none — a scoped read fell back, "+ + "which is the exhaustive listing the index exists to avoid", n) + } +} + +// startedCachedClient brings up a manager against the envtest API server with +// the store's field indexes registered, and returns its cached client. +func startedCachedClient(t *testing.T) ctrlclient.Client { + t.Helper() + + // k8s.NewManager, which is what the two binaries and the integration + // harness call: registering the indexes separately is how all three came + // to be running on the fallback at once, so the constructor is what this + // pins. + mgr, _, err := k8s.NewManager(fixture.env.Config, manager.Options{ + Scheme: fixture.client.Scheme(), + Metrics: metricsserver.Options{BindAddress: "0"}, + HealthProbeBindAddress: "0", + }) + if err != nil { + t.Fatalf("build manager: %v", err) + } + + ctx, cancel := context.WithCancel(context.Background()) + stopped := make(chan struct{}) + + go func() { + defer close(stopped) + + _ = mgr.Start(ctx) + }() + + t.Cleanup(func() { + cancel() + <-stopped + }) + + if !mgr.GetCache().WaitForCacheSync(ctx) { + t.Fatal("cache never synced") + } + + return mgr.GetClient() +} + +// waitFor polls until the condition holds, or fails the test naming what it +// was waiting for. +func waitFor(t *testing.T, cond func() bool, what string) { + t.Helper() + + deadline := time.Now().Add(15 * time.Second) + + for time.Now().Before(deadline) { + if cond() { + return + } + + time.Sleep(50 * time.Millisecond) + } + + t.Fatalf("timed out waiting for %s", what) +} + +// The store's own contract, independent of any cache: a store built on the +// direct client answers the same scoped questions. +func TestScopedReadsOnAnUncachedClient(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + st := k8s.New(fixture.client) + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, &apiv1.ResourceDefinition{Name: "pvc-direct"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + for _, node := range []string{"node-a", "node-b"} { + if err := st.Nodes().Create(ctx, &apiv1.Node{Name: node, Type: "SATELLITE"}); err != nil { + t.Fatalf("seed node %s: %v", node, err) + } + + if err := st.Resources().Create(ctx, + &apiv1.Resource{Name: "pvc-direct", NodeName: node}); err != nil { + t.Fatalf("seed replica on %s: %v", node, err) + } + + if err := st.StoragePools().Create(ctx, &apiv1.StoragePool{ + StoragePoolName: "pool-1", + NodeName: node, + ProviderKind: "LVM_THIN", + }); err != nil { + t.Fatalf("seed pool on %s: %v", node, err) + } + } + + replicas, err := st.Resources().ListByNode(ctx, "node-a") + if err != nil || len(replicas) != 1 || replicas[0].NodeName != "node-a" { + t.Errorf("ListByNode = %v, %v; want the one replica on node-a", replicas, err) + } + + pools, err := st.StoragePools().ListByNode(ctx, "node-a") + if err != nil || len(pools) != 1 || pools[0].NodeName != "node-a" { + t.Errorf("pools ListByNode = %v, %v; want the one pool on node-a", pools, err) + } + + byDefinition, err := st.Resources().ListByDefinition(ctx, "pvc-direct") + if err != nil || len(byDefinition) != 2 { + t.Errorf("ListByDefinition = %v, %v; want both replicas", byDefinition, err) + } + + _ = store.FoldName("") +} + +// A label is written by whoever created the object, and piraeus and operators +// create storage pools with `kubectl apply` and no labels. The node-scoped +// read selected on that label, so those pools were invisible to it — and this +// list is what a `node delete` is refused on and what the cascade removes, so +// an invisible pool is a node deleted with pools still registered against it. +func TestPoolsAppliedWithoutALabelAreStillOnTheNode(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + st := k8s.New(fixture.client) + ctx := t.Context() + + if err := st.Nodes().Create(ctx, &apiv1.Node{Name: "node-hand", Type: "SATELLITE"}); err != nil { + t.Fatalf("seed node: %v", err) + } + + // Written the way an operator writes one: the spec, and nothing else. + applied := &crdv1alpha1.StoragePool{ + ObjectMeta: metav1.ObjectMeta{Name: "pool-hand.node-hand"}, + Spec: crdv1alpha1.StoragePoolSpec{ + NodeName: "node-hand", + PoolName: "pool-hand", + ProviderKind: "LVM_THIN", + }, + } + + if err := fixture.client.Create(ctx, applied); err != nil { + t.Fatalf("apply the pool: %v", err) + } + + pools, err := st.StoragePools().ListByNode(ctx, "node-hand") + if err != nil { + t.Fatalf("ListByNode: %v", err) + } + + if len(pools) != 1 { + t.Fatalf("ListByNode returned %d pools, want the one applied by hand — a pool "+ + "the node-scoped read cannot see is a node deleted out from under it", len(pools)) + } +} + +// A label is written by whoever created the object, and pkg/linstormigrate +// builds Snapshots adopted from a LINSTOR dump with none. This list is what +// `rd d` is refused on and what sweeps the leftovers behind it, so a snapshot +// the read cannot see is a definition deleted with snapshots still on it, and +// a mop-up that misses them too. +func TestSnapshotsAppliedWithoutALabelAreStillOnTheDefinition(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + st := k8s.New(fixture.client) + ctx := t.Context() + + if err := st.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: "pvc-adopted"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + // Written the way the migrator writes one: the spec, and nothing else. + adopted := &crdv1alpha1.Snapshot{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-adopted.snap-adopted"}, + Spec: crdv1alpha1.SnapshotSpec{ + ResourceDefinitionName: "pvc-adopted", + SnapshotName: "snap-adopted", + }, + } + + if err := fixture.client.Create(ctx, adopted); err != nil { + t.Fatalf("apply the snapshot: %v", err) + } + + snaps, err := st.Snapshots().ListByDefinition(ctx, "pvc-adopted") + if err != nil { + t.Fatalf("ListByDefinition: %v", err) + } + + if len(snaps) != 1 { + t.Fatalf("ListByDefinition returned %d snapshots, want the one applied by hand — "+ + "a snapshot this read cannot see is a definition deleted out from under it", + len(snaps)) + } +} + +// The two untyped wordings a missing index reaches this store as, verbatim +// from controller-runtime: the manager cache's, and the fake client's. +var ( + //nolint:staticcheck // verbatim controller-runtime wording; matching it is the point + errCacheHasNoIndex = errors.New("Index with name field:spec.nodeName does not exist") + + errFakeClientHasNoIndex = errors.New("List on GroupVersionKind /v1, Kind=Resource " + + "specifies selector on field spec.nodeName, but no index with name spec.nodeName " + + "has been registered for GroupVersionKind /v1, Kind=Resource") + + errRBACRefused = errors.New("nope") +) + +// refusingClient answers every scoped list with one chosen error. +type refusingClient struct { + ctrlclient.Client + + err error +} + +func (c refusingClient) List(ctx context.Context, list ctrlclient.ObjectList, opts ...ctrlclient.ListOption) error { + if len(opts) > 0 { + return c.err + } + + return c.Client.List(ctx, list, opts...) //nolint:wrapcheck // test decorator +} + +// Falling back to the whole-cluster read is only safe for the one error that +// means "this server cannot answer that selector". A timeout, an RBAC refusal +// or a cancelled context are not statements about the selector: answering them +// with a larger read against the same exhausted budget, and returning nil, +// hides the failure and does the expensive thing at the worst moment. +func TestScopedReadsFallBackOnlyWhenTheSelectorIsRefused(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + seed := k8s.New(fixture.client) + ctx := t.Context() + + if err := seed.Nodes().Create(ctx, &apiv1.Node{Name: "node-err", Type: "SATELLITE"}); err != nil { + t.Fatalf("seed node: %v", err) + } + + for name, tc := range map[string]struct { + err error + wantErr bool + }{ + "the selector is refused": { + err: apierrors.NewBadRequest(`field label not supported: spec.nodeName`), + wantErr: false, + }, + "the cache has no index": { + err: errCacheHasNoIndex, + wantErr: false, + }, + // The fake client the unit suites run on words it differently, and + // matching only the cache's wording turned every scoped read on such + // a store into a 500 instead of the fallback. + "the fake client has no index": { + err: errFakeClientHasNoIndex, + wantErr: false, + }, + "forbidden": {err: apierrors.NewForbidden(schema.GroupResource{}, "x", errRBACRefused), wantErr: true}, + "server timeout": {err: apierrors.NewTimeoutError("gateway timeout", 1), wantErr: true}, + "context expired": {err: context.DeadlineExceeded, wantErr: true}, + } { + t.Run(name, func(t *testing.T) { + st := k8s.New(refusingClient{Client: fixture.client, err: tc.err}) + + _, err := st.Resources().ListByNode(ctx, "node-err") + if tc.wantErr && err == nil { + t.Error("the read failed and the store answered nil; the failure is invisible " + + "and the whole-cluster read was issued in its place") + } + + if !tc.wantErr && err != nil { + t.Errorf("a refused selector must fall back, got %v", err) + } + + _, err = st.StoragePools().ListByNode(ctx, "node-err") + if tc.wantErr && err == nil { + t.Error("pools: the read failed and the store answered nil") + } + + if !tc.wantErr && err != nil { + t.Errorf("pools: a refused selector must fall back, got %v", err) + } + }) + } +} + +// countingReads records every List and Get a client is asked for, so a test +// can say which reader answered. +type countingReads struct { + ctrlclient.Client + + lists atomic.Int64 + gets atomic.Int64 +} + +func (c *countingReads) List(ctx context.Context, list ctrlclient.ObjectList, opts ...ctrlclient.ListOption) error { + c.lists.Add(1) + + return c.Client.List(ctx, list, opts...) //nolint:wrapcheck // test decorator +} + +func (c *countingReads) Get( + ctx context.Context, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption, +) error { + c.gets.Add(1) + + return c.Client.Get(ctx, key, obj, opts...) //nolint:wrapcheck // test decorator +} + +// `node delete` is refused on the node-scoped read and cascades away what it +// names, so the answer is acted on destructively and immediately. A cached +// answer that trails the API server by a beat is not slow, it is wrong in both +// directions: a replica the read misses is a node deleted out from under it, +// and a pool the read misses is left pointing at a node that is gone. +// +// So where the store is given the manager's API reader, that read goes to the +// API server. The test writes through one client and reads through the store, +// with no wait in between — which is precisely what the node-lost integration +// test does, and what a cached read cannot be relied on to answer. +func TestNodeScopedReadsUseTheDirectReaderWhenThereIsOne(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + ctx := t.Context() + seed := k8s.New(fixture.client) + + if err := seed.Nodes().Create(ctx, &apiv1.Node{Name: "node-fresh", Type: "SATELLITE"}); err != nil { + t.Fatalf("seed node: %v", err) + } + + if err := seed.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: "pvc-fresh"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + // A cache that will never hold the replica: it is not watching anything. + stale := &countingReads{Client: startedCachedClient(t)} + st := k8s.NewWithAPIReader(stale, fixture.client) + + // Written after the cached client was built, through a different client — + // the shape the integration harness produces. + if err := seed.Resources().Create(ctx, + &apiv1.Resource{Name: "pvc-fresh", NodeName: "node-fresh"}); err != nil { + t.Fatalf("seed replica: %v", err) + } + + if err := seed.StoragePools().Create(ctx, &apiv1.StoragePool{ + StoragePoolName: "pool-fresh", + NodeName: "node-fresh", + ProviderKind: "LVM_THIN", + }); err != nil { + t.Fatalf("seed pool: %v", err) + } + + replicas, err := st.Resources().ListByNode(ctx, "node-fresh") + if err != nil { + t.Fatalf("ListByNode: %v", err) + } + + if len(replicas) != 1 { + t.Errorf("ListByNode returned %d replicas, want the one just written — a node "+ + "delete decided on this answer would have missed it", len(replicas)) + } + + pools, err := st.StoragePools().ListByNode(ctx, "node-fresh") + if err != nil { + t.Fatalf("pools ListByNode: %v", err) + } + + if len(pools) != 1 { + t.Errorf("pools ListByNode returned %d, want the one just written", len(pools)) + } + + if n := stale.lists.Load(); n != 0 { + t.Errorf("%d list(s) went to the cached client; the node-scoped read is supposed "+ + "to bypass it when an API reader is available", n) + } + + // And the field the node-fate decision turns on. `n lost` refuses on + // ConnectionStatus and then acts, so GetUncached has to be what its name + // says on a store that has a reader. Nothing held the node substore's + // reader before: the read counting above covered listings, and the handler + // test only pins that `n lost` asks for GetUncached, against a double. + // Dropping the reader from the node substore is the defect the controller + // binary had, and it reddens this count. + node, err := st.Nodes().GetUncached(ctx, "node-fresh") + if err != nil { + t.Fatalf("GetUncached: %v", err) + } + + if node.Name != "node-fresh" { + t.Errorf("GetUncached returned %q, want node-fresh", node.Name) + } + + if n := stale.gets.Load(); n != 0 { + t.Errorf("%d get(s) went to the cached client; GetUncached is supposed to read "+ + "the API server when the store has a direct reader", n) + } +} + +// `rd d` is refused on the definition's snapshots and sweeps the ones left +// behind it, reading them with ListByDefinitionUncached, because a snapshot +// that raced the delete and has not reached the informer is a definition +// deleted over it. So on a store with a direct reader, neither the scoped read +// nor its fallback may consult the cache. +// +// The refused selector is the case that matters most: it is what a cluster +// whose CRD predates the selectable field answers, and the fallback that +// handed the read back to the cached ListByDefinition was served from the +// informer there. Nothing held either the reader or the fallback before; the +// sibling substores each redden a named test when their reader is dropped. +func TestUncachedSnapshotReadNeverConsultsTheCache(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + ctx := t.Context() + seed := k8s.New(fixture.client) + + if err := seed.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: "pvc-raced"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + for _, tc := range []struct { + name, snapshot string + reader ctrlclient.Client + }{ + {name: "the selector is served", snapshot: "snap-served", reader: fixture.client}, + {name: "the selector is refused", snapshot: "snap-refused", reader: refusingClient{ + Client: fixture.client, + err: apierrors.NewBadRequest("field label not supported: spec.resourceDefinitionName"), + }}, + } { + // A cache that is not watching snapshots, built before the snapshot + // below exists. wipeAll removes the snapshots when the test ends. + stale := &countingReads{Client: startedCachedClient(t)} + st := k8s.NewWithAPIReader(stale, tc.reader) + + raced := &crdv1alpha1.Snapshot{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-raced." + tc.snapshot}, + Spec: crdv1alpha1.SnapshotSpec{ + ResourceDefinitionName: "pvc-raced", + SnapshotName: tc.snapshot, + }, + } + + if err := fixture.client.Create(ctx, raced); err != nil { + t.Fatalf("%s: seed snapshot: %v", tc.name, err) + } + + snaps, err := st.Snapshots().ListByDefinitionUncached(ctx, "pvc-raced") + if err != nil { + t.Fatalf("%s: ListByDefinitionUncached: %v", tc.name, err) + } + + // Both cases' snapshots exist by the second pass, so count only + // this case's. + found := 0 + + for i := range snaps { + if snaps[i].Name == tc.snapshot { + found++ + } + } + + if found != 1 { + t.Errorf("%s: ListByDefinitionUncached did not return the snapshot just written: "+ + "`rd d` decided on this answer deletes the definition over it", tc.name) + } + + if n := stale.lists.Load(); n != 0 { + t.Errorf("%s: %d list(s) went to the cached client; the uncached snapshot read must "+ + "stay on the direct reader, its fallback included", tc.name, n) + } + } +} + +// errRDReadFailed is a read that actually failed, as opposed to a definition +// that is not there: getParentRD answers (nil, nil) for the missing name and +// for NotFound, because an orphan snapshot is a real shape that must still +// list. +var errRDReadFailed = errors.New("probe: transient failure reading the definition") + +type failingRDGet struct { + ctrlclient.Client +} + +func (f failingRDGet) Get(ctx context.Context, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error { + if _, ok := obj.(*crdv1alpha1.ResourceDefinition); ok { + return errRDReadFailed + } + + return f.Client.Get(ctx, key, obj, opts...) //nolint:wrapcheck // test decorator +} + +// A snapshot listing that cannot read the parent definition used to answer +// success with ResourceDefinitionProps silently absent from every row, so the +// caller could not tell "this definition has no props" from "nobody could read +// them". The orphan case is answered earlier and deliberately, so what is left +// here is a genuine failure and belongs to the caller. +func TestSnapshotListByDefinitionSurfacesAFailedParentRead(t *testing.T) { + if fixture == nil { + t.Skip("envtest assets not installed; run `make setup-envtest` to enable") + } + + t.Cleanup(func() { wipeAll(t, fixture.client) }) + + ctx := t.Context() + seed := k8s.New(fixture.client) + + if err := seed.ResourceDefinitions().Create(ctx, + &apiv1.ResourceDefinition{Name: "pvc-parent"}); err != nil { + t.Fatalf("seed definition: %v", err) + } + + if err := seed.Snapshots().Create(ctx, &apiv1.Snapshot{ + Name: "snap-parent", + ResourceName: "pvc-parent", + Nodes: []string{"n1"}, + }); err != nil { + t.Fatalf("seed snapshot: %v", err) + } + + broken := k8s.New(failingRDGet{Client: fixture.client}) + + if _, err := broken.Snapshots().ListByDefinition(ctx, "pvc-parent"); err == nil { + t.Error("the parent read failed and the listing answered success; the rows come " + + "back without the definition's props and nothing says so") + } + + // The control: an orphan snapshot, whose definition is genuinely absent, + // still lists. That is the case the discarded error was hiding behind. + if err := fixture.client.Delete(ctx, &crdv1alpha1.ResourceDefinition{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-parent"}, + }); err != nil { + t.Fatalf("delete the parent: %v", err) + } + + snaps, err := seed.Snapshots().ListByDefinition(ctx, "pvc-parent") + if err != nil { + t.Fatalf("orphan snapshot listing must still work: %v", err) + } + + if len(snaps) != 1 { + t.Errorf("orphan listing returned %d snapshots, want 1", len(snaps)) + } +} diff --git a/pkg/store/k8s/k8s.go b/pkg/store/k8s/k8s.go index da73acfc..f262ca76 100644 --- a/pkg/store/k8s/k8s.go +++ b/pkg/store/k8s/k8s.go @@ -62,6 +62,10 @@ type Store struct { } // New wraps a controller-runtime client and returns a store.Store. +// +// Not for a store backed by a manager: NewManager returns that one, built from +// the manager's own client and direct reader, and a manager's cached client +// handed to New answers the reads that decide a node's fate from a cache. func New(c ctrlclient.Client) *Store { return NewWithAPIReader(c, nil) } @@ -89,13 +93,13 @@ func New(c ctrlclient.Client) *Store { // cached List still under-reported). func NewWithAPIReader(c ctrlclient.Client, apiReader ctrlclient.Reader) *Store { s := &Store{c: c} - s.nodes = &nodes{c: c} - s.storagePools = &storagePools{c: c} + s.nodes = &nodes{c: c, apiReader: apiReader} + s.storagePools = &storagePools{c: c, apiReader: apiReader} s.resourceGroups = &resourceGroups{c: c} s.resourceDefinitions = &resourceDefinitions{c: c, apiReader: apiReader} - s.resources = &resources{c: c} + s.resources = &resources{c: c, apiReader: apiReader} s.volumeDefinitions = &volumeDefinitions{c: c, apiReader: apiReader} - s.snapshots = &snapshots{c: c} + s.snapshots = &snapshots{c: c, apiReader: apiReader} s.physicalDevices = &physicalDevices{c: c} s.controllerProps = &controllerProps{c: c} s.storagePoolDefinitions = &storagePoolDefinitions{m: map[string]store.StoragePoolDefinition{}} diff --git a/pkg/store/k8s/liveness_budget_test.go b/pkg/store/k8s/liveness_budget_test.go new file mode 100644 index 00000000..c5403281 --- /dev/null +++ b/pkg/store/k8s/liveness_budget_test.go @@ -0,0 +1,118 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s + +import ( + "bytes" + "errors" + "io" + "os" + "path/filepath" + "testing" + "time" + + appsv1 "k8s.io/api/apps/v1" + k8syaml "k8s.io/apimachinery/pkg/util/yaml" +) + +// constructionSlack is what the process spends before registration starts and +// what the kubelet adds on top of the kill arithmetic: container start, flag +// parsing, ctrl.NewManager, and the failing probe's own timeout. +const constructionSlack = 10 * time.Second + +// Kubelet's defaults for the probe fields a manifest leaves unset. +const ( + defaultProbePeriod = 10 * time.Second + defaultProbeFailureThreshold = 3 +) + +// The manifests that run a binary calling NewManager. +// +//nolint:gochecknoglobals // a fixed list of repository paths, read by one test +var managerManifests = []string{ + "stand/blockstor-deploy.yaml", + "stand/blockstor-apiserver-deploy.yaml", + "config/manager/manager.yaml", +} + +// Nothing serves /healthz while NewManager waits on the API server, so the wait +// has to end before the kubelet kills the container. A 60s budget outlived the +// kill in every manifest that ships, which turned the outage the wait rides out +// into a restart with no cause in the log. The kill deadline is read from the +// manifests rather than restated here, so a probe tightened later fails this +// instead of silently reopening the gap. +func TestIndexRegistrationBudgetEndsBeforeLivenessKills(t *testing.T) { + t.Parallel() + + root := filepath.Join("..", "..", "..") + + for _, manifest := range managerManifests { + kills := livenessKillDeadlines(t, filepath.Join(root, manifest)) + if len(kills) == 0 { + t.Errorf("%s: no Deployment container with a liveness probe; the check would pass vacuously", manifest) + + continue + } + + for container, kill := range kills { + if indexRegistrationBudget+constructionSlack > kill { + t.Errorf("%s container %s: the kubelet kills at %s, but registration may wait %s after %s of "+ + "construction, with /healthz not yet served", manifest, container, kill, + indexRegistrationBudget, constructionSlack) + } + } + } +} + +// livenessKillDeadlines returns, per container with a liveness probe, the time +// after start at which three failed probes have restarted it. +func livenessKillDeadlines(t *testing.T, path string) map[string]time.Duration { + t.Helper() + + raw, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read %s: %v", path, err) + } + + decoder := k8syaml.NewYAMLOrJSONDecoder(bytes.NewReader(raw), 4096) + kills := map[string]time.Duration{} + + for { + var deploy appsv1.Deployment + + err := decoder.Decode(&deploy) + if errors.Is(err, io.EOF) { + return kills + } + + if err != nil { + t.Fatalf("decode %s: %v", path, err) + } + + if deploy.Kind != "Deployment" { + continue + } + + for i := range deploy.Spec.Template.Spec.Containers { + container := &deploy.Spec.Template.Spec.Containers[i] + + probe := container.LivenessProbe + if probe == nil { + continue + } + + period := defaultProbePeriod + if probe.PeriodSeconds > 0 { + period = time.Duration(probe.PeriodSeconds) * time.Second + } + + threshold := int32(defaultProbeFailureThreshold) + if probe.FailureThreshold > 0 { + threshold = probe.FailureThreshold + } + + kills[container.Name] = time.Duration(probe.InitialDelaySeconds)*time.Second + + time.Duration(threshold-1)*period + } + } +} diff --git a/pkg/store/k8s/manager_construction_test.go b/pkg/store/k8s/manager_construction_test.go new file mode 100644 index 00000000..c9049d41 --- /dev/null +++ b/pkg/store/k8s/manager_construction_test.go @@ -0,0 +1,174 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s_test + +import ( + "context" + "errors" + "io" + "net" + "net/url" + "strings" + "testing" + "time" + + "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" + metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + + "github.com/cozystack/blockstor/pkg/store/k8s" +) + +// apiServerAfter returns an address that refuses connections until delay has +// passed and then forwards every connection to target, the way an API server +// restarting while the pod starts looks from the pod. +func apiServerAfter(t *testing.T, target string, delay time.Duration) string { + t.Helper() + + reserve, err := (&net.ListenConfig{}).Listen(t.Context(), "tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("reserve a port: %v", err) + } + + addr := reserve.Addr().String() + _ = reserve.Close() + + stop := make(chan struct{}) + t.Cleanup(func() { close(stop) }) + + go func() { + select { + case <-time.After(delay): + case <-stop: + return + } + + listener, err := (&net.ListenConfig{}).Listen(context.Background(), "tcp", addr) + if err != nil { + t.Errorf("listen on %s after the delay: %v", addr, err) + + return + } + + go func() { + <-stop + + _ = listener.Close() + }() + + for { + conn, err := listener.Accept() + if err != nil { + return + } + + go forward(conn, target) + } + }() + + return addr +} + +func forward(conn net.Conn, target string) { + defer func() { _ = conn.Close() }() + + upstream, err := (&net.Dialer{}).DialContext(context.Background(), "tcp", target) + if err != nil { + return + } + + defer func() { _ = upstream.Close() }() + + go func() { _, _ = io.Copy(upstream, conn) }() + + _, _ = io.Copy(conn, upstream) +} + +func managerOptions() ctrl.Options { + return ctrl.Options{ + Scheme: fixture.client.Scheme(), + Metrics: metricsserver.Options{BindAddress: "0"}, + HealthProbeBindAddress: "0", + } +} + +// Both server binaries exit when NewManager fails, and registering the field +// indexes asks the API server for each indexed kind's REST mapping before the +// manager starts. An API server that is unreachable for a moment while the pod +// starts has to be waited for, not turned into a crash loop. +func TestNewManagerWaitsForAnAPIServerThatComesUp(t *testing.T) { + if fixture == nil { + t.Skip("envtest not available") + } + + t.Parallel() + + served, err := url.Parse(fixture.env.Config.Host) + if err != nil || served.Host == "" { + t.Fatalf("parse the envtest API server address %q: %v", fixture.env.Config.Host, err) + } + + late := rest.CopyConfig(fixture.env.Config) + late.Host = "https://" + apiServerAfter(t, served.Host, 2*time.Second) + + mgr, err := k8s.NewIndexedManagerWithin(late, managerOptions(), 30*time.Second) + if err != nil { + t.Fatalf("NewManager against an API server that came up 2s later: %v", err) + } + + if mgr == nil { + t.Fatal("NewManager returned no error and no manager") + } +} + +// The wait is bounded: an API server that never comes back is reported, so the +// pod is restarted by its own exit rather than by a liveness probe it cannot +// answer while it is still constructing. +func TestNewManagerGivesUpWhenTheBudgetIsSpent(t *testing.T) { + if fixture == nil { + t.Skip("envtest not available") + } + + t.Parallel() + + gone := rest.CopyConfig(fixture.env.Config) + gone.Host = "https://127.0.0.1:1" + + started := time.Now() + + _, err := k8s.NewIndexedManagerWithin(gone, managerOptions(), time.Second) + if err == nil { + t.Fatal("NewManager against an API server that never answers returned no error") + } + + if elapsed := time.Since(started); elapsed > 15*time.Second { + t.Errorf("NewManager took %s to give up on a 1s budget", elapsed) + } + + var netErr net.Error + if !errors.As(err, &netErr) && !strings.Contains(err.Error(), "connection refused") { + t.Errorf("the error does not say the API server was unreachable: %v", err) + } +} + +// The cache carries no index for spec.nodeName, because the node-scoped reads +// never reach it: a manager-built store answers them from the API reader. A +// NewManager that handed back a store without that reader would not fail those +// reads, it would answer each of them by listing every object in the cache. +func TestNewManagerStoreReadsNodesPastTheCache(t *testing.T) { + if fixture == nil { + t.Skip("envtest not available") + } + + t.Parallel() + + _, st, err := k8s.NewManager(fixture.env.Config, managerOptions()) + if err != nil { + t.Fatalf("NewManager: %v", err) + } + + if !k8s.NodeScopedReadsBypassTheCache(st) { + t.Error("the store NewManager returned answers node-scoped reads from the cache, " + + "which has no index for them") + } +} diff --git a/pkg/store/k8s/manager_export_test.go b/pkg/store/k8s/manager_export_test.go new file mode 100644 index 00000000..e0be0302 --- /dev/null +++ b/pkg/store/k8s/manager_export_test.go @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s + +import ( + "time" + + "k8s.io/client-go/rest" + ctrl "sigs.k8s.io/controller-runtime" +) + +// NewIndexedManagerWithin is NewManager's manager half, with the registration +// budget the test picks. +func NewIndexedManagerWithin(cfg *rest.Config, opts ctrl.Options, budget time.Duration) (ctrl.Manager, error) { + return newIndexedManager(cfg, opts, budget) +} + +// NodeScopedReadsBypassTheCache reports whether the store answers node-scoped +// reads from a direct reader rather than from its client. +func NodeScopedReadsBypassTheCache(s *Store) bool { + return s.resources.apiReader != nil && s.storagePools.apiReader != nil +} diff --git a/pkg/store/k8s/manager_store_wiring_test.go b/pkg/store/k8s/manager_store_wiring_test.go new file mode 100644 index 00000000..c61c0345 --- /dev/null +++ b/pkg/store/k8s/manager_store_wiring_test.go @@ -0,0 +1,473 @@ +// SPDX-License-Identifier: Apache-2.0 + +package k8s_test + +import ( + "go/ast" + "go/parser" + "go/token" + "io/fs" + "os" + "path" + "path/filepath" + "sort" + "strconv" + "strings" + "testing" + + "github.com/cockroachdb/errors" +) + +// storePackagePath is the import path whose constructors this check guards. +const storePackagePath = "github.com/cozystack/blockstor/pkg/store/k8s" + +// A store built on a manager's cached client without the manager's direct +// reader answers the reads that decide a node's fate from a cache, and says +// nothing while it does. The controller binary served the LINSTOR surface from +// exactly such a store while the apiserver's had the reader. +// +// The structural fix is that NewManager returns the store, built from the +// manager's own client and reader, so a manager-backed binary has one call and +// no second constructor to leave out. What this check stops is the way back. +// +// Outside the store package, production code may call New or NewWithAPIReader +// only at the call sites listed in uncachedStoreConstructions, each of which +// builds over a client with no cache behind it. Any other call fails whatever +// its arguments look like, so a helper that receives the manager's client as a +// parameter is caught at the helper rather than escaping through it. Tests and +// the store package itself are held to the narrower rule: no constructor may be +// handed a manager's client or reader, followed through local variables and +// resolved by import path rather than by the name the package is imported +// under. The walk covers the whole module, skipping only what the Go toolchain +// itself ignores. +func TestManagerBackedStoresComeFromNewManager(t *testing.T) { + t.Parallel() + + // The production call sites outside the store package allowed to build a + // store from a client, keyed by module-relative file and enclosing + // function, with the reason each one is safe. + uncachedStoreConstructions := map[string]string{ + "cmd/blockstor/main.go:openStore": "the native CLI builds a plain client with no informer behind it", + } + + findings, unused, err := storeConstructionFindings(repoRoot(t), uncachedStoreConstructions) + if err != nil { + t.Fatalf("walk the module: %v", err) + } + + for _, where := range findings { + t.Errorf("%s builds a store outside NewManager; take the store NewManager returns, "+ + "or, for a client with no cache behind it, add the call site to "+ + "uncachedStoreConstructions with the reason", where) + } + + for _, site := range unused { + t.Errorf("uncachedStoreConstructions lists %s, which no longer builds a store; "+ + "drop the entry so it cannot sanction a later call under the same name", site) + } +} + +// storeConstructionFindings walks the module under root and returns every +// violation as file:line, plus the allowlist entries no call site used. +func storeConstructionFindings(root string, allowed map[string]string) ([]string, []string, error) { + var findings []string + + used := map[string]bool{} + + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + + if d.IsDir() { + if path != root && goToolchainIgnores(d.Name()) { + return filepath.SkipDir + } + + return nil + } + + if !strings.HasSuffix(path, ".go") { + return nil + } + + rel, _ := filepath.Rel(root, path) + rel = filepath.ToSlash(rel) + + src, rerr := os.ReadFile(path) + if rerr != nil { + return errors.Wrapf(rerr, "read %s", rel) + } + + got, perr := storeConstructionViolations(rel, src, allowed, used) + if perr != nil { + return errors.Wrapf(perr, "analyse %s", rel) + } + + for _, line := range got { + findings = append(findings, rel+":"+strconv.Itoa(line)) + } + + return nil + }) + if err != nil { + return nil, nil, errors.Wrap(err, "walk") + } + + var unused []string + + for site := range allowed { + if !used[site] { + unused = append(unused, site) + } + } + + sort.Strings(unused) + + return findings, unused, nil +} + +// goToolchainIgnores is the set of directories `go build ./...` does not +// descend into: vendor, testdata, and names starting with a dot or an +// underscore. Nothing else is skipped. A name like third_party or bin is +// ordinary to the toolchain, so a construction planted there compiles and has +// to be seen. +func goToolchainIgnores(name string) bool { + return name == "vendor" || name == "testdata" || + strings.HasPrefix(name, ".") || strings.HasPrefix(name, "_") +} + +// The walk skips only what the toolchain skips. A violation planted under +// third_party, which the previous skip list named, compiles and was invisible. +func TestManagerStoreCheckWalksWhatTheToolchainBuilds(t *testing.T) { + t.Parallel() + + root := t.TempDir() + + const violation = `package x +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func f(c any) { _ = storek8s.New(c) } +` + + for _, dir := range []string{"third_party/lib", "bin/tool", "node_modules/pkg", "testdata/fixture", "_scratch", ".work"} { + if err := os.MkdirAll(filepath.Join(root, dir), 0o755); err != nil { + t.Fatalf("mkdir %s: %v", dir, err) + } + + if err := os.WriteFile(filepath.Join(root, dir, "planted.go"), []byte(violation), 0o600); err != nil { + t.Fatalf("plant %s: %v", dir, err) + } + } + + findings, _, err := storeConstructionFindings(root, nil) + if err != nil { + t.Fatalf("walk: %v", err) + } + + sort.Strings(findings) + + want := []string{"bin/tool/planted.go:3", "node_modules/pkg/planted.go:3", "third_party/lib/planted.go:3"} + if strings.Join(findings, " ") != strings.Join(want, " ") { + t.Errorf("findings = %v, want %v: the walk must see every directory the toolchain "+ + "builds and skip only the ones it ignores", findings, want) + } +} + +// The check is only as good as the spellings it recognises, so it is pinned on +// the ones a person actually reaches for. In test code, which is held to the +// manager-derived rule: the direct call, an import alias with nothing +// store-like in it, the client through a local variable, and the reader alone +// are caught, and a CLI-shaped uncached client is not. In production code: a +// helper that takes the client as a parameter is caught, and so is any call +// site not on the allowlist, while the listed one passes. +func TestManagerStoreCheckRecognisesTheSpellings(t *testing.T) { + t.Parallel() + + allowed := map[string]string{"cmd/blockstor/main.go:openStore": "probe"} + + for _, tc := range []struct { + name string + file string + src string + want int + }{ + { + name: "direct", + file: "x/probe_test.go", + src: `package x +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func f(mgr interface{ GetClient() any }) { _ = storek8s.New(mgr.GetClient()) }`, + want: 1, + }, + { + name: "aliasWithoutK8s", + file: "x/probe_test.go", + src: `package x +import persistence "github.com/cozystack/blockstor/pkg/store/k8s" +func f(mgr interface{ GetClient() any }) { _ = persistence.New(mgr.GetClient()) }`, + want: 1, + }, + { + name: "throughALocal", + file: "x/probe_test.go", + src: `package x +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func f(mgr interface{ GetClient() any }) { + c := mgr.GetClient() + cached := c + _ = storek8s.New(cached) +}`, + want: 1, + }, + { + name: "readerOnly", + file: "x/probe_test.go", + src: `package x +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func f(mgr interface{ GetAPIReader() any }, c any) { _ = storek8s.NewWithAPIReader(c, mgr.GetAPIReader()) }`, + want: 1, + }, + { + name: "uncachedClientInATest", + file: "x/probe_test.go", + src: `package x +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func f(c any) { _ = storek8s.New(c) }`, + want: 0, + }, + { + name: "helperTakesTheClient", + file: "cmd/controller/main.go", + src: `package main +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func build(c any) any { return storek8s.New(c) } +func run(mgr interface{ GetClient() any }) { _ = build(mgr.GetClient()) }`, + want: 1, + }, + { + name: "unlistedCallSite", + file: "cmd/blockstor/main.go", + src: `package main +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func openOtherStore(c any) any { return storek8s.New(c) }`, + want: 1, + }, + { + name: "listedCallSite", + file: "cmd/blockstor/main.go", + src: `package main +import storek8s "github.com/cozystack/blockstor/pkg/store/k8s" +func openStore(c any) any { return storek8s.New(c) }`, + want: 0, + }, + } { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + got, err := storeConstructionViolations(tc.file, []byte(tc.src), allowed, map[string]bool{}) + if err != nil { + t.Fatalf("analyse: %v", err) + } + + if len(got) != tc.want { + t.Errorf("found %d violation(s), want %d", len(got), tc.want) + } + }) + } +} + +// storeConstructionViolations returns the lines of rel (a module-relative, +// slash-separated path) where a store is built against the rules above, and +// records in used the allowlist entries it matched. +func storeConstructionViolations( + rel string, src []byte, allowed map[string]string, used map[string]bool, +) ([]int, error) { + fset := token.NewFileSet() + + file, err := parser.ParseFile(fset, rel, src, parser.SkipObjectResolution) + if err != nil { + return nil, errors.Wrap(err, "parse") + } + + // The one sanctioned construction: NewManager builds the store from its own + // manager inside the store package. + inStorePackage := file.Name.Name == "k8s" && path.Dir(rel) == "pkg/store/k8s" + + // Production code outside the store package may build a store only at a + // listed call site. Tests and the store package keep the narrower rule. + allowlistRule := !inStorePackage && !strings.HasSuffix(rel, "_test.go") + + storeLocal := "" + + for _, imp := range file.Imports { + importPath, _ := strconv.Unquote(imp.Path.Value) + if importPath != storePackagePath { + continue + } + + storeLocal = "k8s" + if imp.Name != nil { + storeLocal = imp.Name.Name + } + } + + if storeLocal == "" && !inStorePackage { + return nil, nil + } + + var lines []int + + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Body == nil { + continue + } + + if inStorePackage && fn.Name.Name == "NewManager" { + continue + } + + site := rel + ":" + funcDeclName(fn) + derived := managerDerivedLocals(fn.Body) + + ast.Inspect(fn.Body, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok || !isStoreConstructor(call, storeLocal, inStorePackage) { + return true + } + + if allowlistRule { + if _, listed := allowed[site]; listed { + used[site] = true + } else { + lines = append(lines, fset.Position(call.Pos()).Line) + } + + return true + } + + for _, arg := range call.Args { + if takesFromManager(arg, derived) { + lines = append(lines, fset.Position(call.Pos()).Line) + + break + } + } + + return true + }) + } + + return lines, nil +} + +// funcDeclName names a function the way the allowlist keys it: Name for a +// function, Type.Name for a method. +func funcDeclName(fn *ast.FuncDecl) string { + if fn.Recv == nil || len(fn.Recv.List) == 0 { + return fn.Name.Name + } + + recv := fn.Recv.List[0].Type + if star, ok := recv.(*ast.StarExpr); ok { + recv = star.X + } + + if ident, ok := recv.(*ast.Ident); ok { + return ident.Name + "." + fn.Name.Name + } + + return fn.Name.Name +} + +func isStoreConstructor(call *ast.CallExpr, storeLocal string, inStorePackage bool) bool { + var name string + + switch fun := call.Fun.(type) { + case *ast.SelectorExpr: + pkg, ok := fun.X.(*ast.Ident) + if !ok || storeLocal == "" || pkg.Name != storeLocal { + return false + } + + name = fun.Sel.Name + case *ast.Ident: + if !inStorePackage { + return false + } + + name = fun.Name + default: + return false + } + + return name == "New" || name == "NewWithAPIReader" +} + +// managerDerivedLocals is every local that holds, directly or through another +// local, a value a manager handed out. +func managerDerivedLocals(body *ast.BlockStmt) map[string]bool { + derived := map[string]bool{} + + for changed := true; changed; { + changed = false + + ast.Inspect(body, func(n ast.Node) bool { + assign, ok := n.(*ast.AssignStmt) + if !ok || len(assign.Lhs) != len(assign.Rhs) { + return true + } + + for i, lhs := range assign.Lhs { + ident, ok := lhs.(*ast.Ident) + if !ok || derived[ident.Name] { + continue + } + + if takesFromManager(assign.Rhs[i], derived) { + derived[ident.Name] = true + changed = true + } + } + + return true + }) + } + + return derived +} + +// takesFromManager reports whether an expression is, or contains, a manager's +// client or reader. +func takesFromManager(expr ast.Expr, derived map[string]bool) bool { + found := false + + ast.Inspect(expr, func(n ast.Node) bool { + switch node := n.(type) { + case *ast.SelectorExpr: + if node.Sel.Name == "GetClient" || node.Sel.Name == "GetAPIReader" { + found = true + } + case *ast.Ident: + if derived[node.Name] { + found = true + } + } + + return !found + }) + + return found +} + +func repoRoot(t *testing.T) string { + t.Helper() + + // The package lives at pkg/store/k8s, so the module root is three up. + root, err := filepath.Abs(filepath.Join("..", "..", "..")) + if err != nil { + t.Fatalf("resolve the module root: %v", err) + } + + return root +} diff --git a/pkg/store/k8s/nodes.go b/pkg/store/k8s/nodes.go index 5d14f022..b90527d4 100644 --- a/pkg/store/k8s/nodes.go +++ b/pkg/store/k8s/nodes.go @@ -77,6 +77,11 @@ func patchRetryBackoff() wait.Backoff { // nodes implements store.NodeStore against the Node CRD. type nodes struct { c ctrlclient.Client + + // apiReader is the manager's direct reader, when the store was built + // with one. Only GetUncached uses it — every other read here is on a + // path that either polls for convergence or does not act on the answer. + apiReader ctrlclient.Reader } // List returns all Node CRDs as wire-shape apiv1.Node values, sorted by name. @@ -100,18 +105,18 @@ func (n *nodes) List(ctx context.Context) ([]apiv1.Node, error) { // Get returns the named Node CRD as an apiv1.Node, or ErrNotFound. func (n *nodes) Get(ctx context.Context, name string) (apiv1.Node, error) { - var crd crdv1alpha1.Node - - err := n.c.Get(ctx, types.NamespacedName{Name: Name(name)}, &crd) - if err != nil { - if apierrors.IsNotFound(err) { - return apiv1.Node{}, errors.Wrapf(store.ErrNotFound, "node %q", name) - } + return n.get(ctx, n.c, name) +} - return apiv1.Node{}, errors.Wrapf(err, "get Node %q", name) +// GetUncached reads the node from the API server when the store has a direct +// reader, so a decision taken on ConnectionStatus is not taken on a cached +// value the satellite has already moved past. See store.NodeStore. +func (n *nodes) GetUncached(ctx context.Context, name string) (apiv1.Node, error) { + if n.apiReader != nil { + return n.get(ctx, n.apiReader, name) } - return crdToWireNode(&crd), nil + return n.get(ctx, n.c, name) } // Create persists a new Node CRD from an apiv1.Node value. @@ -391,6 +396,21 @@ func (n *nodes) Delete(ctx context.Context, name string) error { return nil } +func (n *nodes) get(ctx context.Context, reader ctrlclient.Reader, name string) (apiv1.Node, error) { + var crd crdv1alpha1.Node + + err := reader.Get(ctx, types.NamespacedName{Name: Name(name)}, &crd) + if err != nil { + if apierrors.IsNotFound(err) { + return apiv1.Node{}, errors.Wrapf(store.ErrNotFound, "node %q", name) + } + + return apiv1.Node{}, errors.Wrapf(err, "get Node %q", name) + } + + return crdToWireNode(&crd), nil +} + // crdToWireNode flattens a Node CRD into the LINSTOR REST shape. // Phase 10.3: re-emits typed `Spec.SatelliteEndpoint` back into the // wire `Props["SatelliteEndpoint"]` so golinstor + the dispatcher's diff --git a/pkg/store/k8s/resources.go b/pkg/store/k8s/resources.go index 13e1402b..89ea4fa0 100644 --- a/pkg/store/k8s/resources.go +++ b/pkg/store/k8s/resources.go @@ -29,6 +29,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" @@ -42,6 +43,11 @@ const ( type resources struct { c ctrlclient.Client + + // apiReader is the manager's direct, uncached reader. ListByNode uses + // it when it is there; see nodeScopedReader for why that read in + // particular cannot come from a cache. + apiReader ctrlclient.Reader } func resourceCRDName(rd, node string) string { @@ -72,46 +78,59 @@ func (s *resources) List(ctx context.Context) ([]apiv1.Resource, error) { return out, nil } -func (s *resources) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Resource, error) { - // Scan-and-filter on the authoritative Spec.ResourceDefinitionName. - // - // Bug 038: an earlier "fast path" trusted a label-selector - // (`LabelResourceDefinition`) and only fell back to a full scan - // when the selector returned ZERO items, on the assumption that a - // partial-but-correct subset was impossible because "every REST - // writer sets the label". That assumption breaks for MIXED RDs: a - // source RD whose diskful replicas were applied via `kubectl apply` - // (e2e fixtures, operator-authored manifests — NO label) but whose - // auto-tiebreaker witness was stamped by the controller (WITH the - // label). The selector then returned only the labeled witness, the - // fallback was skipped, and the unlabeled diskful replicas became - // invisible. The snapshot-restore / clone handler reads this list - // to resolve the SOURCE pool (storPoolsByNodeFromSourceRD); with - // the diskful replicas hidden it stamped the clone replicas with an - // EMPTY StorPoolName and the satellite failed every reconcile with - // `unknown storage pool ""` (clone.sh never converges). - // - // The List below is served from the controller-runtime informer - // cache (no apiserver round-trip), so filtering on the spec field - // in-memory costs the same as a cache-side label index but is - // correct for labeled and unlabeled replicas alike. - var crdList crdv1alpha1.ResourceList - - err := s.c.List(ctx, &crdList) +// ListByNode asks the API server for the node's replicas instead of pulling +// the whole cluster back and filtering here. +// +// The selector is on the spec.nodeName selectable field the CRD declares. That +// is deliberately not the label the objects usually carry: a replica applied +// by hand has no label, and a selector over it would return a +// partial-but-correct subset — the Bug 038 shape, where the missing replicas +// were invisible rather than an error. +func (s *resources) ListByNode(ctx context.Context, node string) ([]apiv1.Resource, error) { + out, err := s.listScoped(ctx, s.nodeScopedReader(), FieldResourceNodeName, node, + func(r *crdv1alpha1.Resource) bool { return r.Spec.NodeName == node }) if err != nil { - return nil, errors.Wrapf(err, "list Resource CRDs for RD %q", rdName) + return nil, err } - out := make([]apiv1.Resource, 0, len(crdList.Items)) + // Every replica here is on the same node, so the definition is what + // distinguishes them. + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) - for i := range crdList.Items { - if crdList.Items[i].Spec.ResourceDefinitionName != rdName { - continue - } + return out, nil +} - out = append(out, crdToWireResource(&crdList.Items[i])) +func (s *resources) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Resource, error) { + // Scoped on the authoritative Spec.ResourceDefinitionName, never on a + // label. + // + // Bug 038: an earlier "fast path" trusted a label-selector + // (`LabelResourceDefinition`) and only fell back to a full scan when the + // selector returned ZERO items, on the assumption that a + // partial-but-correct subset was impossible because "every REST writer + // sets the label". That assumption breaks for MIXED RDs: a source RD + // whose diskful replicas were applied via `kubectl apply` (e2e fixtures, + // operator-authored manifests — NO label) but whose auto-tiebreaker + // witness was stamped by the controller (WITH the label). The selector + // then returned only the labeled witness, the fallback was skipped, and + // the unlabeled diskful replicas became invisible. The snapshot-restore / + // clone handler reads this list to resolve the SOURCE pool + // (storPoolsByNodeFromSourceRD); with the diskful replicas hidden it + // stamped the clone replicas with an EMPTY StorPoolName and the satellite + // failed every reconcile with `unknown storage pool ""` (clone.sh never + // converges). + // + // A selectable FIELD does not have that failure mode: it selects on the + // spec value every replica carries, whoever wrote it, and a server that + // cannot serve the selector says so instead of answering short. + out, err := s.listScoped(ctx, s.c, FieldResourceDefinitionName, rdName, + func(r *crdv1alpha1.Resource) bool { return r.Spec.ResourceDefinitionName == rdName }) + if err != nil { + return nil, err } + // Every replica here belongs to the same definition, so the node is what + // distinguishes them. sort.Slice(out, func(i, j int) bool { return out[i].NodeName < out[j].NodeName }) return out, nil @@ -997,3 +1016,99 @@ func wireToCRDResourceSpec(in *apiv1.Resource) crdv1alpha1.ResourceSpec { ToggleDiskCancel: in.ToggleDiskCancel, } } + +// listScoped answers a scoped question with a scoped read, and falls back to +// the exhaustive one when the server cannot serve the selector. +// +// The same call has two implementations behind it. Against an uncached reader +// it becomes a fieldSelector on the wire and the API server filters; against +// a manager's cached client it is served from the index NewManager registers +// (see fieldIndexes). Either can be missing — a cluster whose CRD predates the +// selectable field REJECTS the query, and a cache with no index for the field +// fails it — and both fail loudly rather than answering partially, which is +// what makes falling back to the exhaustive read safe rather than a silent +// downgrade to a wrong answer. +// +// The fallback is logged because it is not free: it is the whole-cluster read +// the scoped one exists to avoid, and an operator wondering why a large +// cluster crawls deserves to find out from the logs rather than from a +// profiler. +func (s *resources) listScoped( + ctx context.Context, reader ctrlclient.Reader, field, value string, + keep func(*crdv1alpha1.Resource) bool, +) ([]apiv1.Resource, error) { + var crdList crdv1alpha1.ResourceList + + err := reader.List(ctx, &crdList, ctrlclient.MatchingFields{field: value}) + if err == nil { + out := make([]apiv1.Resource, 0, len(crdList.Items)) + for i := range crdList.Items { + out = append(out, crdToWireResource(&crdList.Items[i])) + } + + return out, nil + } + + if !SelectorUnsupported(err) { + return nil, errors.Wrapf(err, "list Resource CRDs for %s=%q", field, value) + } + + log.FromContext(ctx).V(1).Info("scoped Resource read unavailable; reading every replica instead", + "field", field, "value", value, "reason", err.Error()) + + return s.listExhaustively(ctx, reader, field, value, keep) +} + +// listExhaustively is the pre-selectable-field read: every Resource, filtered +// here. It is what listScoped falls back to, and it stays correct whatever the +// server can or cannot select on. +func (s *resources) listExhaustively( + ctx context.Context, reader ctrlclient.Reader, field, value string, + keep func(*crdv1alpha1.Resource) bool, +) ([]apiv1.Resource, error) { + var crdList crdv1alpha1.ResourceList + + err := reader.List(ctx, &crdList) + if err != nil { + return nil, errors.Wrapf(err, "list Resource CRDs for %s=%q", field, value) + } + + out := make([]apiv1.Resource, 0, len(crdList.Items)) + + for i := range crdList.Items { + if !keep(&crdList.Items[i]) { + continue + } + + out = append(out, crdToWireResource(&crdList.Items[i])) + } + + return out, nil +} + +// nodeScopedReader answers the reads a node's fate is decided on. +// +// `node delete` refuses while anything still references the node, and +// `--force` cascades away what does. Both decisions are made on one read and +// then acted on destructively, so a cached answer that trails the API server +// by a beat is not a slow answer, it is a wrong one in both directions: a +// replica the read misses is a node deleted out from under it, and a pool the +// read misses is left pointing at a node that no longer exists. +// +// There is no cache-retry poll behind these the way there is on the REST +// create paths (get*WithCacheRetry), because there is nothing to converge +// towards — the caller is about to delete, not to read again. +// +// This is not the uncached-Get fallback NewWithAPIReader warns against. That +// warning is about raw Gets, where a fast cached NotFound is the contract and +// a store-level bypass short-circuits the REST layer's convergence wait. This +// is one List on two operator commands that run once per dead node, and the +// field selector still travels: an uncached client sends it to the API server, +// which answers it from the selectable field the CRD declares. +func (s *resources) nodeScopedReader() ctrlclient.Reader { + if s.apiReader != nil { + return s.apiReader + } + + return s.c +} diff --git a/pkg/store/k8s/snapshots.go b/pkg/store/k8s/snapshots.go index e047600a..5a040ba3 100644 --- a/pkg/store/k8s/snapshots.go +++ b/pkg/store/k8s/snapshots.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" @@ -47,6 +48,10 @@ const LabelSnapshotGroupID = "blockstor.io/snapshot-group-id" type snapshots struct { c ctrlclient.Client + + // apiReader is the manager's direct reader when the store has one. Only + // ListByDefinitionUncached reads through it. + apiReader ctrlclient.Reader } func snapshotCRDName(rdName, snapName string) string { @@ -82,25 +87,68 @@ func (s *snapshots) List(ctx context.Context) ([]apiv1.Snapshot, error) { return out, nil } +// ListByDefinition returns the snapshots of one definition, narrowed on the +// spec.resourceDefinitionName selectable field. +// +// It used to select on the definition LABEL, and a label is written by +// whoever created the object: pkg/linstormigrate builds adopted Snapshots +// from a LINSTOR dump with none. This list is what `rd d` is refused on and +// what sweeps the leftovers behind it, so on an adopted cluster a snapshot the +// selector could not see was a definition deleted with snapshots still on it, +// and the mop-up missing them too. Same label blindness the Resource and +// StoragePool reads were moved off, one kind over and on a delete gate. func (s *snapshots) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { var crdList crdv1alpha1.SnapshotList - err := s.c.List(ctx, &crdList, - ctrlclient.MatchingLabels{LabelResourceDefinition: rdName}) + err := s.c.List(ctx, &crdList, ctrlclient.MatchingFields{FieldSnapshotDefinitionName: rdName}) if err != nil { - return nil, errors.Wrapf(err, "list Snapshot CRDs for RD %q", rdName) + if !SelectorUnsupported(err) { + return nil, errors.Wrapf(err, "list Snapshot CRDs for RD %q", rdName) + } + + log.FromContext(ctx).V(1).Info("scoped Snapshot read unavailable; reading every snapshot instead", + "resourceDefinition", rdName, "reason", err.Error()) + + return s.listByDefinitionExhaustively(ctx, s.c, rdName) } - parent, _ := s.getParentRD(ctx, rdName) + return s.wireSnapshots(ctx, rdName, crdList.Items) +} - out := make([]apiv1.Snapshot, 0, len(crdList.Items)) - for i := range crdList.Items { - out = append(out, crdToWireSnapshot(&crdList.Items[i], parent)) +// ListByDefinitionUncached reads the definition's snapshots from the API server +// when a direct reader is wired, and through the cache otherwise. The field +// selector travels either way: the API server answers it from the selectable +// field the CRD declares. +// +// A server that refuses the selector, which is what a cluster whose CRD +// predates the selectable field does, is answered by the exhaustive read on +// the same direct reader. Handing it to ListByDefinition instead put the read +// back on the cache on exactly the cluster the fallback exists for, where the +// index the binaries register serves it from the informer: a snapshot that +// raced the delete and had not reached the informer was invisible to the `rd +// d` refusal and to the sweep, and the definition went over it. The Resource +// and StoragePool reads pass their reader into the exhaustive path the same +// way. +func (s *snapshots) ListByDefinitionUncached(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) { + if s.apiReader == nil { + return s.ListByDefinition(ctx, rdName) } - sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + var crdList crdv1alpha1.SnapshotList - return out, nil + err := s.apiReader.List(ctx, &crdList, ctrlclient.MatchingFields{FieldSnapshotDefinitionName: rdName}) + if err != nil { + if !SelectorUnsupported(err) { + return nil, errors.Wrapf(err, "list Snapshot CRDs for RD %q", rdName) + } + + log.FromContext(ctx).V(1).Info("scoped uncached Snapshot read unavailable; reading every snapshot instead", + "resourceDefinition", rdName, "reason", err.Error()) + + return s.listByDefinitionExhaustively(ctx, s.apiReader, rdName) + } + + return s.wireSnapshots(ctx, rdName, crdList.Items) } func (s *snapshots) Get(ctx context.Context, rdName, snapName string) (apiv1.Snapshot, error) { @@ -476,3 +524,54 @@ func wireToCRDSnapshotSpec(in *apiv1.Snapshot) crdv1alpha1.SnapshotSpec { return spec } + +// listByDefinitionExhaustively filters every snapshot here, on the +// authoritative Spec.ResourceDefinitionName, read through the reader the +// scoped attempt used so a fallback never changes which one answers. +func (s *snapshots) listByDefinitionExhaustively( + ctx context.Context, reader ctrlclient.Reader, rdName string, +) ([]apiv1.Snapshot, error) { + var crdList crdv1alpha1.SnapshotList + + err := reader.List(ctx, &crdList) + if err != nil { + return nil, errors.Wrapf(err, "list Snapshot CRDs for RD %q", rdName) + } + + kept := make([]crdv1alpha1.Snapshot, 0, len(crdList.Items)) + + for i := range crdList.Items { + if crdList.Items[i].Spec.ResourceDefinitionName == rdName { + kept = append(kept, crdList.Items[i]) + } + } + + return s.wireSnapshots(ctx, rdName, kept) +} + +// wireSnapshots converts a definition's snapshots, reading the parent once. +// +// The parent read's error is propagated rather than dropped. A snapshot whose +// definition is GONE is not an error — getParentRD answers (nil, nil) for both +// the missing name and the NotFound, because an orphan snapshot is a real +// shape and must still list. What reaches here is a read that actually failed, +// and swallowing it returned a successful listing with +// ResourceDefinitionProps silently absent from every row: the caller cannot +// tell "this definition has no props" from "nobody could read them". +func (s *snapshots) wireSnapshots( + ctx context.Context, rdName string, items []crdv1alpha1.Snapshot, +) ([]apiv1.Snapshot, error) { + parent, err := s.getParentRD(ctx, rdName) + if err != nil { + return nil, err + } + + out := make([]apiv1.Snapshot, 0, len(items)) + for i := range items { + out = append(out, crdToWireSnapshot(&items[i], parent)) + } + + sort.Slice(out, func(i, j int) bool { return out[i].Name < out[j].Name }) + + return out, nil +} diff --git a/pkg/store/k8s/storage_pools.go b/pkg/store/k8s/storage_pools.go index 409cb4fc..1a02de16 100644 --- a/pkg/store/k8s/storage_pools.go +++ b/pkg/store/k8s/storage_pools.go @@ -28,6 +28,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/retry" ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/log" crdv1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" apiv1 "github.com/cozystack/blockstor/pkg/api/v1" @@ -44,6 +45,12 @@ const ( // storagePools implements store.StoragePoolStore against the StoragePool CRD. type storagePools struct { c ctrlclient.Client + + // apiReader is the manager's direct, uncached reader. The node-scoped + // listing uses it for the reason its Resource sibling does: `node + // delete` is refused on this answer and cascades away what it names, so + // a pool the read misses is one left pointing at a node that is gone. + apiReader ctrlclient.Reader } // crdName encodes the (pool, node) composite key into a single CRD name. @@ -94,15 +101,34 @@ func (s *storagePools) List(ctx context.Context) ([]apiv1.StoragePool, error) { return out, nil } -// ListByNode returns pools on the named node. We use a label selector so -// k8s narrows the list server-side rather than us filtering after the fact. +// ListByNode returns pools on the named node, narrowed server-side on the +// spec.nodeName selectable field. +// +// It used to select on the node LABEL, and a label is written by whoever +// created the object. Piraeus and operators create pools with `kubectl apply` +// and no label, so the selector answered with a partial-but-correct subset — +// the Bug 038 shape the Resource store was already moved off labels for, and +// worse here: this list is what `node delete` refuses on and what the cascade +// removes, so a pool the selector could not see was a node deleted with pools +// still registered against it, and a pool left behind referencing a node that +// no longer exists. +// +// The fallback is the exhaustive read, for a cluster whose CRD predates the +// field; see listScoped on the Resource store for why a rejected selector is +// safe to fall back from and a wrong one would not be. func (s *storagePools) ListByNode(ctx context.Context, node string) ([]apiv1.StoragePool, error) { var crdList crdv1alpha1.StoragePoolList - err := s.c.List(ctx, &crdList, - ctrlclient.MatchingLabels{LabelNodeName: node}) + err := s.nodeScopedReader().List(ctx, &crdList, ctrlclient.MatchingFields{FieldStoragePoolNodeName: node}) if err != nil { - return nil, errors.Wrapf(err, "list StoragePool CRDs on node %q", node) + if !SelectorUnsupported(err) { + return nil, errors.Wrapf(err, "list StoragePool CRDs on node %q", node) + } + + log.FromContext(ctx).V(1).Info("scoped StoragePool read unavailable; reading every pool instead", + "node", node, "reason", err.Error()) + + return s.listByNodeExhaustively(ctx, node) } out := make([]apiv1.StoragePool, 0, len(crdList.Items)) @@ -500,3 +526,39 @@ func wireToCRDStoragePoolSpec(in *apiv1.StoragePool) crdv1alpha1.StoragePoolSpec Props: in.Props, } } + +// listByNodeExhaustively filters every pool here, on the authoritative +// Spec.NodeName. +func (s *storagePools) listByNodeExhaustively(ctx context.Context, node string) ([]apiv1.StoragePool, error) { + var crdList crdv1alpha1.StoragePoolList + + err := s.nodeScopedReader().List(ctx, &crdList) + if err != nil { + return nil, errors.Wrapf(err, "list StoragePool CRDs on node %q", node) + } + + out := make([]apiv1.StoragePool, 0, len(crdList.Items)) + + for i := range crdList.Items { + if crdList.Items[i].Spec.NodeName != node { + continue + } + + out = append(out, crdToWireStoragePool(&crdList.Items[i])) + } + + sort.Slice(out, func(i, j int) bool { + return out[i].StoragePoolName < out[j].StoragePoolName + }) + + return out, nil +} + +// nodeScopedReader mirrors the Resource store's; see the comment there. +func (s *storagePools) nodeScopedReader() ctrlclient.Reader { + if s.apiReader != nil { + return s.apiReader + } + + return s.c +} diff --git a/pkg/store/k8s/volume_definitions.go b/pkg/store/k8s/volume_definitions.go index c1aa40aa..8fed3eb6 100644 --- a/pkg/store/k8s/volume_definitions.go +++ b/pkg/store/k8s/volume_definitions.go @@ -70,6 +70,39 @@ func (s *volumeDefinitions) List(ctx context.Context, rdName string) ([]apiv1.Vo return out, nil } +// ListAll reads every definition's inline volumes from one list of the +// ResourceDefinition CRDs, so the request count does not grow with the number +// of definitions. +// +// Keyed by store.FoldName of the definition's LINSTOR name: the caller holds +// whatever spelling its own objects carry, which for a replica is +// Spec.ResourceDefinitionName and need not match the definition's own. +func (s *volumeDefinitions) ListAll(ctx context.Context) (map[string][]apiv1.VolumeDefinition, error) { + var crdList crdv1alpha1.ResourceDefinitionList + + err := s.c.List(ctx, &crdList) + if err != nil { + return nil, errors.Wrap(err, "list ResourceDefinition CRDs") + } + + out := make(map[string][]apiv1.VolumeDefinition, len(crdList.Items)) + + for i := range crdList.Items { + rd := &crdList.Items[i] + + vds := make([]apiv1.VolumeDefinition, 0, len(rd.Spec.VolumeDefinitions)) + for j := range rd.Spec.VolumeDefinitions { + vds = append(vds, crdToWireVD(&rd.Spec.VolumeDefinitions[j])) + } + + sort.Slice(vds, func(a, b int) bool { return vds[a].VolumeNumber < vds[b].VolumeNumber }) + + out[store.FoldName(OriginalName(&rd.ObjectMeta))] = vds + } + + return out, nil +} + func (s *volumeDefinitions) Get(ctx context.Context, rdName string, volumeNumber int32) (apiv1.VolumeDefinition, error) { rd, err := s.fetchRD(ctx, rdName) if err == nil { diff --git a/pkg/store/store.go b/pkg/store/store.go index 7b5bde93..8bbe196d 100644 --- a/pkg/store/store.go +++ b/pkg/store/store.go @@ -26,6 +26,7 @@ package store import ( "context" + "strings" "github.com/cockroachdb/errors" @@ -47,6 +48,32 @@ var ( type NodeStore interface { List(ctx context.Context) ([]apiv1.Node, error) Get(ctx context.Context, name string) (apiv1.Node, error) + + // GetUncached answers the same question as Get, from the API server + // rather than from a cache that may trail it. + // + // It exists for the one caller that cannot take the fast answer: a + // destructive decision made on the node's own status and acted on + // immediately. `n lost` refuses while the satellite still reports + // ONLINE, and unregisters the node plus cascades away its replicas + // when it does not — so a status a beat behind is wrong in both + // directions. A stale ONLINE refuses the cleanup a dead node needs; + // a stale OFFLINE tears down a node whose satellite is answering. + // + // The node-scoped listings behind the same decision already read this + // way (pkg/store/k8s/resources.go nodeScopedReader); this is the field + // the decision turns on, read from the same place. + // + // Where a store has no direct reader this is Get unchanged. That is + // the CLI's shape, whose client is uncached to begin with, and the + // in-memory store's, which has nothing to be behind. It is NOT a + // shape a manager-backed binary may take: a cached client without + // its manager's reader answers this from the cache and says nothing + // while it does, which is what the controller binary did to its own + // `--enable-rest-api` surface. Take the store pkg/store/k8s.NewManager + // returns with the manager; nothing else builds one. + GetUncached(ctx context.Context, name string) (apiv1.Node, error) + Create(ctx context.Context, n *apiv1.Node) error Update(ctx context.Context, n *apiv1.Node) error Delete(ctx context.Context, name string) error @@ -169,9 +196,27 @@ type ResourceDefinitionStore interface { // composite key is (resource_definition_name, node_name). // Update/Patch follow the annotation contract documented on // ResourceGroupStore (nil = untouched, empty = clear). +// Eleven methods rather than ten because the node-scoped listing earns its +// place: without it every `node delete` answers a question about one node by +// listing every Resource in the cluster. +// +//nolint:interfacebloat // one read shape per question the callers actually ask type ResourceStore interface { List(ctx context.Context) ([]apiv1.Resource, error) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Resource, error) + + // ListByNode returns the replicas hosted on one node. + // + // The node-scoped question is asked on every `node delete`, on the + // refusal path as well as under --force, and answering it by listing + // every Resource in the cluster and filtering client-side is what the + // REST refusal did before it. + // + // On the Kubernetes store the filtering happens outside this process: + // the read goes to an uncached reader, the CLI's client or a manager's + // API reader, which sends a fieldSelector the API server answers because + // the CRD declares spec.nodeName selectable. + ListByNode(ctx context.Context, node string) ([]apiv1.Resource, error) Get(ctx context.Context, rdName, node string) (apiv1.Resource, error) Create(ctx context.Context, r *apiv1.Resource) error Update(ctx context.Context, r *apiv1.Resource) error @@ -241,6 +286,43 @@ type ResourceStore interface { PatchResourceSpec(ctx context.Context, rdName, node string, mutate func(*apiv1.Resource) error) error } +// FoldName canonicalises a LINSTOR object name for use as a map key, or for +// comparing two spellings of the same object. LINSTOR identifiers are +// case-insensitive — `DfltRscGrp` and `dfltrscgrp` address one resource group, +// which is why the Kubernetes store lowercases them on the way to a CRD name +// (pkg/store/k8s/crdname.go). Anything that keys objects by name owes its +// callers the same equality the store itself uses. +// +// That equality is available in process and not on the wire. A CRD's +// selectableFields declare a path, not a transform, so the API server compares +// spec.nodeName and spec.resourceDefinitionName verbatim, and the scoped reads +// built on them compare the same way — including their in-process fallback, +// deliberately, because a fallback that folded would answer a different +// question than the selector it stands in for. A replica whose spec spells its +// definition in a case the definition is not stored under is therefore missed +// by both, and `rd d`'s refusal and sweep read it that way too. +// +// The same boundary reaches `node delete`. Its refusal and its `--force` +// cascade are both node-scoped reads on `spec.nodeName`, while Nodes().Get and +// Delete fold: spell the node in a case its replicas were not written with and +// the reads see nothing, the refusal passes, and the node goes with replicas +// still pointing at it. ReplicasOnNode narrows that to one shape by asking in +// the caller's spelling, the folded one and the node's registered one; a +// replica written under any other case is still missed. `resource list` does +// not narrow its read at all for this reason, and filters the whole listing +// with the case-insensitive comparison instead. +// +// Folding on the write side instead would fold what clients read back: +// crdToWireResource reports these spec values as the object's names, and +// crdname.go's annotation exists precisely to keep the stored spelling +// (DfltRscGrp) rather than the lowercased slug, because linstor-csi and +// runbooks compare those strings. Closing the gap properly means a folded +// field beside the display one, selected on and never rendered — a schema +// change with a migration for adopted objects, not a comparison. +func FoldName(name string) string { + return strings.ToLower(name) +} + // VolumeDefinitionStore persists VolumeDefinition objects. The composite // key is (resource_definition_name, volume_number); upstream LINSTOR keeps // VolumeDefinitions inline on the ResourceDefinition, and so do we (the CRD @@ -248,6 +330,29 @@ type ResourceStore interface { // surface; the implementation stitches it onto the RD CRD. type VolumeDefinitionStore interface { List(ctx context.Context, rdName string) ([]apiv1.VolumeDefinition, error) + + // ListAll returns every definition's volumes in ONE request, keyed by + // FoldName of the resource-definition name — look an entry up with + // FoldName(name), not with the name as you hold it. + // + // The fold is not decoration. A replica names its definition in + // whatever case it was written with, and the definition itself is + // stored in whatever case IT was written with; LINSTOR treats the two + // as the same object and a map does not. Keyed raw, a caller holding + // the replica's spelling silently misses the entry and renders a + // definition as though it had no volumes. + // + // List answers for one definition, and a caller that needs the whole + // cluster's volumes has to call it once per name. On the Kubernetes + // store each of those is a GET of one ResourceDefinition — and the CLI + // client is deliberately uncached, so they are real sequential round + // trips. `resource list` did exactly that to fill its + // sync-percentage column: one LIST plus one GET per definition, which + // is the command an operator runs during an incident. + // + // The volumes live inline on the definition, so a single list already + // carries them. + ListAll(ctx context.Context) (map[string][]apiv1.VolumeDefinition, error) Get(ctx context.Context, rdName string, volumeNumber int32) (apiv1.VolumeDefinition, error) Create(ctx context.Context, rdName string, vd *apiv1.VolumeDefinition) error @@ -292,6 +397,16 @@ type VolumeDefinitionStore interface { type SnapshotStore interface { List(ctx context.Context) ([]apiv1.Snapshot, error) ListByDefinition(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) + + // ListByDefinitionUncached answers the same question from the API server + // when the store has a direct reader. It exists for `rd d`: the refusal + // over existing snapshots and the sweep for the one that raced the delete + // are both acted on at once, and the row most likely to be missing from a + // cache at that moment is exactly the one that raced. Every other listing + // stays on ListByDefinition, since the snapshot view paginates through it + // and an uncached read beside a cached one is the pagination regression + // the store's constructor records. + ListByDefinitionUncached(ctx context.Context, rdName string) ([]apiv1.Snapshot, error) Get(ctx context.Context, rdName, snapName string) (apiv1.Snapshot, error) Create(ctx context.Context, snap *apiv1.Snapshot) error Update(ctx context.Context, snap *apiv1.Snapshot) error diff --git a/pkg/store/storetest/storetest.go b/pkg/store/storetest/storetest.go index eacb7ee9..5beedf4a 100644 --- a/pkg/store/storetest/storetest.go +++ b/pkg/store/storetest/storetest.go @@ -23,6 +23,7 @@ limitations under the License. package storetest import ( + "slices" "testing" "github.com/cockroachdb/errors" @@ -208,6 +209,12 @@ func RunVolumeDefinitionStore(t *testing.T, newStore Factory) { t.Errorf("dup: got %v, want ErrAlreadyExists", err) } }) + runVolumeDefinitionListAllCase(t, newStore) + // List resolves the definition the way ListAll keys it. The in-memory + // store compared verbatim while ListAll folded, so a mixed-case lookup + // answered differently depending on which side of the CLI's bulk-read + // cutoff it landed on. + t.Run("ListFoldsTheDefinitionName", func(t *testing.T) { testVolumeDefinitionListFolds(t, newStore) }) // BUG-048: CreateAutoNumbered allocates the smallest free hole and // the allocation is atomic with the write (the REST handler routes // every number-less `linstor vd c` here). @@ -336,6 +343,58 @@ func RunVolumeDefinitionStore(t *testing.T, newStore Factory) { t.Run("UpdateNilArg", func(t *testing.T) { testVDUpdateNilArg(t, newStore) }) } +// runVolumeDefinitionListAllCase pins ListAll's contract. +// +// ListAll answers for the whole cluster in one request, and keys the +// answer folded. A caller holds whatever spelling its own objects +// carry — for a replica that is Spec.ResourceDefinitionName, which +// need not match the definition's own — and LINSTOR treats the two as +// one object where a map does not. Keyed raw, the lookup silently +// misses and the definition renders as though it had no volumes. +func runVolumeDefinitionListAllCase(t *testing.T, newStore Factory) { + t.Helper() + t.Run("ListAllKeysFolded", func(t *testing.T) { + s := newStore(t) + ctx := t.Context() + + seedRD(t, s, "PVC-Mixed") + seedRD(t, s, "pvc-plain") + + for _, rd := range []string{"PVC-Mixed", "pvc-plain"} { + if err := s.VolumeDefinitions().Create(ctx, rd, + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 1024 * 1024}); err != nil { + t.Fatalf("Create under %s: %v", rd, err) + } + } + + all, err := s.VolumeDefinitions().ListAll(ctx) + if err != nil { + t.Fatalf("ListAll: %v", err) + } + + if len(all) != 2 { + t.Errorf("ListAll returned %d definitions, want 2", len(all)) + } + + // The spelling a replica of that definition carries. + vds, ok := all[store.FoldName("pvc-mixed")] + if !ok { + keys := make([]string, 0, len(all)) + for k := range all { + keys = append(keys, k) + } + + slices.Sort(keys) + + t.Fatalf("ListAll keys = %v, want an entry reachable under the folded name", keys) + } + + if len(vds) != 1 || vds[0].SizeKib != 1024*1024 { + t.Errorf("got %+v, want the one volume that was created", vds) + } + }) +} + // runVolumeDefinitionAutoNumberCases pins the BUG-048 atomic-allocate // contract: sequential adds land at 0, 1, 2 … and a hole opened by a // Delete is reused (upstream LINSTOR's smallest-hole rule). Split out @@ -658,37 +717,30 @@ func RunResourceStore(t *testing.T, newStore Factory) { // Bug-021: nil = untouched / non-nil = replace / empty = clear. // See annotation_contract.go. t.Run("UpdateAnnotationContract", func(t *testing.T) { testResourceUpdateAnnotationContract(t, newStore) }) - t.Run("CreateDuplicate", func(t *testing.T) { - s := newStore(t).Resources() - ctx := t.Context() - r := apiv1.Resource{Name: "pvc-1", NodeName: "n1"} - if err := s.Create(ctx, &r); err != nil { - t.Fatalf("first: %v", err) - } - err := s.Create(ctx, &r) - if !errors.Is(err, store.ErrAlreadyExists) { - t.Errorf("dup: got %v, want ErrAlreadyExists", err) - } + t.Run("CreateDuplicate", func(t *testing.T) { testResourceCreateDuplicate(t, newStore) }) + t.Run("ListByDefinition", func(t *testing.T) { testResourceListByDefinition(t, newStore) }) + // ListByNode is the read `node delete` refuses on and `--force` + // cascades from, and the one this store answers with a field selector + // against the API server and a fallback everywhere else. Both shapes + // have to agree, and the shared suite is the only place that asks them + // the same question — its sibling ListByDefinition has been here since + // the beginning and this one was covered per implementation only. + t.Run("ListByNode", func(t *testing.T) { testResourceListByNode(t, newStore) }) + // The node-delete gate and its --force cascade. Nodes().Get and Delete + // fold the name, so the gate has to find the node's replicas under the + // spelling the operator used, or the refusal passes and the node goes + // with replicas still pointing at it. Pinned on both implementations so + // they cannot drift apart. + t.Run("ReferencesOnNodeUnderAnotherSpelling", func(t *testing.T) { + testReferencesOnNodeUnderAnotherSpelling(t, newStore) }) - t.Run("ListByDefinition", func(t *testing.T) { - s := newStore(t).Resources() - ctx := t.Context() - for _, r := range []apiv1.Resource{ - {Name: "pvc-1", NodeName: "n1"}, - {Name: "pvc-1", NodeName: "n2"}, - {Name: "pvc-2", NodeName: "n1"}, - } { - if err := s.Create(ctx, &r); err != nil { - t.Fatalf("Create %+v: %v", r, err) - } - } - got, err := s.ListByDefinition(ctx, "pvc-1") - if err != nil { - t.Fatalf("ListByDefinition: %v", err) - } - if len(got) != 2 { - t.Errorf("len: got %d, want 2", len(got)) - } + // The other direction: the operator types the canonical lowercase name + // and the node, with its replicas and pools, was registered in upper + // case, which is how adoption from LINSTOR writes it. Asking in the + // typed and the folded spelling is one spelling here, and it found + // nothing. + t.Run("ReferencesOnNodeUnderTheRegisteredSpelling", func(t *testing.T) { + testReferencesOnNodeUnderTheRegisteredSpelling(t, newStore) }) t.Run("DeleteRemoves", func(t *testing.T) { s := newStore(t).Resources() @@ -959,6 +1011,96 @@ func testResourceListSorted(t *testing.T, newStore Factory) { } } +// testResourceListByNode pins what a node-scoped read answers: every replica +// on the node asked for, none from anywhere else, and an empty result rather +// than an error for a node nothing references. +func testResourceListByNode(t *testing.T, newStore Factory) { + t.Helper() + + s := newStore(t).Resources() + ctx := t.Context() + + for _, r := range []apiv1.Resource{ + {Name: "pvc-1", NodeName: "n1"}, + {Name: "pvc-2", NodeName: "n1"}, + {Name: "pvc-3", NodeName: "n2"}, + } { + if err := s.Create(ctx, &r); err != nil { + t.Fatalf("Create %+v: %v", r, err) + } + } + + got, err := s.ListByNode(ctx, "n1") + if err != nil { + t.Fatalf("ListByNode: %v", err) + } + + if len(got) != 2 { + t.Fatalf("len: got %d, want the 2 replicas on n1", len(got)) + } + + for i := range got { + if got[i].NodeName != "n1" { + t.Errorf("ListByNode returned a replica on %q", got[i].NodeName) + } + } + + // A node nothing references answers empty, not an error: `node delete` + // reads this to decide whether to refuse, and an error there is a + // refusal the operator cannot clear. + none, err := s.ListByNode(ctx, "ghost") + if err != nil { + t.Fatalf("ListByNode on an unreferenced node: %v", err) + } + + if len(none) != 0 { + t.Errorf("ListByNode on an unreferenced node returned %d replica(s)", len(none)) + } +} + +func testResourceCreateDuplicate(t *testing.T, newStore Factory) { + t.Helper() + + s := newStore(t).Resources() + ctx := t.Context() + r := apiv1.Resource{Name: "pvc-1", NodeName: "n1"} + + if err := s.Create(ctx, &r); err != nil { + t.Fatalf("first: %v", err) + } + + err := s.Create(ctx, &r) + if !errors.Is(err, store.ErrAlreadyExists) { + t.Errorf("dup: got %v, want ErrAlreadyExists", err) + } +} + +func testResourceListByDefinition(t *testing.T, newStore Factory) { + t.Helper() + + s := newStore(t).Resources() + ctx := t.Context() + + for _, r := range []apiv1.Resource{ + {Name: "pvc-1", NodeName: "n1"}, + {Name: "pvc-1", NodeName: "n2"}, + {Name: "pvc-2", NodeName: "n1"}, + } { + if err := s.Create(ctx, &r); err != nil { + t.Fatalf("Create %+v: %v", r, err) + } + } + + got, err := s.ListByDefinition(ctx, "pvc-1") + if err != nil { + t.Fatalf("ListByDefinition: %v", err) + } + + if len(got) != 2 { + t.Errorf("len: got %d, want 2", len(got)) + } +} + // testResourceDeleteIfTieBreaker pins the Bug 393 conditional-delete // contract. It must behave identically on both the in-memory and the // CRD-backed store: a witness row is reaped, a promoted (diskful) row is @@ -1965,3 +2107,108 @@ func trueBool() *bool { return &v } + +// testVolumeDefinitionListFolds asks for a definition's volumes under a +// spelling it is not stored under, in both directions. The lookup side is +// folded either way, so only a definition stored in mixed case and asked for in +// the canonical spelling tells whether the stored side folds too: that is the +// replica naming a definition `pvc-mixed` whose object was written `PVC-Mixed`. +func testVolumeDefinitionListFolds(t *testing.T, newStore Factory) { + t.Helper() + + for _, tc := range []struct{ stored, asked string }{ + {stored: "PVC-Fold-Stored", asked: "pvc-fold-stored"}, + {stored: "pvc-fold-asked", asked: "PVC-Fold-Asked"}, + } { + s := newStore(t) + ctx := t.Context() + + seedRD(t, s, tc.stored) + + if err := s.VolumeDefinitions().Create(ctx, tc.stored, + &apiv1.VolumeDefinition{VolumeNumber: 0, SizeKib: 1024 * 1024}); err != nil { + t.Fatalf("Create under %q: %v", tc.stored, err) + } + + got, err := s.VolumeDefinitions().List(ctx, tc.asked) + if err != nil { + t.Fatalf("List %q stored as %q: %v", tc.asked, tc.stored, err) + } + + if len(got) != 1 { + t.Errorf("List %q stored as %q returned %d volume(s), want 1", tc.asked, tc.stored, len(got)) + } + } +} + +func testReferencesOnNodeUnderTheRegisteredSpelling(t *testing.T, newStore Factory) { + t.Helper() + + s := newStore(t) + ctx := t.Context() + + if err := s.Nodes().Create(ctx, &apiv1.Node{Name: "NODE-REG", Type: "SATELLITE"}); err != nil { + t.Fatalf("Create node: %v", err) + } + + if err := s.Resources().Create(ctx, &apiv1.Resource{Name: "pvc-reg", NodeName: "NODE-REG"}); err != nil { + t.Fatalf("Create replica: %v", err) + } + + if err := s.StoragePools().Create(ctx, &apiv1.StoragePool{ + StoragePoolName: "pool-reg", NodeName: "NODE-REG", ProviderKind: apiv1.StoragePoolKindFile, + }); err != nil { + t.Fatalf("Create pool: %v", err) + } + + replicas, pools, err := store.ReferencesOnNode(ctx, s, "node-reg") + if err != nil { + t.Fatalf("ReferencesOnNode: %v", err) + } + + if len(replicas) != 1 || len(pools) != 1 { + t.Errorf("ReferencesOnNode typed lowercase found %d replica(s) and %d pool(s), want 1 "+ + "and 1: the refusal would pass and the node would go with both on it", + len(replicas), len(pools)) + } + + if err := store.CascadeOrphansForLostNode(ctx, s, "node-reg"); err != nil { + t.Fatalf("CascadeOrphansForLostNode: %v", err) + } + + left, err := s.Resources().ListByNode(ctx, "NODE-REG") + if err != nil { + t.Fatalf("ListByNode replicas: %v", err) + } + + leftPools, err := s.StoragePools().ListByNode(ctx, "NODE-REG") + if err != nil { + t.Fatalf("ListByNode pools: %v", err) + } + + if len(left) != 0 || len(leftPools) != 0 { + t.Errorf("the lost-node cascade typed lowercase left %d replica(s) and %d pool(s) "+ + "pointing at the node", len(left), len(leftPools)) + } +} + +func testReferencesOnNodeUnderAnotherSpelling(t *testing.T, newStore Factory) { + t.Helper() + + s := newStore(t) + ctx := t.Context() + + if err := s.Resources().Create(ctx, &apiv1.Resource{Name: "pvc-ref", NodeName: "node-ref"}); err != nil { + t.Fatalf("Create replica: %v", err) + } + + replicas, _, err := store.ReferencesOnNode(ctx, s, "NODE-REF") + if err != nil { + t.Fatalf("ReferencesOnNode: %v", err) + } + + if len(replicas) != 1 { + t.Errorf("ReferencesOnNode under another spelling found %d replica(s), want 1 — "+ + "the refusal would pass and the node would go with a replica on it", len(replicas)) + } +} diff --git a/tests/integration/group_a_test.go b/tests/integration/group_a_test.go index f5965d37..c4063c93 100644 --- a/tests/integration/group_a_test.go +++ b/tests/integration/group_a_test.go @@ -374,7 +374,8 @@ func TestGroupANodeLostCascadesOrphans(t *testing.T) { } cli := &harness.CLI{URL: stack.RestURL} - cli.JSON(t, "node", "lost", harness.NodeWorker1) + harness.AssertNotRefused(t, "node lost "+harness.NodeWorker1, + cli.JSON(t, "node", "lost", harness.NodeWorker1)) // worker-1 replica must be gone (handler-driven cascade — not // finalizer-driven, see node_lifecycle.go cascadeOrphansForLostNode). diff --git a/tests/integration/group_e_test.go b/tests/integration/group_e_test.go index aa71b09f..2a666079 100644 --- a/tests/integration/group_e_test.go +++ b/tests/integration/group_e_test.go @@ -643,4 +643,3 @@ func stampOnePool( t.Fatalf("status update StoragePool %s: gave up after %d conflict retries", poolName, maxAttempts) } - diff --git a/tests/integration/harness/fixtures.go b/tests/integration/harness/fixtures.go index 9984ab10..94181aea 100644 --- a/tests/integration/harness/fixtures.go +++ b/tests/integration/harness/fixtures.go @@ -137,9 +137,10 @@ func seedStoragePool(ctx context.Context, t *testing.T, cli client.Client, node, // // The `blockstor.io/node-name` label mirrors what // pkg/store/k8s.(*storagePools).Create stamps on every - // store-created pool: the store's ListByNode runs a label - // selector, so a fixture pool seeded without it is invisible - // to per-node store reads (e.g. the `n lost` SP cascade). + // store-created pool, so the fixture looks like a store write. + // Per-node store reads no longer depend on it: ListByNode + // selects on spec.nodeName, which a pool carries whoever + // wrote it. ObjectMeta: metav1.ObjectMeta{ Name: pool + "." + node, Labels: map[string]string{"blockstor.io/node-name": node}, diff --git a/tests/integration/harness/linstor.go b/tests/integration/harness/linstor.go index f6ffa511..b90ed4da 100644 --- a/tests/integration/harness/linstor.go +++ b/tests/integration/harness/linstor.go @@ -199,3 +199,32 @@ func truncateForLog(buf []byte, limit int) string { return string(buf[:limit]) + "...[truncated]" } + +// AssertNotRefused fails the test when a LINSTOR envelope carries an error, +// naming the message the server actually returned. +// +// The machine-readable CLI exits 0 on refusal envelopes as readily as on +// success, so a caller that ignores the answer cannot tell a performed +// operation from a refused one. The cost is not a missing assertion: the next +// assertion in the test — usually a convergence wait on what the refused +// operation was supposed to do — then times out and blames the wrong thing. +// `node lost` refused for a still-ONLINE satellite spent three CI rounds +// looking like a broken cascade. +// +// LINSTOR marks failure in the ret_code mask's sign bit, so any negative +// ret_code is an error whatever else it carries. +func AssertNotRefused(t *testing.T, what string, envelope []map[string]any) { + t.Helper() + + for _, rc := range envelope { + code, ok := rc["ret_code"].(float64) + if !ok || code >= 0 { + continue + } + + msg, _ := rc["message"].(string) + cause, _ := rc["cause"].(string) + + t.Fatalf("%s was refused, not performed: %s (cause: %s)", what, msg, cause) + } +} diff --git a/tests/integration/harness/manager.go b/tests/integration/harness/manager.go index 93d4deac..cf4ebb47 100644 --- a/tests/integration/harness/manager.go +++ b/tests/integration/harness/manager.go @@ -100,22 +100,15 @@ func StartStack(t *testing.T) *Stack { // `metadata.name == poolName.nodeName`). envtest already // applied them when Start returned. Nothing extra to do. - mgr, err := buildIntegrationManager(env) + mgr, st, err := buildIntegrationManager(env) if err != nil { t.Fatalf("build manager: %v", err) } - // Mirror cmd/apiserver/main.go: the REST-serving store is built - // with the manager's direct (uncached) API reader. CreateAutoNumbered's - // retry loop reads the parent RD through this reader so a conflict- - // retry observes the just-committed VolumeDefinition rather than a - // stale informer-cache revision. Without it (the plain New) the - // cache-lag re-read re-derives the same hole, 409-storms against the - // RD reconciler's concurrent writes, and the slow create makes the - // linstor client re-POST — leaving duplicate auto-numbered VDs - // (BUG-048 de-regress). Production never hit this because the apiserver - // always wires GetAPIReader(); the harness must match. - st := storek8s.NewWithAPIReader(mgr.GetClient(), mgr.GetAPIReader()) + // The REST-serving store comes back from storek8s.NewManager with the + // manager, built on its direct (uncached) API reader, which is what + // CreateAutoNumbered's retry loop reads the parent RD through so a + // conflict-retry observes the just-committed VolumeDefinition (BUG-048). // Wire every reconciler / runnable cmd/controller/main.go // registers. Mirror order exactly so a future split-or-merge @@ -173,7 +166,7 @@ func StartStack(t *testing.T) *Stack { // the test stack drives. LeaderElection off (every replica is // authoritative in tests), metrics disabled (no need for a Prometheus // listener in unit-of-integration tests). -func buildIntegrationManager(env *Env) (manager.Manager, error) { +func buildIntegrationManager(env *Env) (manager.Manager, *storek8s.Store, error) { scheme := clientgoscheme.Scheme utilruntime.Must(blockstoriov1alpha1.AddToScheme(scheme)) @@ -186,7 +179,10 @@ func buildIntegrationManager(env *Env) (manager.Manager, error) { // is the documented escape hatch for test harnesses // (https://pkg.go.dev/sigs.k8s.io/controller-runtime/pkg/config). skipNameValidation := true - mgr, err := ctrl.NewManager(env.Cfg, ctrl.Options{ + // storek8s.NewManager, not ctrl.NewManager: the field indexes the store + // selects on come with it, so this harness cannot drift into exercising + // only the whole-cluster fallback while claiming to mirror the binaries. + mgr, st, err := storek8s.NewManager(env.Cfg, ctrl.Options{ Scheme: scheme, Metrics: metricsserver.Options{BindAddress: "0"}, HealthProbeBindAddress: "0", @@ -196,20 +192,20 @@ func buildIntegrationManager(env *Env) (manager.Manager, error) { }, }) if err != nil { - return nil, fmt.Errorf("new manager: %w", err) + return nil, nil, fmt.Errorf("new manager: %w", err) } err = mgr.AddHealthzCheck("healthz", healthz.Ping) if err != nil { - return nil, fmt.Errorf("add healthz: %w", err) + return nil, nil, fmt.Errorf("add healthz: %w", err) } err = mgr.AddReadyzCheck("readyz", healthz.Ping) if err != nil { - return nil, fmt.Errorf("add readyz: %w", err) + return nil, nil, fmt.Errorf("add readyz: %w", err) } - return mgr, nil + return mgr, st, nil } // wireReconcilers attaches every reconciler / runnable diff --git a/tests/integration/harness/satellite.go b/tests/integration/harness/satellite.go index 3bde33f6..389c4410 100644 --- a/tests/integration/harness/satellite.go +++ b/tests/integration/harness/satellite.go @@ -30,6 +30,7 @@ import ( "sigs.k8s.io/controller-runtime/pkg/client" blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + "github.com/cozystack/blockstor/internal/controller" ) const ( @@ -195,10 +196,23 @@ func (s *Satellite) tickOnce(ctx context.Context) { s.reconcileResources(ctx) } -// reconcileNodes stamps Conditions[Ready]=True and ConnectionStatus -// ONLINE on every Node — the steady-state shape a satellite produces -// after its first heartbeat. Idempotent: only writes when the value -// actually changes. +// reconcileNodes stamps Conditions[Ready] and ConnectionStatus on +// every Node — the steady-state shape a satellite produces after its +// first heartbeat. Idempotent: only writes when the value actually +// changes. +// +// A node under SimulateNodeOffline gets the whole offline shape, not +// just the status field: a stale heartbeat and Ready=Unknown, which is +// what a satellite that stopped reporting leaves behind. Stamping a +// FRESH heartbeat beside an OFFLINE status is a state the real system +// cannot produce, and NodeHeartbeatReconciler decides on the timestamp +// rather than on the status — it would find the beat fresh, write +// ONLINE straight back, and the two writers would then trade the field +// between them. A test that takes a node offline, waits for OFFLINE and +// then acts on it read whichever of the two wrote last: that is what +// made `n lost` refuse against a node the test had just watched go +// offline, with a message naming a satellite nothing was pretending was +// alive. func (s *Satellite) reconcileNodes(ctx context.Context) { var nodes blockstoriov1alpha1.NodeList @@ -209,24 +223,34 @@ func (s *Satellite) reconcileNodes(ctx context.Context) { for i := range nodes.Items { node := &nodes.Items[i] + desiredStatus := blockstoriov1alpha1.NodeConnectionStatusOnline + desiredReady := metav1.ConditionTrue + reason := "SatelliteMockHealthy" + message := "harness/satellite.go stamped Ready" + heartbeat := ptrNow() if s.isNodeOffline(node.Name) { desiredStatus = blockstoriov1alpha1.NodeConnectionStatusOffline + desiredReady = metav1.ConditionUnknown + reason = "SatelliteMockOffline" + message = "harness/satellite.go stopped reporting for this node" + heartbeat = ptrStaleHeartbeat() } - if node.Status.ConnectionStatus == desiredStatus && hasReadyTrue(node.Status.Conditions) { + if node.Status.ConnectionStatus == desiredStatus && + hasReadyStatus(node.Status.Conditions, desiredReady) { continue } patched := node.DeepCopy() patched.Status.ConnectionStatus = desiredStatus - patched.Status.LastHeartbeatTime = ptrNow() + patched.Status.LastHeartbeatTime = heartbeat patched.Status.Conditions = upsertCondition(patched.Status.Conditions, &metav1.Condition{ Type: blockstoriov1alpha1.NodeConditionReady, - Status: metav1.ConditionTrue, - Reason: "SatelliteMockHealthy", - Message: "harness/satellite.go stamped Ready", + Status: desiredReady, + Reason: reason, + Message: message, LastTransitionTime: metav1.Now(), }) @@ -351,12 +375,12 @@ func (s *Satellite) isNodeOffline(node string) bool { return s.nodeOffline[node] } -// hasReadyTrue is a tiny helper kept off the global namespace so +// hasReadyStatus is a tiny helper kept off the global namespace so // internal callers don't accidentally use it as a public assertion. -func hasReadyTrue(conds []metav1.Condition) bool { +func hasReadyStatus(conds []metav1.Condition, want metav1.ConditionStatus) bool { for i := range conds { if conds[i].Type == blockstoriov1alpha1.NodeConditionReady { - return conds[i].Status == metav1.ConditionTrue + return conds[i].Status == want } } @@ -383,6 +407,15 @@ func ptrNow() *metav1.Time { return &now } +// ptrStaleHeartbeat is a heartbeat old enough that the watchdog reads +// the node as unreachable. Comfortably past NodeMonitorGracePeriod, so +// a slow tick or a busy runner cannot land it back inside the window. +func ptrStaleHeartbeat() *metav1.Time { + stale := metav1.NewTime(time.Now().Add(-4 * controller.NodeMonitorGracePeriod)) + + return &stale +} + func providerSupportsSnapshots(kind string) bool { switch strings.ToUpper(kind) { case "ZFS", providerZFSThinUpper, providerLVMThinUpper, "FILE_THIN": diff --git a/tests/integration/harness/satellite_offline_shape_test.go b/tests/integration/harness/satellite_offline_shape_test.go new file mode 100644 index 00000000..9040f093 --- /dev/null +++ b/tests/integration/harness/satellite_offline_shape_test.go @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: Apache-2.0 + +//go:build integration + +package harness + +import ( + "context" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + clientgoscheme "k8s.io/client-go/kubernetes/scheme" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + "github.com/cozystack/blockstor/internal/controller" +) + +// SimulateNodeOffline has to produce a shape NodeHeartbeatReconciler agrees +// with, not just the field a test reads. +// +// The watchdog re-evaluates every Node every NodeMonitorPeriod and decides on +// LastHeartbeatTime, not on ConnectionStatus. Stamping OFFLINE beside a FRESH +// heartbeat is a state no real satellite produces, and it made the two writers +// trade the field: the watchdog read the beat as fresh and wrote ONLINE back, +// the mock wrote OFFLINE again on its next tick, and a test acting on the node +// in between got whichever had gone last. That is what refused `n lost` +// against a node the cascade test had just watched go offline, and it failed +// the way an oscillation does — rarely, on a loaded runner, with a message +// about something else. +// +// Asserted here rather than by holding a live node, because the failure is a +// race: a hold catches it only when it samples inside the window the watchdog +// owns the field, which is neither quick nor reliable. The shape is the cause, +// and it is exact. +func TestSimulateNodeOfflineProducesAShapeTheWatchdogAgreesWith(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("core scheme: %v", err) + } + + if err := blockstoriov1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("blockstor scheme: %v", err) + } + + node := &blockstoriov1alpha1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "worker-1"}, + } + + cli := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(node). + WithStatusSubresource(node). + Build() + + sat := NewSatellite(cli) + sat.SimulateNodeOffline("worker-1") + sat.reconcileNodes(context.Background()) + + var got blockstoriov1alpha1.Node + if err := cli.Get(context.Background(), + types.NamespacedName{Name: "worker-1"}, &got); err != nil { + t.Fatalf("read the node back: %v", err) + } + + if got.Status.ConnectionStatus != blockstoriov1alpha1.NodeConnectionStatusOffline { + t.Errorf("ConnectionStatus = %q, want OFFLINE", got.Status.ConnectionStatus) + } + + // A nil heartbeat is stale by the watchdog's own reading, so it is the one + // other shape that agrees. Anything inside the grace period is the bug. + if got.Status.LastHeartbeatTime != nil { + age := time.Since(got.Status.LastHeartbeatTime.Time) + if age <= controller.NodeMonitorGracePeriod { + t.Errorf("OFFLINE node carries a heartbeat %s old, inside the %s grace "+ + "period: the watchdog reads that as a live satellite and writes "+ + "ONLINE back", age, controller.NodeMonitorGracePeriod) + } + } + + for i := range got.Status.Conditions { + c := got.Status.Conditions[i] + if c.Type == blockstoriov1alpha1.NodeConditionReady && c.Status == metav1.ConditionTrue { + t.Errorf("OFFLINE node carries Ready=True; the watchdog and the mock " + + "disagree about it and will trade the field") + } + } +} + +// The control: a node nobody took offline still gets the healthy shape, so the +// assertions above are about the simulation and not about reconcileNodes +// having stopped writing. +func TestReconcileNodesStillStampsAHealthyNodeOnline(t *testing.T) { + t.Parallel() + + scheme := runtime.NewScheme() + if err := clientgoscheme.AddToScheme(scheme); err != nil { + t.Fatalf("core scheme: %v", err) + } + + if err := blockstoriov1alpha1.AddToScheme(scheme); err != nil { + t.Fatalf("blockstor scheme: %v", err) + } + + node := &blockstoriov1alpha1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "worker-2"}, + } + + cli := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(node). + WithStatusSubresource(node). + Build() + + sat := NewSatellite(cli) + sat.reconcileNodes(context.Background()) + + var got blockstoriov1alpha1.Node + if err := cli.Get(context.Background(), + types.NamespacedName{Name: "worker-2"}, &got); err != nil { + t.Fatalf("read the node back: %v", err) + } + + if got.Status.ConnectionStatus != blockstoriov1alpha1.NodeConnectionStatusOnline { + t.Errorf("ConnectionStatus = %q, want ONLINE", got.Status.ConnectionStatus) + } + + if got.Status.LastHeartbeatTime == nil || + time.Since(got.Status.LastHeartbeatTime.Time) > controller.NodeMonitorGracePeriod { + t.Errorf("healthy node has no fresh heartbeat; the watchdog would flip it OFFLINE") + } +} diff --git a/tests/integration/resource_listbynode_test.go b/tests/integration/resource_listbynode_test.go new file mode 100644 index 00000000..60bd8e2f --- /dev/null +++ b/tests/integration/resource_listbynode_test.go @@ -0,0 +1,182 @@ +//go:build integration + +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "context" + "testing" + "time" + + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/retry" + "sigs.k8s.io/controller-runtime/pkg/client" + + blockstoriov1alpha1 "github.com/cozystack/blockstor/api/v1alpha1" + k8sstore "github.com/cozystack/blockstor/pkg/store/k8s" + "github.com/cozystack/blockstor/tests/integration/harness" +) + +// node delete asks whether one node is still referenced, and it used to answer +// by listing every Resource in the cluster and filtering client-side. The CRD +// declares spec.nodeName as a selectable field so the API server can answer +// it, and that only works if the field is actually declared — a selector on an +// undeclared field is REJECTED, not silently ignored, so this asserts against +// a running API server rather than against the generated YAML. +// +// The loudness is the point. The alternative, a label selector, would return +// a partial-but-correct subset for replicas applied by hand, which is the +// shape that once hid diskful replicas from the clone handler. +func TestResourceNodeFieldSelectorIsServedByTheAPIServer(t *testing.T) { + stack := harness.StartStack(t) + ctx := context.Background() + + seed := []struct{ rd, node string }{ + {"pvc-a", "node-1"}, + {"pvc-b", "node-1"}, + {"pvc-c", "node-2"}, + } + + for _, s := range seed { + res := &blockstoriov1alpha1.Resource{ + ObjectMeta: metav1.ObjectMeta{Name: s.rd + "." + s.node}, + Spec: blockstoriov1alpha1.ResourceSpec{ + ResourceDefinitionName: s.rd, + NodeName: s.node, + }, + } + + if err := stack.Env.Client.Create(ctx, res); err != nil { + t.Fatalf("seed %s on %s: %v", s.rd, s.node, err) + } + } + + var got blockstoriov1alpha1.ResourceList + + err := stack.Env.Client.List(ctx, &got, client.MatchingFields{"spec.nodeName": "node-1"}) + if err != nil { + t.Fatalf("the API server refused a selector on spec.nodeName, so the CRD does "+ + "not declare it and every node-scoped read falls back to listing the "+ + "whole cluster: %v", err) + } + + if len(got.Items) != 2 { + t.Fatalf("selector returned %d replicas, want the 2 on node-1", len(got.Items)) + } + + for i := range got.Items { + if got.Items[i].Spec.NodeName != "node-1" { + t.Errorf("selector returned a replica on %s", got.Items[i].Spec.NodeName) + } + } + + // And through the store, which is what the callers actually use: the + // selector above proves the API server serves it, not that ListByNode + // asks for it. Answering from the fallback would pass the check above + // and still be the whole-cluster read. + replicas, err := k8sstore.New(stack.Env.Client).Resources().ListByNode(ctx, "node-1") + if err != nil { + t.Fatalf("ListByNode: %v", err) + } + + if len(replicas) != 2 { + t.Fatalf("ListByNode returned %d replicas, want the 2 on node-1", len(replicas)) + } + + for i := range replicas { + if replicas[i].NodeName != "node-1" { + t.Errorf("ListByNode returned a replica on %s", replicas[i].NodeName) + } + } +} + +// The store falls back to an exhaustive read when the selector is refused, +// which is what a cluster running an older CRD does. That branch is otherwise +// only reachable on such a cluster, so it is exercised here by taking the +// selectable fields off the live CRD and asking the store the same question. +func TestListByNodeFallsBackWhenTheSelectorIsRefused(t *testing.T) { + stack := harness.StartStack(t) + ctx := context.Background() + + for _, s := range []struct{ rd, node string }{ + {"fb-a", "node-1"}, + {"fb-b", "node-1"}, + {"fb-c", "node-2"}, + } { + res := &blockstoriov1alpha1.Resource{ + ObjectMeta: metav1.ObjectMeta{Name: s.rd + "." + s.node}, + Spec: blockstoriov1alpha1.ResourceSpec{ + ResourceDefinitionName: s.rd, + NodeName: s.node, + }, + } + + if err := stack.Env.Client.Create(ctx, res); err != nil { + t.Fatalf("seed %s on %s: %v", s.rd, s.node, err) + } + } + + stripSelectableFields(t, ctx, stack) + + st := k8sstore.New(stack.Env.Client) + + got, err := st.Resources().ListByNode(ctx, "node-1") + if err != nil { + t.Fatalf("the fallback did not answer: %v", err) + } + + if len(got) != 2 { + t.Fatalf("fallback returned %d replicas, want the 2 on node-1", len(got)) + } + + for i := range got { + if got[i].NodeName != "node-1" { + t.Errorf("fallback returned a replica on %s", got[i].NodeName) + } + } +} + +// stripSelectableFields removes the selectable fields from the live Resource +// CRD and waits until the API server actually refuses a selector on them. +func stripSelectableFields(t *testing.T, ctx context.Context, stack *harness.Stack) { + t.Helper() + + crds := crdClient(t, stack) + + const crdName = "resources.blockstor.cozystack.io" + + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + var crd apiextensionsv1.CustomResourceDefinition + + if err := crds.Get(ctx, types.NamespacedName{Name: crdName}, &crd); err != nil { + return err + } + + for i := range crd.Spec.Versions { + crd.Spec.Versions[i].SelectableFields = nil + } + + return crds.Update(ctx, &crd) + }) + if err != nil { + t.Fatalf("strip the selectable fields: %v", err) + } + + deadline := time.Now().Add(30 * time.Second) + for { + var probe blockstoriov1alpha1.ResourceList + + if stack.Env.Client.List(ctx, &probe, client.MatchingFields{"spec.nodeName": "node-1"}) != nil { + return + } + + if time.Now().After(deadline) { + t.Fatal("the API server never stopped serving the selector") + } + + time.Sleep(200 * time.Millisecond) + } +}