Skip to content

Commit 29b961a

Browse files
kvapsclaude
andauthored
fix(controller): honour canonical DrbdOptions/auto-quorum key + quorum property semantics (corner B) (#97)
* fix(controller): honour canonical DrbdOptions/auto-quorum key The auto-quorum opt-out gate (isAutoQuorumDisabled) read the camelCase DrbdOptions/AutoQuorum property, which no production path ever writes. The upstream CLI, seedAutoQuorumDefaults, and the PropsInfo catalogue all use the kebab-case DrbdOptions/auto-quorum spelling, so a real 'set-property DrbdOptions/auto-quorum disabled' was silently ignored and the reconciler kept clobbering the operator's manual quorum policy on every pass (corner-case B1/B4). Read the canonical kebab key first, keep the camelCase spelling as a forward-compat fallback. The honours-disabled regression test drove the gate with the dead camelCase key and so masked the bug; it now drives with the canonical key and pins both spellings. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * feat(rest): empty-value set-property deletes the key Add a shared applyPropsModify property-bag core implementing the upstream LINSTOR semantic: an override_props entry with an empty value DELETES the key entirely rather than storing an empty string (UG9 auto-quorum NOTE: 'set-property KEY ""' = delete-property). Route the resource-definition and resource-group modify handlers through it so a 'rg/rd set-property DrbdOptions/Resource/on-no-quorum' with no value converges to key absence on both surfaces (corner-case B5 / I1). The explicit delete_props path is preserved and now shares the same core. Other GenericPropsModify sites still use the local pattern and are flagged for a follow-up consolidation. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(harness): prop_value await kind + auto-quorum replays (B1/B2/B5) Add an additive prop_value assertion kind to the replay harness that checks an RD/RG list-properties surface for a key being present with an exact value, or ABSENT when expected is omitted (empty-value=delete). Used to pin the auto-quorum property semantics at operator-CLI level. Two L7 replays: - auto-quorum-disabled-keeps-manual-quorum (B1/B4): disabling auto-quorum stops the reconciler from reverting manual quorum=off; the TieBreaker stays (gated on a separate prop). - auto-quorum-empty-value-restores-automatism (B2/B5): TB created despite auto-quorum=disabled before spawn; clearing the prop with an empty value deletes the key and the automatism resumes. HARNESS CHANGE (flagged): lib.sh gains the prop_value check_assertion case; replay-runner.sh header documents it. Additive only — existing await kinds untouched. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(cli-matrix): auto-quorum disabled keeps manual quorum (B1/B4) L6 operator-CLI cell pinning corner-case B1/B4: with auto-quorum active the automatism overrides a manual quorum=off (B4); after 'set-property DrbdOptions/auto-quorum disabled' the operator's manual quorum=off sticks across reconciles and the TieBreaker stays put (B1). Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(rest): pin default quorum pair as deliberate delta (B6) Corner-case B6: upstream auto-quorum default pair is quorum=majority + on-no-quorum=io-error (UG9 TIP). blockstor deliberately seeds on-no-quorum=suspend-io instead (Bug 297, P1 data-loss class). The majority half matches upstream verbatim; the on-no-quorum value is a documented divergence. Add a guard test pinning the actual blockstor seed so a flip back to io-error fails loudly, and record the divergence as known-delta row 60. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(controller): auto-quorum=disabled stops on-no-quorum reseed (B5) Stand regression (BS v0.1.9): with auto-quorum=disabled set via the real CLI and on-no-quorum cleared (empty=delete), the reconciler re-seeded on-no-quorum=suspend-io because the camelCase-key gate never fired — so an operator could never clear the prop. The kebab-key fix short-circuits ensureTiebreaker before setQuorum's companion seed. Pin it: under auto-quorum=disabled, ensureTiebreaker must not re-stamp quorum or re-seed on-no-quorum. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(rest): quorum-minimum-redundancy passthrough as deliberate delta (B8) Corner-case B8: upstream validates the quorum-minimum-redundancy value and rejects bogus input (FAIL_INVLD_PROP, oracle-confirmed); blockstor is permissive on DrbdOptions values (no per-key validator, drbdadm rejects truly-invalid values at .res render time). Pin valid-value passthrough and record the validation gap as known-delta row 61. Refine row 60 (B6) with the oracle finding that upstream does not stamp on-no-quorum at all. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(harness): short rd names for auto-quorum replays LINSTOR caps resource-definition names at 48 chars; the workflow names alone overflow the default replay-<name>-<rand>. Pin short vars.rd. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * docs(parity): record r-d quorum-prop auto-removal divergence (B3b) Corner-case B3b: upstream removes quorum/on-no-quorum props + prints INFO when r d drops a resource below quorum feasibility; blockstor stamps quorum=off (Bug 309: satellite needs an explicit rendered value) and prints no INFO. DRBD behaviour is equivalent; record as known delta. 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>
1 parent e1ae265 commit 29b961a

12 files changed

Lines changed: 946 additions & 29 deletions

docs/cli-parity-known-deltas.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ Row IDs match the command-catalogue indexes used by `cli-parity-refresh.sh` (see
2727
| 54 | `schedule l` | MISSING_FEATURE | 2026-12-31 | Schedules subsystem deferred to follow-up wave. |
2828
| 55 | `key-value-store list` | WIRE_SHAPE | 2026-09-30 | BS exposes the KVS endpoints but the wire shape lacks `props.LinstorKvs/...` namespace nesting; CSI uses a flat key set so unblocked. |
2929
| 60 | `vd s <rd> <vn> <smaller>` (shrink) | BEHAVIOR | permanent | BLOCKSTOR_STRICTER: upstream LINSTOR has permitted `vd set-size` downward since 1.8.0 for layer stacks that support it (e.g. STORAGE/ZFS-only); BS rejects ALL shrinks with `FAIL_INVLD_VLM_SIZE` (HTTP 400) unless the operator opts in with `force=true` (body field or `?force=true`). Rationale: every BS RD is DRBD-backed and DRBD cannot shrink past the on-disk metadata position without destroying it, so an unconditional reject is the safe default. The `force=true` escape hatch covers the post-`resize2fs -s` operator. Pinned by `tests/e2e/cli-matrix/vd-shrink-rejected.sh` (L6), `replay/vd-resize-full-lifecycle.yaml` (L7), and `pkg/rest/volume_definitions_test.go::TestVolumeDefinitionsUpdateShrink{Rejected,WithForceAccepted}` + `pkg/rest/bug_383_*` (L1). |
30+
| 61 | `rd lp` default `on-no-quorum` (auto-quorum seed) | BEHAVIOR | permanent | Corner-case B6. Upstream auto-quorum default pair is `quorum majority` + `on-no-quorum io-error` (UG9 TIP ~4251-4253). BS seeds `on-no-quorum=suspend-io` instead — Bug 297 (P1 data-loss): io-error freezes the minority replica across partition heal and breaks auto-promote; suspend-io blocks I/O until quorum returns, then resumes cleanly. `quorum majority` half MATCHES upstream verbatim; only the `on-no-quorum` value diverges, by design. Pinned by `TestCornerB_DefaultQuorumPairIsDeliberateDelta`. |
31+
| 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`. |
32+
| 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`. |
3033

3134
## Open (block merge until addressed)
3235

internal/controller/ensure_tiebreaker_test.go

Lines changed: 123 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -565,11 +565,20 @@ func TestEnsureTiebreakerHonoursAutoQuorumDisabled(t *testing.T) {
565565
// Operator opted out of auto-quorum and set the per-RD policy
566566
// explicitly. `quorum=off` + `on-no-quorum=io-error` is the
567567
// "scale-out fast, fail-loud" combo from UG9.
568+
//
569+
// Corner-case B1/B4: drive the gate with the CANONICAL kebab-case
570+
// `DrbdOptions/auto-quorum` key — the exact spelling the upstream
571+
// `linstor (rg|rd) set-property … DrbdOptions/auto-quorum disabled`
572+
// CLI emits. The earlier revision of this test stamped the
573+
// camelCase `DrbdOptions/AutoQuorum`, which the (then-broken) gate
574+
// also read — so the test passed while a real cluster's CLI opt-out
575+
// was silently ignored. Pinning the kebab key here guards against a
576+
// regression back to the dead-key gate.
568577
rd := &blockstoriov1alpha1.ResourceDefinition{
569578
ObjectMeta: metav1.ObjectMeta{Name: "pvc-manual-quorum"},
570579
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
571580
Props: map[string]string{
572-
"DrbdOptions/AutoQuorum": "disabled",
581+
"DrbdOptions/auto-quorum": "disabled",
573582
"DrbdOptions/Resource/quorum": "off",
574583
"DrbdOptions/Resource/on-no-quorum": "io-error",
575584
},
@@ -613,12 +622,92 @@ func TestEnsureTiebreakerHonoursAutoQuorumDisabled(t *testing.T) {
613622

614623
// Auto-quorum opt-out marker must survive the round-trip
615624
// unchanged — a stamp-and-strip refactor would be a regression.
616-
if got := final.Spec.Props["DrbdOptions/AutoQuorum"]; got != "disabled" {
617-
t.Errorf("AutoQuorum prop: got %q, want %q (must round-trip verbatim)",
625+
if got := final.Spec.Props["DrbdOptions/auto-quorum"]; got != "disabled" {
626+
t.Errorf("auto-quorum prop: got %q, want %q (must round-trip verbatim)",
618627
got, "disabled")
619628
}
620629
}
621630

631+
// TestEnsureTiebreakerDisabledDoesNotReseedOnNoQuorum is the B5
632+
// regression that the dev-stand reproduced against the released
633+
// (camelCase-bug) binary: with `DrbdOptions/auto-quorum=disabled` set
634+
// via the real CLI (kebab key) and NO on-no-quorum prop, the
635+
// reconciler must NOT re-seed `on-no-quorum=suspend-io`.
636+
//
637+
// Stand evidence (BS v0.1.9, buggy): after
638+
//
639+
// rd set-property R DrbdOptions/auto-quorum disabled
640+
// rd set-property R DrbdOptions/Resource/on-no-quorum (empty=delete)
641+
//
642+
// the CLI deleted the key, then the reconciler re-stamped
643+
// `on-no-quorum=suspend-io` because isAutoQuorumDisabled read the dead
644+
// camelCase key, returned false, and ran setQuorum's companion seed —
645+
// so an operator could never actually clear the prop. With the
646+
// kebab-key fix, isAutoQuorumDisabled returns true, ensureTiebreaker
647+
// short-circuits before setQuorum, and the prop stays absent.
648+
func TestEnsureTiebreakerDisabledDoesNotReseedOnNoQuorum(t *testing.T) {
649+
t.Parallel()
650+
651+
scheme := newScheme(t)
652+
st := store.NewInMemory()
653+
ctx := context.Background()
654+
655+
// 2 diskful — the shape that, under active auto-quorum, would
656+
// drive quorum=majority + the suspend-io companion seed.
657+
for _, n := range []string{"n1", "n2"} {
658+
if err := st.Nodes().Create(ctx, &apiv1.Node{
659+
Name: n, Type: apiv1.NodeTypeSatellite,
660+
}); err != nil {
661+
t.Fatalf("seed node %s: %v", n, err)
662+
}
663+
664+
if err := st.Resources().Create(ctx, &apiv1.Resource{
665+
Name: "pvc-b5", NodeName: n,
666+
}); err != nil {
667+
t.Fatalf("seed replica %s: %v", n, err)
668+
}
669+
}
670+
671+
// Operator disabled auto-quorum (canonical kebab key) and cleared
672+
// on-no-quorum (the CLI delete already removed it — Props has no
673+
// on-no-quorum entry).
674+
rd := &blockstoriov1alpha1.ResourceDefinition{
675+
ObjectMeta: metav1.ObjectMeta{Name: "pvc-b5"},
676+
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
677+
Props: map[string]string{
678+
"DrbdOptions/auto-quorum": "disabled",
679+
},
680+
},
681+
}
682+
683+
cli := fake.NewClientBuilder().WithScheme(scheme).WithObjects(rd).Build()
684+
685+
rec := &controllerpkg.ResourceDefinitionReconciler{
686+
Client: cli,
687+
Scheme: scheme,
688+
Store: st,
689+
}
690+
691+
if err := rec.EnsureTiebreaker(ctx, rd); err != nil {
692+
t.Fatalf("EnsureTiebreaker: %v", err)
693+
}
694+
695+
final := &blockstoriov1alpha1.ResourceDefinition{}
696+
if err := cli.Get(ctx, types.NamespacedName{Name: "pvc-b5"}, final); err != nil {
697+
t.Fatalf("Get RD: %v", err)
698+
}
699+
700+
if got, present := final.Spec.Props["DrbdOptions/Resource/on-no-quorum"]; present {
701+
t.Errorf("B5: on-no-quorum re-seeded to %q under auto-quorum=disabled; "+
702+
"must stay ABSENT (the deleted prop must not come back)", got)
703+
}
704+
705+
if got, present := final.Spec.Props["DrbdOptions/Resource/quorum"]; present {
706+
t.Errorf("B5: quorum re-stamped to %q under auto-quorum=disabled; "+
707+
"the reconciler must leave quorum policy to the operator", got)
708+
}
709+
}
710+
622711
// TestIsAutoQuorumDisabled pins the helper across the input shapes
623712
// the production code can encounter: nil RD, nil Props, missing key,
624713
// explicit `disabled` (canonical and mixed case), and the two other
@@ -645,7 +734,29 @@ func TestIsAutoQuorumDisabled(t *testing.T) {
645734
want: false,
646735
},
647736
{
648-
name: "disabled (canonical)",
737+
// Corner-case B1/B4: canonical kebab-case key — the exact
738+
// spelling the upstream CLI writes.
739+
name: "disabled (canonical kebab key)",
740+
rd: &blockstoriov1alpha1.ResourceDefinition{
741+
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
742+
Props: map[string]string{"DrbdOptions/auto-quorum": "disabled"},
743+
},
744+
},
745+
want: true,
746+
},
747+
{
748+
name: "Disabled (mixed case from manual paste, kebab key)",
749+
rd: &blockstoriov1alpha1.ResourceDefinition{
750+
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
751+
Props: map[string]string{"DrbdOptions/auto-quorum": "Disabled"},
752+
},
753+
},
754+
want: true,
755+
},
756+
{
757+
// Forward-compat fallback: a legacy hand-stamped camelCase
758+
// value must still opt out.
759+
name: "disabled (legacy camelCase key fallback)",
649760
rd: &blockstoriov1alpha1.ResourceDefinition{
650761
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
651762
Props: map[string]string{"DrbdOptions/AutoQuorum": "disabled"},
@@ -654,10 +765,14 @@ func TestIsAutoQuorumDisabled(t *testing.T) {
654765
want: true,
655766
},
656767
{
657-
name: "Disabled (mixed case from manual paste)",
768+
// The canonical kebab key wins over a stale legacy value.
769+
name: "kebab disabled overrides legacy non-disabled",
658770
rd: &blockstoriov1alpha1.ResourceDefinition{
659771
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
660-
Props: map[string]string{"DrbdOptions/AutoQuorum": "Disabled"},
772+
Props: map[string]string{
773+
"DrbdOptions/auto-quorum": "disabled",
774+
"DrbdOptions/AutoQuorum": "io-error",
775+
},
661776
},
662777
},
663778
want: true,
@@ -666,7 +781,7 @@ func TestIsAutoQuorumDisabled(t *testing.T) {
666781
name: "suspend-io (auto-set instruction, not disable)",
667782
rd: &blockstoriov1alpha1.ResourceDefinition{
668783
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
669-
Props: map[string]string{"DrbdOptions/AutoQuorum": "suspend-io"},
784+
Props: map[string]string{"DrbdOptions/auto-quorum": "suspend-io"},
670785
},
671786
},
672787
want: false,
@@ -675,7 +790,7 @@ func TestIsAutoQuorumDisabled(t *testing.T) {
675790
name: "io-error (auto-set instruction, not disable)",
676791
rd: &blockstoriov1alpha1.ResourceDefinition{
677792
Spec: blockstoriov1alpha1.ResourceDefinitionSpec{
678-
Props: map[string]string{"DrbdOptions/AutoQuorum": "io-error"},
793+
Props: map[string]string{"DrbdOptions/auto-quorum": "io-error"},
679794
},
680795
},
681796
want: false,

internal/controller/resourcedefinition_controller.go

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -496,9 +496,29 @@ func isAutoQuorumDisabled(rd *blockstoriov1alpha1.ResourceDefinition) bool {
496496
return false
497497
}
498498

499-
const propKey = "DrbdOptions/AutoQuorum"
499+
// Corner-case B1/B4: the canonical upstream key is the kebab-case
500+
// `DrbdOptions/auto-quorum` — that is what the `linstor (rg|rd)
501+
// set-property … DrbdOptions/auto-quorum disabled` CLI writes, what
502+
// `seedAutoQuorumDefaults` stamps on RD create, and what the
503+
// PropsInfo catalogue advertises. The earlier camelCase spelling
504+
// (`DrbdOptions/AutoQuorum`) was never written by any production
505+
// path, so the gate silently never fired against a real cluster:
506+
// auto-quorum=disabled set via the CLI was ignored and the
507+
// reconciler kept overwriting the operator's manual quorum policy
508+
// on every pass. Read the kebab key first; keep the camelCase
509+
// spelling as a forward-compat fallback so any hand-stamped legacy
510+
// value still opts out.
511+
const (
512+
propKey = "DrbdOptions/auto-quorum"
513+
legacyPropKey = "DrbdOptions/AutoQuorum"
514+
)
515+
516+
value, ok := rd.Spec.Props[propKey]
517+
if !ok {
518+
value = rd.Spec.Props[legacyPropKey]
519+
}
500520

501-
return strings.EqualFold(rd.Spec.Props[propKey], "disabled")
521+
return strings.EqualFold(value, "disabled")
502522
}
503523

504524
// isTiebreakerSuppressed reports whether the operator recently

0 commit comments

Comments
 (0)