Skip to content

Commit a939c9b

Browse files
kvapsclaude
andauthored
fix(placer): upgrade auto-tiebreaker witness on --auto-place over place-count (corner-D2b) (#111)
* fix(placer): upgrade auto-tiebreaker witness on --auto-place over place-count (corner-D2b) The autoplacer treated a node holding the auto-tiebreaker witness (DISKLESS + TIE_BREAKER) as fully "taken", so `r c --auto-place +1` (and the absolute `--auto-place 3`) on the standard 2-diskful + 1-witness shape failed with a shortfall ("Not enough nodes ... Replica count: 3") even though the witness node was available to host a diskful replica. Upstream LINSTOR upgrades the witness in place. Mark a witness-only node as an UPGRADE candidate instead of taken: the placer now promotes the existing witness (strip DISKLESS+TIE_BREAKER, stamp StorPoolName) via PatchResourceSpec rather than creating a fresh Resource that would collide. The flag transition is shared with the explicit `r c <node> --storage-pool` toggle-disk path through the new apiv1.PromoteWitnessFlags helper so the two paths can never drift. Co-Authored-By: Claude <noreply@anthropic.com> Signed-off-by: Andrei Kvapil <kvapss@gmail.com> * test(corner-d2b): pin autoplace witness-upgrade at L6 + L7 L6: new cli-matrix cell r-c-autoplace-plus-one-over-tb.sh asserts the auto-tiebreaker witness on the third node is upgraded IN PLACE by `r c --auto-place +1` (witness node hosts a diskful replica afterward, 0 tiebreakers, 3 diskful total). L7: extend replay/r-c-autoplace-plus-one.yaml with tiebreaker_present (before +1) and no_tiebreaker (after +1) assertions so the witness upgrade is observable at the operator-CLI replay tier, not just the +1 arithmetic. 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 2e247b9 commit a939c9b

6 files changed

Lines changed: 556 additions & 43 deletions

File tree

pkg/api/v1/resource.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -300,3 +300,45 @@ type StoragePoolModify struct {
300300
UUID string `json:"uuid,omitempty"`
301301
State string `json:"state,omitempty"`
302302
}
303+
304+
// PromoteWitnessFlags returns the Flags slice for a diskless / TIE_BREAKER
305+
// witness promoted to diskful, plus whether the input was actually a
306+
// diskless witness. It is the single source of truth for the
307+
// "witness → diskful" flag transition shared by the explicit
308+
// `linstor r c <node> <rd> --storage-pool ...` toggle-disk path
309+
// (pkg/rest.promoteDisklessReplica) and the autoplace path
310+
// (pkg/placer, when `--auto-place +1` lands on a node that already
311+
// holds the auto-tiebreaker witness — corner-D2b).
312+
//
313+
// wantDiskful=true drops both DISKLESS and TIE_BREAKER (full promote to
314+
// diskful). wantDiskful=false drops only TIE_BREAKER and keeps DISKLESS
315+
// (the linstor-csi make-available fallback: a plain DISKLESS replica,
316+
// no longer a controller-owned witness). The second return is true when
317+
// at least one of DISKLESS / TIE_BREAKER was present, so callers can
318+
// distinguish "promoted a witness" from "the replica was already
319+
// diskful" (a real conflict the caller must surface as 409).
320+
//
321+
// The input slice is never mutated — a fresh slice is returned.
322+
func PromoteWitnessFlags(flags []string, wantDiskful bool) ([]string, bool) {
323+
wasDiskless := false
324+
keep := make([]string, 0, len(flags))
325+
326+
for _, flag := range flags {
327+
switch flag {
328+
case ResourceFlagTieBreaker:
329+
wasDiskless = true
330+
case ResourceFlagDiskless:
331+
wasDiskless = true
332+
333+
if wantDiskful {
334+
continue
335+
}
336+
337+
keep = append(keep, flag)
338+
default:
339+
keep = append(keep, flag)
340+
}
341+
}
342+
343+
return keep, wasDiskless
344+
}
Lines changed: 200 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,200 @@
1+
// SPDX-License-Identifier: Apache-2.0
2+
3+
/*
4+
Copyright 2026 Cozystack contributors.
5+
6+
Licensed under the Apache License, Version 2.0 (the "License");
7+
you may not use this file except in compliance with the License.
8+
You may obtain a copy of the License at
9+
10+
http://www.apache.org/licenses/LICENSE-2.0
11+
12+
Unless required by applicable law or agreed to in writing, software
13+
distributed under the License is distributed on an "AS IS" BASIS,
14+
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
See the License for the specific language governing permissions and
16+
limitations under the License.
17+
*/
18+
19+
package placer_test
20+
21+
import (
22+
"slices"
23+
"testing"
24+
25+
apiv1 "github.com/cozystack/blockstor/pkg/api/v1"
26+
"github.com/cozystack/blockstor/pkg/placer"
27+
"github.com/cozystack/blockstor/pkg/store"
28+
)
29+
30+
// seedTwoDiskfulPlusWitness builds the standard place-count-2 steady
31+
// state on a 3-node cluster: a diskful replica on n1 and n2, plus an
32+
// auto-tiebreaker witness (DISKLESS + TIE_BREAKER) on n3. This is the
33+
// exact topology the corner-D2b bug reproduced on.
34+
func seedTwoDiskfulPlusWitness(t *testing.T, st store.Store) {
35+
t.Helper()
36+
37+
ctx := t.Context()
38+
39+
seedStore(t, st, []string{"n1", "n2", "n3"})
40+
41+
for _, n := range []string{"n1", "n2"} {
42+
if err := st.Resources().Create(ctx, &apiv1.Resource{
43+
Name: "pvc-1", NodeName: n,
44+
Props: map[string]string{"StorPoolName": "pool"},
45+
}); err != nil {
46+
t.Fatalf("seed diskful %s: %v", n, err)
47+
}
48+
}
49+
50+
if err := st.Resources().Create(ctx, &apiv1.Resource{
51+
Name: "pvc-1", NodeName: "n3",
52+
Flags: []string{apiv1.ResourceFlagDiskless, apiv1.ResourceFlagTieBreaker},
53+
}); err != nil {
54+
t.Fatalf("seed witness n3: %v", err)
55+
}
56+
}
57+
58+
// assertWitnessUpgraded asserts that after the placer ran, n3's replica
59+
// is diskful: DISKLESS and TIE_BREAKER are gone and StorPoolName is set.
60+
// It also asserts there are exactly 3 Resources (the witness was
61+
// upgraded IN PLACE, not duplicated by a 4th replica somewhere).
62+
func assertWitnessUpgraded(t *testing.T, st store.Store) {
63+
t.Helper()
64+
65+
got, _ := st.Resources().ListByDefinition(t.Context(), "pvc-1")
66+
if len(got) != 3 {
67+
t.Fatalf("total resources: got %d, want 3 (witness upgraded in place, not duplicated); %+v", len(got), got)
68+
}
69+
70+
var n3 *apiv1.Resource
71+
72+
for i := range got {
73+
if got[i].NodeName == "n3" {
74+
n3 = &got[i]
75+
}
76+
}
77+
78+
if n3 == nil {
79+
t.Fatalf("n3 replica missing after placement; %+v", got)
80+
}
81+
82+
if slices.Contains(n3.Flags, apiv1.ResourceFlagDiskless) {
83+
t.Errorf("n3 still DISKLESS after upgrade: %+v", n3.Flags)
84+
}
85+
86+
if slices.Contains(n3.Flags, apiv1.ResourceFlagTieBreaker) {
87+
t.Errorf("n3 still TIE_BREAKER after upgrade: %+v", n3.Flags)
88+
}
89+
90+
if n3.Props["StorPoolName"] != "pool" {
91+
t.Errorf("n3 StorPoolName: got %q, want \"pool\" (backing disk stamped)", n3.Props["StorPoolName"])
92+
}
93+
}
94+
95+
// TestPlaceUpgradesTiebreakerAbsolutePlaceCount is corner-D2b's
96+
// absolute-form pin: the standard 2-diskful + 1-auto-tiebreaker shape,
97+
// then `--auto-place 3` (PlaceCount=3, the value the REST handler hands
98+
// the placer for both `--auto-place 3` and the `+1` shorthand). Before
99+
// the fix the placer treated the witness-holding node as fully "taken"
100+
// and reported a shortfall ("Not enough nodes ... Replica count: 3")
101+
// even though n3 was available to host a diskful replica. The fix marks
102+
// the witness node as an UPGRADE candidate so the placer promotes the
103+
// witness in place.
104+
func TestPlaceUpgradesTiebreakerAbsolutePlaceCount(t *testing.T) {
105+
t.Parallel()
106+
107+
st := store.NewInMemory()
108+
seedTwoDiskfulPlusWitness(t, st)
109+
110+
placed, want, err := placer.New(st).Place(t.Context(), "pvc-1", &apiv1.AutoSelectFilter{PlaceCount: 3})
111+
if err != nil {
112+
t.Fatalf("Place: %v", err)
113+
}
114+
115+
if placed != 3 || want != 3 {
116+
t.Errorf("placed/want: got %d/%d, want 3/3 (witness upgraded fills the gap)", placed, want)
117+
}
118+
119+
assertWitnessUpgraded(t, st)
120+
}
121+
122+
// TestPlaceUpgradesTiebreakerNotDoubleCounted guards the gap-fill
123+
// arithmetic for the `+1` shorthand. The REST handler lowers
124+
// `--auto-place +1` on a 2-diskful RD to PlaceCount=3 (existing diskful
125+
// count + 1). The witness on n3 must NOT count as one of the 2 existing
126+
// diskful (place_count is diskful-only — countDiskfulReplicas already
127+
// skips DISKLESS), so the effective target is exactly 3 and the witness
128+
// fills slot 3. This pins that the promote lands precisely one new
129+
// diskful and never spills to a (non-existent) 4th node.
130+
func TestPlaceUpgradesTiebreakerNotDoubleCounted(t *testing.T) {
131+
t.Parallel()
132+
133+
st := store.NewInMemory()
134+
seedTwoDiskfulPlusWitness(t, st)
135+
136+
// PlaceCount=3 == count(existing diskful)=2 + additional 1, the
137+
// value resolveAdditionalPlaceCount computes for `--auto-place +1`.
138+
placed, _, err := placer.New(st).Place(t.Context(), "pvc-1", &apiv1.AutoSelectFilter{PlaceCount: 3})
139+
if err != nil {
140+
t.Fatalf("Place: %v", err)
141+
}
142+
143+
if placed != 3 {
144+
t.Errorf("placed: got %d, want 3", placed)
145+
}
146+
147+
assertWitnessUpgraded(t, st)
148+
149+
// Idempotency: re-running the same target must not create a 4th
150+
// replica or flip anything back to diskless.
151+
placed2, _, err := placer.New(st).Place(t.Context(), "pvc-1", &apiv1.AutoSelectFilter{PlaceCount: 3})
152+
if err != nil {
153+
t.Fatalf("Place (re-run): %v", err)
154+
}
155+
156+
if placed2 != 3 {
157+
t.Errorf("placed (re-run): got %d, want 3 (idempotent)", placed2)
158+
}
159+
160+
assertWitnessUpgraded(t, st)
161+
}
162+
163+
// TestPlaceDoesNotUpgradeTiebreakerWhenTargetMet guards the no-op edge:
164+
// the same 2-diskful + 1-witness shape but place_count=2 (already
165+
// satisfied). The witness must stay a witness — the placer must not
166+
// opportunistically upgrade it just because it became an upgrade
167+
// candidate. Pins that the upgrade only fires to fill a real gap.
168+
func TestPlaceDoesNotUpgradeTiebreakerWhenTargetMet(t *testing.T) {
169+
t.Parallel()
170+
171+
st := store.NewInMemory()
172+
seedTwoDiskfulPlusWitness(t, st)
173+
174+
placed, _, err := placer.New(st).Place(t.Context(), "pvc-1", &apiv1.AutoSelectFilter{PlaceCount: 2})
175+
if err != nil {
176+
t.Fatalf("Place: %v", err)
177+
}
178+
179+
if placed != 2 {
180+
t.Errorf("placed: got %d, want 2 (target already met)", placed)
181+
}
182+
183+
got, _ := st.Resources().ListByDefinition(t.Context(), "pvc-1")
184+
185+
var n3 *apiv1.Resource
186+
187+
for i := range got {
188+
if got[i].NodeName == "n3" {
189+
n3 = &got[i]
190+
}
191+
}
192+
193+
if n3 == nil {
194+
t.Fatalf("n3 witness vanished; %+v", got)
195+
}
196+
197+
if !slices.Contains(n3.Flags, apiv1.ResourceFlagTieBreaker) {
198+
t.Errorf("n3 witness was upgraded despite target being met: %+v", n3.Flags)
199+
}
200+
}

0 commit comments

Comments
 (0)