feat(cli): native blockstor CLI speaking Kubernetes directly - #181
feat(cli): native blockstor CLI speaking Kubernetes directly#181Andrei Kvapil (kvaps) wants to merge 31 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
| func physicalStorageCreateDevicePool(ctx context.Context, run *runContext) error { | ||
| const wantArgs = 3 // provider, node, at least one device | ||
|
|
||
| if len(run.Flags.Positionals) < wantArgs { | ||
| return fmt.Errorf("%w: create-device-pool needs a provider, a node and a device", command.ErrUsage) | ||
| } | ||
|
|
||
| token := strings.ToLower(run.Flags.Positionals[0]) | ||
|
|
||
| provider, known := storageProviders[token] | ||
| if !known { | ||
| return fmt.Errorf("%w: unknown storage provider %q", command.ErrUsage, run.Flags.Positionals[0]) | ||
| } | ||
|
|
||
| node := run.Flags.Positionals[1] | ||
| devices := run.Flags.Positionals[2:] | ||
|
|
||
| poolName := run.Flags.Values["pool-name"] | ||
| if poolName == "" { | ||
| return fmt.Errorf("%w: create-device-pool needs --pool-name", command.ErrUsage) | ||
| } | ||
|
|
||
| attach := attachRequest(provider, poolName, token) | ||
|
|
||
| err := stampDevices(ctx, run, node, devices, attach) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| pool := &apiv1.StoragePool{ | ||
| NodeName: node, | ||
| StoragePoolName: poolName, | ||
| ProviderKind: provider.kind, | ||
| Props: attachProps(provider, attach), | ||
| } | ||
|
|
||
| err = run.Store.StoragePools().Create(ctx, pool) | ||
| if err != nil && !isAlreadyExists(err) { | ||
| return fmt.Errorf("create storage pool %s on %s: %w", poolName, node, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
No rollback if StoragePools().Create (or a later device lookup) fails after devices are already stamped.
stampDevices runs first and persists AttachTo on each matched device; StoragePools().Create runs afterward. If create fails with a real error (not AlreadyExists), or if a later device in a multi-device call isn't found, the already-stamped device(s) are left pointing at a pool CR that was never created, with no automatic cleanup. The satellite will keep failing to reconcile that attach request until manually fixed.
The ordering is clearly a deliberate trade-off against the opposite failure (an orphaned, un-attachable pool) — but the current code accepts the orphaned-device failure mode silently. Consider a best-effort compensating cleanup on the failure path:
♻️ Suggested compensating cleanup
err = run.Store.StoragePools().Create(ctx, pool)
if err != nil && !isAlreadyExists(err) {
+ // Best-effort: don't leave devices pointing at a pool that was
+ // never created.
+ unstampDevices(ctx, run, node, devices)
return fmt.Errorf("create storage pool %s on %s: %w", poolName, node, err)
}Also applies to: 149-180
🤖 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/physical.go` around lines 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.
| func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error { | ||
| src := run.Flags.Values["migrate-from"] | ||
| pool := run.Flags.Values["storage-pool"] | ||
|
|
||
| srcRes, err := run.Store.Resources().Get(ctx, rdName, src) | ||
| if err != nil { | ||
| return fmt.Errorf("migrate-disk: source replica %s on %s: %w", rdName, src, err) | ||
| } | ||
|
|
||
| if slices.Contains(srcRes.Flags, apiv1.ResourceFlagDiskless) { | ||
| return fmt.Errorf("migrate-disk: source replica %s on %s has no diskful storage to migrate: %w", | ||
| rdName, src, errNothingToMigrate) | ||
| } | ||
|
|
||
| if srcRes.State.InUse != nil && *srcRes.State.InUse { | ||
| return fmt.Errorf("migrate-disk: source replica %s on %s is Primary InUse: %w", | ||
| rdName, src, errSourceInUse) | ||
| } | ||
|
|
||
| _, err = run.Store.Resources().Get(ctx, rdName, dst) | ||
| if isNotFound(err) { | ||
| return createMigrationTarget(ctx, run, dst, rdName, pool, src) | ||
| } | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("get resource %s on %s: %w", rdName, dst, err) | ||
| } | ||
|
|
||
| return patchResource(ctx, run, dst, rdName, func(res *apiv1.Resource) { | ||
| stampProp(res, storPoolNameProp, pool) | ||
| stampProp(res, migratingFromProp, src) | ||
| res.Flags = setFlag(res.Flags, apiv1.ResourceFlagDiskless, false) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)internal/cli/resource\.go$|reconciler|migration|migrate|BlockstorMigratingFrom|UpToDate|Diskless' || true
echo "== resource.go outline =="
ast-grep outline internal/cli/resource.go --view compact || true
echo "== relevant resource.go lines =="
sed -n '120,270p' internal/cli/resource.go
echo "== constants related to migrating props =="
rg -n "BlockstorMigratingFrom|migratingFromProp|storage-pool|migrate-from|cmd.Migrate|migrateDisk|createMigrationTarget|migrate-disk" -S .Repository: cozystack/blockstor
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== controller migration contract lines =="
sed -n '1,120p' internal/controller/resource_migration_controller.go
sed -n '160,230p' internal/controller/resource_migration_controller.go
echo "== controller migration test lines =="
sed -n '1,90p' internal/controller/resource_migration_controller_test.go
sed -n '130,195p' internal/controller/resource_migration_controller_test.go
echo "== satellite migration/prune relevant lines =="
sed -n '5360,5485p' pkg/satellite/reconciler.go
sed -n '3345,3405p' pkg/satellite/reconciler.go
sed -n '3775,3820p' pkg/satellite/reconciler.go
echo "== validateMigrateSrc relevant lines =="
sed -n '392,455p' pkg/rest/resource_toggle_disk.go
echo "== CLI validateMigrateSrc and dispatch =="
sed -n '1,80p' internal/cli/resource.go
sed -n '45,75p' internal/cli/resource.go
echo "== read-only invariant probe: CLI/REST validation functions and migrator predicates =="
python3 - <<'PY'
from pathlib import Path
for path in [Path("internal/cli/resource.go"), Path("pkg/rest/resource_toggle_disk.go"), Path("internal/controller/resource_migration_controller.go"), Path("pkg/satellite/reconciler.go")]:
text = path.read_text()
print(f"\n--- {path} ---")
for tag in [
"func validateMigrateNodeSrc",
"func validateMigrateSrc",
"if src == dst",
"if srcNode == dstNode",
"BlockstorMigratingFrom",
"MigratingFromProp",
"UpToDate",
"clearMigratingFrom",
"delete source", "DELETE",
]:
lines = [n for n,l in enumerate(text.splitlines(),1) if tag.lower() in l.lower()]
print(f"{tag}: {lines[:30]}")
PYRepository: cozystack/blockstor
Length of output: 30515
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ResourceMigrationReconciler Reconcile source =="
sed -n '62,120p' internal/controller/resource_migration_controller.go
echo "== REST migration body tests relevant lines =="
sed -n '140,230p' pkg/rest/resource_toggle_disk_test.go
sed -n '1,140p' pkg/rest/migrate_disk_bodyless_test.go
echo "== parse delete source expression =="
python3 - <<'PY'
from pathlib import Path
text = Path("internal/controller/resource_migration_controller.go").read_text()
for name in ("delete src Resource via Kubernetes client.Delete", "deletes the source Resource CRD"):
print(text.find(name))
print("contains `.Delete(` nearby source prune? ", ".Delete(" in text)
print(text.splitlines()[119:124])
PYRepository: cozystack/blockstor
Length of output: 10405
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact ResourceMigrationReconciler Reconcile body =="
python3 - <<'PY'
from pathlib import Path
text = Path("internal/controller/resource_migration_controller.go").read_text().splitlines()
after = next(i for i,l in enumerate(text,1) if "stamped and clears" in l or '"resource", dst.Name, "src", srcNode)' in l)
for i in range(max(1, after-14), min(len(text), after+20)+1):
print(f"{i:4}: {text[i-1]}")
PY
echo "== source prune test expectations =="
python3 - <<'PY'
from pathlib import Path
text = Path("internal/controller/resource_migration_controller_test.go").read_text()
for needle in ["src after dst UpToDate", "src must be deleted", "src Resource still present", "Get src"]:
idx = text.find(needle)
print(f"== {needle} @ {idx} ==")
for line in text[max(0, idx-600): min(len(text), idx+800)].splitlines():
print(line)
PY
echo "== REST self-node validation context =="
python3 - <<'PY'
from pathlib import Path
text = Path("pkg/rest/resource_toggle_disk.go").read_text().splitlines()
for i,l in enumerate(text,1):
if '"blockstorMigrateDiskDuplicateSource"' in l or 'srcNode == dstNode' in l or 'get resource' in l.lower():
lo=max(1,i-12); hi=min(len(text),i+22)
print(f"\n--- {i} ---")
for n in range(lo, hi+1):
print(f"{n}: {text[n-1]}")
PYRepository: cozystack/blockstor
Length of output: 6606
Reject self-referential --migrate-from before stamping the migration destination.
migrateDisk does not enforce src != dst, and the migration reconciler later deletes <rd>.<src-node> once BlockstorMigratingFrom is resolved. Passing the same node as both destination and --migrate-from can therefore mark a replica as its own source and prune its only diskful copy after the destination volumes are considered UpToDate.
🛡️ Proposed guard
func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error {
src := run.Flags.Values["migrate-from"]
pool := run.Flags.Values["storage-pool"]
+
+ if src == dst {
+ return fmt.Errorf("%w: migrate-from source and destination are both %s", command.ErrUsage, dst)
+ }
srcRes, err := run.Store.Resources().Get(ctx, rdName, src)📝 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.
| func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error { | |
| src := run.Flags.Values["migrate-from"] | |
| pool := run.Flags.Values["storage-pool"] | |
| srcRes, err := run.Store.Resources().Get(ctx, rdName, src) | |
| if err != nil { | |
| return fmt.Errorf("migrate-disk: source replica %s on %s: %w", rdName, src, err) | |
| } | |
| if slices.Contains(srcRes.Flags, apiv1.ResourceFlagDiskless) { | |
| return fmt.Errorf("migrate-disk: source replica %s on %s has no diskful storage to migrate: %w", | |
| rdName, src, errNothingToMigrate) | |
| } | |
| if srcRes.State.InUse != nil && *srcRes.State.InUse { | |
| return fmt.Errorf("migrate-disk: source replica %s on %s is Primary InUse: %w", | |
| rdName, src, errSourceInUse) | |
| } | |
| _, err = run.Store.Resources().Get(ctx, rdName, dst) | |
| if isNotFound(err) { | |
| return createMigrationTarget(ctx, run, dst, rdName, pool, src) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("get resource %s on %s: %w", rdName, dst, err) | |
| } | |
| return patchResource(ctx, run, dst, rdName, func(res *apiv1.Resource) { | |
| stampProp(res, storPoolNameProp, pool) | |
| stampProp(res, migratingFromProp, src) | |
| res.Flags = setFlag(res.Flags, apiv1.ResourceFlagDiskless, false) | |
| }) | |
| } | |
| func migrateDisk(ctx context.Context, run *runContext, dst, rdName string) error { | |
| src := run.Flags.Values["migrate-from"] | |
| pool := run.Flags.Values["storage-pool"] | |
| if src == dst { | |
| return fmt.Errorf("%w: migrate-from source and destination are both %s", command.ErrUsage, dst) | |
| } | |
| srcRes, err := run.Store.Resources().Get(ctx, rdName, src) | |
| if err != nil { | |
| return fmt.Errorf("migrate-disk: source replica %s on %s: %w", rdName, src, err) | |
| } | |
| if slices.Contains(srcRes.Flags, apiv1.ResourceFlagDiskless) { | |
| return fmt.Errorf("migrate-disk: source replica %s on %s has no diskful storage to migrate: %w", | |
| rdName, src, errNothingToMigrate) | |
| } | |
| if srcRes.State.InUse != nil && *srcRes.State.InUse { | |
| return fmt.Errorf("migrate-disk: source replica %s on %s is Primary InUse: %w", | |
| rdName, src, errSourceInUse) | |
| } | |
| _, err = run.Store.Resources().Get(ctx, rdName, dst) | |
| if isNotFound(err) { | |
| return createMigrationTarget(ctx, run, dst, rdName, pool, src) | |
| } | |
| if err != nil { | |
| return fmt.Errorf("get resource %s on %s: %w", rdName, dst, err) | |
| } | |
| return patchResource(ctx, run, dst, rdName, func(res *apiv1.Resource) { | |
| stampProp(res, storPoolNameProp, pool) | |
| stampProp(res, migratingFromProp, src) | |
| res.Flags = setFlag(res.Flags, apiv1.ResourceFlagDiskless, false) | |
| }) | |
| } |
🤖 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/resource.go` around lines 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.
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 |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
NOT LGTM — builds clean and all tests pass, but a set of behavioural defects in the CLI (empty-passphrase acceptance, partial-write-then-blocked-retry, exit-code and machine-output gaps) need addressing. No cluster-state surface (no charts/migrations/RBAC/CRD schema changes), so upgrade/fresh-install phases are N/A.
Findings
[MAJOR] internal/cli/encryption.go:44, empty passphrase is accepted silently
encryptionPassphrase returns a positional without checking for emptiness. encryption create-passphrase "" (or -p "", since -p parses as the boolean --pastable and "" falls through to positionals) writes an empty master key to the Secret and exits 0. passphrase.Read then returns "" for both a missing Secret and an empty value, so enter-passphrase <real> reports "no passphrase; create one first" while create-passphrase <real> reports "already set … mismatch" — two contradictory diagnoses with no CLI path out (manual Secret deletion required), and on a fresh cluster this weakens volume encryption to an empty key. Reject an empty passphrase as command.ErrUsage.
[MAJOR] internal/cli/place.go:253, spawn-resources creates the ResourceDefinition before validating sizes
spawnDefinition runs before the ParseSize/checkVolumeSize loop, and volumes are created one at a time. rg spawn grp pvc-x 32X creates pvc-x, then errors on the bad size; the corrected retry rg spawn grp pvc-x 32M fails in spawnDefinition (a plain non-idempotent Create) with "already exists". A size typo leaves an orphan definition/partial volumes and blocks the natural retry until a manual delete. This contradicts the validate-before-write discipline the PR itself applies in snapshotRestoreVolumeDefinition. Validate all sizes before the first write.
[MINOR] internal/cli/handlers.go applyLimit, malformed/negative --limit is swallowed (fail-open)
--limit banana returns the full list with exit 0 and no diagnostic; --limit 0 returns zero rows (inverting the usual "0 == unlimited"). Every other numeric flag wraps command.ErrUsage (exit 2). Validate --limit at parse time and decide --limit 0 explicitly; add a test for the malformed case.
[MINOR] internal/cli/help.go:31, per-command --help exits 2
isHelpRequest inspects only argv[0], so blockstor r l --help is rejected as "unknown flag" with exit 2. The upstream argparse client prints per-command help with exit 0.
[MINOR] internal/cli/app.go, color.ParseMode runs after StoreFor
With an unavailable kubeconfig, r l --color=bogus exits 10 ("load kubeconfig") instead of 2, so the same class of client-side error is classified differently depending on cluster reachability, and a known-invalid invocation still opens a cluster connection. Move ParseMode before StoreFor.
[MINOR] internal/cli/definition.go:204, query-size-info -m drops the computed max size
The machine branch emits only pools; the computed maxVolumeSizeKib — the whole point of the command, and the table's headline column — exists only in the table branch. -m consumers cannot obtain it.
[MINOR] internal/cli/pool.go volumeGroupList, vg list -m drops the parent resource-group
Machine output is a flattened []VolumeGroup with no parent-group name; across two or more groups the rows are ambiguous. The table has a ResourceGroup column, the JSON does not.
[MINOR] internal/cli/drbdopts.go applyDRBDFlags, contradictory set/unset is nondeterministic
Iterating flags.Values (a map), rd drbd-options pvc-x --max-buffers=8000 --unset-max-buffers resolves to set or delete depending on map iteration order. Reject the contradiction or define precedence.
[MINOR] internal/cli/view/resource.go:99, sync percentage is dead in production
stateCell prints SyncTarget(NN%) only when VolumeSizesKib is populated, but the only production caller (handlers.go:77) never populates it — only the unit test does (resource_test.go:161). During resync the operator sees a bare SyncTarget, though docs/cli-design.md promises the percentage and color.normalise deliberately strips (NN%). The test is also vacuous coverage for a path production never takes. Populate VolumeSizesKib in the resource list handler or drop the feature and the doc claim.
[MINOR] internal/cli/output/machine.go:46, MachineSingle is dead code
Zero callers; every -m path goes through MachineList (double-nested [[...]]). The godoc asserts singletons are emitted flat, but no verb does so and no test covers it. Wire the intended verbs to it with a test, or drop it and correct the doc.
[MINOR] internal/cli/snapshot.go snapshotCreateMultiple, partial batch write
Snapshots are created one at a time with GroupSize = len(pairs); a failure on the Nth leaves a group whose members are fewer than its declared GroupSize. (Controller-side consequence under a suspend-io barrier not verified here.)
Caveats
- Exit-code model is internally consistent (usage/parse maps to 2, everything else to 10) but its upstream parity is unverified: semantic refusals (shrink-without-
--force, size-out-of-bounds, passphrase mismatch,snapshot rollback) return 10, not 2. If the upstream client returns 2 for any of them, a script branching on the code misclassifies a permanent client-side rejection as a retryable API failure. Pin these codes with tests. - Hermetic review: no live cluster contacted. This PR has no cluster-state surface (no charts, migrations, RBAC, CRD schema/storage changes; printer columns are additive), so there is no upgrade/fresh-install path to exercise.
Recommended follow-ups
- Run the
tests/e2e/cli-matrixsuite pointed atblockstorinstead of the python client (the author's stated acceptance criterion). It is the only layer that can confirm real-cluster exit codes, machine-output jq paths, and server-side table parity.
An empty passphrase is refused rather than stored. `passphrase.Read` cannot tell an empty Secret from a missing one, so an empty master key left create reporting "already set" and enter reporting "none set" — contradictory, with no way out through this CLI — while encrypting every volume with nothing. spawn-resources validates every size before it writes anything. It used to create the definition first, so a size typo left an orphan and the corrected retry then failed on "already exists": one typo cost a manual delete. create-multiple likewise checks every definition up front and unwinds what it created on a mid-batch failure, because a group with fewer members than its declared size strands the suspend-io barrier the controller opens only once the group is whole. Colour is parsed before the cluster is opened, so the same typo no longer exits 10 on an unreachable cluster and 2 on a reachable one, and a known-invalid invocation opens no connection. A malformed --limit is rejected instead of quietly returning everything, and `r l --help` answers about the command rather than failing as an unknown flag. Contradictory `--knob=x --unset-knob` is refused rather than resolved by map iteration order. Two machine outputs were poorer than the tables they mirror: query-size-info omitted the computed size that is the point of the command, and vg list omitted the parent group that makes its rows unambiguous. The sync percentage was dead in production — the only caller never populated the sizes it needs — so the handler now supplies them, keyed per definition rather than per volume number. MachineSingle had no callers and a godoc describing behaviour no verb implemented; both are gone. Exit codes for the semantic refusals are now pinned by test. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Reviewed at ed15a892e8ae51719db3b444da872a83fc98babd against merge-base b873285cd4ec48e47bfac256454ea016532553be, so the diff is exactly the delta. Build, go vet, and go test ./api/... ./internal/... ./pkg/drbd/... are green in a clean checkout.
Blocking
CRITICAL — snapshot create <rd> <snap> silently captures nothing (phantom backup)
The common no-nodes form writes a Snapshot with an empty Nodes slice and empty VolumeDefinitions straight into the store:
internal/cli/write_more.go:429-434—Nodes: run.Flags.Positionals[:count-2]is empty forsnapshot create <rd> <snap>, andVolumeDefinitionsis never set.pkg/store/k8s/snapshots.go:457-475—wireToCRDSnapshotSpeccopiesNodesverbatim and only emitsVolumeDefinitionswhen non-empty. No hydration happens in the store.- The hydration that makes a snapshot real lives only in the REST layer the CLI bypasses:
pkg/rest/snapshots.gohydrateSnapshotFromRDdefaultsNodestolistDiskfulNodes(rd)and copiesVolumeDefinitionsfrom the source RD. internal/controller/snapshot_controller.go:159-162explicitly treats an emptySpec.Nodesas degenerate and returns without capturing — its own comment states this is "unreachable in production" precisely because "the apiserver populates Spec.Nodes via hydrateSnapshotFromRD before persisting".- Satellites gate on
slices.Contains(snap.Spec.Nodes, self)(pkg/satellite/controllers/snapshot.go:90).
Net effect: blockstor snapshot create <rd> <snap> returns exit 0, no data is captured, and view.snapshotState reports the snapshot as Successful. Worse, because VolumeDefinitions is also empty, a later snapshot resource restore of such a snapshot hydrates zero volumes — again exit 0. snapshotCreateMultiple (internal/cli/snapshot.go:91-99) has the same hole when -n is omitted.
This falsifies the PR's central premise that going straight to the store yields "the same DTOs the REST apiserver would return without duplicating a line of wire↔CRD translation": the node/VD hydration is exactly that translation, and it is load-bearing.
Fix: hydrate Nodes (default to diskful replicas of the RD) and VolumeDefinitions (copy from the RD) client-side before Snapshots().Create, or route snapshot creation through the same hydration helper. Add a regression test asserting a created snapshot carries non-empty Nodes and VolumeDefinitions for the no-nodes form.
MAJOR — resource-definition clone snapshots diskless replicas and aborts
internal/cli/definition.go:150-153 — ensureCloneSnapshot appends every replica's node to snap.Nodes with no DISKLESS filter, whereas the REST clone path filters diskless/tie-breaker nodes out (pkg/rest/snapshots.go:503, :1086, :1174 — listDiskfulNodes). A source RD with a diskless tie-breaker replica produces a snapshot whose diskless node's satellite has no backing volume to snapshot → per-node Failed → the snapshot is stamped FAILED and the clone aborts. Filter to diskful replicas here as the REST path does.
MAJOR — docs claim server-side apply, implementation uses lossy wholesale Update
docs/cli-design.md:46 states "Modifications go through server-side apply with the CLI's own field manager." Nothing in the implementation does SSA. The write verbs do Get→mutate→Update: internal/cli/props.go:61-69, node.go:211-225 (evacuate/restore flags), write.go:154-168, write_more.go:271-288, write_more.go:480-503, pool.go:177-194. pkg/store/store.go documents repeatedly that the wholesale Update "silently drops concurrent peer additions/mutations… un-retried wire-snapshot replace (Bug 204b)" and provides PatchResourceSpec / PatchNodeSpec / PatchProps / PatchResourceGroup / PatchResourceDefinitionSpec / PatchVolumeDefinitionSpec for exactly these mutations. Concretely, blockstor r sp <node> <rd> <k> <v> racing the tie-breaker/migration reconciler's PatchResourceSpec reverts a freshly stamped flag with no error. Either use the Patch* APIs for the RMW verbs, or correct the design doc to describe the actual (non-SSA) semantics and its concurrency trade-off.
MAJOR — -l (short form of --layer-list) is accepted, consumes its value, and is silently dropped
internal/cli/flags.go:106 registers -l as a value flag, but assign (flags.go:326-339) has no case for it, so its value lands in Values["l"], which nothing reads — every consumer reads Values["layer-list"] (write.go:232, place.go:136, definition.go:61, write_more.go:537). blockstor rd create pvc-x -l drbd,storage exits 0 and creates the definition with the layer-stack override silently discarded. If -l is intended to carry a luks layer, the resulting volume is provisioned without that layer and with no signal. This directly contradicts this file's own principle (flags.go:194-196: "A flag that is parsed and then ignored is worse than one that is refused"). Fix: add case "-l", flagLayerList: to assign, or drop -l from valueFlags so it fails loudly. No test exercises -l — add a parse test asserting -l a,b populates the same field as --layer-list a,b.
Non-blocking (MINOR)
resource-group query-max-volume-sizeunder-reports capacity.internal/cli/definition.go:252-273dedups one pool per node (seen[pool.NodeName]) while iterating in store order, before thesort.SliceStablebyFreeCapacity. A node whose first-listed pool is small (or reportsFreeCapacity=0) shadows its larger pool, so the command can report a size smaller than reality — or0, which the view tells the operator means "cannot be placed at all". Sort first, or track the max per node.resource create --auto-placeignores extra positionals.internal/cli/write_more.go:346-362takesPositionals[0]as the definition and silently drops the rest; on a definition/node name collision it acts on the wrong object. Reject extra positionals in this branch.controller versionrequires a reachable cluster.internal/cli/app.go:169callsStoreForunconditionally before the handler, butcontrollerVersion(write.go:265-272) only prints the compiled-in version. Without a kubeconfig it exits 10 instead of printing the version.--limitis silently ignored by several listings.applyLimitis wired only intolisting[T]andresourceList.volumeDefinitionList(handlers.go:269),volumeGroupList(pool.go:239), andnodeInfo(node.go:234) accept--limit(validated at parse time) and then return everything.- Negative counts are accepted and silently no-op.
parseInt32(write_more.go:43-50) has no non-negative check.--place-count -3yieldsPlaceCount=-3, whichautoPlacetreats as "nothing to do" (exit 0);--vlmnr -5addresses a negative volume number instead of being refused.
Notes / follow-ups
- Machine-output (
-m,[[…]]) parity with the real consumer (tests/e2e/cli-matrixjq expressions) cannot be verified statically and the suite has not been run per the PR body; that remains the stated acceptance criterion before dropping the python dependency. - The additive CRD printer columns are upgrade-safe: only
additionalPrinterColumnsare added under the existingv1alpha1;served/storageare unchanged, so stored objects are unaffected.printcolumns_test.gopins the column set and is non-vacuous. - Verified clean: exit-code classification (usage vs API, colour parsed before cluster open), size math (overflow guard, 4 MiB/16 TiB bounds, shrink requires
--forcewith bounds still enforced), byte-identical colour-escape stripping, constant-time passphrase compare with empty-passphrase rejection, and noun/verb alias resolution.
snapshot create wrote a Snapshot with no nodes and no volume layout. The snapshot controller treats an empty Spec.Nodes as degenerate and returns without capturing, so the command exited 0, the listing showed the snapshot as healthy, and there was no data behind it. Restoring such a snapshot hydrated zero volumes, also with exit 0. The hydration that makes a snapshot real — nodes defaulted to the diskful replicas, volume definitions copied from the source — lives in the apiserver's hydrateSnapshotFromRD, not in the store. It is wire-to-CRD translation, and it is load-bearing, so a client that talks to the store directly has to carry it too. That is a real correction to this PR's premise, and the design doc no longer claims otherwise. Selecting the nodes properly also fixes clone: it snapshotted every replica including diskless witnesses, and a witness has no volume to capture, so that node failed and took the clone with it. A definition with no diskful replica at all is now refused rather than recorded as a success with nothing behind it. `-l` consumed its value and dropped it, so a layer stack pinned with the short form — a LUKS layer, say — was silently discarded and the volume came up without it. The design doc's server-side-apply claim was never true of the implementation; it now describes the actual read-modify-write semantics and records the Patch* migration as outstanding. 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: 1
🤖 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/snapshot.go`:
- Around line 212-226: Update the snapshot hydration flow around
snap.VolumeDefinitions so it returns an error when the list remains empty after
loading VolumeDefinitions().List, including when the replica initially has no
definitions. Preserve the existing population behavior for non-empty results,
and add a regression test covering a diskful replica with no volume definitions.
🪄 Autofix
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: e8150c8e-3e9c-4b5c-b5c8-36c28f272cfc
📒 Files selected for processing (6)
docs/cli-design.mdinternal/cli/definition.gointernal/cli/flags.gointernal/cli/snapshot.gointernal/cli/snapshot_test.gointernal/cli/write_more.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/cli/flags.go
- internal/cli/definition.go
- internal/cli/write_more.go
| if len(snap.VolumeDefinitions) == 0 { | ||
| vds, vdErr := run.Store.VolumeDefinitions().List(ctx, snap.ResourceName) | ||
| if vdErr != nil { | ||
| return fmt.Errorf("list volume definitions of %s: %w", snap.ResourceName, vdErr) | ||
| } | ||
|
|
||
| snap.VolumeDefinitions = make([]apiv1.SnapshotVolumeDef, 0, len(vds)) | ||
| for i := range vds { | ||
| snap.VolumeDefinitions = append(snap.VolumeDefinitions, apiv1.SnapshotVolumeDef{ | ||
| VolumeNumber: vds[i].VolumeNumber, | ||
| SizeKib: vds[i].SizeKib, | ||
| VolumeDefinitionProps: vds[i].Props, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject snapshots that have no volume definitions.
Line 212 hydrates snap.VolumeDefinitions, but it accepts an empty vds result. The command can then create a snapshot that restores successfully with zero volumes. This is the failure described in the function comment.
Return an error after hydration when len(snap.VolumeDefinitions) == 0. Add a regression test with a diskful replica and no volume definitions.
Proposed fix
if len(snap.VolumeDefinitions) == 0 {
vds, vdErr := run.Store.VolumeDefinitions().List(ctx, snap.ResourceName)
if vdErr != nil {
return fmt.Errorf("list volume definitions of %s: %w", snap.ResourceName, vdErr)
}
snap.VolumeDefinitions = make([]apiv1.SnapshotVolumeDef, 0, len(vds))
for i := range vds {
snap.VolumeDefinitions = append(snap.VolumeDefinitions, apiv1.SnapshotVolumeDef{
VolumeNumber: vds[i].VolumeNumber,
SizeKib: vds[i].SizeKib,
VolumeDefinitionProps: vds[i].Props,
})
}
}
+ if len(snap.VolumeDefinitions) == 0 {
+ return fmt.Errorf("%s has no volume definitions to snapshot", snap.ResourceName)
+ }📝 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.
| if len(snap.VolumeDefinitions) == 0 { | |
| vds, vdErr := run.Store.VolumeDefinitions().List(ctx, snap.ResourceName) | |
| if vdErr != nil { | |
| return fmt.Errorf("list volume definitions of %s: %w", snap.ResourceName, vdErr) | |
| } | |
| snap.VolumeDefinitions = make([]apiv1.SnapshotVolumeDef, 0, len(vds)) | |
| for i := range vds { | |
| snap.VolumeDefinitions = append(snap.VolumeDefinitions, apiv1.SnapshotVolumeDef{ | |
| VolumeNumber: vds[i].VolumeNumber, | |
| SizeKib: vds[i].SizeKib, | |
| VolumeDefinitionProps: vds[i].Props, | |
| }) | |
| } | |
| } | |
| if len(snap.VolumeDefinitions) == 0 { | |
| vds, vdErr := run.Store.VolumeDefinitions().List(ctx, snap.ResourceName) | |
| if vdErr != nil { | |
| return fmt.Errorf("list volume definitions of %s: %w", snap.ResourceName, vdErr) | |
| } | |
| snap.VolumeDefinitions = make([]apiv1.SnapshotVolumeDef, 0, len(vds)) | |
| for i := range vds { | |
| snap.VolumeDefinitions = append(snap.VolumeDefinitions, apiv1.SnapshotVolumeDef{ | |
| VolumeNumber: vds[i].VolumeNumber, | |
| SizeKib: vds[i].SizeKib, | |
| VolumeDefinitionProps: vds[i].Props, | |
| }) | |
| } | |
| } | |
| if len(snap.VolumeDefinitions) == 0 { | |
| return fmt.Errorf("%s has no volume definitions to snapshot", snap.ResourceName) | |
| } |
🤖 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 212 - 226, Update the snapshot
hydration flow around snap.VolumeDefinitions so it returns an error when the
list remains empty after loading VolumeDefinitions().List, including when the
replica initially has no definitions. Preserve the existing population behavior
for non-empty results, and add a regression test covering a diskful replica with
no volume definitions.
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Re-reviewed at 1618b4d10f0a4d4910cb630f5a12d9da164435c0. The round-1 blockers (phantom snapshot create, clone snapshotting diskless nodes, dropped -l, the SSA doc claim) are genuinely fixed with regression tests — thank you. This round goes deeper into the write and restore verbs and surfaces a systemic issue that the fixes above do not touch.
Root cause (design-level)
The CLI writes to the store directly (internal/cli/app.go StoreFor), bypassing pkg/rest. The python client this CLI replaces spoke to the REST surface and therefore inherited a whole layer of destructive-operation guards and pre-flight checks that live only in pkg/rest. The controller and satellite do not re-enforce most of them, so those safety refusals are simply gone for a store-direct client. The design note's premise that going straight to the store is "more correct" holds for reads and for creates (where the controllers do the allocation), but not for the destructive/mutating verbs: there the store is not an equivalent of the REST DTO, it is the layer underneath the guards.
The right shape is to route the destructive verbs through the same guard logic REST uses (extract a shared pre-flight package), or to move these invariants into the controller/admission so that any client — CLI, REST, or a future one — is safe. Patching each verb individually will keep leaking cases.
Concrete instances below. Line refs are at the reviewed SHA.
Critical (data loss)
-
migrate-diskcan delete the only diskful replica.internal/cli/resource.go:139-179:migrateDiskacceptssrc == dst(e.g.resource toggle-disk nodeA rd --migrate-from nodeA) and a destination that is already diskful, then stampsmigratingFromon it. The migration reconciler (internal/controller/resource_migration_controller.gopruneSrc) has no "last diskful replica" guard and deletes the source — which here is the same, and possibly only, diskful replica. The REST path rejects a diskful destination with 409. Rejectsrc == dstand an already-diskful destination in the CLI, or add the guard to the reconciler. -
resource deletebypasses the last-UpToDate-mid-resync guard (U130).internal/cli/write_more.go:398-414callsStore.Resources().Deletedirectly, with no sibling scan and no--forcehandling. Deleting the last UpToDate diskful replica while a peer is still SyncTarget strands the resync with no source — unrecoverable. The refusal exists only inpkg/rest/resource_delete_last_uptodate_u130.go. -
Restore onto nodes that do not hold the snapshot silently restores nothing.
internal/cli/snapshot.goplaceRestored(around 415-444) uses the requested nodes without validating they are insnap.Nodes. The satellite exhausts its restore-from-snapshot budget and degrades to a blankCreateVolume, presenting an empty replica as a good copy, with exit 0. The REST layer has exactly this guard (validateRestoreNodesHoldSnapshot, flagged P0 data integrity). Validate the target nodes againstsnap.Nodesbefore creating replicas. -
create-device-poolwipes devices before the pool is created, with no rollback.internal/cli/physical.go:64-83persistsAttachTo{Wipe:true}(which the satellite acts on) beforeStoragePools().Create. If the create fails, the devices are wiped with no registered pool. AdditionallyAlreadyExistson the pool is swallowed (exit 0 even if the existing pool has a different provider kind/props than requested), and the device-matching loop can match one device token against multiple entries across its aliases and stamp wipe on more than intended. Create the pool first, or make the wipe conditional on a successful create; fail loud on a conflicting existing pool.
Major
-
Shrink guard has a TOCTOU hole.
internal/cli/write_more.go:271-288:checkResizecompares the requested size against aVolumeDefinitions().Getthat can be served from a stale informer cache, and the subsequentUpdatewrites the absolute size with no re-validation against the freshly-read object. A resize racing a concurrent grow (CSI) can pass the>= cachedcheck without--forceand truncate a live, larger volume. The 4 MiB / 16 TiB bounds themselves are correctly enforced on every path including under--force— this is only the shrink-vs-current comparison. -
snapshot resource restoreignores the positional node names its own grammar uses.internal/cli/snapshot.goplaceRestoredreads only-n/--nodes; a restore given trailing positional node names (the upstream grammar) falls through to allsnap.Nodes, silently widening the restore scope. This contradicts the repository's own acceptance harnesstests/e2e/cli-matrix/snap-r-rst-stamps-resources.sh, which passes node names positionally and documents that a--node-nameflag is rejected — yet theplaceRestoreddoc comment promises exactly that flag. This path is not parity-compatible with the grammar the cli-matrix suite exercises. -
A value flag swallows the following token even when it is another flag.
internal/cli/flags.go:172-179:resource list -n --faultyparses asNodes=["--faulty"]and drops--faulty, returning an empty table with exit 0 — read by an operator as "no faulty resources". Same shape silently swaps intent for--storage-pool --force rd1. A value flag should reject a following token that looks like a flag. -
snapshot listshows "Successful" for a snapshot that has not been captured yet.internal/cli/view/views.go:184-200snapshotStatereturns "Successful" for any snapshot lacking a failure flag; it never checks a positive success marker. Mid-capture (no flags yet, empty Created column) it renders as Successful, so an operator or script reads a phantom backup as done. The REST/store contract stamps success only when every diskful peer has reported.
Also, each citing a REST counterpart the CLI omits:
internal/cli/snapshot.gorestore of a snapshot with noVolumeDefinitionsyields a zero-volume resource, exit 0 (no equivalent of the REST empty-shell refusal).internal/cli/snapshot.gosourcePoolOndoes not skip DISKLESS replicas, so a restored replica can be pinned to a diskless pool and never converge (REST filters diskless).internal/cli/definition.goensureCloneSnapshotreuses an existingclone-<target>snapshot without checking its state, so a failed clone poisons every retry until the snapshot is deleted by hand.internal/cli/write_more.gosnapshotCreateand friends skip the REST pre-flight that refuses a non-snapshot-capable pool (thick-LVM), which can silently invalidate on COW overflow.internal/cli/write_more.gonodeDeleteandinternal/cli/pool.gostoragePoolDeleteskip the in-use / evicted-node refusals, leaving orphan CRDs and broken reconcile.- Evacuate/restore (
node.gopatchNodeFlags), property sets,resource-group modify, andvolume-group createuse the wholesaleUpdatewhere the store provides conflict-safePatch*methods; a concurrent reconciler write is silently lost. (The design note now documents this for the property verbs; the node/group verbs have the same gap.) internal/cli/place.go+Nauto-place counts non-DISKLESS replicas while the placer's own tally excludes INACTIVE / evicted / lost, so+1can place more than one.internal/cli/snapshot.gotwo-step restore (volume-definition restore into a resource, then resource restore into the same one) fails onAlreadyExistsbecause the resource-restore verb unconditionally creates the definition and one handler serves both spellings.internal/cli/pool.govolumeGroupListignores its positional resource-group argument and lists every group;volume-group createon the same noun takes it positionally, so the grammar is split within one noun.- A
create-multipleinterrupted by ctx-cancel or SIGKILL can leave a short group (members < GroupSize); the controller then requeues it every second indefinitely with no assembly deadline and no GC.
Minor
resource-group adjust on a typo'd group name is a no-op with exit 0; --place-count 0 / --auto-place +0 are swallowed and fall back to the group policy; mixing --place-count and --auto-place resolves by loop order, not command-line order, dropping one silently; -p-value is a dead entry in valueFlags that eats an argument nothing reads; foreign value flags and -m are accepted on verbs that ignore them (empty stdout, exit 0); query-max-volume-size dedups one pool per node in list order before sorting by free capacity, so it can under-report; --limit is ignored by volumeDefinitionList / volumeGroupList / nodeInfo; negative --place-count / --vlmnr are accepted and silently no-op or address a negative volume; resource create --auto-place ignores extra positionals; controller version requires a reachable cluster to print the compiled-in version.
What is verified clean
hydrateSnapshot / diskfulNodesOf (the round-1 fix) are correct and mirror the REST hydration; the resize bounds hold under --force; the store's Update carries across controller-allocated identities (DRBD port / node-id / minor / seeded volumes), so a modify does not trigger a DRBD resync; snapshotCreateMultiple rolls back exactly the snapshots it created; delete verbs address exactly the named object; exit-code classification (usage = 2, API = 10) is consistent; table rendering is byte-identical after escape stripping; the machine [[…]] envelope is uniform; the constant-time passphrase compare is fine.
Recommend running the tests/e2e/cli-matrix suite against a stand before merge — several of the findings above (6 in particular) are things that suite is written to catch.
`modify` was the only verb on resource-definition and resource-group without a short form, while list/create/delete next to it all had one. Upstream spells it `m`, so `rd m` and `rg m` now resolve. `rd modify --resource-group <name>` also wrote the name through without checking it exists. Nothing downstream catches that: the controller treats an already materialised definition as self-sufficient, so a typo was accepted, stored, and only surfaced much later, when someone spawned from the definition and found no placement policy to work from. The group is now looked up first and a miss is refused, leaving the definition untouched. The partial-update test seeds both groups it moves between; the new rejection test covers the miss. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
`blockstor rd --help` died in the resolver as an unknown subcommand, and `blockstor rd modify --help` printed the entire command tree. Both are answers to questions nobody asked. Every verb in the registry now carries the argument synopsis that follows it, so `<noun> --help` lists that object's verbs with their arguments and `<noun> <verb> --help` prints just that command's usage line and its aliases. The synopses are not invented: each comes from the handler that implements the command — its doc comment, its positional checks, or the usage error it already raises. A drift guard requires every verb to either document its arguments or be listed as taking none, so help cannot go blank as verbs are added. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The CLI is not the only thing writing these CRDs. The satellite and the migration reconciler stamp properties and flags on the same objects, and another operator may be running the same verb at the same time. Every mutating verb here read an object, edited it locally and wrote the whole thing back, so whatever landed in between was reverted — silently, with exit 0. There is no wholesale Update left in the CLI. The property verbs, drbd-options, node evacuate/restore, resource-definition modify, resource-group modify, volume-group create, volume-definition set-size and the physical-device attach all go through the store's Patch* entry points, which fetch current state, apply the change to it, and retry the whole cycle on conflict. What the caller hands over is what changed, not what the object should become: the property accessors expose an edit(change) instead of a set(whole map), so the CLI can no longer express "make the bag exactly this" — only "put this key in it". Decisions move inside the patch for the same reason. set-size compared the requested size against a size read beforehand, so a concurrent grow left the decision made against a size that no longer existed and the absolute write truncated a live volume; the check now runs against the state the write lands on. volume-group create picks the next free volume number inside the patch, so two concurrent creates cannot pick the same one. Two store gaps closed on the way: ControllerPropsStore had only Set(whole map) — its own doc said partial updates belong in REST, which stopped being true once the CLI wrote CRDs directly — and PhysicalDeviceStore had no patch at all, so its attach CAS guard covered only the attach half of the race. The volume size bounds move onto the CRD as Minimum/Maximum. A bound enforced by one client is not a bound the data is subject to; there the API server holds it for the CLI, the REST layer, a controller and a stray kubectl apply alike. The shared store conformance fixtures used toy sizes below the floor and now use realistic ones. Both races are covered by tests that drive a real competing write through the store, and both were verified to fail without the fix: the volume ends at 4 GiB after a concurrent grow to 8 GiB, and the reconciler's property key disappears. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Raised from inside the store patch it came back wrapped in two layers of store context — 'update resource definition X: patch ResourceDefinition "X": usage: ...'. Whether the command names a knob at all is a property of the command line, not of the object, so it is decided before the store is touched. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Moving the edit inside the patch moved the "nothing to change" check after it, so a usage error cost a no-op write to the API server first. Which fields the command edits is a property of the command line, so it is decided up front; the changed-bookkeeping the closure needed for it goes away with it. Verified against a live cluster: resourceVersion is unchanged across the refusal. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Three findings from an independent review of the concurrency change. The device-attach guard was silently dropped. Moving stampDevices from PhysicalDevices().Update to PatchPhysicalDeviceSpec lost the check that lives only in Update — refuse a device some other pool already claimed. The attach carries Wipe, so overwriting a live claim does not merely re-point a record: it wipes the disk backing that pool, and a /dev/sdN name that shifted across a reboot reaches the path by accident rather than by operator error. The check is back, inside the patch closure, which is stronger than where it was: two concurrent create-device-pool runs both see AttachTo=nil when they look, so only a check made against the state the write lands on can reject the loser. The CRD size bound contradicted the REST spawn path, which skipped the [4096 KiB, 16 TiB] gate on the reasoning that the bound would apply later on a `vd c`. With the bound on the CRD field there is no later: the API server rejects midway through the handler, after the resource definition is created and while its volumes are being added, leaving the half-built definition that handler's validate-before-write block exists to prevent and returning a raw schema error instead of the rejection envelope. Spawn now applies validateVDSize, which emits the identical message for the non-positive class, so that wire shape is unchanged. The oversub probes used toy KiB capacities so their ratios read easily; they are scaled by 1024 and assert exactly what they did. The bound's retroactivity is now spelled out on the field: it validates on update too, so an object already outside it would be unwritable. No path can produce one — that is what the spawn gate above is for — so there is nothing to ratchet for, and the note says what the fix would be if there were. Also: `encryption create-passphrase` / `enter-passphrase` were listed as taking no arguments, so the help drift guard actively prevented documenting the passphrase they require. Every fix has a test verified to fail without it. Assisted-By: Claude Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Round 4's systemic finding is addressed: there is no wholesale What changedEvery mutating verb now goes through the store's The distinction that matters is not which method is called but what the caller hands over. A verb that reads an object, edits it locally and writes the result back replaces everything, so a key another writer added in between is reverted. A verb that hands over a change has it applied to whatever the object currently is. The property accessors therefore expose The same reasoning moves decisions inside the patch. Two store gaps closed on the way. What moved out of the client entirelyThe volume size bounds are now The shrink refusal stays in the write path deliberately: it is a policy with a The shared store conformance fixtures used toy sizes below the new floor and now use realistic ones. EvidenceBoth races have tests that drive a real competing write through the store, and both were verified to fail without the fix: Measured on a real 3-node cluster, 12 concurrent
Before the fix eight of those writers surfaced a raw Kubernetes 409 to the operator and the rest were lost silently, so eleven of twelve operator commands did nothing. Two follow-ups came out of self-review after the main change: the
Also in this push:
Independent review of the changeI ran an independent review over the concurrency commit before pushing. It returned four findings; all four held up against the code, and one was a regression the change itself introduced. The device-attach CAS guard was silently dropped. Moving The CRD bound contradicted the REST spawn path. Spawn skipped the The bound's retroactivity is now spelled out on the field. It validates on update as well as create, so an object already outside it would be unwritable. Nothing can produce one now — that is what the spawn gate is for — so there is nothing to ratchet for, and the note records what the fix would be ( The help drift guard prevented documenting a required argument on Each fix has a test proven to fail without it: |
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.
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.