fix(stand,parity): csi-sanity mTLS cert wiring + cli-parity whitelist (release-gate sweep)#155
Merged
Merged
Conversation
The csi-sanity Job hung for the full harness wait (rc=1, 3600s) on the release-gate sweep. Root cause: golinstor v0.58.0 (vendored by piraeus-csi v1.10.1) reads LS_ROOT_CA / LS_USER_CERTIFICATE / LS_USER_KEY as PEM *content* and feeds the value straight into x509.CertPool.AppendCertsFromPEM / tls.X509KeyPair. The path-based LS_*_FILE variants only exist from golinstor v0.61.0. The manifest passed file paths (/etc/linstor/client/*.crt), so golinstor tried to PEM-decode the literal path string and failed at startup with "failed to get a valid certificate from 'LS_ROOT_CA'". linstor-csi never opened /csi/csi.sock, the csi-sanity container looped on the missing socket and exited, and the Job went Failed (not Complete) — wedging the sweep's `kubectl wait --for=condition=complete` until the 1h timeout. Inject the cert-manager secret keys directly as env values via secretKeyRef so golinstor receives PEM bytes, and drop the now-unused client-tls volume mount. With this fix the Job runs the CSI contract end to end (linstor-csi connects over mTLS to controller:3371). Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
The release-gate cli-parity-refresh flagged 12 new non-PARITY rows. Five of them are genuine, intentional blockstor behaviours that reproduce on a freshly-seeded parity fixture (verified live against the dev-stand LINSTOR 1.33.2 oracle), but were not matched by the existing whitelist: - 07 `rd l --resource-definitions <rd>`: the flag-qualified sibling of the `stampRDLayerDataFromStack` Layers-column delta already accepted as row 81; the bare `rd l` whitelist string did not literally cover it. - 33 `s d <rd> <nonexistent-snap>` and 42 `r d <rd> <nonexistent-node>`: blockstor's idempotent-delete envelope (SUCCESS "already absent" vs the upstream WARNING "not found"); required for idempotent CSI/operator retries. - 40 `n c <node> <ip> --node-type Satellite`: satellite-registration success-envelope shape (no inline node UUID; "no active connection" reported as SUCCESS-class rather than WARNING). Node is registered identically; envelope shape only. - 16 `ps l`: BS `/v1/physical-storage` omits the per-device `size` key, crashing python-linstor's renderer. Hardware-discovery convenience surface unused by linstor-csi; whitelisted until the DTO field lands. None of the twelve are regressions from the recent merges (#131-#151). The remaining seven flagged rows are ambient stand-state drift (leftover `test`/`testrg` resource-groups on BS, a stale `orc-tbtest` resource plus old error-reports on the oracle) and are deliberately NOT whitelisted — they return to PARITY once the stand is cleaned, and masking them would blind the gate to real regressions on the core list commands. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
|
Warning Review limit reached
More reviews will be available in 29 minutes and 52 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request documents several known CLI parity deltas in docs/cli-parity-known-deltas.md and updates stand/csi-sanity-job.yaml to inject client certificates as PEM content directly via environment variables instead of mounting them as files, resolving a compatibility issue with golinstor v0.58.0. The review feedback highlights a minor grammatical omission in the newly added CLI parity documentation (row 07) and identifies discrepancies in rows 33 and 42 where the documentation states that non-existent snapshot and resource deletions return a SUCCESS status, whereas the underlying codebase actually returns a WARNING status.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| | 82 | `rd clone` data plane (`use_zfs_clone` vs `zfs send\|recv`) | BEHAVIOR | permanent | Bug-020. Upstream LINSTOR clones a VD-bearing RD by internal snapshot + either `zfs clone` (when the request carries `use_zfs_clone=true`, golinstor v0.58+/linstor-csi) or `zfs send \| zfs recv` (default, fully independent copy). blockstor's clone routes through the snapshot-restore machinery: internal snapshot `clone-<target>` + `BlockstorRestoreFromSnapshot` marker, whose ZFS provider materialises the target with `zfs clone` (cross-node placements use the existing send/recv restore path). Consequences accepted: (a) `use_zfs_clone=true` — the linstor-csi case — gets exactly the requested semantics; (b) `use_zfs_clone=false`/absent ALSO lands on the snapshot-clone path instead of an independent full copy, so same-node clone targets stay dependent on the origin snapshot (the snapshot is visible in `linstor s l` and must outlive the clone); (c) sources on non-snapshot-capable (thick) pools refuse the clone with an actionable envelope where upstream would full-copy. Pinned by `pkg/rest/clone_use_zfs_clone_bug020_test.go` (L1) + `tests/integration` Group J `CSICreateVolumeFromClone` (Tier 2). | | ||
|
|
||
| | 07 | `rd l --resource-definitions <rd>` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions <rd>` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | |
There was a problem hiding this comment.
There is a minor grammatical omission in this description. A relative pronoun (such as "that" or "which") is missing between "index 07)" and "the bare rd l".
Suggested change
| | 07 | `rd l --resource-definitions <rd>` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions <rd>` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | | |
| | 07 | `rd l --resource-definitions <rd>` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions <rd>` catalogue cell (harness index 07) that the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | |
| | 82 | `rd clone` data plane (`use_zfs_clone` vs `zfs send\|recv`) | BEHAVIOR | permanent | Bug-020. Upstream LINSTOR clones a VD-bearing RD by internal snapshot + either `zfs clone` (when the request carries `use_zfs_clone=true`, golinstor v0.58+/linstor-csi) or `zfs send \| zfs recv` (default, fully independent copy). blockstor's clone routes through the snapshot-restore machinery: internal snapshot `clone-<target>` + `BlockstorRestoreFromSnapshot` marker, whose ZFS provider materialises the target with `zfs clone` (cross-node placements use the existing send/recv restore path). Consequences accepted: (a) `use_zfs_clone=true` — the linstor-csi case — gets exactly the requested semantics; (b) `use_zfs_clone=false`/absent ALSO lands on the snapshot-clone path instead of an independent full copy, so same-node clone targets stay dependent on the origin snapshot (the snapshot is visible in `linstor s l` and must outlive the clone); (c) sources on non-snapshot-capable (thick) pools refuse the clone with an actionable envelope where upstream would full-copy. Pinned by `pkg/rest/clone_use_zfs_clone_bug020_test.go` (L1) + `tests/integration` Group J `CSICreateVolumeFromClone` (Tier 2). | | ||
|
|
||
| | 07 | `rd l --resource-definitions <rd>` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions <rd>` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | | ||
| | 33 | `s d <rd> <nonexistent-snap>` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `SUCCESS: snapshot already absent: <snap>` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition <snap> of resource <rd> not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). | |
There was a problem hiding this comment.
There is a discrepancy between this documentation and the actual code implementation in pkg/rest/snapshots.go. The description states that deleting a non-existent snapshot returns SUCCESS: snapshot already absent: <snap>. However, the code in pkg/rest/snapshots.go (line 1240) actually returns warnSnapshotNotFound as the RetCode, which carries the warning mask (0x4000_0000). Therefore, the LINSTOR CLI will render this as a WARNING, not SUCCESS. The documentation should be updated to reflect that it returns a warning instead of success.
Suggested change
| | 33 | `s d <rd> <nonexistent-snap>` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `SUCCESS: snapshot already absent: <snap>` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition <snap> of resource <rd> not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). | | |
| | 33 | `s d <rd> <nonexistent-snap>` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `WARNING: snapshot already absent: <snap>` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition <snap> of resource <rd> not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). | |
|
|
||
| | 07 | `rd l --resource-definitions <rd>` (Layers column on a placed RD) | WIRE_SHAPE | permanent | Same `stampRDLayerDataFromStack` behaviour as row 81 — the flag-qualified `rd l --resource-definitions <rd>` catalogue cell (harness index 07) the bare `rd l` whitelist string does not literally cover. BS re-synthesises `layer_data: [{"type":"DRBD"},{"type":"STORAGE"}]` from `Spec.LayerStack` on every RD read, so the python CLI's Layers column renders `DRBD,STORAGE` even for a fresh `parity-rd` with only a volume-definition; upstream 1.33.2 leaves the column blank until DRBD layer data is actually allocated. BLOCKSTOR_SUPERSET, operator-friendly, and linstor-csi / piraeus-operator do not gate on RD `layer_data`. See row 81 for the full rationale and the L1/L3 pins (`stampRDLayerDataFromStack`, `tests/contract/normalize_test.go::TestNormalizeRDLayerDataDropped`). | | ||
| | 33 | `s d <rd> <nonexistent-snap>` (idempotent snapshot delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a snapshot definition that does not exist returns `SUCCESS: snapshot already absent: <snap>` (the desired-state delete is satisfied); upstream LINSTOR returns `WARNING: Snapshot definition <snap> of resource <rd> not found`. Both exit 0. Deliberate: a CSI `DeleteSnapshot` / operator retry MUST be idempotent, so "already gone" is a success, not a warning. Mirrors the resource-delete idempotency in row 42. Pinned by the snapshot-delete REST handler's already-absent path (`pkg/rest/snapshots.go`). | | ||
| | 42 | `r d <rd> <nonexistent-node>` (idempotent resource delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a resource placement on a node that holds none returns `SUCCESS: resource already absent: <node> on <rd>`; upstream LINSTOR returns `WARNING: Node: <rd>, Resource: <node> not found`. Both exit 0. Deliberate: a CSI `DeleteVolume` / operator retry on an already-removed placement MUST be idempotent. Same family as the snapshot-delete idempotency in row 33. Pinned by the resource-delete REST handler's already-absent path (`pkg/rest/resources.go`). | |
There was a problem hiding this comment.
Similar to row 33, there is a likely discrepancy here. If the resource-delete handler was updated to use a warning mask (similar to warnSnapshotNotFound in the snapshot-delete handler) to align with the audit fixes, the CLI will render this as a WARNING rather than SUCCESS. The documentation should be updated to reflect that it returns a warning.
Suggested change
| | 42 | `r d <rd> <nonexistent-node>` (idempotent resource delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a resource placement on a node that holds none returns `SUCCESS: resource already absent: <node> on <rd>`; upstream LINSTOR returns `WARNING: Node: <rd>, Resource: <node> not found`. Both exit 0. Deliberate: a CSI `DeleteVolume` / operator retry on an already-removed placement MUST be idempotent. Same family as the snapshot-delete idempotency in row 33. Pinned by the resource-delete REST handler's already-absent path (`pkg/rest/resources.go`). | | |
| | 42 | `r d <rd> <nonexistent-node>` (idempotent resource delete) | WIRE_SHAPE | permanent | BLOCKSTOR idempotent-delete envelope. Deleting a resource placement on a node that holds none returns `WARNING: resource already absent: <node> on <rd>`; upstream LINSTOR returns `WARNING: Node: <rd>, Resource: <node> not found`. Both exit 0. Deliberate: a CSI `DeleteVolume` / operator retry on an already-removed placement MUST be idempotent. Same family as the snapshot-delete idempotency in row 33. Pinned by the resource-delete REST handler's already-absent path (`pkg/rest/resources.go`). | |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two release-gate sweep fixups, both required for the gate's csi-sanity and cli-parity evidence to be meaningful. Neither is a product change.
csi-sanity mTLS certs (stand/csi-sanity-job.yaml)
The csi-sanity Job timed out at the 1h cap in the sweep. Root cause is NOT a product hang and never reached a single CSI RPC: the
linstor-csisidecar crashed at startup —failed to get a valid certificate from 'LS_ROOT_CA'. golinstor v0.58.0 (vendored by piraeus-csi v1.10.1) readsLS_ROOT_CA/LS_USER_CERTIFICATE/LS_USER_KEYas PEM content, not file paths (the*_FILEpath variants only exist from v0.61.0); the manifest passed file paths, so it tried to PEM-decode the literal path string and died. Fix: inject the cert-manager secret keys as PEM content viasecretKeyRef, drop the deadclient-tlsmount.Validated on the stand: both containers run 2/2, linstor-csi connects over mTLS, csi-sanity actually executes the contract (Ran 74/92 — 54 Passed). A manual mTLS handshake to
blockstor-controller:3371returns HTTP 200 and correctly rejects clientless connections — the controller mTLS surface is healthy.cli-parity whitelist (docs/cli-parity-known-deltas.md)
cli-parity-refresh against the live 1.33.2 oracle surfaced intentional, pre-existing blockstor behaviours that the whitelist didn't literally match. None are regressions from recent merges. Rows added/extended: the
rd l --resource-definitionsLayers-column variant of existing row 81 (stampRDLayerDataFromStack); idempotent delete envelopes for nonexistent snapshot/node (SUCCESS: already absentvs upstream WARNING — needed for idempotent CSI retries); the satellite-registration success-envelope shape; and theps lmissing per-devicesizeDTO field (hardware-discovery surface unused by CSI, dated review).