feat(cli): native blockstor CLI speaking Kubernetes directly - #181
feat(cli): native blockstor CLI speaking Kubernetes directly#181Andrei Kvapil (kvaps) wants to merge 47 commits into
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (27)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughAdds the ChangesBlockstor CLI and resource presentation
Estimated code review effort: 5 (Critical) | ~120 minutes Merge Risk: 🟡 Moderate · up to A failed device-pool operation can leave earlier devices attached even though the overall command failed, resulting in partial cluster state that requires cleanup. The rollback behavior should be corrected or explicitly accepted before merging. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 91.13% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 327 functions across 51 files. (2 skipped: 2 unsupported.) ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Groundwork for the native blockstor CLI (docs/cli-design.md). The CRDs carried no additionalPrinterColumns at all, so `kubectl get resources` showed NAME/AGE and nothing an operator could act on. Each kind now prints the fields that matter for triage — node type/address/ status, pool node/provider/capacity, resource definition/node/pool/ node-id/port/state/in-use, and so on — which makes plain kubectl useful on its own and gives the CLI a server-side table path. The set is pinned by a test so it cannot silently drift. internal/cli/color classifies blockstor and DRBD state strings into healthy / transitional / broken / neutral and paints them green / yellow / red. Colour is load-bearing during an incident, so it is kept; an unrecognised state is deliberately neutral rather than green, so a future DRBD state cannot masquerade as healthy. Painting requires an interactive terminal and honours --color, NO_COLOR and TERM=dumb, so piped output stays byte-identical for the shell harnesses that grep it. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The noun/verb grammar and its short aliases as data, so the command tree, the help output and the tests all read one source. A command added without its alias, or an alias that shadows another command, fails a test instead of surprising an operator mid-incident. Resolution is position-aware because the upstream grammar reuses tokens by slot: `sp` is the storage-pool noun in slot 1 and set-property in slot 2, `c` is controller or create, `s` is snapshot or set-size. Nested verbs (`snapshot resource restore` / `s r rst`) resolve longest-match-first, and everything after the command path is handed back verbatim — the upstream grammar allows a flag before or after the positionals, so the per-command parser owns it. Unknown nouns and verbs return ErrUsage, which carries the client-side rejection class this repo's replay workflows assert as exit 2 (an API-level rejection is 10). The surface itself was assembled from real invocations in tests/e2e/cli-matrix, tests/operator-harness, tests/e2e and stand/, and a test asserts every command those harnesses exercise is present — that list is what has to be complete before the upstream client can be dropped. No upstream client source was consulted. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
One renderer for every view. Tables served by the API server from the CRDs' printer columns and tables assembled client-side from store DTOs are both metav1.Table, so layout, padding and colour are decided in exactly one place. The layout is a contract rather than a preference: shell in this repo parses these tables with `awk -F'|'` at fixed indexes, so a row begins with the separator — that leading empty field is what puts Usage on $5 and State on $7 for a resource row. A test asserts those exact positions, so a column reordering fails here instead of silently making a harness read the wrong cell. Colour is applied around the value only, after widths are measured on the plain text. That invariant is tested directly: stripping the escapes from a painted render must reproduce the plain render byte-for-byte, which is what keeps a coloured table aligned and keeps piped output parseable. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The first cross-kind view: a resource row joins the replica, its DRBD layer and its volumes into the seven columns the harnesses read by index. The State cell carries the contracts this repo asserts elsewhere in shell, so each one is now a test: a tie-breaker renders the literal `TieBreaker` (that exact token, case included), a replica under deletion renders `DELETING` whatever its disk says, a converged replica renders a bare `UpToDate` with no percentage, and a syncing one carries its progress computed from the satellite's out-of-sync figure. Two judgement calls worth naming. Usage is tri-state: a satellite that has not reported yet leaves the cell blank rather than claiming the replica is Unused. And `--faulty` treats a replica with no observed disk state as NOT faulty — absence of data is not evidence of breakage, and listing those would bury the real fault an operator ran the command to find. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The CLI now runs end to end: it resolves the command, opens the CRD-backed store from the ambient kubeconfig, renders a table (or the machine-readable envelope) and returns a meaningful exit code. Exit codes mirror the client this replaces because scripts branch on the difference: 0 success, 2 a client-side rejection (unknown command or flag), 10 an API-level failure. Diagnostics go to stderr so a pipeline reading stdout gets clean data. The store client is deliberately NOT cached. A cache would reintroduce the read-your-writes lag the multi-replica apiserver has to retry around, and a CLI process that lists once has nothing to gain from an informer — so this client always sees its own writes. Flag parsing walks the whole argument tail rather than stopping at the first positional: the upstream grammar allows a flag before or after the positionals, and both spellings appear in this repo's scripts. A bare `--` ends parsing, which is what lets a negative volume number through. Machine output is the double-nested `[[obj, ...]]` envelope every jq expression in tests/e2e/cli-matrix and the operator harness is written against; singletons stay flat, matching the upstream shape. `resource list` and `node list` are wired; UnimplementedCommands reports the rest of the registered surface so the gap between what the grammar advertises and what works is visible rather than discovered mid-incident. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
storage-pool, resource-definition, volume-definition, volume, snapshot and resource-group listings, each carrying the contracts this repo's scripts assert: CanSnapshots renders True/False, sizes render in MiB/GiB rather than raw KiB, the layer stack is visible on a definition row, and a snapshot row contains its own name. The storage-pool State cell is the reason that view is assembled here rather than served from a printer column: a pool whose backing store vanished out-of-band still has a healthy-looking CRD, and reporting Ok there is exactly the regression this repo's recovery test watches for. All eight listings now share one generic implementation — fetch, filter, then either the machine envelope or a rendered table — so a new listing cannot accidentally skip the -m branch or the -n/-r filters. 13 of the ~83 registered commands are implemented; UnimplementedCommands reports the rest. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Writes start here, with the two behaviours scripts depend on most. Setting a property to an EMPTY value DELETES the key. That is not a nicety: replay workflows in this repo restore a cluster's automatic behaviour by setting a property to "" and then assert the key is gone from list-properties. One accessor shape serves every noun, so the rule cannot drift between resource-definition, node and controller. Deleting an object that is already gone SUCCEEDS. Teardown paths rely on that idempotence; a non-zero exit there would fail cleanup runs that are otherwise fine. 22 of the ~83 registered commands now work. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Adds the create/delete/modify verbs for nodes, volume definitions, resources, snapshots and resource groups, plus the binary size parser they share. Sizes are parsed explicitly rather than with a permissive library: the suffixes are binary, so getting one wrong would provision a volume three orders of magnitude off. Numbers destined for int32 API fields are range-checked instead of truncated, so a wrapped volume number cannot address a volume the operator did not name. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Resources, storage pools, resource groups, volume definitions and volume groups get set-property, list-properties and delete-property, alongside the nouns that already had them. The three verbs are registered from a single accessor table, and a registry test now fails if a noun grows set-property without the other two: half a property surface is worse than none, because a runbook can set a key it can neither read back nor undo. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Registering a pool writes the backing name under the StorDriver key its provider actually reads; a pool created under the wrong key is permanently un-reconcilable, so the provider table is pinned by test. A thin LVM pool must be named <volume-group>/<thin-pool> — guessing the missing half would point the pool at storage that does not exist. error-reports list is refused rather than served: the reports live in the controller process's memory, not in any API object, and an empty table would read as "no errors" during an incident. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
toggle-disk covers the four shapes operators use: --cancel unwinds an in-flight conversion without touching DISKLESS (the reconciler clears it only once the rollback really completed), --migrate-from is strict add-before-drop and leaves the source replica in place until the copy is durable, --diskless forces storage-free, and the pool-bearing form promotes. Promotion clears TIE_BREAKER as well as DISKLESS: a diskful replica left carrying TIE_BREAKER is counted as a witness by the tiebreaker reconciler, which then double-counts the slot. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Evacuating a node with a mounted volume is refused, because latching EVICTED silently would let the autoplacer and the migration reconciler strand it; --force is the operator's conscious override. A replica the satellite has not reported on yet is "unknown", not "in use", so it does not block the drain. node lost cascade-deletes the dead satellite's replicas and pools here rather than leaving it to a finalizer the departed satellite would have had to run — otherwise every orphan hangs forever and the next definition that recycles the name is bricked. Surviving peers are left for the tiebreaker reconciler. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A DRBD knob is stored under the property key for its section, and the
section decides which .res block the value is rendered into. Writing a
net{} knob such as verify-alg under the resource namespace lands it in
options{}, where drbdadm rejects the whole file and every later adjust
for that resource fails — so the knob-to-namespace table is pinned by
test and an unrecognised knob is refused rather than guessed at.
The render catalogue stays the single source for the knobs it carries;
the new table only covers the ones it does not, so the two cannot
drift.
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Placement calls the controller's own placer rather than reimplementing the choice client-side: two answers to "where does this replica go?" would drift apart the moment either changed. A shortfall is reported on stderr and exits 0. Over-committed requests are deferred best-effort placement here — the rebalance reconciler tops the resource up when capacity appears — so failing would break every runbook that provisions ahead of the hardware. The `+N` delta counts only diskful replicas, matching the placer's own tally: counting a tiebreaker witness would make `+1` on a two-replicas-plus-witness resource place nothing. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
create-passphrase writes the cluster master key to the Secret the controller and satellites read. An existing passphrase is never silently replaced: rotating the master key would leave every existing LUKS volume undecryptable. Re-running with the same value stays a success so a script's pre-flight step is idempotent. enter-passphrase cannot be delivered from here — unlocking is state inside the controller process, not a Kubernetes object. It verifies the passphrase and then says where the unlock has to go, rather than exiting 0 and leaving the operator believing the cluster is unlocked. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Every command the grammar advertises now has a handler, and the coverage test fails rather than logs when one goes missing: a command an operator finds in the help and reaches for mid-incident must do something. A restore lands replicas on the nodes that hold the snapshot, in the pool the source uses there — never via the placer, because a replica on a different backend makes the satellite pipe the snapshot stream into a receiver that never converges. Clone is that same path behind an internal snapshot, so the two cannot diverge. create-multiple stamps one group id across the batch; separate suspend-io barriers would give snapshots that are individually consistent but not consistent with each other. In-place rollback stays refused, and the refusal names the recoverable alternative. The size queries report the physical bound from the pools a replica set would occupy; the controller's oversubscription policy is not reproduced here, so the figure can only be more conservative. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The command tree is generated from the registry, so help cannot advertise something that does not dispatch. An explicit `help` prints to stdout and exits 0 so it can be piped; naming no command at all is still a malformed invocation, so the tree goes to stderr and the exit code stays the client-side rejection scripts branch on. The design doc now records the two commands a CRD-only client cannot serve — error report listing and passphrase unlock both act on state held in the controller process — and the one query that is deliberately more conservative than the controller's. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
An explicit placement request now FAILS when the placer cannot seat every replica — the operator asked for N and must find out they did not get N. Only a group spawn or rebalance succeeds-and-reports, where the place count is a target the rebalance reconciler keeps working towards. The two contracts had been collapsed into one. set-size refuses a shrink without --force: nothing here shrinks the filesystem first, so a smaller block device under a live filesystem truncates it. The 4 MiB floor and 16 TiB ceiling hold even under --force — below DRBD's per-device minimum the satellite loops on create-md forever instead of failing. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The reports are a ring buffer in the controller process's memory, so a client that speaks to the API server has nothing to list. Carrying the verb only to refuse it is worse than not advertising it. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The verb proves the operator knows the cluster master key, and that is what now happens: a constant-time compare against the Secret, failing on a wrong value or on a cluster that has none. Serving this over REST additionally flips an in-memory flag in the controller, which this CLI cannot do — but that flag's only reader sets state.suspended on LUKS resources in the REST view. It gates nothing (the LUKS create check reads the Secret, and so do the satellites) and it is per-process, so across apiserver replicas it already disagrees with itself. Refusing the whole command over a display flag was disproportionate; the CLI now does the part that has an effect and says on stderr what it did not touch. Both encryption verbs compare in constant time: a byte-by-byte compare leaks where two passphrases first differ, which is enough to recover the master key one character at a time. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
9f35b1b to
e20b56f
Compare
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
api/v1alpha1/printcolumns_test.go (1)
42-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin column types and JSONPaths too.
The test accepts correct names with broken
typeorJSONPath, allowing blank or incorrectkubectl getoutput. Assert the orderedName,Type, andJSONPathfor each served column.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/v1alpha1/printcolumns_test.go` around lines 42 - 48, Update the print-column expectations in the test around the `want` map to include each column’s ordered `Name`, `Type`, and `JSONPath`, rather than names alone. Compare the served column definitions against these complete expectations so incorrect or blank types and paths fail while preserving column order.internal/cli/handlers.go (1)
136-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate handler definition for
resource list-volumesandvolume list.Lines 136-141 are byte-identical to the
volume listhandler at lines 78-83 (same fetch, filter, view, and state columns). Extract a sharedhandlervariable so the two aliases can't silently diverge if one is updated later.♻️ Suggested consolidation
- "volume list": listing("resources", - fetchResources, - keepResource, - func(resources []apiv1.Resource, _ *runContext) *metav1.Table { return view.VolumeList(resources) }, - "State", - ), + "volume list": volumeListHandler,- "resource list-volumes": listing("resources", - fetchResources, - keepResource, - func(resources []apiv1.Resource, _ *runContext) *metav1.Table { return view.VolumeList(resources) }, - "State", - ), + "resource list-volumes": volumeListHandler,//nolint:gochecknoglobals // static dispatch table var volumeListHandler = listing("resources", fetchResources, keepResource, func(resources []apiv1.Resource, _ *runContext) *metav1.Table { return view.VolumeList(resources) }, "State", )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/handlers.go` around lines 136 - 141, Extract the duplicated listing definition into a shared volumeListHandler variable, using the existing fetchResources, keepResource, view.VolumeList, and "State" configuration. Replace both the "resource list-volumes" and "volume list" entries in the dispatch table with this shared handler so the aliases remain synchronized.internal/cli/view/resource.go (1)
221-240: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated "first non-terminal volume" scan between
worstVolumeandisFaulty.Both functions independently walk
res.Volumes, skip emptyDiskState, and checkterminalStatesfor the same "non-converged" criterion. Keeping this logic in one place would prevent the display (worstVolume) and the--faultyfilter (isFaulty) from silently diverging if the terminal-state classification changes later.♻️ Proposed consolidation
+// nonTerminalVolume returns the first volume whose disk state is +// reported and not converged. +func nonTerminalVolume(res *apiv1.Resource) *apiv1.Volume { + for i := range res.Volumes { + state := strings.ToLower(res.Volumes[i].State.DiskState) + if state == "" { + continue + } + + if _, terminal := terminalStates[state]; !terminal { + return &res.Volumes[i] + } + } + + return nil +} + func worstVolume(res *apiv1.Resource) *apiv1.Volume { if len(res.Volumes) == 0 { return nil } - - for i := range res.Volumes { - state := strings.ToLower(res.Volumes[i].State.DiskState) - if state == "" { - continue - } - - if _, terminal := terminalStates[state]; !terminal { - return &res.Volumes[i] - } - } - + if v := nonTerminalVolume(res); v != nil { + return v + } return &res.Volumes[0] } func isFaulty(res *apiv1.Resource) bool { - for i := range res.Volumes { - state := strings.ToLower(res.Volumes[i].State.DiskState) - if state == "" { - continue - } - - if _, terminal := terminalStates[state]; !terminal { - return true - } - } - - return false + return nonTerminalVolume(res) != nil }Also applies to: 261-278
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/view/resource.go` around lines 221 - 240, Consolidate the duplicated volume-state scan used by worstVolume and isFaulty into a shared helper that selects the first non-terminal volume while skipping empty DiskState values. Update both callers to reuse this helper and preserve worstVolume’s fallback to the first volume when no non-terminal volume exists, keeping terminalStates as the single classification source.internal/cli/snapshot.go (1)
286-338: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRedundant per-node listing, and a silent empty-pool fallback.
Two things in this pair of functions:
sourcePoolOnre-lists all replicas ofsrcRD(a Kubernetes API call) once per node insideplaceRestored's loop. Hoisting theListByDefinitioncall outside the loop avoids N redundant round-trips for an N-node restore.- If the source definition has no replica with a
StorPoolNameset at all,fallbackstays""andsourcePoolOnreturns("", nil)— no error.placeRestoredthen stamps that empty string viastampPropon the new replica rather than surfacing a failure, which could silently create a replica with a blank storage-pool property.♻️ Proposed fix: hoist the list call out of the loop
func placeRestored(ctx context.Context, run *runContext, srcRD, rdName string, snap *apiv1.Snapshot) error { nodes := run.Flags.Nodes if len(nodes) == 0 { nodes = snap.Nodes } + replicas, err := run.Store.Resources().ListByDefinition(ctx, srcRD) + if err != nil { + return fmt.Errorf("list replicas of %s: %w", srcRD, err) + } + for _, node := range nodes { res := &apiv1.Resource{Name: rdName, NodeName: node} - pool, err := sourcePoolOn(ctx, run, srcRD, node) - if err != nil { - return err - } + pool := sourcePoolFor(replicas, node) stampProp(res, storPoolNameProp, pool) - err = run.Store.Resources().Create(ctx, res) + err = run.Store.Resources().Create(ctx, res) if err != nil { return fmt.Errorf("create restored replica %s on %s: %w", rdName, node, err) } } return nil }Please confirm whether
stampProptreats an empty value as "leave unset" (matching pre-restore behavior when no pool is pinned) or writes an explicit empty property that downstream code might misinterpret as "no default pool" versus "unset".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/snapshot.go` around lines 286 - 338, Update placeRestored to call Resources().ListByDefinition once before iterating nodes, then pass the retrieved replicas into sourcePoolOn instead of re-listing per node. Change sourcePoolOn to return an error when no replica has a non-empty storPoolNameProp, and ensure placeRestored propagates that error before stamping the property; verify stampProp’s empty-value behavior and preserve the intended unset-versus-empty semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml`:
- Around line 24-26: The CRD printer columns use array-valued fields for the
Layers/Nodes summaries. In
config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:24-26,
config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml:24-26, and
config/crd/bases/blockstor.cozystack.io_snapshots.yaml:24-26, replace those
paths with the controller-provided scalar string summary/status field, or remove
the columns if no such field exists.
In `@docs/cli-design.md`:
- Around line 15-21: Add a shell or console language tag to the fenced command
example in the CLI command documentation, changing the opening fence from an
untyped fence while leaving the command contents unchanged.
In `@internal/cli/definition.go`:
- Around line 186-211: Update resourceGroupQuerySizeInfo to compute
maxVolumeSizeKib using the selected group and candidate pools before the
machine-output branch, then pass machineOut the same size-information payload
represented by view.SizeInfoRows, including the resource-group name, computed
maximum size, and pools. Preserve the existing table rendering behavior and
ensure query-max-volume-size machine output reports the computed size rather
than raw pools alone.
In `@internal/cli/flags.go`:
- Around line 76-107: The valueFlags table currently treats -l separately from
--layer-list, causing assign() to store the short form under a different key
than resourceDefinitionModify reads. Update the flag alias configuration around
valueFlags so -l is folded onto the canonical --layer-list key, ensuring both
forms populate Values["layer-list"] and trigger the same behavior.
In `@internal/cli/node.go`:
- Around line 211-225: Update patchNodeFlags to use NodeStore.PatchNodeSpec
instead of the current Get-then-wholesale Update sequence. Build the patch from
the requested flag change using setFlag semantics, preserve the existing
node-not-found and update error context, and ensure concurrent node flag edits
are merged rather than overwritten.
In `@internal/cli/physical.go`:
- Around line 42-84: Update physicalStorageCreateDevicePool and the
device-stamping flow around stampDevices to track which devices were
successfully stamped, then perform best-effort compensating cleanup if a later
device lookup fails or StoragePools().Create returns a non-AlreadyExists error.
Cleanup must remove the pool attachment from only those devices, preserve the
original operation error, and avoid changing the existing AlreadyExists
behavior.
In `@internal/cli/resource.go`:
- Around line 147-180: Update migrateDisk to reject a self-referential migration
when the migrate-from value src equals the destination dst, returning the
existing migration validation error before fetching or stamping the destination
resource. Preserve normal source validation and migration behavior when src and
dst differ.
In `@internal/cli/write_more.go`:
- Around line 156-195: Update volumeDefinitionCreate to validate sizeKib with
the same checkResize bounds used by volumeDefinitionSetSize before constructing
or storing the VolumeDefinition. Return the validation error and preserve the
existing explicit and automatic numbering flows.
In `@internal/cli/write.go`:
- Around line 57-90: Eliminate the stale read/update window between setProperty
and objectProps.set by changing the setter contract to accept a mutation
callback or single-key delta instead of a precomputed property map. Update
setProperty to pass an add/delete operation, and have objectProps.set/apply
perform the fresh GET, mutate the retrieved bag, and update it, preserving
deletion for empty values; add conflict retry if supported by the existing store
patterns.
---
Nitpick comments:
In `@api/v1alpha1/printcolumns_test.go`:
- Around line 42-48: Update the print-column expectations in the test around the
`want` map to include each column’s ordered `Name`, `Type`, and `JSONPath`,
rather than names alone. Compare the served column definitions against these
complete expectations so incorrect or blank types and paths fail while
preserving column order.
In `@internal/cli/handlers.go`:
- Around line 136-141: Extract the duplicated listing definition into a shared
volumeListHandler variable, using the existing fetchResources, keepResource,
view.VolumeList, and "State" configuration. Replace both the "resource
list-volumes" and "volume list" entries in the dispatch table with this shared
handler so the aliases remain synchronized.
In `@internal/cli/snapshot.go`:
- Around line 286-338: Update placeRestored to call Resources().ListByDefinition
once before iterating nodes, then pass the retrieved replicas into sourcePoolOn
instead of re-listing per node. Change sourcePoolOn to return an error when no
replica has a non-empty storPoolNameProp, and ensure placeRestored propagates
that error before stamping the property; verify stampProp’s empty-value behavior
and preserve the intended unset-versus-empty semantics.
In `@internal/cli/view/resource.go`:
- Around line 221-240: Consolidate the duplicated volume-state scan used by
worstVolume and isFaulty into a shared helper that selects the first
non-terminal volume while skipping empty DiskState values. Update both callers
to reuse this helper and preserve worstVolume’s fallback to the first volume
when no non-terminal volume exists, keeping terminalStates as the single
classification source.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c4e32cd8-3f3f-4ba2-b793-61160c1f433c
📒 Files selected for processing (59)
Makefileapi/v1alpha1/node_types.goapi/v1alpha1/printcolumns_test.goapi/v1alpha1/resource_types.goapi/v1alpha1/resourcedefinition_types.goapi/v1alpha1/resourcegroup_types.goapi/v1alpha1/snapshot_types.goapi/v1alpha1/storagepool_types.gocmd/blockstor/main.goconfig/crd/bases/blockstor.cozystack.io_nodes.yamlconfig/crd/bases/blockstor.cozystack.io_resourcedefinitions.yamlconfig/crd/bases/blockstor.cozystack.io_resourcegroups.yamlconfig/crd/bases/blockstor.cozystack.io_resources.yamlconfig/crd/bases/blockstor.cozystack.io_snapshots.yamlconfig/crd/bases/blockstor.cozystack.io_storagepools.yamldocs/cli-design.mdinternal/cli/app.gointernal/cli/app_test.gointernal/cli/color/color.gointernal/cli/color/color_test.gointernal/cli/command/registry.gointernal/cli/command/registry_test.gointernal/cli/definition.gointernal/cli/definition_test.gointernal/cli/drbdopts.gointernal/cli/drbdopts_test.gointernal/cli/encryption.gointernal/cli/encryption_test.gointernal/cli/flags.gointernal/cli/handlers.gointernal/cli/help.gointernal/cli/node.gointernal/cli/node_test.gointernal/cli/output/machine.gointernal/cli/output/machine_test.gointernal/cli/physical.gointernal/cli/place.gointernal/cli/place_test.gointernal/cli/pool.gointernal/cli/pool_test.gointernal/cli/props.gointernal/cli/props_test.gointernal/cli/resource.gointernal/cli/resource_test.gointernal/cli/snapshot.gointernal/cli/snapshot_test.gointernal/cli/table/table.gointernal/cli/table/table_test.gointernal/cli/view/node.gointernal/cli/view/resource.gointernal/cli/view/resource_test.gointernal/cli/view/views.gointernal/cli/view/views_test.gointernal/cli/write.gointernal/cli/write_more.gointernal/cli/write_more_test.gointernal/cli/write_test.gopkg/drbd/flagkeys.gopkg/drbd/flagkeys_test.go
| - jsonPath: .spec.layerStack | ||
| name: Layers | ||
| type: string |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the CRD files and inspect the relevant statuses.spec.columns entries.
for f in \
config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml \
config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml \
config/crd/bases/blockstor.cozystack.io_snapshots.yaml
do
echo "===== $f ====="
if [ -f "$f" ]; then
nl -ba "$f" | sed -n '1,80p'
else
echo "missing"
fi
done
echo "===== search for printer columns and relevant spec fields ====="
rg -n "jsonPath:|\\.spec\\.(layerStack|selectFilter\\.layerStack|nodes)|type: string|selectFilter|layerStack|nodes:" config/crd/bases -SRepository: cozystack/blockstor
Length of output: 275
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml \
config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml \
config/crd/bases/blockstor.cozystack.io_snapshots.yaml
do
echo "===== $f ====="
if [ -f "$f" ]; then
cat -n "$f" | sed -n '1,90p'
else
echo "missing"
fi
done
echo "===== search for printer columns and relevant spec fields ====="
rg -n "jsonPath:|spec\.(layerStack|selectFilter\.layerStack|nodes)|type: string|selectFilter|layerStack|nodes:" config/crd/bases -SRepository: cozystack/blockstor
Length of output: 35857
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
import yaml
files = [
Path("config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml"),
Path("config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml"),
Path("config/crd/bases/blockstor.cozystack.io_snapshots.yaml"),
]
columns = []
for p in files:
docs = list(yaml.safe_load_all(p.read_text()))
for doc in docs:
if not doc or doc.get("kind") != "CustomResourceDefinition":
continue
name = doc["metadata"]["name"]
schema = doc["spec"]["versions"][0]["schema"]["openAPIV3Schema"]["properties"]["spec"]
for col in doc["spec"]["versions"][0]["additionalPrinterColumns"]:
path = col["jsonPath"]
# Normalize JSONPath slices/indices for lookup.
lookup = [p.strip() for p in re.sub(r'(\[\d+?\])', lambda m: '.'+m.group(1), path.split(".spec")[-1]).split(".") if p] if ".spec" in path else []
current = schema
prop_path = []
for part in lookup:
if part.startswith("["):
idx = int(part.strip("[]"))
if isinstance(current, list):
if idx < 0 or idx >= len(current):
break
current = current[idx]
prop_path.append(part)
continue
else:
next_elem = None
for prop in current.get("properties", {}) if isinstance(current, dict) else []:
if prop == "properties":
continue
pattern = prop.replace("*", ".*")
if re.fullmatch(pattern, part):
next_elem = (prop, current["properties"][prop])
break
if not next_elem:
break
prop_path.append(next_elem[0])
current = next_elem[1]
current_type = current.get("type") if isinstance(current, dict) else None
columns.append((name, col["line"] if hasattr(col, "line") else None, path, col["type"], current_type, prop_path))
print("printer_columns_analysis")
for name, line, jmp, declared, current_type, prop_path in columns:
print(f"{name}:{jmp}:{declared}:current_type={current_type}:path={prop_path}")
PYRepository: cozystack/blockstor
Length of output: 274
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Manually parse the relevant CRD schema property/type declarations without PyYAML.
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml"),
Path("config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml"),
Path("config/crd/bases/blockstor.cozystack.io_snapshots.yaml"),
]
queries = {
"ResourceDefinition": ".spec.layerStack",
"ResourceGroup": ".spec.selectFilter.layerStack",
"Snapshot": ".spec.nodes",
}
for p in files:
text = p.read_text()
docs = re.split(r'^---\n', text, flags=re.M)
for doc in docs:
if 'name: blockstor.cozystack.io_' in doc:
name = doc.split("metadata:\n name: ")[1].split("\n", 1)[0].strip() if 'metadata:\n name: ' in doc else "unknown"
columns_match = re.search(r'^\s*- jsonPath:\s*(.*?)\n(?:\s*\S+.*?\n)+', doc, re.S|re.M)
columns_match = re.search(r'additionalPrinterColumns:(.*?)^ name: v1alpha1', doc, re.S|re.M)
columns_text = columns_match.group(1) if columns_match else ""
cols = []
for block in re.split(r'^\s*- jsonPath:\s*', columns_text, flags=re.M)[1:]:
jp = block.split("\n", 1)[0].strip()
tp = re.search(r'\n\s*type:\s*(\S+)', block)
cols.append((jp, tp.group(1) if tp else None))
print(f"===== {p.name}: kind={name} columns =====")
for jp, tp in cols:
if jp == queries.get(name):
type_type_match = re.search(re.escape(jp.replace(".", r"\."))+r"\n\s*type:\s*(\S+)", doc)
array_match = re.search(r'\n\s*type:\s*array\n(?:\s+\S+.*?\n)+?\s+\S+.*?\n\s*type:\s*string', doc, re.S)
# simple next type after exact path
type_type_match = re.search(re.escape(jp)+r"\n(\s*)type:\s*(\S+)", doc)
print(f"{jp} declared_printer={tp} schema_type={type_type_match.group(2) if type_type_match else None}")
if name == "Snapshot":
# inspect .spec.nodes schema block
n = doc.find(re.escape(".spec.nodes"))
start = doc.find("properties:", n)
snippet = doc[start:start+800] if start > 0 else ""
print("nodes schema block lines 120-137:")
lines = doc.splitlines()
for i, line in enumerate(lines[120:140], 121):
print(f"{i}: {line}")
PY
echo "===== Kubernetes JSONPath printer-column behavior docs references ====="
curl -sSL 'https://kubernetes.io/docs/reference/using-api/api-concepts/#standard-output-formats' \
| grep -n -i -C 3 'additionalPrinterColumns\|JSONPath\|type:' \
| head -n 120 || true
curl -sSL 'https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/#custom-columns' \
| grep -n -i -C 3 'additionalPrinterColumns\|JSONPath\|type:' \
| head -n 120 || trueRepository: cozystack/blockstor
Length of output: 37084
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
files = [
Path("config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml"),
Path("config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml"),
Path("config/crd/bases/blockstor.cozystack.io_snapshots.yaml"),
]
for p in files:
text = p.read_text()
docs = re.split(r'^---\n', text, flags=re.M)
for doc in docs:
if 'name: blockstor.cozystack.io_' not in doc:
continue
name = re.search(r'name:\s*([\w+-]+)\.blockstor\.cozystack\.io', doc)
name = name.group(1) if name else "unknown"
schema = doc.split("openAPIV3Schema:", 1)[1] if "openAPIV3Schema:" in doc else doc
for jp, col_type in [
("ResourceDefinition", ".spec.layerStack", "string"),
("ResourceGroup", ".spec.selectFilter.layerStack", "string"),
("Snapshot", ".spec.nodes", "string"),
]:
if name != jp:
continue
# Extract the immediate next type after the exact jsonPath.
m = re.search(re.escape(jp) + r"\s*\n\s*type:\s*(\S+)", schema)
print(f"{name}: jsonPath={jp} printerType={col_type} schemaType={m.group(1) if m else None}")
PY
echo "===== source usages of relevant fields ====="
rg -n "layerStack|selectFilter|selectFilter\.layerStack|nodes:" . --glob '!vendor/**' --glob '!dist/**' --glob '!build/**' | sed -n '1,220p'Repository: cozystack/blockstor
Length of output: 18508
🌐 Web query:
Kubernetes CustomResourceDefinition additionalPrinterColumns JSONPath array result type mismatch printer-column omitted
💡 Result:
In Kubernetes CustomResourceDefinitions (CRDs), the additionalPrinterColumns configuration is designed to extract values from a custom resource to display in kubectl table output [1][2]. If a JSONPath expression targets an array, the observed behavior—where the column may appear empty, behave inconsistently, or fail to display multiple elements—is a known area of historical complexity in Kubernetes [3][4][5]. Key Technical Context: 1. Historical Limitation: Early versions of Kubernetes often restricted or inconsistently handled JSONPaths that returned array types in additionalPrinterColumns [3][5]. Historically, the system might have only evaluated and printed the first matching result, or failed to handle the array-to-string conversion expected for a flat table column [3][5]. 2. Improved Support: Subsequent updates, such as the changes introduced to improve support for more complex JSONPaths, have allowed for better handling of paths that return multiple values [4]. However, these are still subject to the requirement that the resulting data must be representable as a string within the table cell [1][6]. 3. Requirement for Simple Paths: While documentation often refers to "simple JSON paths" [1][6], "simple" in this context usually refers to the structure of the path itself rather than the complexity of the data it points to. If the JSONPath evaluates to an object or an array, kubectl must effectively serialize that data into a string format to display it in a table column [7]. Troubleshooting Steps: If you are experiencing a type mismatch or missing data: - Ensure the type defined in your CustomResourceColumnDefinition (e.g., string, integer) matches the expected output of your JSONPath [1][6]. If the path returns an array, defining the column type as string is generally required, and kubectl will typically join the array elements into a comma-separated string [7]. - Verify the JSONPath expression is valid. For example, using [*] can help ensure you are selecting all elements, but be mindful of the performance cost of complex path evaluations on the API server [4][5]. - If the output remains empty or missing, it may indicate that the API server is unable to resolve the path or that the resulting data type is incompatible with the declared column type. As a workaround, some users find it necessary to manage this data at the application layer by creating specific, scalar fields in the CRD status (e.g., a pre-computed string field) rather than relying on dynamic JSONPath extraction for complex data structures [4][5].
Citations:
- 1: https://kubernetes.io/docs/reference/kubernetes-api/apiextensions/custom-resource-definition-v1/
- 2: https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definitions/
- 3: additionalPrinterColumns for CRD's doesn't work in k8s 1.11 for columns with array data kubernetes/kubectl#517
- 4: Adding support for complex json paths in AdditionalPrinterColumns kubernetes/kubernetes#101205
- 5: https://groups.google.com/g/kubernetes-sig-api-machinery/c/GxXWe6T8DoM
- 6: https://www.bookstack.cn/read/kubernetes-1.30-en/a615272337ab930a.md
- 7: https://oneuptime.com/blog/post/2026-02-09-custom-resource-printer-columns/view
🌐 Web query:
Kubernetes API machinery printer columns JSONPath array result type string omit
💡 Result:
In Kubernetes Custom Resource Definition (CRD) additional printer columns, the support for arrays in JSONPath is limited and often yields unexpected results. When you use a JSONPath that targets an array (e.g.,.spec.items[*].name), the Kubernetes server-side printing logic typically returns only the first match rather than the full array [1][2]. This behavior is a known limitation that persists in many versions, where the system does not automatically join or format array elements for display in the column [1][2]. Regarding the "omit" or "empty" result type: 1. Handling Missing Data: If a field is missing, the column may appear empty [3][4]. While Kubernetes JSONPath generally allows accessing fields, you cannot explicitly configure an "omit" property in the CRD definition to hide the column if the value is missing. The visibility of a column is instead governed by its priority [5][6]. 2. Column Priority: You can use the priority field in the CustomResourceColumnDefinition [5]. Columns with a priority greater than 0 are considered "wide" columns and are only displayed when the user explicitly requests wider output (e.g., via -o wide), effectively omitting them from standard output if space is limited [5][6]. 3. Escaping: If you are encountering empty results for fields that you believe exist, ensure you are correctly escaping special characters (such as periods or slashes in labels) using backslashes (e.g.,.metadata.labels.key.name) [3][7][4]. Single quotes are sometimes required around the path for specific characters to be parsed correctly [3]. Because there is no native "omit if empty" feature for printer columns, if your goal is to handle complex array data or conditional visibility, the recommended workaround is often to define a specific status field in your API that pre-formats the data (e.g., as a single string) [2]. This allows you to point the printer column to a simple string field rather than an array, ensuring consistent output [2].
Citations:
- 1: additionalPrinterColumns for CRD's doesn't work in k8s 1.11 for columns with array data kubernetes/kubectl#517
- 2: https://groups.google.com/g/kubernetes-sig-api-machinery/c/GxXWe6T8DoM
- 3: https://stackoverflow.com/questions/74464069/json-path-semantics-different-in-kubectl-and-additional-printer-columns-in-custo
- 4: [BUG] UI - additionalPrinterColumns values empty when fields have periods or slashes in them rancher/dashboard#10777
- 5: https://docs.w3cub.com/kubernetes/reference/kubernetes-api/extend-resources/custom-resource-definition-v1/index
- 6: https://kubebuilder.io/reference/markers/crd
- 7: https://kubernetes.io/docs/reference/kubectl/jsonpath/
🌐 Web query:
Kubernetes API printer columns jsonpath arrays type string join comma kubectl get -o wide
💡 Result:
In Kubernetes, CustomResourceDefinition (CRD) additionalPrinterColumns do not support complex JSONPath templates or native array joining [1][2][3]. The jsonPath field in a CRD specification is restricted to simple JSONPath expressions that must start with a dot (.) and point to a specific field [2][3]. If you use a JSONPath that returns an array (e.g., .spec.items[*]), Kubernetes will typically display only the first element of that array rather than joining the elements with commas [1][4]. For scenarios where you need to display joined, formatted, or combined data (like X/Y or comma-separated lists) in a table format, you have the following options: 1. Use kubectl -o custom-columns: Unlike CRD printer columns, the kubectl custom-columns output format supports more flexible JSONPath templates [5]. You can command line the desired output without modifying the CRD: kubectl get -o custom-columns=NAME:.metadata.name,MY_COL:.spec.items[*] 2. Process in the application or controller: Since CRD printer columns are limited, the community standard approach is to create a specific, pre-formatted string field in your CRD's status block (e.g., status.itemsSummary). Your controller or operator then computes this joined string (e.g., using strings.Join()) and updates the object status [1]. You then point your CRD additionalPrinterColumns to that pre-computed string field [1]. 3. Use alternative output formats: If you need complex manipulation or formatting of lists for human-readable output, consider using kubectl -o json or kubectl -o yaml piped into tools like jq (e.g., kubectl get <resource> -o json | jq '.items[].spec.items | join(",")'), which natively supports advanced data transformation and string joining [6].
Citations:
- 1: https://groups.google.com/g/kubernetes-sig-api-machinery/c/GxXWe6T8DoM
- 2: https://groups.google.com/g/kubernetes-dev/c/hYLa75_q_fk
- 3: feature: support the (X/Y) display mode for the printcolumn field in CR resource kubernetes/kubernetes#127246
- 4: Human readable status from conditions for a CRD using additionalPrinterColumns kubernetes/kubernetes#67268
- 5: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/get/customcolumn.go
- 6: https://kubernetes.io/docs/reference/kubectl/jsonpath/
Use a scalar summary field for populated array printer columns.
These paths target array-valued CRD fields, so the Layers/Nodes columns can render inconsistent or omitted values in kubectl table output. Replace them with a controller-provided string summary/status field, or remove the columns.
- config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:24
- config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml:24
- config/crd/bases/blockstor.cozystack.io_snapshots.yaml:24
📍 Affects 3 files
config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml#L24-L26(this comment)config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml#L24-L26config/crd/bases/blockstor.cozystack.io_snapshots.yaml#L24-L26
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml` around
lines 24 - 26, The CRD printer columns use array-valued fields for the
Layers/Nodes summaries. In
config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:24-26,
config/crd/bases/blockstor.cozystack.io_resourcegroups.yaml:24-26, and
config/crd/bases/blockstor.cozystack.io_snapshots.yaml:24-26, replace those
paths with the controller-provided scalar string summary/status field, or remove
the columns if no such field exists.
Source: MCP tools
| ``` | ||
| blockstor node list blockstor n l | ||
| blockstor storage-pool list blockstor sp l | ||
| blockstor resource-definition create pvc-x blockstor rd c pvc-x | ||
| blockstor resource toggle-disk n1 pvc-x blockstor r td n1 pvc-x | ||
| blockstor volume-definition set-size pvc-x 0 10G blockstor vd s pvc-x 0 10G | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the fenced command block.
Use ```shell or ```console instead of an untyped fence so Markdown tooling can validate and render the example consistently.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 15-15: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cli-design.md` around lines 15 - 21, Add a shell or console language tag
to the fenced command example in the CLI command documentation, changing the
opening fence from an untyped fence while leaving the command contents
unchanged.
Source: Linters/SAST tools
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict: NOT LGTM
Build and go test ./... are green, but several behavioral defects reach paying clusters. 5 blockers + 4 minor.
Blockers
-
Size bounds bypassed on create/spawn + unchecked int64 overflow —
internal/cli/write_more.go.
checkResize(floor 4 MiB / ceiling 16 TiB) is called only fromset-size(:252).volume-definition create(:156) andresource-group spawn-resources(:258) writeSizeKibwith no bounds check, andParseSizemultipliesvalue*multiplierunchecked —ParseSize("17179869184T") = (0, nil). Sovolume-definition create rd 17179869184TstoressizeKib: 0. Per the code's own comment the satellite then loops ondrbdadm create-mdforever — a silent hang on legal input, with no Event/Ready=False. No server-side backstop exists (CRDsizeKibhas nominimum, no CEL, no admission webhook).write_more_test.go:54even pins a sub-floor1024Kcreate as success. Fix: enforce the floor/ceiling (and an overflow guard) on create and spawn; correct the test. -
Flags parsed but never consumed, silently wrong output —
internal/cli/flags.go,handlers.go.
--storage-pools,-o/--output-fmt/--output-version,--limit,--controllers,-p/--pastablehave zero readers.r l -o jsonprints the human table with exit 0;sp l --storage-pools Xdoes not filter. A script doingr l -o json | jqgets malformed input with no error. Fix: either wire these flags to behavior or reject them as unsupported. -
--faultymisses connection failures —internal/cli/view/resource.go:265.
isFaultyinspects only volumeDiskState, neverLayerObject.Drbd.Connections. A replica with local disk UpToDate but a StandAlone/NetworkFailure peer (split-brain) is dropped by--faulty, contradicting the troubleshooting runbooks. Fix: treat a non-Connected DRBD connection as faulty. -
--faultyignored in machine mode —internal/cli/handlers.go:234.
The-mbranch serializes the set filtered only by node/resource;FaultyOnlyis applied only on the human render path.r l --faulty -mreturns ALL replicas. Fix: apply the faulty filter before machine serialization. -
Multi-line cell breaks the box table and the
awk -F'|'contract —internal/cli/view/views.go:262,table/table.go.
selectFilterCelljoins parts with\nand the renderer writes them verbatim, while table.go's docstring declares the pipe layout a parsing contract. Any resource-group with a StoragePool/LayerStack renders a row split mid-cell. Fix: render multi-value cells without embedded newlines (or escape them).
Minor
ParseSizerejects10GiB/10Gi/10GBdespite the comment promising it tolerates them; theiBtrim is dead code (the switch keys on the last byte first).write_more.go:67-84.query-size-infooverestimates the max placeable size: no per-node pool dedup, does not excludePoolMissing, ignoresSelectFilter.StoragePoolList, diverges from the real placer on all three.definition.go:215.node delete-property n1 key oops(extra positional) silently SETSkey=oopsinstead of deleting.props.go:37towrite.go:69.- Bool flag with inline value:
--force=falseenables Force (opposite of intent);-p=secretdrops the value.flags.go:130.
Note
The constant-time passphrase compare uses crypto/subtle correctly, but returns 0 immediately on a length mismatch (passphrase length leaks) and runs client-side after the full Secret was already read via the caller's RBAC, so the timing threat model in the comment does not apply here.
Volume sizes are now bounded on every path that writes one, not just resize. ParseSize checks the multiplication instead of assuming it: `17179869184T` overflowed int64 to exactly zero, and zero is the one size the satellite cannot fail on — it loops on create-md forever. Nothing downstream catches it, since the CRD has no minimum, no CEL rule and no webhook. The suffixes the comment promised (10GiB, 10Gi, 10GB) now actually parse. --faulty was judging on disk state alone, so a replica with an UpToDate disk and a StandAlone peer — the split-brain the runbooks send operators to find this way — was dropped. It now looks at the peer links too, and it filters rather than decorating the render, so `-m` no longer returns every replica for the one command whose purpose is to narrow to the broken ones. Flags that were parsed and then ignored are either wired or refused: --storage-pools filters, --limit caps, --pastable drops the borders, -o/--output-fmt selects or rejects, and --controllers says out loud that the cluster comes from the kubeconfig instead of silently reading a different one than the operator named. A bool flag with an inline value is honoured (`--force=false` disables) or rejected, rather than inverted or dropped. Also: no cell embeds a newline, which was splitting group rows mid-cell and breaking the awk contract the renderer documents; delete-property ignores a stray trailing positional instead of setting the key it was asked to remove; the size query dedups per node, skips missing pools and honours the pool list, so it stops promising placements the placer would refuse. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/review_fixes_test.go`:
- Around line 103-125: Extend TestNoCellBreaksTheRowLayout with a direct
table-renderer case containing a cell value such as “before\nafter”. Render the
table and assert the newline-containing value is sanitized into a single table
row, while preserving the existing row-layout validation.
- Around line 229-232: Strengthen the inline boolean coverage in the test around
app.Run: assert that --force=false is accepted and produces the expected
domain/non-usage failure rather than merely any non-zero exit, then add a
separate --force=true invocation that succeeds. Keep the existing newApp setup
and command arguments, changing only the assertions needed to distinguish parsed
false from invalid usage.
In `@internal/cli/table/table.go`:
- Around line 115-129: Update Options.line to construct pastable rows directly
from the cells and widths instead of post-processing the bordered output, so
literal " | " sequences within headers or cell values remain unchanged. Preserve
the existing alignment, trimming, color handling, and trailing-newline behavior
while removing the separator-based ReplaceAll transformation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9672ba0d-12c4-42f1-a703-4a2c94d83dda
📒 Files selected for processing (13)
internal/cli/app.gointernal/cli/definition.gointernal/cli/encryption.gointernal/cli/flags.gointernal/cli/handlers.gointernal/cli/place.gointernal/cli/props.gointernal/cli/review_fixes_test.gointernal/cli/table/table.gointernal/cli/view/resource.gointernal/cli/view/views.gointernal/cli/write_more.gointernal/cli/write_more_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- internal/cli/app.go
- internal/cli/handlers.go
- internal/cli/view/resource.go
- internal/cli/definition.go
- internal/cli/encryption.go
- internal/cli/write_more_test.go
- internal/cli/write_more.go
- internal/cli/props.go
- internal/cli/place.go
- internal/cli/flags.go
- internal/cli/view/views.go
| func TestNoCellBreaksTheRowLayout(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| app, out, errBuf := newApp(t, func(ctx context.Context, backend store.Store) { | ||
| _ = backend.ResourceGroups().Create(ctx, &apiv1.ResourceGroup{ | ||
| Name: "grp", | ||
| SelectFilter: apiv1.AutoSelectFilter{ | ||
| PlaceCount: 3, StoragePool: "data", LayerStack: []string{"DRBD", "STORAGE"}, | ||
| }, | ||
| }) | ||
| }) | ||
|
|
||
| if got := app.Run(t.Context(), []string{"rg", "l"}); got != 0 { | ||
| t.Fatalf("exit = %d (stderr: %s)", got, errBuf.String()) | ||
| } | ||
|
|
||
| for _, line := range strings.Split(strings.TrimRight(out.String(), "\n"), "\n") { | ||
| if !strings.HasPrefix(line, "|") && !strings.HasPrefix(line, "+") { | ||
| t.Errorf("row layout broken by a multi-line cell:\n%s", out.String()) | ||
|
|
||
| break | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Exercise an actual newline-containing cell.
Lines 107-112 seed only newline-free values, so this test can pass without validating newline sanitization. Add a direct table-renderer case with a cell such as "before\nafter" and assert it produces one table row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/review_fixes_test.go` around lines 103 - 125, Extend
TestNoCellBreaksTheRowLayout with a direct table-renderer case containing a cell
value such as “before\nafter”. Render the table and assert the
newline-containing value is sanitized into a single table row, while preserving
the existing row-layout validation.
| app, _, _ := newApp(t, seed) | ||
| if got := app.Run(t.Context(), []string{"vd", "s", "pvc-x", "0", "1G", "--force=false"}); got == 0 { | ||
| t.Error("--force=false enabled force") | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Distinguish false from an invalid inline flag.
At Line 230, any non-zero exit passes—including rejection of --force=false as invalid usage. Assert the expected non-usage/domain failure and add a --force=true case that succeeds, so the test proves inline booleans are parsed rather than rejected.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/review_fixes_test.go` around lines 229 - 232, Strengthen the
inline boolean coverage in the test around app.Run: assert that --force=false is
accepted and produces the expected domain/non-usage failure rather than merely
any non-zero exit, then add a separate --force=true invocation that succeeds.
Keep the existing newApp setup and command arguments, changing only the
assertions needed to distinguish parsed false from invalid usage.
| // line renders one row, bordered or bare. | ||
| func (o Options) line(cells, headers []string, widths []int, painted map[string]struct{}) string { | ||
| rendered := line(cells, headers, widths, painted, o.Color) | ||
| if !o.Pastable { | ||
| return rendered | ||
| } | ||
|
|
||
| // Strip the leading "| " and the pipe separators, leaving the | ||
| // alignment the widths already produced. | ||
| bare := strings.TrimPrefix(rendered, "| ") | ||
| bare = strings.ReplaceAll(bare, " | ", " ") | ||
| bare = strings.TrimSuffix(bare, " |\n") | ||
|
|
||
| return strings.TrimRight(bare, " ") + "\n" | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve cell values when rendering pastable rows.
At Line 125, ReplaceAll(" | ", " ") also rewrites literal | within a header or cell value. Build the bare row directly instead of post-processing the bordered representation.
Proposed fix
func (o Options) line(cells, headers []string, widths []int, painted map[string]struct{}) string {
- rendered := line(cells, headers, widths, painted, o.Color)
- if !o.Pastable {
- return rendered
- }
-
- bare := strings.TrimPrefix(rendered, "| ")
- bare = strings.ReplaceAll(bare, " | ", " ")
- bare = strings.TrimSuffix(bare, " |\n")
-
- return strings.TrimRight(bare, " ") + "\n"
+ if !o.Pastable {
+ return line(cells, headers, widths, painted, o.Color)
+ }
+
+ var bare strings.Builder
+ for i, cell := range cells {
+ if i > 0 {
+ bare.WriteString(" ")
+ }
+ rendered := cell
+ if _, ok := painted[headers[i]]; ok {
+ rendered = paint.PaintState(cell, o.Color)
+ }
+ bare.WriteString(rendered)
+ bare.WriteString(strings.Repeat(" ", widths[i]-displayWidth(cell)))
+ }
+ return strings.TrimRight(bare.String(), " ") + "\n"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // line renders one row, bordered or bare. | |
| func (o Options) line(cells, headers []string, widths []int, painted map[string]struct{}) string { | |
| rendered := line(cells, headers, widths, painted, o.Color) | |
| if !o.Pastable { | |
| return rendered | |
| } | |
| // Strip the leading "| " and the pipe separators, leaving the | |
| // alignment the widths already produced. | |
| bare := strings.TrimPrefix(rendered, "| ") | |
| bare = strings.ReplaceAll(bare, " | ", " ") | |
| bare = strings.TrimSuffix(bare, " |\n") | |
| return strings.TrimRight(bare, " ") + "\n" | |
| } | |
| // line renders one row, bordered or bare. | |
| func (o Options) line(cells, headers []string, widths []int, painted map[string]struct{}) string { | |
| if !o.Pastable { | |
| return line(cells, headers, widths, painted, o.Color) | |
| } | |
| var bare strings.Builder | |
| for i, cell := range cells { | |
| if i > 0 { | |
| bare.WriteString(" ") | |
| } | |
| rendered := cell | |
| if _, ok := painted[headers[i]]; ok { | |
| rendered = paint.PaintState(cell, o.Color) | |
| } | |
| bare.WriteString(rendered) | |
| bare.WriteString(strings.Repeat(" ", widths[i]-displayWidth(cell))) | |
| } | |
| return strings.TrimRight(bare.String(), " ") + "\n" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/cli/table/table.go` around lines 115 - 129, Update Options.line to
construct pastable rows directly from the cells and widths instead of
post-processing the bordered output, so literal " | " sequences within headers
or cell values remain unchanged. Preserve the existing alignment, trimming,
color handling, and trailing-newline behavior while removing the separator-based
ReplaceAll transformation.
|
Thanks — all nine hold up against the code. Nothing here was a false positive, and two of them were pinned the wrong way round by my own tests. Fixed in 7ec29b1. 1. Size bounds and overflow. Confirmed on both halves. 2. Flags parsed but never consumed. Confirmed — all five had zero readers. 3. 4. 5. Multi-line cell. Confirmed — 6. Confirmed, including the dead 7. Confirmed on all three counts. The query now dedups candidates per node (a node with three eligible pools still hosts one replica), skips 8. Confirmed. 9. Confirmed both ways. A value-less flag given an inline value now parses it as a boolean ( On the note: you are right and the comment was overclaiming. This runs client-side after the caller's own RBAC already let them read the Secret, so there is no remote attacker to time, and Each finding has a regression test in |
The size floor is a Minimum on the CRD field now, so a 1 KiB fixture is refused by the API server on seed rather than merely being unrealistic. Same treatment the store conformance fixtures got. Full integration suite verified locally against envtest. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM
Reviewed the diff against merge-base b873285c (71 files, +12931/-57). This is a large, genuinely well-built additive feature: a native CLI that speaks the CRDs directly. go build, go vet, go test ./... are green, make generate manifests (controller-gen v0.20.1) shows zero drift, the printer-column markers match the generated CRDs and their test, and the load-bearing contracts the PR body claims all hold when checked against code (constant-time passphrase via crypto/subtle, resize shrink-guard with bounds preserved under --force, explicit-place-fails vs group-spawn-defers, optimistic-concurrency decisions kept inside the store's fetch-mutate-patch closures). The design is careful and the test coverage is real, not theatre.
One blocking defect and a set of non-blocking notes below. The blocker is a data-restore path that reports success while producing a replica the satellite can never bring up, which the repo already knows is a real failure mode. It is a small fix.
Findings
[MAJOR] internal/cli/snapshot.go:450-473 (sourcePoolOn) and :428-442 (placeRestored): a restore whose source has no diskful replica carrying a pool silently creates a pool-less replica and exits 0. sourcePoolOn returns ("", nil) when no live replica of the source RD carries a StorPoolName (all replicas diskless, or the RD has zero replicas). placeRestored then calls stampProp(res, storPoolNameProp, ""), which internal/cli/resource.go:251-254 is a documented no-op on an empty value, so the restored replica is created diskful (no ResourceFlagDiskless) with no storage pool, Store.Resources().Create succeeds, and the verb returns success. There is no CRD validation requiring storagePool on a non-diskless Resource, so nothing rejects it. The repo already documents exactly this end state as a real, previously-hit bug at pkg/store/k8s/resources.go:88-92: "it stamped the clone replicas with an EMPTY StorPoolName and the satellite failed every reconcile with unknown storage pool "" (clone.sh never converges)". That earlier fix closed one cause (diskful replicas hidden by a label selector); sourcePoolOn reopens the same outcome for a different cause and does it silently. The trigger is the disaster-recovery case restore exists for: an operator captures a snapshot, the source later degrades to diskless-only or its replicas are removed, and blockstor snapshot resource restore is run against the surviving snapshot. sourcePoolOn's own doc comment claims it "still pins a backend" in this case; it does not. Not covered by tests: TestSnapshotResourceRestore (internal/cli/snapshot_test.go:60) always seeds source replicas with Props: {"StorPoolName": "data"}, so the empty-fallback branch is never exercised. Fix: return an error from sourcePoolOn when no pool resolves (let the operator supply --storage-pool), rather than ("", nil). I am rating this MAJOR rather than CRITICAL because no existing data is lost: the source and snapshot are untouched, only the freshly-created target is unusable, and it is recoverable by deleting and retrying with an explicit pool.
[MINOR] api/v1alpha1/resourcedefinition_types.go:161-165: SizeKib gains plain Minimum=4096 / Maximum=17179869184 on an existing served field, and the in-code claim "No path can produce an out-of-range volume, so there is nothing to ratchet for" is overstated for in-place upgrades. Before this PR the REST spawn fast path accepted any positive size ("We don't apply the full Bug 155 gate here"), so a pre-upgrade cluster can already hold a ResourceDefinition with a sizeKib in 1..4095 or >16 TiB. After the CRD upgrade, on Kubernetes without CRD validation ratcheting (pre-1.30 or the gate off), any spec write to such an object is rejected against the new bound, including writes that do not touch sizeKib, so the object becomes spec-unwritable. Mitigating it: such sizes are below/above DRBD's own floor/ceiling so those RDs were already non-functional, k8s >= 1.30 ratcheting skips unchanged-field validation, and Delete never validates spec so cleanup still works. This is why it is MINOR, not blocking. The clean shape is the optionalOldSelf CEL grandfather this very file already uses for drbdPort and the name rule; failing that, drop the "nothing to ratchet" wording and state the upgrade assumption explicitly.
[MINOR] internal/cli/snapshot.go:397-413 (hydrateVolumes) + :305-352 / :357-395 (restore verbs), and internal/cli/place.go:239-283 (resourceGroupSpawn): the restore and spawn verbs strand a half-created RD plus partial volume-definitions on a mid-loop store failure, with no rollback. snapshotRestoreResource creates the RD, then hydrateVolumes loops VolumeDefinitions().Create per volume with no unwind, then places replicas. A failure on volume 2 of N leaves the RD and volume 0 behind and never places replicas; the identical retry then fails on ErrAlreadyExists and needs a manual resource-definition delete first. This is the exact "half-restored" state the code's own comment at :373-375 says it wants to avoid, and the sibling snapshotCreateMultiple (:76-133) does unwind via rollbackSnapshots, so the discipline is inconsistent within the same file. The mid-loop failure is not hypothetical here: the size bound added by this same PR will reject an old snapshot whose recorded SizeKib is now out of range, and the code comments cite observed 409 conflicts on RD-create races. Recoverable, hence MINOR, but worth the same rollback the neighbours have.
[MINOR] pkg/store/store.go:322,344: the two new shared-interface methods land without store-conformance coverage. PhysicalDeviceStore.PatchPhysicalDeviceSpec and ControllerPropsStore.PatchProps are implemented twice (inmemory + k8s) and carry the non-trivial logic (retry-on-conflict, IsNotFound to store.ErrNotFound translation, the label-only-on-non-empty-NodeName behavior at pkg/store/k8s/physicaldevices.go:163-165), but the storetest suite has no PatchProps case and no PhysicalDevices runner at all. They are exercised only indirectly through the CLI tests, so a future divergence between the inmemory and k8s implementations would not be caught by the shared suite.
[MINOR] internal/cli/definition.go:156-183 (ensureCloneSnapshot): a clone can silently reuse a stale snapshot across a failed-then-retried attempt. The deterministic snapshot name is an intentional idempotency choice, but the Get-then-reuse-if-found check does not verify the found snapshot still matches the source's current volume layout. If a first clone takes the snapshot then fails later (e.g. in the un-rolled-back hydrateVolumes above) and the operator adds a volume to the source before retrying, the retry reuses the old snapshot and produces a target definition missing the new volume, with exit 0.
[NIT] Two small ones. First, the exit-code contract: errSizeOutOfBounds and errNoAutoShrink (internal/cli/write_more.go:308-312) are plain errors not wrapped in ErrUsage, so two purely client-side refusals (a sub-floor size, a shrink without --force) exit 10, while other equally-local rejections on the same verbs exit 2. The behavior is deliberate and pinned by TestSemanticRefusalExitCodes, and it matches upstream LINSTOR (semantic refusals come back as an API-level rc), so no code change is needed; the PR body's shorthand "2 = client-side rejection" is just looser than the code's real line ("2 = usage/grammar, 10 = operation refusal incl. semantic"). Worth tightening the wording. Second, pkg/store/inmemory_physicaldevice.go:128-151 (PatchPhysicalDeviceSpec) hands mutate a shallow copy whose pointer fields still alias the stored value, so an in-place mutation through those pointers followed by a mutate error would leak into the store despite the rollback appearance. The only current caller reassigns the pointer wholesale, so it is not triggered today; a defensive deep-copy would harden it before a second caller appears.
Checked and correct
go build ./...,go vet ./...,go test ./...green;make generate manifests(controller-gen v0.20.1) zero drift inapi/andconfig/; thezz_generated.deepcopy.gonet -2 lines is only the SPDX header, whichhack/boilerplate.go.txtdoes not carry, so it is a genuine regeneration, not a hand-edit.- CRD printer-columns match the
+kubebuilder:printcolumnmarkers andapi/v1alpha1/printcolumns_test.go; theSizeKibbound is consistent across marker, CRD YAML, and themin/maxVolumeDefinitionSizeKibcode constants. - Constant-time passphrase:
internal/cli/encryption.go:110,158both usesubtle.ConstantTimeCompare; the passphrase is not logged or embedded in an error. Resize:checkVolumeSizeruns before the shrink refusal, so--forcewaives the shrink but never the bounds, inside the patch closure that closes the concurrent-grow TOCTOU; both covered non-vacuously. Placement: explicit place passesbestEffort=false(errors on shortfall), group spawn/adjustbestEffort=true. Exit-code invariant holds: no genuine API error is ever reported as 2, no grammar error as 10. pkg/rest/spawn.gosize gate runs before anyStore.Create, so it cannot half-build an RD; the new store methods copy-then-mutate-then-commit-on-success and translate missing objects toErrNotFound; their only callers are the new CLI, so no direct blast radius on the running controller or REST.table.go/view/*traced nil/empty/short-row paths without a panic; dispatch/registry longest-match and flag parsing are sound.
sourcePoolOn returned ("", nil) when the source had no diskful replica
carrying a pool, and stamping an empty value is a documented no-op — so
the restore created a diskful replica with no storage pool, Create
accepted it, and the verb exited 0. Nothing rejects that object: the CRD
does not require the field. The satellite then fails every reconcile
with `unknown storage pool ""`, an end state this repository has already
been bitten by from a different cause.
The trigger is the case restore exists for: a snapshot outlives its
source's diskful replicas, and there is no pool left to infer. The
operator knows where it should land, so --storage-pool now takes
precedence and an unresolvable pool is refused rather than guessed.
The refusal lands after the definition was created, which exposed the
neighbouring gap: a restore that dies partway left its definition and
whatever volumes it had behind, turning the corrected retry into
"already exists". It unwinds now, the way snapshot create-multiple
already did. A rollback that itself fails does not replace the original
error.
A retried clone could also reuse a snapshot that no longer describes its
source: the deterministic name makes the retry idempotent, but "found"
is not "still right", and a volume added between attempts produced a
target silently missing it. Reused snapshots are checked against the
source's current layout and a stale one is refused — not re-taken, since
it may be the only copy of something.
The size bound goes back to Minimum/Maximum. The grandfathering shape
drbdPort uses is unavailable here: volumeDefinitions is an unkeyed list,
so the API server cannot correlate items across an update and rejects
oldSelf outright — envtest refuses to install the CRD at all. Making the
list correlatable would change merge semantics for every client of a
served API, which does not belong in this change, so the upgrade
assumption is stated on the field instead of claimed away.
The two store methods this PR added now have shared conformance
coverage, which earned itself immediately: it found that the envtest
wipe never cleared PhysicalDevices, and that this kind keeps everything
but AttachTo in status, so a spec round trip cannot carry it. The
in-memory patch also detached its pointer fields — a struct copy is
shallow, so a mutator editing through one of them reached the store
whether or not it went on to fail, making the rollback only apparent.
Each fix has a test verified to fail without it.
Assisted-By: Claude
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Thanks — all six findings are addressed in the latest push. Concurrent writes. Every write verb now goes through a fetch → mutate → patch cycle with retry-on-conflict instead of replacing a stale wire snapshot wholesale. The property accessor exposes Attach CAS guard. You were right that moving Restore with no resolvable pool. Size bounds. The bound landed as Ergonomics. Two regression tests cover the concurrency change specifically: one asserts a concurrent peer's key survives a property write, the other that a size change refuses to shrink against a concurrent grow. The behaviour was also measured against a live cluster before and after the change, driving twelve concurrent writers against one property bag: one key of twelve survived on the old write path, thirteen of thirteen on the new one. |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
REQUEST CHANGES
The CLI is a solid direction (dropping the GPL Python client, talking to the CRDs directly), and most of the surface is careful: constant-time passphrase compare, CAS-guarded resize, idempotent deletes, upgrade-safe CEL rules. Two things block merge, both on the destructive verbs, and both diverge from the REST/Python path this CLI claims parity with, in the dangerous direction.
The [CRITICAL] and [MAJOR] are silent data loss on a plausible operator mistake. The rest are correctness and ergonomics notes, inline.
Blocking
-
create-device-poolwipes a device that carries a live signature.stampDevicesonly refuses a device another pool already claimed and then stampsWipe: true; it never consultsFree/SignatureFoundorPhase. The REST handler for the same verb does (pkg/rest/physical_storage.go:508-528), and there is no downstream backstop — the satellite runswipefs --all --forceunconditionally on the flag (pkg/satellite/attach.go:80). A fat-fingered path, or a stale/dev/sdXafter a device-letter reshuffle, wipes a real disk. The Python client refuses. Inline detail on the line. -
toggle-diskdemotes the last diskful replica with no guard. On an already-diskful replica with no--storage-pool, it flips to diskless unconditionally — no last-diskful-replica check, no in-use check, no--force, exit 0. The satellite reclaims the sole backing volume (reconciler.go:1356/1437DeleteVolume). Upstream LINSTORtoggle-diskrefuses removing the last diskful replica. Inline detail on the line.
Recommended before dropping the Python client
- Add CLI-layer negative tests for both destructive verbs: a
create-device-poolagainst aSignatureFounddevice asserting the refusal, and atoggle-diskagainst a place-count-1 resource asserting the refusal. There is currently nophysical_test.go, andresource_test.goonly pins the flag flip. - Run
tests/e2e/cli-matrixagainst a stand pointed at the native binary, with a signatured-device case, before the Python client is removed.
The remaining findings (clone-snapshot staleness, controller version needing a kubeconfig, passphrase-on-argv, and the smaller ones) are inline.
| // the loser. The store's Update carried an | ||
| // equivalent guard against a snapshot; this one is | ||
| // evaluated inside the fetch-mutate-write window. | ||
| if dev.AttachTo != nil { |
There was a problem hiding this comment.
[CRITICAL] create-device-pool wipes a device carrying a live signature; the REST/Python path refuses
stampDevices matches a device by name and, inside the patch, refuses only a device another pool already claimed (dev.AttachTo != nil). It then stamps AttachTo with Wipe: true (hardcoded in attachRequest). It never consults the Free/SignatureFound condition or Phase.
The REST handler for the same verb does: pkg/rest/physical_storage.go:508-528 skips Phase != Available and refuses (returns the busy device) when dev.Free != nil && !*dev.Free; pkg/store/k8s/physicaldevices.go:255-274 documents that condition as existing for exactly that gate. Once Wipe: true is set there is no downstream backstop: pkg/satellite/attach.go:80 runs wipeDevice (wipefs --all --force, then pvcreate --force) unconditionally on the flag.
So an operator who names a device that unexpectedly holds a live filesystem / PV / zpool / DRBD signature (a fat-fingered path, or a stale /dev/sdX after a device-letter reshuffle) gets it wiped, whereas the Python client this CLI is at parity with returns the busy reason and refuses.
Fix: before stamping, refuse a device whose Free == false (surface FreeReason) and skip Phase != Available, mirroring pickAttachTargets; add a negative test with a SignatureFound device asserting the refusal.
| } | ||
|
|
||
| wasDiskless := slices.Contains(res.Flags, apiv1.ResourceFlagDiskless) | ||
| if !wasDiskless && run.Flags.Values["storage-pool"] == "" { |
There was a problem hiding this comment.
[MAJOR] toggle-disk demotes the last diskful replica to diskless with no guard (data loss)
toggle-disk <node> <rd> on an already-diskful replica with no --storage-pool calls setDiskless(...,true) unconditionally: no last-diskful-replica check, no in-use/Primary check, no --force, exit 0. The native CLI writes the CRD directly, so any REST-side guard is bypassed.
The satellite acts on the flag with no guard either: pkg/satellite/reconciler.go:1307-1361 (applyStorageIfDiskful, diskless branch) detaches DRBD, closes LUKS, then reclaimVolumesForDiskless (reconciler.go:1356/1437) calls provider.DeleteVolume, destroying the backing LV/zvol. No last-diskful / redundancy refusal exists on this path.
For a place-count-1 resource, one r td n1 res1 flips the only data-bearing replica to DISKLESS and the sole backing volume is reclaimed — data gone, reported success. Upstream LINSTOR toggle-disk refuses removing the last diskful replica; this CLI omits that guard. resource_test.go pins the flag flip but does not model replica count or the reconciler reclaim, so it does not make this case safe.
Fix: refuse the flip when it would remove the last diskful replica (require --force), mirroring upstream, and add a negative test on a place-count-1 resource.
| for _, wanted := range devices { | ||
| found := false | ||
|
|
||
| for i := range known { |
There was a problem hiding this comment.
[MINOR] device-match loop has no break; one token can stamp several devices
After found = true the inner loop keeps scanning, so a single operator token that matches more than one PhysicalDevice record (two records sharing a volatile CurrentDevPath after a /dev/sdX reshuffle, since deviceMatches compares CurrentDevPath) stamps Wipe: true on every match in one pass. This compounds the CRITICAL above. Fix: break after the first match, or refuse an ambiguous multi-match.
| if err != nil { | ||
| return fmt.Errorf("list volumes of %s: %w", src.Name, err) | ||
| } | ||
|
|
There was a problem hiding this comment.
[MODERATE] clone-snapshot staleness judged by volume count only, not size
checkCloneSnapshotIsCurrent compares only len(current) == len(snap.VolumeDefinitions) and returns nil on equality. A source resize leaves the count unchanged, so a reused clone snapshot taken before the resize passes the check and the retried rd clone materialises the target with the old layout, reporting success. The size data needed for a correct check exists but is unused (snap.VolumeDefinitions[i].SizeKib). Reachable via interrupted-clone → resize source → retry. Fix: also compare per-volume sizes.
| return a.fail(fmt.Errorf("%w: %w", command.ErrUsage, err)) | ||
| } | ||
|
|
||
| backend, err := a.StoreFor(ctx) |
There was a problem hiding this comment.
[MODERATE] dispatch opens the store (kubeconfig) for every command, incl. controller version
dispatch calls StoreFor(ctx) unconditionally before running any handler and fails if it errors. controllerVersion (internal/cli/write.go:266-274) only prints the version and never touches the cluster, but it never gets to run without cluster access: blockstor controller version on a host with no kubeconfig exits 10 instead of printing the version, breaking the standard binary sanity check in CI images. TestControllerVersion passes only because tests inject an in-memory store. Fix: exempt version / local-only commands from the store-open.
|
|
||
| if len(run.Flags.Positionals) > 0 && run.Flags.Positionals[0] != "" { | ||
| return run.Flags.Positionals[0], nil | ||
| } |
There was a problem hiding this comment.
[MINOR] cluster master passphrase accepted only via argv (shell history / /proc exposure)
encryptionPassphrase reads only Flags.Values["passphrase"] or the positional; there is no stdin/prompt/file path. So encryption create-passphrase / enter-passphrase take the cluster master key on the command line, where it lands in shell history and is visible in /proc/<pid>/cmdline to any local user for the duration of the call. Fix: accept the passphrase from stdin or an interactive prompt (or a file), and document argv as discouraged.
| // Explicit `--node-name` values win when the operator gave them. | ||
| func placeRestored(ctx context.Context, run *runContext, srcRD, rdName string, snap *apiv1.Snapshot) error { | ||
| nodes := run.Flags.Nodes | ||
| if len(nodes) == 0 { |
There was a problem hiding this comment.
[MINOR] restore with an empty node set places zero replicas and exits 0
placeRestored falls back to snap.Nodes and iterates; if both --node-name and snap.Nodes are empty (a Snapshot CR not written through this CLI's hydrateSnapshot, or a degenerate one) the loop runs zero times and the restore reports success with no replicas and no data. Same for an empty hydrated VolumeDefinitions. This is the silent-success-with-no-data case hydrateSnapshot refuses on the create side via errNothingToCapture; the restore side has no matching guard. Fix: refuse an empty node set (and empty volume set).
| for i := range snap.VolumeDefinitions { | ||
| svd := &snap.VolumeDefinitions[i] | ||
|
|
||
| err := run.Store.VolumeDefinitions().Create(ctx, rdName, &apiv1.VolumeDefinition{ |
There was a problem hiding this comment.
[MINOR] restore onto an existing definition leaves a partial volume set on failure
snapshotRestoreVolumeDefinition pre-checks number collisions before writing, but hydrateVolumes then creates volumes one at a time with no unwind. A mid-loop Create failure (transient store error, or a concurrent create after the pre-check) leaves the pre-existing definition carrying a partial subset of the snapshot's volumes. snapshotRestoreResource rolls back its own RD; this variant operates on an RD it does not own and does not remove the volumes it added. Fix: track and delete the volumes this call added on failure, or document the window.
| // in-cluster service-account namespace when running as a pod, the | ||
| // BLOCKSTOR_NAMESPACE override otherwise, and the deployment default | ||
| // last. | ||
| func namespace() string { |
There was a problem hiding this comment.
[MINOR] namespace() comment contradicts the precedence the code implements
The doc says the in-cluster service-account namespace applies when running as a pod and BLOCKSTOR_NAMESPACE is the override otherwise, but the code checks BLOCKSTOR_NAMESPACE first (env wins even inside a pod), then the SA file, then the default. Env-first is a sensible precedence; the comment describes a different order and will mislead an operator debugging where the passphrase Secret is resolved. Fix: reword the comment to match env → SA file → default.
create-device-pool stamped an attach carrying Wipe: true after checking only whether another pool had already claimed the device. It never consulted the satellite's Free condition or the device phase, so a path typed from the wrong host's lsblk, or a stale /dev/sdX after a device-letter reshuffle, wiped a disk that was in use. The REST handler for the same verb gates on those signals; this path writes the CRD directly and bypassed it. The gate now lives inside the patch, so it is evaluated against the state the write lands on, and a token that matches several device records is refused rather than stamping every match. toggle-disk demoted a replica to diskless with no guard at all. The satellite reconciles that flag by detaching DRBD and deleting the backing volume, so the bare form took the last copy of the data with it. It now refuses the last diskful replica and a replica a consumer still holds open, with --force as the documented override, matching upstream LINSTOR. Both refusals ship tests that go red against the pre-fix code, and the acceptance cases pin that the gates did not become blanket denials: a free device still attaches, a device with no discovery verdict still attaches so a bootstrap is not blocked, and a demotion still works wherever the definition keeps another copy. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
A reused clone snapshot was judged current by volume count alone, so a source that had been resized since the snapshot was taken still matched and the retry materialised the target at the old size, reporting success. The check now compares the captured sizes too, keyed by volume number rather than position. `controller version` opened the store before running, so it exited 10 on a host with no kubeconfig — the sanity check a CI image runs on a freshly built binary, before any cluster exists. Commands that answer from the binary alone are exempt from the store open. A restore that resolved no nodes walked its loop zero times and reported success with no replicas and no data, and a restore onto a definition it does not own left the volumes it had already created behind when one failed midway. Both are refused and unwound now, matching the guard the capture side already had. The cluster master passphrase could only be given on the command line, where it lands in shell history and stays readable in /proc/<pid>/cmdline. Omitting it now reads from stdin — without echo on a terminal, as a plain line when piped or redirected. Also corrects the namespace() comment, which described the opposite of the precedence the code implements. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
CI's gosec flags the uintptr-to-int conversion os.File.Fd forces on the terminal check. A range check answers it without a nolint directive, which the local linter would call unused: a value that does not fit is not a descriptor any syscall returned, so falling through to the plain-line path is the correct reading of that input anyway. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Thanks — both blocking findings were real, and the CRITICAL one was a genuine hole rather than a missing belt on an existing brace. create-device-pool. The gate is in, and it lives inside the patch closure so it is evaluated against the state the write lands on rather than the list read up front: a device another pool claimed, a device not in the Available phase, and a device whose satellite-stamped toggle-disk. It refuses the last diskful replica and a replica a consumer still holds open, with Both refusals ship tests that go red against the pre-fix code. The acceptance cases are pinned too, because the risk with a gate like this is that it quietly becomes a blanket denial: a free device still attaches, a device with no discovery verdict still attaches, and a demotion still works wherever the definition keeps another copy. The existing The rest: the clone-snapshot check compares per-volume sizes keyed by volume number, not just cardinality; |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed at f172da4 (current HEAD, includes today's three fixes). The write-path core is solid — RetryOnConflict actually re-reads, ParseSize overflow is handled, exit codes are consistent, no wholesale Update — and build/vet/go test ./.../golangci-lint are green. What blocks is that the new CLI writes CRDs directly and, on two paths, re-introduces already-fixed bugs that the REST layer guards against. Both fail silently (exit 0) and leave the satellite hanging.
[MAJOR] internal/cli/write_more.go:379-388 (plus promote resource.go:141-150, migration-target resource.go:243-254) — empty storage pool on a diskful replica, re-introducing Bug 364
The CLI sets res.Props["StorPoolName"] only when --storage-pool is given; otherwise Props stays empty. The REST handler for the same verb resolves an omitted pool via resolveStorPoolForFreshCreate (autoplace.go:1574), taking RG.SelectFilter.StoragePoolList[0] — the fix added specifically for Bug 364, with a regression test. The store does no defaulting, and resolveStorPoolForFreshCreate is never called from internal/cli/. The satellite fails unknown storage pool "" (reconciler.go:1473) and the replica hangs in Provisioning. blockstor r c dev-worker-1 pvc-1 against an RG-with-StoragePoolList → exit 0 plus a phantom replica. Same failure on promote (stampProp with an empty pool is a no-op) and migration target. Note aad6709 closed only the restore path — sourcePoolOn in snapshot.go refuses exactly this state for restore, so create/promote/migrate now contradict the PR's own guard.
[MAJOR] internal/cli/write_more.go:43-50, :197-203 — negative volume numbers, re-introducing Bug 365
parseInt32 uses strconv.ParseInt(text, 10, 32), which accepts any int32 including negatives (the comment only guards against wrap, not sign), and --vlmnr flows straight into vd.VolumeNumber with no lower bound. REST rejects this explicitly with ErrVolumeNumberBelowMinimum (volume_definitions.go:522; the comment at :509 notes the devnode derived from a negative VlmNr hangs the satellite). blockstor vd c pvc-1 1G --vlmnr -3 revives the fixed bug on a parallel write-path. Same permissiveness on --vlmnr for volume-group create (pool.go).
[MINOR] internal/cli/physical.go:159-187 — create-device-pool: no unwind on partial failure, and a correct retry is locked out
Note 376423f closed the data-loss vector here by gating on Free/phase, so this is no longer a destroy-data finding. What remains: stampDevices resolves and patches devices one at a time, and StoragePool is created only after the loop. A typo on the Nth device leaves the first ones stamped with no pool, and a corrected re-run is refused because claimDevice (:234-237) sees dev.AttachTo != nil — the operator is stuck without a manual CR edit. REST validates every target via pickFreeDeviceForAttach before the first write, and this same PR does validate-all-before-write in resourceGroupSpawn (place.go:253-259). Suggest hoisting the resolve out of the loop.
[MINOR] internal/cli/write_more.go:127-134 — node create does not validate the address (REST has nodes_bug120_address_validation); --port via strconv.Atoi with no range, silently truncated on the int32 cast (nodes.go:269). --port 4294970000 stores 2704, exit 0.
[MINOR] internal/cli/place.go:62 — a negative/nonsensical --place-count succeeds silently as a no-op (if filter.PlaceCount <= 0 { return nil }); REST validates it (bug_367). A typo'd counter yields 0 replicas and exit 0.
[MINOR] internal/cli/table/table.go — values coming from the CRD (arbitrary, via set-property or the satellite's FreeMessage) are rendered verbatim; a |, newline, or ANSI in a value breaks the documented awk -F'|' contract and injects escapes into the terminal even under --color=never. The CLI's own ANSI is kept out of the width math correctly; incoming values are not stripped.
[MINOR] bin/controller-gen — the symlink was retargeted to an absolute path inside a disposable worktree (.claude/worktrees/blockstor-cli/bin/controller-gen-v0.20.1). Broken for everyone, and for the author once the worktree is cleaned up; breaks make generate.
[MINOR] internal/cli/write_more.go:67 — ParseSize accepts 16GiB/16Gi/16GB but rejects 16gib/16Gib (trailing b is stripped first, leaving 16gi, byte-unit i unrecognised). Separately 512B reads as 512 KiB (×1024), usually caught by the 4-MiB floor.
Requesting changes on the two MAJORs (Bug 364 / Bug 365 re-introduction). The rest are minors. Nice work on the write-path core.
…date input A diskful replica created without --storage-pool was written with an empty Props["StorPoolName"]. The satellite binds on that field and fails with `unknown storage pool ""`, leaving the replica in Provisioning for good; the REST path resolves an omitted pool from a diskful sibling and then from the resource group's SelectFilter, where linstor-csi lands the storage class's pool name. This CLI writes the CRD directly and carried no resolution at all, so the same chain now runs on create, on promotion, and on a migration target. A volume number was parsed as any int32. A negative one reaches the CRD, the satellite derives a device node from it, and the replica hangs waiting for a DRBD-ID that can never be allocated. Bounded to DRBD-9's addressable range on both paths that accept --vlmnr, matching the REST validator. create-device-pool resolved and stamped devices one at a time, so a typo in the last of several names left the earlier ones claimed by a pool that was never created — and the corrected re-run was then refused because those devices looked like someone else's claim. Every name is resolved before the first write now. Also: a place count below one is refused rather than reported as a completed placement that made no replicas; a node address is checked and a satellite port is held inside the TCP range instead of being truncated by the int32 cast; table cells carrying a pipe, a newline or an escape sequence are rendered inert, since they come from operator- set properties and satellite messages and otherwise forge a column, a row, or terminal escapes under --color=never; and ParseSize accepts the lowercase suffix spellings and reads a byte-denominated size as bytes rather than as KiB. The bin/controller-gen symlink is relative again. It pointed at an absolute path — into a disposable worktree after the last round, and into one developer's checkout before that — which broke `make generate` for everyone else. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Both blocking findings were re-introductions, and you were right to flag them as such rather than as new bugs. Bug 364. The omitted-pool resolution is in, following Bug 365. Volume numbers are bounded to DRBD-9's addressable range on both paths that take create-device-pool. Every name is resolved before the first write. You are right that the partial-stamp was the worse half of that finding: the operator was left with devices claimed by a pool that never existed and a corrected re-run refused, needing a manual CR edit to escape. The minors are all in: a place count below one is refused instead of reported as a completed placement; the node address is validated and the satellite port bounded rather than truncated by the int32 cast; table cells carrying a pipe, a newline or an escape sequence are rendered inert;
One thing this round surfaced that is outside the PR: |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
NOT LGTM
Everything below was reproduced by running it. Where I state an exit code or a stored value, that is observed output, not a reading of the code. The two API-server findings were reproduced against a real apiserver via envtest, not against a fake client.
These rounds keep landing on the same thing. Each round finds another invariant that exists on the REST edge and not on the CLI path, because the CLI writes CRDs directly and imports nothing from pkg/rest. Moving the size bound into the CRD was the right shape of answer, since it holds for every writer including kubectl apply. The findings below are the remaining instances of the same class, and two of them are destructive.
New findings
[CRITICAL] internal/cli/props.go:75 storage-pool set-property rewrites and deletes the immutable StorDriver keys
REST refuses these before writing, via refuseSPDriverPropMutation (pkg/rest/storage_pools.go:813), whose own comment names the consequence: the pool silently moves to a different or nonexistent backend while every replica keeps reporting UpToDate. The CLI edits pool.Props through PatchStoragePoolSpec with no key filter; the string immutable does not appear anywhere under internal/cli.
All six keys, three arms each, all exit 0:
sp set-property node-1 data StorDriver/LvmVg hijacked exit=0, props now map[StorDriver/LvmVg:hijacked]
sp set-property node-1 data StorDriver/LvmVg "" exit=0, props now map[]
sp delete-property node-1 data StorDriver/LvmVg exit=0, props now map[]
Identical for StorPoolName, ThinPool, ZPool, ZPoolThin, FileDir. Feeding the same patches to refuseSPDriverPropMutation returns refused for all six on both the override and the delete arm.
[CRITICAL] the layer stack is not validated on any CLI path: internal/cli/write.go:235, definition.go:94, write_more.go:594
splitList only trims. pkg/rest/layer_validation.go:60 holds the allowlist, the terminal-STORAGE rule, DRBD at index 0, the duplicate check and LUKS-below-DRBD. The CRD field is a bare []string with no enum and no CEL, so there is no server-side backstop either.
Five stacks accepted with exit 0 and stored verbatim: DRDB,STORAGE, DRBD,LUSK,STORAGE, LUKS,DRBD,STORAGE, STORAGE,DRBD, CACHE,STORAGE. REST refuses all five. rd modify --layer-list DRDB,STORAGE also exits 0 and overwrites a live DRBD,STORAGE.
The harm, measured on the satellite predicates rather than assumed:
needsDRBD([DRDB STORAGE]) = false
needsLUKS([DRBD LUSK STORAGE]) = false
A one-character typo therefore turns replication off: no .res is rendered, drbdadm never runs, and the volume comes up as a single local copy while the operator believes it is replicated. The LUKS typo turns encryption off the same way. Both predicates ignore ORDER, so LUKS,DRBD,STORAGE and STORAGE,DRBD do not produce a wrong data path; for those two the finding is the CLI and REST asymmetry only. The typo and unknown-layer cases are the dangerous ones.
[MAJOR] api/v1alpha1/resourcedefinition_types.go:178 the new Minimum does not ratchet, because volumeDefinitions is an atomic list
This corrects my own MINOR from the 27 Aug round, where I rated it non-blocking on the grounds that k8s >= 1.30 ratcheting skips validation for fields an update does not change. Ratcheting correlates per field, and volumeDefinitions carries no x-kubernetes-list-type, so it is atomic and the comparison is over the whole slice.
Against a real apiserver (envtest, k8s 1.35):
bounds removed from the live CRD, pre-upgrade RD written with volume 0 at 1024 KiB, bounds restored
update that leaves volumeDefinitions alone (adds spec.props): accepted
update that appends volume 1 at 8192 KiB, well inside the bounds:
ResourceDefinition "probe13-legacy" is invalid: spec.volumeDefinitions[0].sizeKib:
Invalid value: 1024: spec.volumeDefinitions[0].sizeKib in body should be greater than or equal to 4096
The rejection names volume 0, which the operator did not touch. Any grandfathered sub-floor volume makes every subsequent write that touches the list fail, and the controller itself writes that list: patchRDVolumeMinors rewrites spec.volumeDefinitions to stamp drbdMinor. The CRD comment already knows the ratchet form is unavailable here because the list is unkeyed; the conclusion drawn from that needs to flip.
The upper bound deserves its own line. 16 TiB is below what DRBD 9 and upstream LINSTOR handle, pkg/linstormigrate/convert.go copies sizes verbatim, and before this PR rg spawn accepted any positive size. A LINSTOR cluster holding a 20 TiB volume now fails at apply time during migration.
[MAJOR] pkg/rest/snapshot_restore.go:477 a restore from a sub-floor snapshot leaves a half-built RD
The Snapshot CRD has no bounds on sizeKib, so a pre-upgrade snapshot recording 1024 KiB stores and lists fine. Restoring from it now fails, after the target definition has been created:
POST /v1/resource-definitions/probe14-src/snapshot-restore-resource/probe14-snap
status=500 store error: update RD "probe14-dst" to add volume 0: ... sizeKib: Invalid value: 1024
half-built target: RD "probe14-dst" exists with 0 volumeDefinitions
retry against the orphan: status=409 resource definition "probe14-dst": object already exists
No reconciler removed the orphan, and the retry is refused, so there is no recovery path short of deleting it by hand. This is the exact shape the new comment in pkg/rest/spawn.go says the validate-before-write block exists to prevent; spawn got that treatment and the restore handlers did not.
[MAJOR] internal/cli/physical.go:113 create-device-pool lvmthin names the VG differently from REST
REST runs the pool name through splitLvmThinPoolName: vg/thin splits on the slash, and a bare name gets the upstream linstor_ prefix. The CLI writes the token into both fields untouched:
--pool-name data CLI VGName=data ThinPoolName=data REST: linstor_data / data
--pool-name vg/thin CLI VGName=vg/thin ThinPoolName=vg/thin REST: vg / thin
The satellite branches on vgExists(dev.AttachTo.VGName) between vgcreate and vgextend. For a pool created earlier through the Python client the host carries linstor_data, so adding a disk with this CLI creates a second VG named data on that disk instead of extending the existing one, and the disk is consumed without capacity reaching the pool. With --pool-name vg/thin the slash also ends up in the StoragePool object name. The CLI is inconsistent with itself here: storage-pool create does split the slash.
[MAJOR] internal/cli/physical.go:86 create-device-pool adopts a foreign pool and reports success
The pool-creation leg swallows AlreadyExists, so an existing pool of a different provider is silently taken over while the device is stamped for the requested one:
exit=0, stderr empty
device AttachTo={StoragePoolName:data ProviderKind:ZFS ZPoolName:data Wipe:true}
registered pool: kind="LVM" props=map[StorDriver/LvmVg:someone-elses-vg]
The device is marked for wiping under a ZFS pool named data, while node-1/data is thick LVM over someone else's VG.
[MAJOR] internal/cli/write.go:258 and the other delete verbs: no cascade, and nothing to inherit one from
REST returns 409 while snapshots reference an RD and then runs cascadeDeleteResources; its comment explains that without the cascade the children never get a DeletionTimestamp, the satellite finalizer never runs, drbdadm down never happens, and kernel state survives to collide with the next definition of the same name. Observed:
rd delete pvc-p12 exit=0, RD gone, 2 orphaned replicas, 1 orphaned snapshot, 1 orphaned volume definition
node delete node-1 exit=0, 1 replica, 1 storage pool, 1 physical device still reference the node
sp delete node-1 data exit=0, pool gone, replica still carries StorPoolName="data"
There is nothing to inherit the cascade from: grepping SetControllerReference|SetOwnerReferences|OwnerReferences across pkg/store, internal/controller and pkg/satellite returns no non-test hit. node lost in this same PR does cascade correctly, so the pattern is already here.
[MAJOR] internal/cli/definition.go:117 clone skips the REST preconditions
REST holds cloneTargetPreexists, cloneSnapshotPreconditionsHold and clonePoolsSupportSnapshots (pkg/rest/rd_clone.go). Cloning from a pool with SupportsSnapshot:false exits 0 and creates the internal snapshot and the clone definition, where REST answers 400. Cloning onto an occupied target takes the internal snapshot first and only then fails on the create, leaving clone-occupied behind; REST refuses before the first write.
[MAJOR] internal/cli/pool.go:72 storage-pool create validates neither the node nor the advertised backing
sp c lvm ghost-node data vg0 exit=0, node inventory empty, pool persisted on "ghost-node"
sp c lvm node-1 data vg-nonexistent exit=0, node advertises DiscoveredVGs="vg-real"
REST answers 404 for the unknown node and, for the second, no VG named "vg-nonexistent" on node "node-1", satellite advertised VGs: "vg-real". The CLI registers a pool the satellite can never reconcile, and it lists in sp l looking real with empty capacity.
[MAJOR] internal/cli/write.go LUKS definitions are accepted with no passphrase
rd create x -l DRBD,LUKS,STORAGE exits 0 on a cluster where encryption create-passphrase was never run and the controller property bag is empty. REST refuses via refuseLUKSWithoutPassphrase. The Secret-reading machinery is already in this PR, in internal/cli/encryption.go.
Still open from my earlier rounds
These are not new. Items 1 to 4 are from the 10 Aug round and are still reproducible on the current head.
- [CRITICAL] restore onto nodes that do not hold the snapshot.
placeRestored(internal/cli/snapshot.go:483) takes--nodesverbatim:restore --nodes node-9withsnap.Nodes=[node-1]exits 0 and the replica lands on node-9. REST refuses throughvalidateRestoreNodesHoldSnapshot, with a dedicated regression test carrying the data-integrity rationale. - [CRITICAL] resource delete bypasses the U130 guard.
r delete node-1 pvc-p9without--forceexits 0 and leavesnode-2in SyncTarget with no source. Feeding the identical structs toresourceMidSyncDeleteRefusalreturns true, so REST would answer 409 on the same input. - [CRITICAL] create-device-pool stamps devices before the pool exists, and the retry is locked out. Confirmed by ordering rather than by reading: when the stamp fails the pool does not exist at all, which is only possible if the stamp runs first. An identical re-run against the command's own half-finished state is refused with
device /dev/sdb on node-1 is already attached, so a corrected retry needs a manual CR edit. - [CRITICAL] migrate-disk accepts src equal to dst, and it is worse than I described.
r toggle-disk node-1 pvc-p6 --migrate-from node-1exits 0 and stampsBlockstorMigratingFrom:node-1. Driving the reconciler then shows it resolve the source to the same object and prune it:pvc-self.node-1 not foundafterpruneSrc. The only diskful replica deletes itself once its volumes reach UpToDate, and the reconcile afterwards retries forever against the object it removed. No--forceanywhere on that path. - [MAJOR] a value flag swallows the next flag.
resource list -n --faulty -mand the long form both giveNodes=["--faulty"], an empty result and exit 0. Control on the same seed:resource list --faulty -mreturns the Inconsistent replica, so the empty result above is the swallowed value and not the filter working.
Scope note
I deliberately left out findings I could not confirm by execution in this round, including the identifier-validation gap and the listing round-trip counts. They are real enough to look at, but they are not in this review because I did not run them.
| // `<node> <pool>`. | ||
| // | ||
| //nolint:gochecknoglobals,dupl // static accessor table; the parallel shape is the point | ||
| var storagePoolProps = objectProps("storage pool", 2, // (node, pool) |
There was a problem hiding this comment.
[CRITICAL] storage-pool set-property rewrites and deletes the immutable StorDriver keys
REST refuses these before writing via refuseSPDriverPropMutation (pkg/rest/storage_pools.go:813), whose comment names the consequence: the pool silently moves to a different or nonexistent backend while every replica keeps reporting UpToDate. This accessor edits pool.Props with no key filter, and the string immutable does not appear anywhere under internal/cli.
All six keys, three arms each, all exit 0:
sp set-property node-1 data StorDriver/LvmVg hijacked exit=0, props now map[StorDriver/LvmVg:hijacked]
sp set-property node-1 data StorDriver/LvmVg "" exit=0, props now map[]
sp delete-property node-1 data StorDriver/LvmVg exit=0, props now map[]
Feeding the same patches to refuseSPDriverPropMutation returns refused for all six on both arms.
| } | ||
|
|
||
| if layers := run.Flags.Values["layer-list"]; layers != "" { | ||
| def.LayerStack = splitList(layers) |
There was a problem hiding this comment.
[CRITICAL] the layer stack is not validated on any CLI path
splitList only trims, and the CRD field is a bare []string with no enum and no CEL, so there is no server-side backstop either. pkg/rest/layer_validation.go:60 holds the allowlist and the ordering rules and refuses all five stacks below; the CLI stores them verbatim with exit 0: DRDB,STORAGE, DRBD,LUSK,STORAGE, LUKS,DRBD,STORAGE, STORAGE,DRBD, CACHE,STORAGE. rd modify also overwrites a live DRBD,STORAGE with DRDB,STORAGE.
Measured on the satellite predicates:
needsDRBD([DRDB STORAGE]) = false
needsLUKS([DRBD LUSK STORAGE]) = false
A one-character typo turns replication off (no .res, drbdadm never runs) or turns encryption off. Both predicates ignore order, so the two ordering cases are a CLI and REST asymmetry only, not a wrong data path.
| // installed base ever does carry one, the list has to become | ||
| // correlatable before this can ratchet. | ||
| // | ||
| // +kubebuilder:validation:Minimum=4096 |
There was a problem hiding this comment.
[MAJOR] the new Minimum does not ratchet, because volumeDefinitions is an atomic list
This corrects my own MINOR from the 27 Aug round, where I rated it non-blocking because k8s >= 1.30 ratcheting skips validation for unchanged fields. Ratcheting correlates per field, and volumeDefinitions carries no x-kubernetes-list-type, so it is atomic and the comparison covers the whole slice.
Against a real apiserver (envtest, k8s 1.35), with a grandfathered volume 0 at 1024 KiB:
update that leaves volumeDefinitions alone (adds spec.props): accepted
update that appends volume 1 at 8192 KiB, well inside the bounds:
spec.volumeDefinitions[0].sizeKib: Invalid value: 1024: ... should be greater than or equal to 4096
The rejection names the volume the operator did not touch, and patchRDVolumeMinors rewrites this same list to stamp drbdMinor. The comment already knows the ratchet form is unavailable here; the conclusion drawn from that needs to flip.
| attach.ZPoolName = poolName | ||
| case strings.HasPrefix(token, "file"): | ||
| attach.Directory = poolName | ||
| case strings.HasPrefix(token, "lvm"): |
There was a problem hiding this comment.
[MAJOR] lvmthin names the VG differently from REST for the same command
REST runs the pool name through splitLvmThinPoolName: vg/thin splits on the slash and a bare name gets the upstream linstor_ prefix. Here the token goes into both fields untouched:
--pool-name data CLI VGName=data ThinPoolName=data REST: linstor_data / data
--pool-name vg/thin CLI VGName=vg/thin ThinPoolName=vg/thin REST: vg / thin
The satellite branches on vgExists(dev.AttachTo.VGName) between vgcreate and vgextend. For a pool created earlier through the Python client the host carries linstor_data, so adding a disk here creates a second VG named data on that disk instead of extending the existing one, and the capacity never reaches the pool. storage-pool create in this same PR does split the slash.
| } | ||
|
|
||
| err = run.Store.StoragePools().Create(ctx, pool) | ||
| if err != nil && !isAlreadyExists(err) { |
There was a problem hiding this comment.
[MAJOR] create-device-pool adopts a foreign pool and reports success
Swallowing AlreadyExists here takes over an existing pool of a different provider while the device is stamped for the requested one:
exit=0, stderr empty
device AttachTo={StoragePoolName:data ProviderKind:ZFS ZPoolName:data Wipe:true}
registered pool: kind="LVM" props=map[StorDriver/LvmVg:someone-elses-vg]
The device is marked for wiping under a ZFS pool named data while node-1/data is thick LVM over someone else's VG.
|
|
||
| // storagePoolCreate implements `storage-pool create <provider> <node> | ||
| // <pool> [<backing>]`. | ||
| func storagePoolCreate(ctx context.Context, run *runContext) error { |
There was a problem hiding this comment.
[MAJOR] storage-pool create validates neither the node nor the advertised backing
sp c lvm ghost-node data vg0 exit=0, node inventory empty, pool persisted on "ghost-node"
sp c lvm node-1 data vg-nonexistent exit=0, node advertises DiscoveredVGs="vg-real"
REST answers 404 for the unknown node and, for the second, no VG named "vg-nonexistent" on node "node-1", satellite advertised VGs: "vg-real" via refuseUnknownBackingStorage. The pool registered here can never be reconciled, and it lists in sp l looking real with empty capacity.
| // replica on a different backend makes the satellite pipe the | ||
| // snapshot stream into the wrong receiver, which never converges. | ||
| // Explicit `--node-name` values win when the operator gave them. | ||
| func placeRestored(ctx context.Context, run *runContext, srcRD, rdName string, snap *apiv1.Snapshot) error { |
There was a problem hiding this comment.
[CRITICAL] still open from 10 Aug: restore onto nodes that do not hold the snapshot
Not a new finding: this is item 3 of my 10 Aug round, still reproducible on this head. --nodes is taken verbatim:
restore --nodes node-9 while snap.Nodes=[node-1]: exit=0, replica landed on node-9
REST refuses through validateRestoreNodesHoldSnapshot, which carries the data-integrity rationale and a dedicated regression test (pkg/rest/bug_397_restore_snapshotless_node_test.go). The comment above this function states that explicit values win, which is the behaviour REST treats as the defect.
|
|
||
| // resourceDelete implements `resource delete <node> <rd>`, | ||
| // idempotently. | ||
| func resourceDelete(ctx context.Context, run *runContext) error { |
There was a problem hiding this comment.
[CRITICAL] still open from 10 Aug: resource delete bypasses the U130 guard
Item 2 of my 10 Aug round, still reproducible. A plain Delete in a loop, no sibling scan, no --force:
r delete node-1 pvc-p9 (no --force): exit=0, replicas left: [node-2=SyncTarget]
The SyncTarget lost its only source. Feeding the identical structs to resourceMidSyncDeleteRefusal returns true, so REST would answer 409 on the same input.
| // SOURCE IS LEFT ALONE. The migration reconciler removes it only once | ||
| // the destination's volumes are UpToDate, so redundancy holds for the | ||
| // whole resync instead of dropping for its duration. | ||
| func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error { |
There was a problem hiding this comment.
[CRITICAL] still open from 10 Aug: migrate-disk accepts src equal to dst, and it is worse than I described
Item 1 of my 10 Aug round. There is no src/dst comparison here or in REST:
r toggle-disk node-1 pvc-p6 --migrate-from node-1: exit=0
props=map[BlockstorMigratingFrom:node-1 StorPoolName:data]
Driving ResourceMigrationReconciler afterwards shows it resolve the source to the same object and prune it: pvc-self.node-1 not found after pruneSrc. The only diskful replica deletes itself once its volumes reach UpToDate, and the reconcile then retries forever against the object it removed. The diskless-source and Primary-InUse guards added since that round do not cover this.
| } | ||
|
|
||
| i++ | ||
| value = args[i] |
There was a problem hiding this comment.
[MAJOR] still open from 10 Aug: a value flag swallows the next flag
Item 7 of my 10 Aug round. The value is taken from the next argument with no check for a leading dash:
resource list -n --faulty -m exit=0, stdout=[[]]
resource list --nodes --faulty -m exit=0, stdout=[[]]
Nodes becomes ["--faulty"], the filter matches no node, and --faulty is never applied. Control on the same seed: resource list --faulty -m returns the Inconsistent replica, so the empty result is the swallowed value rather than the filter working. A script pages through an empty list and gets exit 0.
Each review round has found another rule that exists on the REST edge and not on the CLI path, because the CLI writes the CRDs directly and imports nothing from pkg/rest. Rather than copy the rules a fourth time, they move to pkg/validate and both doors delegate: a rule enforced on one door only is not a rule, it is a habit of that door, and the other one is the way around it. The layer stack was unchecked on every CLI path. The satellite asks whether the stack CONTAINS a layer, so an unrecognised token is not a loud failure downstream — it is a silently absent layer. `DRDB,STORAGE` brought the volume up as a single local copy while the operator believed it was replicated, and `DRBD,LUSK,STORAGE` wrote plaintext. The field carries no enum and no CEL rule, so there was no backstop anywhere on that path. `migrate-disk` accepted the same node as source and destination. The reconciler resolves both ends to one object, waits for the destination to reach UpToDate — which it already is — then prunes the source, deleting the replica it was asked to move. There is no --force for this: no argument makes that the intended outcome. The immutable StorDriver keys were editable through set-property and delete-property. Rewriting one does not migrate anything: the pool keeps its name and its replicas keep reporting UpToDate while the driver points somewhere else. The guard compares the property bag before and after the edit rather than inspecting the requested operation, so set, delete and delete-namespace are covered by one rule instead of three. A restore onto nodes that do not hold the snapshot created a replica with nothing behind it and reported success. Deleting the last complete copy while a peer was still syncing from it stranded that peer — upstream U130, which REST has answered 409 on since it was written; --force overrides. create-device-pool stamped the devices before registering the pool, so a typo in the last of several names left the earlier disks claimed by a pool that never existed, and the corrected re-run met its own leftovers. The pool is registered first now, and a device already attached to the pool being built is treated as done rather than as someone else's claim. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…size bound The sizeKib bound comes off the CRD. spec.volumeDefinitions carries no list-map key, so it is an atomic list and ratcheting compares it whole: any update that touches the list re-validates every element, including ones written before the bound existed. One grandfathered sub-floor volume would therefore reject every later write to its definition — the controller's own included, since patchRDVolumeMinors rewrites that list to stamp drbdMinor. The rule moves to pkg/validate, where both writers apply it to sizes as they arrive. The ceiling moves with it: 16 TiB is below what DRBD 9 and upstream LINSTOR handle, and linstormigrate copies sizes verbatim, so a cluster holding anything larger failed part-way through its own migration. Clone now answers its preconditions before the first write. It takes an internal snapshot of the source and restores the target from it, so a check that runs after that snapshot is not a precondition: cloning onto an occupied name took the snapshot and failed afterwards, leaving it behind. A source on a pool that cannot take a copy-on-write snapshot is refused rather than discovered by the data plane. storage-pool create validates the node and the advertised backing, LUKS definitions are refused on a cluster with no passphrase, and a value flag no longer swallows the flag after it — `resource list -n --faulty` read "--faulty" as the node name and returned an empty result with exit 0, which reads as a filter that worked. The in-memory stores were handing patch mutators a shallow struct copy, so the maps and slices in it still addressed the stored object: a mutator that refused an edit had already applied it. That made the new immutable-property guard look like it worked while the property changed anyway. Every patch now mutates a real copy, which is what makes a refusal inside a patch mean anything. The Makefile symlinked its downloaded tools by absolute path, so bin/controller-gen pointed at whichever checkout last ran make and the staleness check never matched, re-downloading every time. Both are relative now. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The bound used to be enforced twice — by the writers and by the schema. The schema copy came off because spec.volumeDefinitions is an atomic list, so any update touching it re-validated every element and one grandfathered sub-floor volume would reject every later write to that definition, the controller's own included. That trade gives up the kubectl-apply backstop, so what is left needs a gate at operator level: an L6 cell and an L7 replay drive rd c -> vd c below the floor, past the ceiling, and inside the range, and check that the refusals are refusals and the accept still accepts. Honest scope, stated in both files: this drives the REST path, which is what the harness can reach. The native CLI enforces the same rule from pkg/validate and is covered by the Go tests — the harness bootstraps the python linstor client and has no vehicle for the native binary today. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
CI ran the macOS controller-gen on Linux and died with 'cannot execute binary file'. The binaries have been in the index since 239e952, 32 MB of platform-specific artifacts committed despite the repository's own /bin/ ignore rule — gitignore does not apply to files already tracked. It was latent until now because the staleness guard never matched: it compared readlink's output against an absolute path while the symlink held a relative one, so make re-downloaded the right binary on every run and the committed one was dead weight. Making the guard work turned that dead weight into a hard failure, which is the honest sequence — the guard was right to fix and the binaries were always wrong to track. They stay on disk locally; only the index changes. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
TestVDResizeRejectsAboveMax encoded the old 16 TiB ceiling, which this branch raised to DRBD 9's documented 1 PiB — 16 TiB is below what DRBD 9 and upstream LINSTOR handle, and linstormigrate copies sizes verbatim, so a cluster holding a larger volume failed part-way through its own migration. Caught by CI rather than locally: the integration suite is behind a build tag, so plain 'go test ./...' does not run it. Run with the tag before pushing next time. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
NOT LGTM, but the shape of this round is different from the last three: the diagnosis landed, pkg/validate is the right move, and most of what I filed is genuinely closed. What blocks is a regression the shallow-copy fix introduced, one data-shaped finding that is still open, and the fact that removing the CRD bound left the field unprotected for every writer that is neither the CLI nor REST.
Checked and correct
All five findings from the 10 Aug round are closed, each confirmed by running it rather than by reading the diff:
restore --nodes node-9 with snap.Nodes=[node-1] exit=10 node does not hold this snapshot
r delete node-1 pvc-p9 (peer in SyncTarget) exit=10 refusing to delete the last complete copy while a peer is still syncing from it (pass --force to override)
create-device-pool with a failing stamp pool node-1/data present=true, so the pool is created first
identical re-run after a half-finished attempt exit=0, the run resumes and finishes
toggle-disk node-1 rd --migrate-from node-1 exit=10 source and destination are both node-1
resource list -n --faulty -m exit=2 flag "-n" needs a value, but the next argument is the flag "--faulty"
From the last round: immutable StorDriver keys are refused on all six keys and all three arms (18 subtests, property is immutable after pool creation), the layer stack is refused on rd create and on rd modify in all five forms, storage-pool create now refuses an unknown node and an unadvertised backing store, and the clone preconditions run before the first write. The relative tool symlinks also close my old bin/controller-gen note.
[MAJOR] the shallow-copy fix broke the Bug-021 annotation contract in three stores
pkg/store/inmemory_resource.go:172, and the same two lines in inmemory_resource_definition.go and inmemory_resource_group.go:
r.Annotations = carryAnnotationsOnNil(r.Annotations, prevAnnotations) // r is the discarded copy
s.m[key] = working // working is what gets storedThe carry writes into the local struct copy that the function throws away, while the clone is what lands in the map. So a mutator that leaves Annotations nil now clears them, which is the contract the line exists to prevent.
OBSERVED after a patch that nils Annotations: map[string]string(nil) (resource, resource definition, resource group)
Restoring the three files from 086da075 makes the same probe report the annotations surviving, so this arrived with the fix rather than before it.
Scope, stated honestly: the Kubernetes store builds a fresh wire object and writes only after the mutator returns, so production is not affected and what regressed is the in-memory double. That double is what the invariant tests run against, which is why it matters: the guard that is supposed to catch an annotation-clearing patch no longer catches it.
Two more things in the same change. cloneForPatch copies through a JSON round trip, so it drops fields tagged json:"-", and PoolMissing (pkg/api/v1/storage_pool.go:56) is one of them: an unrelated sp set-property on a pool the satellite flagged as missing silently clears the flag that placer.go:1279 and the Faulty column read. And "all seven" is seven of eight: inMemoryNodes.PatchProps (pkg/store/inmemory.go:425) still hands the mutator the live stored map. Reverting the fix in six of the seven patched paths leaves the suite green, so only one path is held by a test.
[MAJOR] with the bound off the CRD, sizeKib has no backstop outside the CLI and REST
Reproduced on a real apiserver, not argued:
sizeKib=0 ACCEPTED and PERSISTED as 0
sizeKib=-1 ACCEPTED and PERSISTED as -1
sizeKib=9223372036854775807 ACCEPTED and PERSISTED
kubectl-edit-shaped update of a live RD, 8192 to 0 KiB: accepted, no refusal at any layer
Zero is the one value the field's own comment says must never reach the satellite, because it loops on drbdadm create-md forever. There is no CEL rule on the field and no validating webhook for ResourceDefinition in config/, so a kubectl apply, a GitOps controller, or blockstor's own controller writing the list can store a size no provider will ever materialise.
Dropping the bound was not the only way out of the ratcheting trap, and I checked the alternative rather than asserting it. Marking the list keyed restores correlation and keeps the bound enforceable:
volumeDefinitions marked x-kubernetes-list-type=map keyed on volumeNumber, floor removed
pre-upgrade RD written with volume 0 at 1024 KiB
floor restored (minimum=4096) with the list still keyed
appending volume 1 at 8192 KiB: ACCEPTED, while volume 0 stays at 1024 KiB
creating a fresh RD at 1024 KiB: refused, spec.volumeDefinitions[0].sizeKib in body should be greater than or equal to 4096
The grandfathered volume no longer poisons later writes, the floor still holds for everything new, and the apiserver accepted the list-type change on the existing CRD, so no new API version is needed. One migration risk, measured: the current atomic list accepts two entries sharing a volumeNumber (I checked, it does), and under a keyed list such an object becomes unwritable until fixed. Neither writer produces that shape today.
[MAJOR] the field's own documentation now contradicts itself, in kubectl explain
api/v1alpha1/resourcedefinition_types.go:142 still opens with "The bounds are enforced HERE, by the API server, rather than in whichever client happens to be writing: the CLI talks to these CRDs directly, so a check that lives in one client is not a check the data is subject to", and forty lines later the same comment says the rule now lives in pkg/validate and gives up the kubectl apply backstop. Both paragraphs shipped into config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:409, so an operator reads both in kubectl explain.
The same staleness follows the ceiling: pkg/rest/volume_definitions.go:497 still calls 16 TiB DRBD's hard per-device ceiling directly above the constant that is now 1 PiB, and :631 prints to the operator pick a size between 4194304 KiB (~4 MiB) and 1099511627776 KiB (~16 TiB), where the number is 1 PiB and the gloss is the old value. pkg/rest/spawn.go:112 justifies its guard by "the bound is now Minimum/Maximum on the CRD field", which this PR removed.
[MAJOR] still open from the last round
create-device-pool still adopts a foreign pool and reports success: the AlreadyExists leg (internal/cli/physical.go:92) takes over an existing pool of a different provider while stamping the device for the requested one, so the device is marked Wipe:true under a ZFS pool named data while node-1/data stays thick LVM over someone else's VG, exit 0. Of everything left, this is the one with a data-loss shape.
Also open: the lvmthin naming divergence (--pool-name data writes data into both fields where REST produces linstor_data and data; vg/thin puts a slash in the VG name), and the delete verbs still leave orphans (rd delete leaves 2 replicas, a snapshot and a volume definition; node delete leaves a replica, a pool and a device).
[MINOR] guards that nothing holds in place
Reverting each new guard and running the narrowest package leaves the suite green for six of them, including two rules this PR is built around: pkg/validate/snapshot.go:46 (restore nodes) and pkg/validate/resource_delete.go:56 (U130, 175 lines). TestRestoreRefusesNodesWithoutTheSnapshot is vacuous: its fixture fails for an unrelated reason, so it passes with and without the guard. TestCloneRefusesOccupiedTargetBeforeWriting is vacuous the same way (its source has no replica, so no internal snapshot is created either way). pkg/validate itself has no _test.go at all, which is a lot of weight on a package covered only transitively.
[MINOR] pkg/validate is shared for layers only, and one copy has already diverged
Layers delegate. Size shares constants but not the function. Props, restore-nodes and U130 are three pairs of independent implementations, U130 being two 175-line copies with two separate DRBD state tables. The props pair has already drifted: pkg/validate compares states, so re-setting a key to its current value passes, while REST inspects the operation and refuses the same command.
[MINOR] LUKS is guarded on one writer of three, and is stricter than REST
checkLUKSPrerequisite is called only from rd create (internal/cli/write.go:260). rd modify --layer-list (definition.go:95) and rg create --layer-list (write_more.go:667) write the same stack with no check, both exit 0. Separately the CLI reads only the Secret while REST also accepts the legacy DrbdOptions/EncryptPassphrase prop, so on a cluster predating the Secret path the CLI refuses what REST accepts.
valueAfter (flags.go:392) also rejects any value starting with a dash, so encryption create-passphrase --passphrase -s3cret fails with a usage error. --passphrase=-s3cret works, but a leading dash in a passphrase is not exotic.
Not verified
golangci-lint locally is 2.12.2 against the project's pinned 2.11.4 and its cache resolved paths into an unrelated clone, so I report no lint findings. 54 integration cases need the Python client on PATH and did not run.
|
|
||
| // Bug-021: same annotation contract as Update — a closure that | ||
| // nils the map means "untouched", not "clear". | ||
| r.Annotations = carryAnnotationsOnNil(r.Annotations, prevAnnotations) |
There was a problem hiding this comment.
[MAJOR] the carry writes into the discarded copy, so the Bug-021 contract is gone
r is the local struct copy this function throws away; working is what lands in the map on the next line. So a mutator that leaves Annotations nil now clears them, which is the contract this line exists to prevent. Same two lines in inmemory_resource_definition.go and inmemory_resource_group.go.
OBSERVED after a patch that nils Annotations: map[string]string(nil) (resource, resource definition, resource group)
Restoring the three files from 086da075 makes the same probe report the annotations surviving, so this arrived with the fix.
Scope stated honestly: the Kubernetes store builds a fresh wire object and writes only after the mutator returns, so production is unaffected and what regressed is the in-memory double. That double is what the invariant tests run against, which is the point: the guard meant to catch an annotation-clearing patch no longer catches it.
| func cloneForPatch[T any](in T) (T, error) { | ||
| var out T | ||
|
|
||
| raw, err := json.Marshal(in) |
There was a problem hiding this comment.
[MAJOR] a JSON round trip drops fields tagged json:"-"
PoolMissing (pkg/api/v1/storage_pool.go:56) is tagged json:"-" and is the flag placer.go:1279 uses to drop a pool and the CLI uses to paint Faulty. Cloning through JSON silently resets it, so an unrelated sp set-property on a pool the satellite flagged as missing makes it look healthy again.
OBSERVED PoolMissing before=true, after an unrelated set-property=false
Related: "all seven" is seven of eight, inMemoryNodes.PatchProps (pkg/store/inmemory.go:425) still hands the mutator the live stored map. And reverting the fix in six of the seven patched paths leaves the suite green, so one path is held by a test.
| VolumeNumber int32 `json:"volumeNumber"` | ||
| SizeKib int64 `json:"sizeKib"` | ||
|
|
||
| // sizeKib is the volume size. The bounds are enforced HERE, by the |
There was a problem hiding this comment.
[MAJOR] this comment and the one 40 lines below now contradict each other, and both ship into kubectl explain
This paragraph says the bounds are enforced by the API server and that a check living in one client is not a check the data is subject to. Forty lines below, the same comment says the rule now lives in pkg/validate and gives up the kubectl apply backstop. Both went into config/crd/bases/blockstor.cozystack.io_resourcedefinitions.yaml:409, so the operator reads both.
With the bound gone, the apiserver accepts what no writer can materialise:
sizeKib=0 ACCEPTED and PERSISTED as 0
sizeKib=-1 ACCEPTED and PERSISTED as -1
kubectl-edit-shaped update of a live RD, 8192 to 0 KiB: accepted
Zero is the value this very comment calls the one that must never reach the satellite. There is no CEL rule on the field and no validating webhook for ResourceDefinition in config/.
The keyed-list alternative is in the review body: it restores ratcheting and keeps the bound, and I ran it rather than assuming it.
| // state. This order leaves the harmless remainder instead, a | ||
| // registered pool with fewer devices than intended, which the same | ||
| // command completes on re-run. | ||
| err := run.Store.StoragePools().Create(ctx, pool) |
There was a problem hiding this comment.
[MAJOR] still open: AlreadyExists here adopts a foreign pool and reports success
Swallowing AlreadyExists takes over an existing pool of a different provider while the device is stamped for the requested one:
exit=0, stderr empty
device AttachTo={StoragePoolName:data ProviderKind:ZFS ZPoolName:data Wipe:true}
registered pool: kind="LVM" props=map[StorDriver/LvmVg:someone-elses-vg]
The device is marked for wiping under a ZFS pool named data while node-1/data stays thick LVM over someone else's VG. Of everything still open this is the one with a data-loss shape.
Note the asymmetry this round introduced: storage-pool create gained checkPoolTarget, this path writes the same object without it, so create-device-pool zfs ghost-node /dev/sdb exits 0 against an empty node inventory.
| // which trivially holds it. | ||
| // - A snapshot that records no nodes at all: nothing to check against, so | ||
| // the caller's own emptiness guard is the one that applies. | ||
| func RestoreNodesHoldSnapshot(named, snapshotNodes []string) error { |
There was a problem hiding this comment.
[MINOR] this guard is not held by any test
Replacing the body with return nil leaves the suite green. TestRestoreRefusesNodesWithoutTheSnapshot is vacuous: its fixture fails for an unrelated reason, so it passes with and without the guard. Under the mutation a probe reports restore --nodes node-9 exit=0 while snap.Nodes=[node-1]; restored replicas landed on [node-9], i.e. the CRITICAL from the 10 Aug round returns silently.
pkg/validate has no _test.go at all, which is a lot of weight for a package that now holds the invariants of the whole PR.
| // disk state has not been projected yet counts as a possible last source: | ||
| // concluding "not a source" from an unknown projection is precisely the | ||
| // false-allow that strands the peer. | ||
| func MidSyncDeleteRefusal(target *apiv1.Resource, siblings []apiv1.Resource) bool { |
There was a problem hiding this comment.
[MINOR] U130 is unheld by tests here and is a second copy of the REST rule
return false leaves ./pkg/validate/... ./internal/cli/... ./pkg/rest/... green, so after the move the rule is pinned in neither place.
It is also 175 lines duplicating pkg/rest/resource_delete_last_uptodate_u130.go:85, with a second independent table of DRBD state strings. Same for props and restore-nodes: the shared package currently shares layers only, size shares constants but not the function. The props pair has already drifted, pkg/validate compares states so re-setting a key to its current value passes, REST inspects the operation and refuses it.
| return err | ||
| } | ||
|
|
||
| luksErr := checkLUKSPrerequisite(ctx, run, parsed) |
There was a problem hiding this comment.
[MINOR] the LUKS prerequisite is checked on one writer of three
rd modify --layer-list (definition.go:95) and rg create --layer-list (write_more.go:667) write the same stack with no check, both exit 0 on a cluster with no passphrase.
Separately this guard is stricter than REST: it reads only the Secret, while pkg/rest/resource_definitions.go:896 also accepts the legacy DrbdOptions/EncryptPassphrase prop, so on a cluster predating the Secret path the CLI refuses what REST accepts. Two signals: with the prop set the CLI exits 10, and refuseLUKSWithoutPassphrase on the same state returns nil.
| } | ||
|
|
||
| next := args[i+1] | ||
| if strings.HasPrefix(next, "-") && next != "-" { |
There was a problem hiding this comment.
[MINOR] any value starting with a dash is rejected, including a passphrase
encryption create-passphrase --passphrase -s3cret exits 2 with flag "--passphrase" needs a value, but the next argument is the flag "-s3cret". --passphrase=-s3cret and a -- terminator work, but a leading dash in a passphrase is not exotic. The same applies to --vlmnr -1, which a comment in parseFlags names as an expected input.
The guard that refuses a flag as a flag's value exists because `resource list -n --faulty` read "--faulty" as the node name and produced an empty filter, no results and exit 0. That reasoning holds for identifiers, which never start with a dash. A passphrase is not an identifier. `encryption create-passphrase --passphrase -s3cret` was rejected as a usage error, and the form that does work, `--passphrase=-s3cret`, is not discoverable from the message. Exempt the flags whose value is free-form bytes and keep the guard everywhere else. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…sources checkLUKSPrerequisite ran on `rd create` only. `rd modify --layer-list` and `rg create --layer-list` wrote the same stack unchecked, and a group's stack is inherited by every definition spawned from it, so the unguarded path reached more volumes than the guarded one. A LUKS layer with no cluster passphrase brings the replicas up plaintext. The check also read only the encryption Secret, while REST accepts the legacy controller prop as well. On a cluster provisioned before the Secret existed, the CLI refused what the REST door on the same cluster accepted. Consult both, in the same order REST does. Each of the three call sites is held by a test that goes red when that one guard is removed. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Both passed without the guard they name. The restore test called `s rr`, which is not a subcommand, so it was refused before reaching anything. Spelled out, it then still passed with the guard removed, because the source had no diskful replica and the restore failed for a missing storage pool instead. It now runs against a complete fixture and asserts a positive control first: the same restore onto a node that holds the snapshot succeeds. The clone test seeded a source with no replicas, so no internal snapshot was created either way and the assertion that none was left behind held trivially. With replicas seeded, removing the guard leaves one behind and the test says so. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
pkg/validate carries the rules both write doors depend on and had no test of its own, so every rule was held only by whichever caller happened to reach it. Reverting six of the guards left the suite green. Cover each exported rule directly. Every one is checked against a mutation: neutering the layer order, the size floor, the mid-sync delete refusal, the restore-node check or the immutable-prop set now fails a named test. The props pair had already drifted: validate compared states, so setting a backing key to the value it already has passed, while REST inspects the operation and refuses the same command. State comparison stays, because it catches set, delete and delete-namespace alike, and a named-key check runs beside it so both doors answer alike. Also drop the claim in size.go that the bound deliberately does not live on the CRD. It does now, and the keyed list is what makes it enforceable. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
`create-device-pool --pool-name data` on lvmthin wrote `data` into both the volume group and the thin pool, while the REST handler for the same verb produced `linstor_data` and `data`. A volume group and the thin LV inside it are different objects in different namespaces, which is what upstream's prefix keeps apart. A `vg/thin` value went into the volume-group field whole, and a slash is not legal in a VG name. Both doors now call one derivation, in the package that owns the object they write, so the rule cannot drift again. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
`rd delete` dropped the definition and left its replicas behind, and `node delete` dropped the node and left its replicas, pools and devices behind. Neither failed: the objects simply stayed, naming a parent that no longer exists. A replica with no definition above it never receives a deletion stamp, so the satellite finalizer never runs and `drbdadm down` never happens — the DRBD minor, port and peer entries stay live on every node, and the next create with the same name collides with them. The REST door has done this since it was written. Both now call one implementation: - `rd delete` refuses while snapshots remain, before tearing anything down, then cascades to the replicas. A refusal after the cascade would leave the cluster half-dismantled and no retry reconciles that. - `node delete` refuses while replicas or pools still name the node, and `--force` is the explicit "this node is gone" decision that cascades. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The package was shared for layer stacks only. The mid-sync delete refusal, the restore-node check and the immutable storage-pool prop set were each implemented twice, once per write door — the delete refusal as two 175-line copies carrying two separate DRBD state tables. Two implementations of one invariant is the drift this package exists to remove, and the props pair had already drifted. The rules now live in one place and each door keeps only its own shaping: the REST envelope and status code, the CLI exit code. The state tables were compared value by value before collapsing them, and the carve-outs on the restore check matched on both sides. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Everything from this round is addressed, including the three items still open from earlier ones. Each fix is held by a test that goes red when that one change is reverted, and I ran every one of those mutations — two of them caught mistakes in my own new tests, which I mention below because they are the same failure mode you have flagged three rounds running. [MAJOR] the annotation regression. Correct, and it was mine. The carry wrote into the discarded copy while the clone was stored, so a mutator leaving The JSON round trip and the eighth path. Both real. [MAJOR] [MAJOR] the field documenting itself both ways. Rewritten as one argument. The stale ceiling gloss is gone from [MAJOR] lvmthin naming. Both halves fixed, and by removing the second implementation rather than patching it: the derivation now lives in Delete verbs leaving orphans. [MINOR] the vacuous tests. Both were worse than your description. [MINOR] [MINOR] the props drift. Resolved toward REST, since refusing the named key is the upstream contract. The state comparison stays, because it catches set, delete and delete-namespace alike, and a named-key check runs beside it so setting an immutable key to the value it already has is refused on both doors. [MINOR] the three duplicated pairs. Collapsed onto [MINOR] LUKS on one writer of three, and stricter than REST. [MINOR]
|
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
NOT LGTM. Most of the round landed, and the collapse onto pkg/validate is verified rather than assumed. What blocks is one regression in the new delete path, two executable contracts that now contradict each other, and the fact that the keyed list I asked for is not held by the test written for it.
Checked and correct
The collapse is behaviour-preserving where it matters. I compared the old REST implementations against the shared ones on 29 boundary inputs, including a replica with no siblings, a Diskless sibling, a TIE_BREAKER sibling, a sibling in SyncTarget, a target already DELETING, an unprojected target and an empty node list on restore. U130 and the immutable-prop set agree on every one of them, set and order included.
cloneForPatch by reflection is right and is held by four tests: the JSON round trip, the no-copy shape and both refusal paths go red when reverted. PoolMissing survives a patch now.
lvmthin naming is closed on both doors, and both mutations (dropping the linstor_ prefix, dropping the slash split) go red in internal/cli and pkg/rest together. valueAfter is pinned on both sides: opaque values may lead with a dash, identifiers still refuse a flag as their value. The vacuous guard tests now open with a positive control, and that control immediately caught another instance of the same mistake, which is the point of writing it that way.
The keyed list works: ratcheting spares a grandfathered volume, duplicate volumeNumber is refused by the apiserver, and patchRDVolumeMinors still round-trips (TestBug433VDResizePreservesDRBDMinor passes).
[MAJOR] pkg/store/cascade.go:160 node delete now refuses every node that was registered through REST
ReferencesOnNode lists all storage pools on the node with no filter. The REST refusal it replaces skips DfltDisklessStorPool explicitly (pkg/rest/nodes.go:1289), and its comment says why: REST creates that pool on every node create, so counting it made the refusal fire on freshly created idle nodes.
So a node registered by piraeus, linstor-csi or the linstor client can never be deleted through blockstor n d: the refusal names a pool the operator cannot remove, while linstor n d on the same cluster succeeds. The reply says both doors call one implementation, which is true for the two cascade functions and not for the refusal.
[MAJOR] two executable operator contracts now demand a refusal that no longer happens, and contradict a third
tests/e2e/cli-matrix/vd-resize-bounds-rejected.sh:105 fails if vd set-size <rd> 0 16385G succeeds, and tests/operator-harness/replay/vd-resize-bounds-rejected.yaml expects the same refusal. 16385 GiB is 17179869184 KiB, comfortably under the new 1 PiB ceiling, so validateVDSize returns nil and the cell fails. Meanwhile tests/e2e/cli-matrix/vd-size-bounds-after-crd-move.sh:73 requires 20T to be accepted. The two cells are now mutually exclusive.
Neither runs in PR CI, so this surfaces on the stand. Five more files carry prose calling 16 TiB the current ceiling.
[MAJOR] tests/integration/vd_size_ratchet_test.go:58 the ratchet test does not exercise the case it was written for
The fixture seeds SizeKib: vdBoundsMinSizeKib, which is 4096, exactly the minimum, under a comment describing a volume below the floor. A value at the bound is valid under any schema, so the test passes on an atomic list too. Removing x-kubernetes-list-type: map from the CRD leaves it green as well, so nothing currently pins the mechanism that makes the bound survivable on upgrade. Seeding below the floor (1024 KiB, written through the store the way an in-place upgrade leaves it) is what makes the test load-bearing.
[MINOR] the annotation fix is pinned on one store of three
The code is right in all three, but only Resources() has a test: reverting working.Annotations to the discarded copy in inmemory_resource_definition.go or inmemory_resource_group.go leaves the whole unit suite green.
Same shape elsewhere in this round: checkAdoptablePool in create-device-pool has no coverage at all (removing it changes nothing), the CLI wiring of StoragePoolPropNamed has none (the rule is covered in pkg/validate, the call site is not), and dropping PropFileDir from ImmutableStoragePoolProps() is caught by nothing, because the test that claims to cover every backing key derives its expectation from the same list.
[MINOR] internal/cli/encryption.go:322 the LUKS guard fails open where REST fails closed
On a props read error the CLI returns nil, which allows the write; REST at the same point returns the error and answers 400. NotFound collapses to an empty map upstream of this, so a fresh cluster still refuses correctly, and this only opens on a real apiserver failure. It is still a direction change: the CLI was fail-closed here before.
Related, and the reason I mention it in the same breath: the argument that guarding only create leaves the door open was applied to the CLI only. pkg/rest/resource_groups.go:166 validates a group's layer stack without calling refuseLUKSWithoutPassphrase, and :493 lets the stack be patched afterwards. A group created through REST with DRBD,LUKS,STORAGE on a passphrase-less cluster still spawns plaintext replicas. The drift did not close, it changed sides.
[MINOR] the restore refusal changed its message ordering
pkg/validate/snapshot.go:94 sorts the missing nodes; the REST implementation reported them in the order the operator passed them. --node-name n9 --node-name n8 used to come back as n9, n8 and now comes back as n8, n9. Cosmetic unless something greps it, but it is a wire-visible change that the collapse did not mention.
And node delete now does a full Resources().List(ctx) on every call including the non-force path, which on the Kubernetes store lists every Resource in the cluster to answer a question about one node.
Not verified
The local golangci-lint is 2.12.2 against the pinned 2.11.4 and its cache resolved paths into an unrelated clone, so I report no lint findings. 78 integration cases need the Python client on PATH and did not run.
|
|
||
| poolRefs := make([]string, 0, len(pools)) | ||
| for i := range pools { | ||
| poolRefs = append(poolRefs, pools[i].StoragePoolName) |
There was a problem hiding this comment.
[MAJOR] this counts DfltDisklessStorPool, so node delete refuses every REST-registered node
The REST refusal this replaces skips that pool explicitly (pkg/rest/nodes.go:1289), and its comment says why: REST creates it on every node create, so counting it made the refusal fire on freshly created idle nodes.
A node registered by piraeus, linstor-csi or the linstor client therefore cannot be deleted through blockstor n d: the refusal names a pool the operator cannot remove, while linstor n d on the same cluster succeeds. Both doors share the cascade functions but not the refusal.
| ObjectMeta: metav1.ObjectMeta{Name: "ratchet-legacy"}, | ||
| Spec: blockstoriov1alpha1.ResourceDefinitionSpec{ | ||
| VolumeDefinitions: []blockstoriov1alpha1.ResourceDefinitionVolume{ | ||
| {VolumeNumber: 0, SizeKib: vdBoundsMinSizeKib}, |
There was a problem hiding this comment.
[MAJOR] the fixture is at the bound, not below it, so this test passes on an atomic list too
vdBoundsMinSizeKib is 4096, exactly the minimum, under a comment describing a volume below the floor. A value at the bound is valid under any schema, so this does not exercise ratcheting: removing x-kubernetes-list-type: map from the CRD also leaves it green.
Seeding 1024 KiB through the store, the way an in-place upgrade leaves it, is what makes the test hold the mechanism.
| // nils the map means "untouched", not "clear". | ||
| rd.Annotations = carryAnnotationsOnNil(rd.Annotations, prevAnnotations) | ||
| s.m[name] = rd | ||
| working.Annotations = carryAnnotationsOnNil(working.Annotations, prevAnnotations) |
There was a problem hiding this comment.
[MINOR] the annotation fix is pinned on one store of three
The code is correct here, but reverting this line (and the same one in inmemory_resource_group.go) to the discarded copy leaves the entire unit suite green. Only Resources() has a test.
Same shape elsewhere in this round: checkAdoptablePool has no coverage, the CLI wiring of StoragePoolPropNamed has none, and dropping PropFileDir from ImmutableStoragePoolProps() is caught by nothing because the test that claims to cover every backing key derives its expectation from that same list.
| // REST door on the same cluster creates without complaint. | ||
| props, err := run.Store.ControllerProps().Get(ctx) | ||
| if err != nil { | ||
| return nil //nolint:nilerr // cannot read the props: see above |
There was a problem hiding this comment.
[MINOR] fails open where REST fails closed
On a props read error this allows the write; REST at the same point returns the error and answers 400. NotFound collapses to an empty map upstream, so a fresh cluster still refuses correctly and this only opens on a real apiserver failure. It is still a direction change, the CLI was fail-closed here.
Related: the argument that guarding only create leaves the door open was applied to the CLI only. pkg/rest/resource_groups.go:166 validates a group's stack without calling refuseLUKSWithoutPassphrase, and :493 lets it be patched afterwards, so a group created through REST with DRBD,LUKS,STORAGE on a passphrase-less cluster still spawns plaintext replicas.
| } | ||
| } | ||
|
|
||
| sort.Strings(missing) |
There was a problem hiding this comment.
[MINOR] sorting changed a wire-visible refusal message
The REST implementation reported the missing nodes in the order the operator passed them. --node-name n9 --node-name n8 used to come back as n9, n8 and now comes back as n8, n9. Cosmetic unless something greps it, but the collapse did not mention it.
2 participants
Adds
blockstor, a native CLI that reproduces the command surface operators already know and speaks the Kubernetes API directly, so the upstream python client can be dropped as a runtime dependency.Going straight to the CRDs is not just one hop shorter — it is more correct. The store layer is already a reusable library, so the CLI gets the same DTOs the REST apiserver would return without duplicating a line of wire↔CRD translation. And because a CLI reads through a non-cached client rather than an informer cache behind N replicas, the cross-replica cache lag the apiserver carries retry machinery for cannot occur here at all.
The grammar is the one operators and this repository's harnesses already type:
blockstor resource listandblockstor r l,storage-pool createandsp c, three-tokensnapshot resource restore. Exit codes keep the convention scripts branch on — 0 success, 2 a client-side rejection, 10 an API-level failure. Tables are built asmetav1.Tableand the CRDs gained the printer columns they never had, sokubectl getand the CLI agree on what a row looks like. Colour is preserved, gated on a TTY, and applied so that stripping the escapes reproduces the plain rendering byte for byte.Where the controller already owns a decision, the CLI calls it rather than reimplementing it: placement goes through
pkg/placer, the same code the resource-group controllers run. Two answers to "where should this replica go?" would drift apart the moment either changed. Several contracts that differ per verb are preserved rather than smoothed over — an explicit placement request fails on a shortfall while a group spawn defers to the rebalance reconciler; a resize refuses to shrink without--force, and the size bounds hold even with it.error-reportsis deliberately absent: the reports are a ring buffer in the controller process's memory, so a client that speaks to the API server has nothing to list.encryption enter-passphraseverifies the passphrase against the cluster Secret in constant time and succeeds, noting on stderr that the controller's own in-memory flag — which only drives the Suspended/Available column in its REST view, and gates nothing — is untouched.The upstream python client is GPL. Its source was not read, quoted or translated. What is reproduced here is the interface — command names, flag names, column names, colour semantics — taken from this repository's own tests, scripts and parity documentation, and the implementation is written against the blockstor API types.
Design notes and the full test plan are in
docs/cli-design.md.Testing
go test ./...— green; 148 test cases across dispatch, flag parsing, rendering, views, machine output and every write verb. They run in the existingUnit testsCI job, which enumerates packages dynamically.golangci-lint run ./...— 0 issues.set-propertywithoutlist-propertiesanddelete-property.tests/e2e/cli-matrixsuite against a stand, pointed atblockstorinstead of the python client. That is the acceptance criterion for actually dropping the dependency and is the natural follow-up.Summary by CodeRabbit
blockstorCLI for managing nodes, resources, snapshots, storage pools, resource groups, properties, encryption, placement, and DRBD options.