fix(effectiveprops): controller-tier DRBD options must lose to closer scopes (corner C)#98
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR refactors effective property resolution in blockstor to enforce correct DRBD options precedence across scopes (controller → resource group → resource definition → resource), adds comprehensive unit and integration tests validating the precedence logic, and documents three corner-case parity deltas in the CLI surface. ChangesDRBD Options Precedence Implementation and Validation
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 refactors the DRBD options resolution logic to correctly enforce the precedence hierarchy (Controller -> ResourceGroup -> ResourceDefinition -> Resource), ensuring that closer scopes override higher scopes. It also adds extensive unit, integration, and E2E tests to cover various corner cases (C1-C6) and updates the known deltas documentation. The review feedback suggests a performance optimization in pkg/effectiveprops/effective.go to avoid redundant map allocations and copies by passing the properties directly to drbd.ResolveOptions.
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.
Comment on lines
+107
to
+112
| ctrlMerged := mergeScopeProps(ctrlProps, nil) | ||
| rgMerged := mergeScopeProps(rgProps, rgInfo.Props) | ||
| rdMerged := mergeScopeProps(rdProps, rdInfo.Props) | ||
| resMerged := mergeScopeProps(resProps, target.Spec.Props) | ||
|
|
||
| out := drbd.ResolveOptions(ctrlMerged, rgMerged, rdMerged, resMerged) |
There was a problem hiding this comment.
Performance Optimization: Avoid Redundant Map Allocations and Copies
In the current implementation, ctrlMerged, rgMerged, and rdMerged are created by copying the respective properties into new maps via mergeScopeProps:
ctrlMerged:ctrlPropsis already a fresh map returned byemitScopeProps. CreatingctrlMergedviamergeScopeProps(ctrlProps, nil)simply duplicates this map unnecessarily.rgMergedandrdMerged:rgInfo.PropsandrdInfo.Propscontain only non-DRBD properties. However,drbd.ResolveOptionsexplicitly filters out and ignores any keys that do not start withPropPrefix("DrbdOptions/") for the first three scopes (controller,resourceGroup, andresourceDef). Therefore, mergingrgInfo.PropsandrdInfo.Propshas no effect on the final output, making these merge operations redundant.
By passing ctrlProps, rgProps, and rdProps directly to drbd.ResolveOptions, we can avoid 3 unnecessary map allocations and multiple maps.Copy operations on every reconciliation pass.
Suggested change
| ctrlMerged := mergeScopeProps(ctrlProps, nil) | |
| rgMerged := mergeScopeProps(rgProps, rgInfo.Props) | |
| rdMerged := mergeScopeProps(rdProps, rdInfo.Props) | |
| resMerged := mergeScopeProps(resProps, target.Spec.Props) | |
| out := drbd.ResolveOptions(ctrlMerged, rgMerged, rdMerged, resMerged) | |
| resMerged := mergeScopeProps(resProps, target.Spec.Props) | |
| out := drbd.ResolveOptions(ctrlProps, rgProps, rdProps, resMerged) |
Andrei Kvapil (kvaps)
force-pushed
the
corner/c-drbd-options-hierarchy
branch
from
63617b1 to
6209de8
Compare
Member
Author
|
Confirmatory oracle evidence for C3-C6 (captured on the stand against upstream 1.33.2 after the PR was opened):
Branch was rebased onto main (known-deltas row renumbering only, no code changes). |
… scopes controller drbd-options persists into ControllerConfig.Spec.ExtraProps (raw, untyped), while rd/rg drbd-options are transcoded into the typed Spec.DRBDOptions. The effective-props resolver copied the controller ExtraProps AFTER the RG/RD/Resource typed+raw merge, so a cluster-wide controller default wrongly overrode a closer scope's explicit override — violating LINSTOR's 'closer to the resource wins' rule. Flatten each scope (Controller/RG/RD/Resource) into a single DRBD-props map (typed-emitted ∪ ExtraProps) and run them all through one precedence walk (drbd.ResolveOptions), so closer scopes win regardless of whether a value was stored typed or as an ExtraProp. Controller-tier inheritance (value reaches a resource that sets nothing) is preserved. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…(C3) linstor rd/rg/r drbd-options --unset-<opt> deletes the flattened DrbdOptions/... key; the store transcode round-trip must clear the matching typed slot so the value reverts to inherited/default, while sibling overrides in the same section survive untouched. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
…(C1/C2) Add an operator-CLI convergence gate for the effectiveprops controller- tier precedence fix: - L6 cli-matrix/drbd-options-hierarchy-controller.sh: set max-buffers at the controller, autoplace an RD, assert drbdsetup show inherits it (C1), then set a closer RD-level max-buffers and assert it wins (C2). - L7 replay/drbd-options-hierarchy-controller.yaml: same sequence under the replay harness. - New additive await kind 'drbd_option' in operator-harness/lib.sh that probes drbdsetup show on a node's satellite pod, documented in replay-runner.sh. SHARED-HARNESS CHANGE (additive only). Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Corner-cases C4 (connection-scoped resource drbd-peer-options), C5 (node-connection drbd-peer-options) and C6 (per-object-class option validation) are unimplemented / permissive on main. Document them as accepted deltas (#56/#57/#58) and pin current behavior: - C4: PUT to the bare resource-connections drbd-peer-options path is not handled (no silent persist) — canary flips if the surface gets wired. - C6: a net{}-class option on a volume-definition is accepted and stored permissively (upstream rejects with FAIL_INVLD_PROP) — canary flips when the option-class validator lands. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Andrei Kvapil (kvaps)
force-pushed
the
corner/c-drbd-options-hierarchy
branch
from
6209de8 to
af9afbf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
pkg/effectiveprops/effective_test.go (1)
143-206: ⚡ Quick winAdd a resource-scope override assertion to this precedence regression.
This refactor also changes
resProps := emitScopeProps(target.Spec.DRBDOptions, target.Spec.ExtraProps), but the L1 coverage stops at RD. A break whereResourceno longer overrides RD/RG/controller would still pass this suite.➕ Suggested test extension
func TestResolve_RGOverridesController_RDOverridesRG(t *testing.T) { scheme := newScheme(t) @@ if got["DrbdOptions/Net/max-buffers"] != "2000" { t.Fatalf("RG must win over controller when RD is unset: got %q, want 2000", got["DrbdOptions/Net/max-buffers"]) } + + // Resource must win over every parent scope as the closest level. + rd.Spec.DRBDOptions = &blockstoriov1alpha1.DRBDOptions{ + Net: &blockstoriov1alpha1.DRBDNetOptions{MaxBuffers: int32Ptr(3000)}, + } + res.Spec.DRBDOptions = &blockstoriov1alpha1.DRBDOptions{ + Net: &blockstoriov1alpha1.DRBDNetOptions{MaxBuffers: int32Ptr(4000)}, + } + + got, err = effectiveprops.Resolve(context.Background(), client.Reader(cli), res, rd) + if err != nil { + t.Fatalf("Resolve (resource override): %v", err) + } + + if got["DrbdOptions/Net/max-buffers"] != "4000" { + t.Fatalf("resource must win over RD/RG/controller: got %q, want 4000", + got["DrbdOptions/Net/max-buffers"]) + } }🤖 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 `@pkg/effectiveprops/effective_test.go` around lines 143 - 206, The test is missing a check that a Resource-scoped override beats RD/RG/controller; add a Resource override and assert it wins: set res.Spec.DRBDOptions (or res.Spec.ExtraProps) to a value (e.g. MaxBuffers=4000 or ExtraProps "DrbdOptions/Net/max-buffers":"4000"), call effectiveprops.Resolve(...) again and assert got["DrbdOptions/Net/max-buffers"] == "4000"; this verifies the refactor around emitScopeProps/target.Spec.* hasn't broken resource-level precedence.tests/e2e/cli-matrix/drbd-options-hierarchy-controller.sh (1)
48-56: ⚡ Quick winUse
register_strict_cleanuphere instead of a bare EXIT trap.This cell bypasses the shared e2e cleanup path, so it misses the suite’s standard resource cleanup and cluster-state verification behavior.
Based on learnings: "Applies to tests/e2e/*.sh : Register cleanup for e2e test scenarios via
register_strict_cleanupfromtests/e2e/lib.shinstead of baretrapdirectives to ensure proper resource cleanup and cluster state verification"🤖 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 `@tests/e2e/cli-matrix/drbd-options-hierarchy-controller.sh` around lines 48 - 56, Replace the bare EXIT trap with the shared e2e cleanup registrar: keep the cleanup() function (which unsets controller-level knob via "${LCTL[@]}" controller drbd-options --unset-max-buffers, calls delete_rd "$RD", assert_no_orphans "$RD" and linstor_cli_teardown) but remove the trap cleanup EXIT and instead call register_strict_cleanup cleanup so the test uses the suite’s standard strict cleanup and cluster-state verification logic.
🤖 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 `@docs/cli-parity-known-deltas.md`:
- Line 34: The delta row for "node-connection drbd-peer-options" (Corner-case
C5) references a stale pointer "`#56`"; update that reference in
docs/cli-parity-known-deltas.md to the current row id or an explicit issue/PR
link so traceability is restored—locate the table row containing the command
`node-connection drbd-peer-options` and replace "deferred with `#56`" with the
correct identifier (e.g., the new row number or the canonical issue/PR URL) and
ensure the surrounding text still reads correctly.
In `@pkg/rest/drbd_options_corner_cases_c4_c6_test.go`:
- Around line 52-59: The test currently treats any non-2xx as success, which
allows unrelated 5xx responses to pass; update the assertion in the test that
inspects resp.StatusCode (the block that currently checks "if resp.StatusCode >=
200 && resp.StatusCode < 300") to explicitly require the mux fall-through codes
by asserting resp.StatusCode == 404 || resp.StatusCode == 405, and adjust the
t.Fatalf message in that failure branch to mention the expected 404/405 rather
than any non-2xx so the test fails for wired/broken handlers.
In `@tests/e2e/cli-matrix/drbd-options-hierarchy-controller.sh`:
- Around line 101-115: Summary: the test probes kernel DRBD state before
confirming controller/RD Status has converged, causing race failures; fix by
inserting explicit Status-convergence waits. After wait_replica_count "$RD" 2
and before the first wait_max_buffers call, call the existing e2e helper that
waits for LINSTOR resource status convergence (e.g., wait_resource_status or
wait_resource_converged) for "$RD" on the chosen node so the controller settings
are applied; likewise after running "${LCTL[@]} resource-definition drbd-options
--max-buffers ..." add another status-convergence wait for "$RD" before calling
wait_max_buffers again so the RD override has reconciled into the
satellite/kernel; use the same helper that other tests use for asserting
REST→satellite→kernel convergence.
In `@tests/operator-harness/replay/drbd-options-hierarchy-controller.yaml`:
- Around line 23-25: Add an explicit default for the substitution key {{rd}}
inside the top-level vars map (alongside sp: stand) so the replay YAML does not
rely on runner-side fallbacks; ensure the file's top-level metadata also
includes name (kebab-case matching the filename), description, and
prerequisites.min_nodes, and that steps[] include cmd[], expect_exit (and
optional await), with optional teardown[] and invariants[] to comply with the
replay YAML contract.
- Around line 52-58: The test added an unsupported await kind "drbd_option"
(await.kind: drbd_option) that violates the replay YAML contract; either add
"drbd_option" to the replay validation/allowlist or change the test to use an
allowed kind. Locate the replay schema/validator that enforces await.kind (the
validation rule that currently permits only replica_count, disk_state,
all_uptodate, replica_diskless, no_tiebreaker, sync_clean, resource_absent,
rd_absent, vd_size_kib, pvc_capacity, pod_md5_invariant) and update it to
include "drbd_option", and ensure any related error messages/tests are adjusted,
or revert the two occurrences in drbd-options-hierarchy-controller.yaml (lines
with await.kind: drbd_option) to one of the permitted kinds.
---
Nitpick comments:
In `@pkg/effectiveprops/effective_test.go`:
- Around line 143-206: The test is missing a check that a Resource-scoped
override beats RD/RG/controller; add a Resource override and assert it wins: set
res.Spec.DRBDOptions (or res.Spec.ExtraProps) to a value (e.g. MaxBuffers=4000
or ExtraProps "DrbdOptions/Net/max-buffers":"4000"), call
effectiveprops.Resolve(...) again and assert got["DrbdOptions/Net/max-buffers"]
== "4000"; this verifies the refactor around emitScopeProps/target.Spec.* hasn't
broken resource-level precedence.
In `@tests/e2e/cli-matrix/drbd-options-hierarchy-controller.sh`:
- Around line 48-56: Replace the bare EXIT trap with the shared e2e cleanup
registrar: keep the cleanup() function (which unsets controller-level knob via
"${LCTL[@]}" controller drbd-options --unset-max-buffers, calls delete_rd "$RD",
assert_no_orphans "$RD" and linstor_cli_teardown) but remove the trap cleanup
EXIT and instead call register_strict_cleanup cleanup so the test uses the
suite’s standard strict cleanup and cluster-state verification logic.
🪄 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
Run ID: b53af3b8-37dd-43e2-adb8-e89768d4c3a6
📒 Files selected for processing (9)
docs/cli-parity-known-deltas.mdpkg/effectiveprops/effective.gopkg/effectiveprops/effective_test.gopkg/rest/drbd_options_corner_cases_c4_c6_test.gopkg/store/k8s/drbd_unset_revert_c3_test.gotests/e2e/cli-matrix/drbd-options-hierarchy-controller.shtests/operator-harness/lib.shtests/operator-harness/replay-runner.shtests/operator-harness/replay/drbd-options-hierarchy-controller.yaml
| | 62 | `rd sp DrbdOptions/Resource/quorum-minimum-redundancy <bad>` | BEHAVIOR | permanent | Corner-case B8. Upstream VALIDATES generic DRBD option values against the DRBD schema and rejects e.g. `quorum-minimum-redundancy bogus` with FAIL_INVLD_PROP (oracle: `value 'bogus' is not valid … has to match (1-32) or (all\|majority\|off)`, rc=10). BS's property bag is permissive — it accepts and stores any DrbdOptions value verbatim (no per-key value validator); an invalid value is rejected downstream by drbdadm at `.res` render time. Valid values pass through and propagate to the `options{}` block correctly. Pinned by `TestCornerB_QuorumMinimumRedundancyPassthrough`. | | ||
| | 63 | `r d` quorum-property auto-removal INFO + prop strip | BEHAVIOR | permanent | Corner-case B3b. Upstream LINSTOR, on an `r d` that drops a 3-node resource below quorum feasibility, REMOVES `DrbdOptions/Resource/quorum` + `on-no-quorum` from the RD and prints two INFO lines ("…was removed as there are not enough resources for quorum", UG9 ~1380-1401). BS instead STAMPS `DrbdOptions/Resource/quorum=off` (does not remove it) and prints no INFO — Bug 309: the satellite renders an explicit quorum value into the `.res`; an absent prop would fall back to a DRBD built-in that differs from the intended `off`. Resulting DRBD behaviour (`quorum off`) is equivalent; the divergence is the visible prop shape (`off` present vs absent) and the missing INFO envelope. Stamp-off pinned by `TestEnsureTiebreakerOffOnSingleReplica`. | | ||
| | 64 | `resource-connection drbd-peer-options` / `resource drbd-peer-options` | MISSING_FEATURE | 2026-12-31 | Corner-case C4: per-(rd,nodeA,nodeB) connection-scoped DRBD peer-options are NOT wired on `main` — the resource-connections REST surface only implements `/paths` (multi-path addresses). The early scenario-5.W04 in-memory registry did not survive the K8s-store migration. Controller/RG/RD/Resource-scope DRBD options (the common path) work; per-pair tuning is deferred to a dedicated feature PR. CSI does not depend on per-pair peer-options. | | ||
| | 65 | `node-connection drbd-peer-options` | MISSING_FEATURE | 2026-12-31 | Corner-case C5: `node-connection set-property` / `drbd-peer-options` props are persisted on `ControllerConfig.Spec.NodeConnections` and round-trip via list/get, but are NOT consumed by the `.res` render path — so they do not apply to the resources of the node pair. Persist-only today; render wiring deferred with #56. | |
There was a problem hiding this comment.
Stale delta-row reference (#56) should be updated.
Line 34 references “deferred with #56”, but #56 is not present in the current accept-list, so this pointer is now ambiguous after renumbering. Please update it to the current row id (or explicit issue/PR reference) to keep traceability intact.
Based on learnings: "When adding a new CLI verb or wire-shape change, update docs/cli-parity-known-deltas.md with intentional new divergence (row id, command, delta_kind, accepted_until, why)".
🤖 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-parity-known-deltas.md` at line 34, The delta row for
"node-connection drbd-peer-options" (Corner-case C5) references a stale pointer
"`#56`"; update that reference in docs/cli-parity-known-deltas.md to the current
row id or an explicit issue/PR link so traceability is restored—locate the table
row containing the command `node-connection drbd-peer-options` and replace
"deferred with `#56`" with the correct identifier (e.g., the new row number or the
canonical issue/PR URL) and ensure the surrounding text still reads correctly.
Comment on lines
+52
to
+59
| // No handler is registered for the bare peer-options path, so the | ||
| // mux falls through. We accept any non-2xx (404 / 405) — the point | ||
| // is that the option is NOT silently persisted as if it took effect. | ||
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | ||
| t.Fatalf("C4 delta drift: resource-connection drbd-peer-options PUT returned %d "+ | ||
| "(connection-scoped peer-options now appear wired — update known-deltas #56 "+ | ||
| "and add a real round-trip/render test)", resp.StatusCode) | ||
| } |
There was a problem hiding this comment.
Tighten the route-absence assertion to 404/405.
This canary currently passes on any non-2xx, including unrelated 5xx failures. Since the intended contract here is mux fall-through for an unwired endpoint, assert 404/405 explicitly so a broken-but-wired handler does not silently keep C4 pinned.
Suggested change
- // No handler is registered for the bare peer-options path, so the
- // mux falls through. We accept any non-2xx (404 / 405) — the point
- // is that the option is NOT silently persisted as if it took effect.
- if resp.StatusCode >= 200 && resp.StatusCode < 300 {
+ // No handler is registered for the bare peer-options path, so the
+ // mux should fall through with a routing-level failure.
+ if resp.StatusCode != http.StatusNotFound && resp.StatusCode != http.StatusMethodNotAllowed {
t.Fatalf("C4 delta drift: resource-connection drbd-peer-options PUT returned %d "+
"(connection-scoped peer-options now appear wired — update known-deltas `#56` "+
"and add a real round-trip/render test)", resp.StatusCode)
}📝 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.
Suggested change
| // No handler is registered for the bare peer-options path, so the | |
| // mux falls through. We accept any non-2xx (404 / 405) — the point | |
| // is that the option is NOT silently persisted as if it took effect. | |
| if resp.StatusCode >= 200 && resp.StatusCode < 300 { | |
| t.Fatalf("C4 delta drift: resource-connection drbd-peer-options PUT returned %d "+ | |
| "(connection-scoped peer-options now appear wired — update known-deltas #56 "+ | |
| "and add a real round-trip/render test)", resp.StatusCode) | |
| } | |
| // No handler is registered for the bare peer-options path, so the | |
| // mux should fall through with a routing-level failure. | |
| if resp.StatusCode != http.StatusNotFound && resp.StatusCode != http.StatusMethodNotAllowed { | |
| t.Fatalf("C4 delta drift: resource-connection drbd-peer-options PUT returned %d "+ | |
| "(connection-scoped peer-options now appear wired — update known-deltas `#56` "+ | |
| "and add a real round-trip/render test)", resp.StatusCode) | |
| } |
🤖 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 `@pkg/rest/drbd_options_corner_cases_c4_c6_test.go` around lines 52 - 59, The
test currently treats any non-2xx as success, which allows unrelated 5xx
responses to pass; update the assertion in the test that inspects
resp.StatusCode (the block that currently checks "if resp.StatusCode >= 200 &&
resp.StatusCode < 300") to explicitly require the mux fall-through codes by
asserting resp.StatusCode == 404 || resp.StatusCode == 405, and adjust the
t.Fatalf message in that failure branch to mention the expected 404/405 rather
than any non-2xx so the test fails for wired/broken handlers.
Comment on lines
+101
to
+115
| echo ">> wait for 2 diskful replicas" | ||
| wait_replica_count "$RD" 2 | ||
|
|
||
| # Pick a diskful node to probe the kernel config on. | ||
| node=$(linstor_diskful_nodes "$RD" | head -1) | ||
| [[ -n "$node" ]] || die "no diskful node for $RD" | ||
|
|
||
| echo ">> [C1] assert controller max-buffers ($CTRL_MB) inherited into $RD on $node" | ||
| wait_max_buffers "$node" "$CTRL_MB" | ||
|
|
||
| echo ">> [C2] rd drbd-options --max-buffers $RD_MB $RD (closer scope override)" | ||
| "${LCTL[@]}" resource-definition drbd-options --max-buffers "$RD_MB" "$RD" >/dev/null | ||
|
|
||
| echo ">> [C2] assert RD max-buffers ($RD_MB) now wins over controller ($CTRL_MB) on $node" | ||
| wait_max_buffers "$node" "$RD_MB" |
There was a problem hiding this comment.
Assert Status convergence before probing drbdsetup show.
This L6 cell waits for replica count only, then reads kernel DRBD state for both C1 and C2. That leaves the assertion racing a not-yet-converged reconcile/adjust cycle, which is outside the cli-matrix contract for these tests.
As per coding guidelines, "L1 unit tests must run on every commit using go test ./..." is not applicable here, but "L6 operator-CLI e2e tests must test real linstor CLI → REST → satellite → DRBD kernel interactions and assert Status convergence under tests/e2e/cli-matrix/"
🤖 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 `@tests/e2e/cli-matrix/drbd-options-hierarchy-controller.sh` around lines 101 -
115, Summary: the test probes kernel DRBD state before confirming controller/RD
Status has converged, causing race failures; fix by inserting explicit
Status-convergence waits. After wait_replica_count "$RD" 2 and before the first
wait_max_buffers call, call the existing e2e helper that waits for LINSTOR
resource status convergence (e.g., wait_resource_status or
wait_resource_converged) for "$RD" on the chosen node so the controller settings
are applied; likewise after running "${LCTL[@]} resource-definition drbd-options
--max-buffers ..." add another status-convergence wait for "$RD" before calling
wait_max_buffers again so the RD override has reconciled into the
satellite/kernel; use the same helper that other tests use for asserting
REST→satellite→kernel convergence.
Comment on lines
+23
to
+25
| vars: | ||
| sp: stand | ||
|
|
There was a problem hiding this comment.
Add an explicit default for {{rd}} under vars.
The runner has a fallback today, but this replay file contract requires defaults for the substitutions it uses instead of relying on runner-side implicit behavior.
As per coding guidelines, "Replay YAML files must include: name (kebab-case, matches filename), description, prerequisites.min_nodes, vars with defaults for {{rd}}, {{sp}}, etc., steps[] with cmd[], expect_exit, and optional await assertion, teardown[], and invariants[]"
🤖 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 `@tests/operator-harness/replay/drbd-options-hierarchy-controller.yaml` around
lines 23 - 25, Add an explicit default for the substitution key {{rd}} inside
the top-level vars map (alongside sp: stand) so the replay YAML does not rely on
runner-side fallbacks; ensure the file's top-level metadata also includes name
(kebab-case matching the filename), description, and prerequisites.min_nodes,
and that steps[] include cmd[], expect_exit (and optional await), with optional
teardown[] and invariants[] to comply with the replay YAML contract.
Comment on lines
+52
to
+58
| await: | ||
| kind: drbd_option | ||
| rd: "{{rd}}" | ||
| node: "{{node1}}" | ||
| key: max-buffers | ||
| expected: "36864" | ||
| timeout_s: 120 |
There was a problem hiding this comment.
await.kind: drbd_option is outside the replay YAML contract.
This workflow introduces a new await kind, but the authoritative replay schema still restricts await.kind to the existing allowlist. Please update that contract/validation in the same PR, or keep this workflow on the supported set.
As per coding guidelines, "Replay YAML await.kind must use one of: replica_count, disk_state, all_uptodate, replica_diskless, no_tiebreaker, sync_clean, resource_absent, rd_absent, vd_size_kib, pvc_capacity, pod_md5_invariant"
Also applies to: 62-68
🤖 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 `@tests/operator-harness/replay/drbd-options-hierarchy-controller.yaml` around
lines 52 - 58, The test added an unsupported await kind "drbd_option"
(await.kind: drbd_option) that violates the replay YAML contract; either add
"drbd_option" to the replay validation/allowlist or change the test to use an
allowed kind. Locate the replay schema/validator that enforces await.kind (the
validation rule that currently permits only replica_count, disk_state,
all_uptodate, replica_diskless, no_tiebreaker, sync_clean, resource_absent,
rd_absent, vd_size_kib, pvc_capacity, pod_md5_invariant) and update it to
include "drbd_option", and ensure any related error messages/tests are adjusted,
or revert the two occurrences in drbd-options-hierarchy-controller.yaml (lines
with await.kind: drbd_option) to one of the permitted kinds.
…render The hierarchy-funnel refactor routed every scope's props through drbd.ResolveOptions, which by design only threads through NON-DRBD props from the most-specific (resource) scope. That silently dropped load-bearing non-DRBD ExtraProps carried on the RG/RD scopes — most importantly `FileSystem/Type` (and `FileSystem/MkfsParams`), stamped on the RD by `linstor rd set-property FileSystem/Type ext4` and by the linstor-csi CreateVolume path. The satellite gates its mkfs / `primary --force` seed path on `dr.GetProps()["FileSystem/Type"]` (hasFileSystemConfigured). With the key dropped, a fresh replica never mkfs-writes, the DRBD current-UUID never rotates past the deterministic day0 GI, and the controller's append-only RD.Spec.Initialized latch (ensureSkipInitSyncDecision) never flips — the `respawn-standalone-wedge` e2e fails in SETUP with "RD.Spec.Initialized never latched true after 3 UpToDate replicas". Re-overlay the non-DRBD ExtraProps from the RG then RD scope after the precedence walk (RD wins over RG; resource already won via ResolveOptions). DRBD keys are skipped — already resolved by the walk. The controller scope is deliberately not re-overlaid, preserving the C1 contract that cluster-wide non-DRBD knobs (Aux/zone) do not leak onto every resource. Adds TestResolve_RDNonDRBDExtraPropSurvives pinning the exact production RD shape (on-no-quorum typed, auto-quorum + FileSystem/Type in ExtraProps) that the prior analytic reconstruction missed. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
Member
Author
|
Root cause of the 3× Fix: re-overlay non- |
This was referenced Jun 4, 2026
Andrei Kvapil (kvaps)
added a commit
that referenced
this pull request
Jun 4, 2026
…ine (#101) * fix(stand): neutralize backtick command substitution in config-patch heredoc The unquoted <<YAML heredoc carried comment lines with backtick pairs (lvextend/drbdmeta/open(...)). On hosts where those binaries exist on PATH the backticks execute as command substitution and corrupt the generated config-patch.yaml, so talosctl rejects it and 'make up' fails. CI runners lack the binaries, which is why this only bit dev hosts. The heredoc must stay unquoted for $INSTALL_IMG expansion, so drop the backticks from the comments instead. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(e2e): widen respawn-wedge Initialized-latch wait to 180s The RD.Spec.Initialized latch rides DRBD current-UUID rotation through the satellite observer and informer-cache trail; under loaded CI runners the 60s window lapses while the latch is merely late. Seen during PR #98 triage where the late latch was initially mistaken for a flake before the real regression was isolated. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(cli-matrix): count only diskful replicas in drbd-options hierarchy cell wait_replica_count counts ALL Resource rows including the auto-tiebreaker witness, so a healthy 2-diskful + 1-TB layout (3 rows) never satisfied count=2. Count diskful nodes instead; the L7 replay twin already passes on the stand. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> --------- Signed-off-by: Andrei Kvapil <kvapss@gmail.com> Co-authored-by: Claude <noreply@anthropic.com>
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.
Summary
Closes the group-C corner cases of the LINSTOR parity campaign (DRBD options hierarchy). One real precedence bug fixed,
--unsetrevert pinned, three deliberate divergences whitelisted. Validated against an upstream LINSTOR 1.33.2 oracle on the dev stand.The bug (C1/C2)
Controller-tier DRBD options did not lose to closer scopes in the effective-properties resolver: a
linstor controller drbd-options --max-buffers=36864would keep winning over a later RD-level override (rd drbd-options --max-buffers=8192 <rd>). Upstream semantics ("closer to the resource wins") give the RD value precedence. Fixed inpkg/effectivepropsso the chain resolves controller < RG < RD.Oracle evidence (upstream 1.33.2, captured on the stand): controller-level value inherited by a fresh resource (36864 in the rendered net{} section), then RD override flips it to 8192; net options land in
net {}, peer-devicec-*options land in the per-peer connectiondisk {}section — both controllers route sections identically after the fix.Pinned (C3)
--unset-<drbd-opt>deletes the key and the effective value reverts to the inherited/default tier (pkg/store/k8s/drbd_unset_revert_c3_test.go).Deliberate divergences (known-deltas rows 56-58)
resource drbd-peer-options/resource-connection drbd-peer-optionsalias parity — wire-shape pinned, divergence in connection-section routing recorded.node-connection drbd-peer-optionssurface (deciseconds scope) — not implemented; rejection pinned.--protocolon a VD); blockstor's prop bag is permissive at the wire, render-level rejection. Pinned.Tests
pkg/effectiveprops/effective_test.go(precedence matrix),pkg/rest/drbd_options_corner_cases_c4_c6_test.go,pkg/store/k8s/drbd_unset_revert_c3_test.go.tests/e2e/cli-matrix/drbd-options-hierarchy-controller.sh(controller set → inherit → RD override → kernel-visible via drbdsetup show).tests/operator-harness/replay/drbd-options-hierarchy-controller.yaml.tests/operator-harness/lib.sh+replay-runner.sh— new assertion support for the options check.Stand validation
The L6 cell reproduces the bug on the stand's pinned v0.1.9 image (controller value 36864 survives RD override — pre-fix repro) while the oracle shows the target 8192; the cell passes only with this PR's image. The launcher will deploy this branch build to the dev stand and verify L6+L7 PASS before merge.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation