From f006541aea0247e1e37b8142edfc6717aad18dec Mon Sep 17 00:00:00 2001 From: Andrei Kvapil Date: Mon, 2 Nov 2026 20:10:13 +0100 Subject: [PATCH] fix(drbd): forget a removed peer through the kernel, address drbdmeta without a minor forget-peer went through drbdmeta with the peer as a positional argument. drbdmeta's usage is `forget-peer --node-id {val}` in 9.22 and 9.35 alike, so every call exited 10 and a removed peer's metadata slot was never reclaimed. Fixing only the flag would have been worse: both callers run right after del-peer, with the disk attached, and drbdmeta would rewrite the whole superblock under the kernel, whose next metadata write reverts it, and a kernel write between drbdmeta's read and write would be lost. The peer is now forgotten with `drbdsetup forget-peer `, once for the resource: the kernel clears the slot on every volume and syncs the metadata itself, as upstream drbdadm does first. The remaining drbdmeta calls named the device `/`, which is no device name drbdmeta knows; it worked by accident, with a lookup that answered -1 and a never-firing `minor < 0` check on an unsigned field. They now pass `-`, the name drbdmeta takes as no minor. They run on a disk the kernel does not hold, or only read it. Assisted-by: LLM Signed-off-by: Andrei Kvapil --- docs/architecture.md | 2 +- pkg/drbd/drbdadm.go | 72 ++++----- pkg/drbd/drbdadm_p2_test.go | 10 +- pkg/drbd/drbdadm_test.go | 36 ++--- pkg/drbd/drbdsetup_show.go | 4 +- .../controllers/observer_record_gi_test.go | 2 +- pkg/satellite/forget_peer_internal_test.go | 143 ++++++++++++++++++ pkg/satellite/kernel_cleanup.go | 47 +++--- pkg/satellite/kernel_cleanup_test.go | 6 +- pkg/satellite/reconciler.go | 43 +++--- pkg/satellite/reconciler_bug028_test.go | 2 +- pkg/satellite/reconciler_drbd_test.go | 42 +++-- .../reconciler_skip_init_sync_gate_test.go | 2 +- 13 files changed, 270 insertions(+), 141 deletions(-) create mode 100644 pkg/satellite/forget_peer_internal_test.go diff --git a/docs/architecture.md b/docs/architecture.md index ef030735..4b1a6893 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -134,7 +134,7 @@ device — hours on multi-TiB volumes. The skip pipeline (Phase 4. Satellite reconciler's `applyDRBD` runs ``` - drbdmeta --force / v09 internal set-gi ::0:0 + drbdmeta --force - v09 internal set-gi ::0:0 ``` between `drbdadm create-md` and `drbdadm adjust` on first diff --git a/pkg/drbd/drbdadm.go b/pkg/drbd/drbdadm.go index 244fcce8..2708177a 100644 --- a/pkg/drbd/drbdadm.go +++ b/pkg/drbd/drbdadm.go @@ -31,6 +31,17 @@ import ( "github.com/cozystack/blockstor/pkg/storage" ) +// drbdmetaNoMinor is the device drbdmeta is given for a backing device +// addressed directly, without a DRBD minor: `-` is the name drbdmeta itself +// treats as "no minor", and it then takes no minor lock and skips its +// attached check. These calls used to pass `/`, which is no +// device name drbdmeta knows. It worked by accident: drbdmeta's minor lookup +// answers -1, its `minor < 0` refusal never fires because the field is +// unsigned, and it went on to lock and check minor -1. `-` says what the +// calls mean. They all run on a backing device the kernel does not hold +// (set-gi before `up`) or only read it (get-gi, show-gi, dump-md). +const drbdmetaNoMinor = "-" + // Adm is a thin wrapper around the `drbdadm` CLI. It exists so the // satellite reconciler can be unit-tested without a real DRBD kernel // module present: production injects storage.RealExec, tests inject @@ -495,7 +506,7 @@ func (a *Adm) SetGIString(ctx context.Context, resource string, volume int32, de target := fmt.Sprintf("%s/%d", resource, volume) _, err := a.exec.Run(ctx, - "drbdmeta", "--force", target, "v09", device, "internal", + "drbdmeta", "--force", drbdmetaNoMinor, "v09", device, "internal", "set-gi", "--node-id", strconv.Itoa(int(peerNodeID)), gi) if err != nil { return errors.Wrapf(err, "drbdmeta set-gi %s --node-id %d", target, peerNodeID) @@ -504,37 +515,26 @@ func (a *Adm) SetGIString(ctx context.Context, resource string, volume int32, de return nil } -// ForgetPeer clears a peer's GI / bitmap slot from this replica's -// on-disk DRBD metadata via `drbdmeta / v09 -// internal forget-peer `. Must run AFTER DelPeer -// (which clears the kernel-side connection slot) on a per-volume -// basis — DRBD-9 v09 metadata stores per-peer slots in the -// per-volume metadata block, one slot per peer node-id. -// -// Why this matters: DelPeer only severs the kernel connection. -// The on-disk slot keeps the peer's last-known GI and dirty -// bitmap forever — eating one of the MaxPeers-1 metadata slots -// `drbdadm create-md --max-peers=15` carved out at first -// activation. After enough permanent-node-removal cycles the -// resource exhausts its slot pool and the next replica add -// fails with `drbdmeta create-md` running out of room. Calling -// forget-peer in the per-node-removal path keeps the slot pool -// recyclable. -// -// Idempotent on a slot that's already empty: drbdmeta exits zero -// with a no-op warning. A missing metadata block (resource never -// fully initialised) bubbles up as an error so the caller can -// log and continue — the slot leakage we're trying to prevent -// can't have accumulated on a resource that has no metadata to -// begin with. -func (a *Adm) ForgetPeer(ctx context.Context, resource string, volume int32, device string, peerNodeID int32) error { - target := fmt.Sprintf("%s/%d", resource, volume) - - _, err := a.exec.Run(ctx, - "drbdmeta", "--force", target, "v09", device, "internal", - "forget-peer", strconv.Itoa(int(peerNodeID))) +// ForgetPeer reclaims a removed peer's slot in the metadata of every +// volume of resource, through the kernel: `drbdsetup forget-peer +// `. Run it after `del-peer`, while the resource is up. +// +// Without it, `del-peer` only severs the kernel connection; the metadata +// still records the departed peer's GI and bitmap slot. After enough +// permanent node removals the v09 layout runs out of its +// `--max-peers=MaxPeers-1` slots and the next replica add fails. +// +// The kernel clears the slot in memory on every volume and syncs the +// metadata itself, and refuses while a connection to that node still +// exists, which is what upstream drbdadm does first too. drbdmeta is not +// used here: on a disk the kernel has attached it would rewrite the whole +// superblock under the kernel, whose next metadata write reverts the +// change, and a kernel write between drbdmeta's read and its write would +// be lost. +func (a *Adm) ForgetPeer(ctx context.Context, resource string, peerNodeID int32) error { + _, err := a.exec.Run(ctx, "drbdsetup", "forget-peer", resource, strconv.Itoa(int(peerNodeID))) if err != nil { - return errors.Wrapf(err, "drbdmeta forget-peer %s --node-id %d", target, peerNodeID) + return errors.Wrapf(err, "drbdsetup forget-peer %s %d", resource, peerNodeID) } return nil @@ -797,7 +797,7 @@ func (a *Adm) ApplyAL(ctx context.Context, resource string) error { return a.run(ctx, "apply-al", resource) } -// WipeMd runs `drbdmeta --force / v09 internal +// WipeMd runs `drbdmeta --force - v09 internal // wipe-md` — the deliberate-wipe counterpart to CreateMD. Zeroes // the metadata block on the lower disk so a subsequent CreateMD // starts from a clean slate. Operator-recovery use case: a @@ -814,7 +814,7 @@ func (a *Adm) WipeMd(ctx context.Context, resource string, volume int32, device target := fmt.Sprintf("%s/%d", resource, volume) _, err := a.exec.Run(ctx, - "drbdmeta", "--force", target, "v09", device, "internal", + "drbdmeta", "--force", drbdmetaNoMinor, "v09", device, "internal", "wipe-md") if err != nil { return errors.Wrapf(err, "drbdmeta wipe-md %s", target) @@ -823,7 +823,7 @@ func (a *Adm) WipeMd(ctx context.Context, resource string, volume int32, device return nil } -// ShowGI runs `drbdmeta --force / v09 internal +// ShowGI runs `drbdmeta --force - v09 internal // show-gi` and returns the raw stdout — the on-disk generation // UUID tuple, peer slot table, and bitmap-UUID per peer. Used // for verification (compare against a peer's view to triage @@ -845,7 +845,7 @@ func (a *Adm) ShowGI(ctx context.Context, resource string, volume int32, device target := fmt.Sprintf("%s/%d", resource, volume) out, err := a.exec.Run(ctx, - "drbdmeta", "--force", target, "v09", device, "internal", + "drbdmeta", "--force", drbdmetaNoMinor, "v09", device, "internal", "show-gi") if err != nil { return nil, errors.Wrapf(err, "drbdmeta show-gi %s", target) @@ -866,7 +866,7 @@ func (a *Adm) GetGI(ctx context.Context, resource string, volume int32, device s target := fmt.Sprintf("%s/%d", resource, volume) out, err := a.exec.Run(ctx, - "drbdmeta", "--force", target, "v09", device, "internal", + "drbdmeta", "--force", drbdmetaNoMinor, "v09", device, "internal", "get-gi", "--node-id", "0") if err != nil { return "", errors.Wrapf(err, "drbdmeta get-gi %s", target) diff --git a/pkg/drbd/drbdadm_p2_test.go b/pkg/drbd/drbdadm_p2_test.go index 9c5d1b3b..fc89ca81 100644 --- a/pkg/drbd/drbdadm_p2_test.go +++ b/pkg/drbd/drbdadm_p2_test.go @@ -176,7 +176,7 @@ func TestAdmWipeMdInvokesDrbdmeta(t *testing.T) { t.Fatalf("WipeMd: %v", err) } - want := "drbdmeta --force pvc-1/0 v09 /dev/dm-3 internal wipe-md" + want := "drbdmeta --force - v09 /dev/dm-3 internal wipe-md" if !slices.Contains(fx.CommandLines(), want) { t.Errorf("expected %q in calls; got %v", want, fx.CommandLines()) } @@ -188,7 +188,7 @@ func TestAdmWipeMdInvokesDrbdmeta(t *testing.T) { // emits the terser machine-parseable tuple). func TestAdmShowGIInvokesDrbdmeta(t *testing.T) { fx := storage.NewFakeExec() - fx.Expect("drbdmeta --force pvc-1/0 v09 /dev/dm-3 internal show-gi", storage.FakeResponse{ + fx.Expect("drbdmeta --force - v09 /dev/dm-3 internal show-gi", storage.FakeResponse{ Stdout: []byte(`+--< Current data generation UUID >- | 78A0DDDABCDEF000 `), @@ -201,7 +201,7 @@ func TestAdmShowGIInvokesDrbdmeta(t *testing.T) { t.Fatalf("ShowGI: %v", err) } - want := "drbdmeta --force pvc-1/0 v09 /dev/dm-3 internal show-gi" + want := "drbdmeta --force - v09 /dev/dm-3 internal show-gi" if !slices.Contains(fx.CommandLines(), want) { t.Errorf("expected %q in calls; got %v", want, fx.CommandLines()) } @@ -221,7 +221,7 @@ func TestAdmGetGIInvokesDrbdmeta(t *testing.T) { // `--node-id 0` is mandatory on DRBD 9.2+/9.3 ("get-gi requires the // --node-id option"); the device-level current-UUID is identical in // every node-id slot so 0 is representative. - fx.Expect("drbdmeta --force pvc-1/0 v09 /dev/dm-3 internal get-gi --node-id 0", storage.FakeResponse{ + fx.Expect("drbdmeta --force - v09 /dev/dm-3 internal get-gi --node-id 0", storage.FakeResponse{ Stdout: []byte("78A0DDDABCDEF000:78A0DDDABCDEF000:0:0\n"), }) @@ -232,7 +232,7 @@ func TestAdmGetGIInvokesDrbdmeta(t *testing.T) { t.Fatalf("GetGI: %v", err) } - want := "drbdmeta --force pvc-1/0 v09 /dev/dm-3 internal get-gi --node-id 0" + want := "drbdmeta --force - v09 /dev/dm-3 internal get-gi --node-id 0" if !slices.Contains(fx.CommandLines(), want) { t.Errorf("expected %q in calls; got %v", want, fx.CommandLines()) } diff --git a/pkg/drbd/drbdadm_test.go b/pkg/drbd/drbdadm_test.go index e9fc4d8d..0b2f6a3f 100644 --- a/pkg/drbd/drbdadm_test.go +++ b/pkg/drbd/drbdadm_test.go @@ -254,45 +254,27 @@ func TestAdmSetGIInvokesDrbdmeta(t *testing.T) { t.Fatalf("SetGI: %v", err) } - want := "drbdmeta --force pvc-1/0 v09 /dev/dm-3 internal set-gi --node-id 1 78A0DDDABCDEF000:78A0DDDABCDEF000:0:0" + want := "drbdmeta --force - v09 /dev/dm-3 internal set-gi --node-id 1 78A0DDDABCDEF000:78A0DDDABCDEF000:0:0" if !slices.Contains(fx.CommandLines(), want) { t.Errorf("expected %q in calls; got %v", want, fx.CommandLines()) } } -// TestAdmForgetPeerInvokesDrbdmeta pins the on-disk slot -// reclaim command shape: `drbdmeta --force / v09 -// internal forget-peer `. -// -// Without forget-peer, `drbdadm del-peer` only severs the kernel -// connection; the metadata block still records the departed -// peer's GI / bitmap slot. After enough permanent node-removal -// cycles the v09 metadata layout exhausts its -// `--max-peers=MaxPeers-1` slot pool and the next replica add -// fails with drbdmeta refusing to allocate. -// -// Two invariants pinned: -// -// - `` MUST be passed as the trailing positional -// argument (not `--node-id`). The forget-peer subcommand -// takes the node-id positionally; a regression that copied -// SetGI's `--node-id ` shape would surface as drbdmeta -// refusing the call with a usage error. -// - The `--force` flag MUST be present so drbdmeta accepts the -// mutation against in-use metadata (the resource is still -// loaded when forget-peer runs). -func TestAdmForgetPeerInvokesDrbdmeta(t *testing.T) { +// ForgetPeer reclaims a removed peer's metadata slot through the kernel: +// `drbdsetup forget-peer `, once for the resource. +// drbdmeta is never used for it: on an attached disk it would rewrite the +// superblock under the kernel. +func TestAdmForgetPeerGoesThroughTheKernel(t *testing.T) { fx := storage.NewFakeExec() adm := drbd.NewAdm(fx) - err := adm.ForgetPeer(t.Context(), "pvc-1", 0, "/dev/dm-3", 2) + err := adm.ForgetPeer(t.Context(), "pvc-1", 2) if err != nil { t.Fatalf("ForgetPeer: %v", err) } - want := "drbdmeta --force pvc-1/0 v09 /dev/dm-3 internal forget-peer 2" - if !slices.Contains(fx.CommandLines(), want) { - t.Errorf("expected %q in calls; got %v", want, fx.CommandLines()) + if got := fx.CommandLines(); !slices.Equal(got, []string{"drbdsetup forget-peer pvc-1 2"}) { + t.Errorf("calls %v, want exactly drbdsetup forget-peer pvc-1 2", got) } } diff --git a/pkg/drbd/drbdsetup_show.go b/pkg/drbd/drbdsetup_show.go index 33a9d6e9..b54d8b2c 100644 --- a/pkg/drbd/drbdsetup_show.go +++ b/pkg/drbd/drbdsetup_show.go @@ -59,8 +59,8 @@ type KernelSlot struct { Name string // NodeID is the peer's DRBD-9 node-id observed by the kernel. - // Used as the `--node-id` argument to `drbdmeta forget-peer` - // when clearing the per-volume metadata slot. + // Used as the node-id argument to `drbdsetup forget-peer` when + // clearing the peer's metadata slot. NodeID int32 // ConnectionState is the `.connections[].connection-state` token diff --git a/pkg/satellite/controllers/observer_record_gi_test.go b/pkg/satellite/controllers/observer_record_gi_test.go index 9c24afaa..de967c39 100644 --- a/pkg/satellite/controllers/observer_record_gi_test.go +++ b/pkg/satellite/controllers/observer_record_gi_test.go @@ -37,7 +37,7 @@ func TestObserverRecordsTheGIOfDataBearingVolumes(t *testing.T) { t.Parallel() fx := storage.NewFakeExec() - fx.Expect("drbdmeta --force pvc-r/0 v09 /dev/vg/pvc-r_00000 internal get-gi --node-id 0", + fx.Expect("drbdmeta --force - v09 /dev/vg/pvc-r_00000 internal get-gi --node-id 0", storage.FakeResponse{Stdout: []byte("6E3A5D0B2C1F4A96:0:0:0\n")}) recorded := map[int32]string{} diff --git a/pkg/satellite/forget_peer_internal_test.go b/pkg/satellite/forget_peer_internal_test.go new file mode 100644 index 00000000..74abca83 --- /dev/null +++ b/pkg/satellite/forget_peer_internal_test.go @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: Apache-2.0 + +/* +Copyright 2026 Cozystack contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package satellite + +import ( + "context" + "os" + "path/filepath" + "slices" + "strings" + "testing" + + "github.com/go-logr/logr" + + "github.com/cozystack/blockstor/pkg/drbd" + intent "github.com/cozystack/blockstor/pkg/satellite/intent" + "github.com/cozystack/blockstor/pkg/storage" +) + +// A peer removed from a resource with several volumes is forgotten once, +// through the kernel, after del-peer, and drbdmeta is never run against the +// attached disks: it would rewrite the superblock under the kernel. +func TestRemovedPeerIsForgottenThroughTheKernelOnce(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + resPath := filepath.Join(dir, "pvc-fp.res") + + res := "resource pvc-fp {\n on n1 {\n node-id 0;\n }\n on n2 {\n node-id 1;\n }\n}\n" + if err := os.WriteFile(resPath, []byte(res), 0o600); err != nil { + t.Fatalf("write .res: %v", err) + } + + fx := storage.NewFakeExec() + rec := NewReconciler(ReconcilerConfig{Adm: drbd.NewAdm(fx), StateDir: dir, NodeName: "n1"}) + + dr := &intent.DesiredResource{Name: "pvc-fp", NodeName: "n1"} + devices := map[int32]string{0: "/dev/vg/pvc-fp_00000", 1: "/dev/vg/pvc-fp_00001"} + + if err := rec.tearDownRemovedPeers(context.Background(), dr, resPath, devices); err != nil { + t.Fatalf("tearDownRemovedPeers: %v", err) + } + + calls := fx.CommandLines() + + del := slices.Index(calls, "drbdadm del-peer n2:pvc-fp") + forget := slices.Index(calls, "drbdsetup forget-peer pvc-fp 1") + + if del < 0 || forget < del { + t.Fatalf("want del-peer, then drbdsetup forget-peer; got %v", calls) + } + + forgets := 0 + + for _, c := range calls { + if strings.Contains(c, "forget-peer") { + forgets++ + } + + if strings.HasPrefix(c, "drbdmeta") { + t.Errorf("drbdmeta ran against an attached disk: %s", c) + } + } + + if forgets != 1 { + t.Errorf("forget-peer ran %d times, want once for the resource; got %v", forgets, calls) + } +} + +// The prune path forgets a stale slot after del-peer too: the kernel refuses +// forget-peer while the connection to that node exists. +func TestPrunedSlotIsForgottenAfterDelPeer(t *testing.T) { + t.Parallel() + + fx := storage.NewFakeExec() + rec := NewReconciler(ReconcilerConfig{Adm: drbd.NewAdm(fx), StateDir: t.TempDir(), NodeName: "n1"}) + + slot := &drbd.KernelSlot{Name: "n2", NodeID: 1, ConnectionState: "StandAlone"} + devices := map[int32]string{0: "/dev/vg/pvc-fp_00000"} + + if err := rec.teardownKernelSlot(context.Background(), "pvc-fp", slot, devices, logr.Discard()); err != nil { + t.Fatalf("teardownKernelSlot: %v", err) + } + + calls := fx.CommandLines() + + del := slices.Index(calls, "drbdadm del-peer n2:pvc-fp") + forget := slices.Index(calls, "drbdsetup forget-peer pvc-fp 1") + + if del < 0 || forget < del { + t.Fatalf("want del-peer, then drbdsetup forget-peer; got %v", calls) + } +} + +// A diskless replica has no metadata to clear a slot in, so neither path +// asks the kernel to forget the peer. +func TestADisklessReplicaForgetsNoPeer(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + resPath := filepath.Join(dir, "pvc-fp.res") + + res := "resource pvc-fp {\n on n1 {\n node-id 0;\n }\n on n2 {\n node-id 1;\n }\n}\n" + if err := os.WriteFile(resPath, []byte(res), 0o600); err != nil { + t.Fatalf("write .res: %v", err) + } + + fx := storage.NewFakeExec() + rec := NewReconciler(ReconcilerConfig{Adm: drbd.NewAdm(fx), StateDir: dir, NodeName: "n1"}) + diskless := map[int32]string{0: ""} + + err := rec.tearDownRemovedPeers(context.Background(), &intent.DesiredResource{Name: "pvc-fp", NodeName: "n1"}, resPath, diskless) + if err != nil { + t.Fatalf("tearDownRemovedPeers: %v", err) + } + + slot := &drbd.KernelSlot{Name: "n3", NodeID: 2, ConnectionState: "StandAlone"} + if err := rec.teardownKernelSlot(context.Background(), "pvc-fp", slot, diskless, logr.Discard()); err != nil { + t.Fatalf("teardownKernelSlot: %v", err) + } + + for _, c := range fx.CommandLines() { + if strings.Contains(c, "forget-peer") { + t.Errorf("a diskless replica ran %s", c) + } + } +} diff --git a/pkg/satellite/kernel_cleanup.go b/pkg/satellite/kernel_cleanup.go index d4a07492..b0cb44de 100644 --- a/pkg/satellite/kernel_cleanup.go +++ b/pkg/satellite/kernel_cleanup.go @@ -144,7 +144,7 @@ func (r *Reconciler) PruneStaleKernelSlots( expectedSet[n] = struct{}{} } - err = r.prunePass1Unexpected(ctx, rdName, slots, expectedSet, vols, devices, logger) + err = r.prunePass1Unexpected(ctx, rdName, slots, expectedSet, devices, logger) if err != nil { return err } @@ -165,7 +165,6 @@ func (r *Reconciler) prunePass1Unexpected( rdName string, slots map[string]drbd.KernelSlot, expectedSet map[string]struct{}, - vols []int32, devices map[int32]string, logger logr.Logger, ) error { @@ -174,7 +173,7 @@ func (r *Reconciler) prunePass1Unexpected( continue } - teardownErr := r.teardownKernelSlot(ctx, rdName, &slot, vols, devices, logger.WithValues("pass", pruneLogPass1, "peer", name)) + teardownErr := r.teardownKernelSlot(ctx, rdName, &slot, devices, logger.WithValues("pass", pruneLogPass1, "peer", name)) if teardownErr != nil { return errors.Wrapf(teardownErr, "PruneStaleKernelSlots pass 1 (unexpected peer %s)", name) } @@ -227,7 +226,7 @@ func (r *Reconciler) prunePass3Stuck( continue } - teardownErr := r.teardownKernelSlot(ctx, rdName, &slot, vols, devices, logger.WithValues("pass", pruneLogPass3, "peer", name)) + teardownErr := r.teardownKernelSlot(ctx, rdName, &slot, devices, logger.WithValues("pass", pruneLogPass3, "peer", name)) if teardownErr != nil { logger.Info("Pass 3: teardown of stuck slot failed; will retry next reconcile", "peer", name, @@ -243,11 +242,9 @@ func (r *Reconciler) prunePass3Stuck( } } -// teardownKernelSlot is the per-slot del-peer + per-volume forget-peer -// fan-out shared between Pass 1 and Pass 3. del-peer is mandatory; -// forget-peer is per-volume best-effort because v09 metadata lives -// in the per-volume block and a missing device path (DISKLESS local -// replica) means there's no metadata to clean up for that volume. +// teardownKernelSlot is the per-slot del-peer + forget-peer shared +// between Pass 1 and Pass 3. del-peer is mandatory; forget-peer, through +// the kernel and once for the resource, is best-effort. // // Mirrors tearDownRemovedPeers' del-peer-then-forget-peer ordering // without reusing it directly — that function is .res-file-driven and @@ -256,7 +253,6 @@ func (r *Reconciler) teardownKernelSlot( ctx context.Context, rdName string, slot *drbd.KernelSlot, - vols []int32, devices map[int32]string, logger logr.Logger, ) error { @@ -269,26 +265,19 @@ func (r *Reconciler) teardownKernelSlot( return errors.Wrapf(err, "drbdadm del-peer %s:%s", slot.Name, rdName) } - // forget-peer is per-volume. Skip volumes without a resolved - // device path — that's the diskless-local case where no metadata - // exists to clean. Errors are non-fatal: a leaked metadata slot - // is a slow correctness drag (eats one of MaxPeers-1 budget - // entries) that the next reconcile will retry, while del-peer - // success above already closed the immediate kernel-connection - // leak. - for _, vol := range vols { - device, ok := devices[vol] - if !ok || device == "" { - continue - } + // forget-peer clears the peer's slot on every volume at once, + // through the kernel. A local replica with no device at all is + // DISKLESS and has no metadata to clean. Errors are non-fatal: a + // leaked slot eats one of the MaxPeers-1 budget entries and the next + // reconcile retries, while del-peer above already closed the kernel + // connection. + if !anyDevice(devices) { + return nil + } - forgetErr := r.cfg.Adm.ForgetPeer(ctx, rdName, vol, device, slot.NodeID) - if forgetErr != nil { - logger.Info("PruneStaleKernelSlots: forget-peer failed (non-fatal)", - "volume", vol, - "device", device, - "err", forgetErr.Error()) - } + forgetErr := r.cfg.Adm.ForgetPeer(ctx, rdName, slot.NodeID) + if forgetErr != nil { + logger.Info("PruneStaleKernelSlots: forget-peer failed (non-fatal)", "err", forgetErr.Error()) } return nil diff --git a/pkg/satellite/kernel_cleanup_test.go b/pkg/satellite/kernel_cleanup_test.go index 0c5c5293..5032f2ae 100644 --- a/pkg/satellite/kernel_cleanup_test.go +++ b/pkg/satellite/kernel_cleanup_test.go @@ -76,7 +76,7 @@ func newKernelCleanupReconciler(t *testing.T) (*Reconciler, *storage.FakeExec) { // TestPruneStaleKernelSlots_Pass1RemovesUnexpectedPeer: when a kernel // slot exists for a peer name K8s no longer expects, Pass 1 must // issue `drbdadm del-peer` (correctness — live connection) plus -// `drbdmeta forget-peer` per volume (slow-leak — metadata slot). +// `drbdsetup forget-peer` once for the resource (slow leak: metadata slot). // The healthy expected peer must NOT be touched. func TestPruneStaleKernelSlots_Pass1RemovesUnexpectedPeer(t *testing.T) { r, fx := newKernelCleanupReconciler(t) @@ -94,7 +94,7 @@ func TestPruneStaleKernelSlots_Pass1RemovesUnexpectedPeer(t *testing.T) { // del-peer + forget-peer for worker-2, nothing for worker-1. mustContain(t, cmds, "drbdadm del-peer worker-2:pvc-1") - mustContain(t, cmds, "drbdmeta --force pvc-1/0 v09 /dev/zvol/pool/pvc-1 internal forget-peer 2") + mustContain(t, cmds, "drbdsetup forget-peer pvc-1 2") mustNotContain(t, cmds, "drbdadm del-peer worker-1:pvc-1") } @@ -149,7 +149,7 @@ func TestPruneStaleKernelSlots_Pass3DebouncesStuckSlot(t *testing.T) { cmds = fx.CommandLines() mustContain(t, cmds, "drbdadm del-peer worker-2:pvc-1") - mustContain(t, cmds, "drbdmeta --force pvc-1/0 v09 /dev/zvol/pool/pvc-1 internal forget-peer 2") + mustContain(t, cmds, "drbdsetup forget-peer pvc-1 2") // After successful teardown, the debounce entry must be cleared // so a re-incarnated peer of the same name starts a fresh timer. diff --git a/pkg/satellite/reconciler.go b/pkg/satellite/reconciler.go index cc7c18d9..947b7854 100644 --- a/pkg/satellite/reconciler.go +++ b/pkg/satellite/reconciler.go @@ -1869,35 +1869,36 @@ func (r *Reconciler) tearDownRemovedPeers(ctx context.Context, dr *intent.Desire return errors.Wrapf(err, "del-peer %s from %s", peer, dr.GetName()) } - // forget-peer is per-volume because v09 metadata lives in - // the per-volume block. Skip volumes without a device path - // (DISKLESS local replica — no metadata to clean) and - // peers without a resolvable node-id (.res malformed / - // races a brand-new resource being torn down before its - // peer ever rendered). + // forget-peer reclaims the peer's metadata slot on every + // volume at once, through the kernel. Skip a peer without a + // resolvable node-id (.res malformed, or a brand-new resource + // torn down before its peer ever rendered) and a local replica + // with no device at all (DISKLESS: no metadata to clean). peerID, hasID := peerIDs[peer] - if !hasID { + if !hasID || !anyDevice(devices) { continue } - for volNum, device := range devices { - if device == "" { - continue - } + // forget-peer errors are non-fatal: a stale slot leaks one of + // the MaxPeers-1 budget entries but the resource keeps serving + // I/O, and the next reconcile retries. del-peer errors still + // bubble (above): those leak a live kernel connection. + _ = r.cfg.Adm.ForgetPeer(ctx, dr.GetName(), peerID) + } - // forget-peer errors are non-fatal: a stale on-disk - // slot leaks one of the MaxPeers-1 budget entries but - // the resource keeps serving I/O. The next reconcile - // retries; if the leak persists, the eventual - // create-md exhaustion surfaces a louder error than - // any log line here could. del-peer errors still - // bubble (above) — those leak a live kernel - // connection, a faster correctness issue. - _ = r.cfg.Adm.ForgetPeer(ctx, dr.GetName(), volNum, device, peerID) + return nil +} + +// anyDevice reports whether the local replica has a device for at least +// one volume, so it has metadata a forgotten peer's slot can be cleared in. +func anyDevice(devices map[int32]string) bool { + for _, device := range devices { + if device != "" { + return true } } - return nil + return false } // computeRemovedPeers diffs the previously-rendered .res file against diff --git a/pkg/satellite/reconciler_bug028_test.go b/pkg/satellite/reconciler_bug028_test.go index 927073b2..e7336ddd 100644 --- a/pkg/satellite/reconciler_bug028_test.go +++ b/pkg/satellite/reconciler_bug028_test.go @@ -99,7 +99,7 @@ func expectThinBacking(fx *storage.FakeExec, rd string) string { } func expectGetGI(fx *storage.FakeExec, rd, device, currentGI string) { - fx.Expect("drbdmeta --force "+rd+"/0 v09 "+device+" internal get-gi --node-id 0", + fx.Expect("drbdmeta --force - v09 "+device+" internal get-gi --node-id 0", storage.FakeResponse{Stdout: []byte(currentGI + ":0000000000000000:0:0:1:1:0:0:0:0\n")}) } diff --git a/pkg/satellite/reconciler_drbd_test.go b/pkg/satellite/reconciler_drbd_test.go index 26a50c38..b0ca03a9 100644 --- a/pkg/satellite/reconciler_drbd_test.go +++ b/pkg/satellite/reconciler_drbd_test.go @@ -3279,11 +3279,11 @@ func TestApplyLateAddedVolumeWinnerSeedsUpToDate(t *testing.T) { // THE FIX: vol-1's local-slot set-gi MUST carry the winner shape — // current=day0, Consistent(idx4=1)+UpToDate(idx5=1). The set-gi - // target is `/1` (vol-1 only); vol-0 is never re-seeded. + // goes to vol-1's backing device only; vol-0 is never re-seeded. var vol1LocalSlot string for _, line := range calls { - if strings.Contains(line, "pvc-late-win/1 ") && strings.Contains(line, "set-gi --node-id 0 ") { + if strings.Contains(line, "pvc-late-win_00001 ") && strings.Contains(line, "set-gi --node-id 0 ") { vol1LocalSlot = line } } @@ -3316,7 +3316,7 @@ func TestApplyLateAddedVolumeWinnerSeedsUpToDate(t *testing.T) { // HasMD=true, so it never joins the fresh-seed set. A set-gi against // `/0` would wipe vol-0's live GI + bitmap state. for _, line := range calls { - if strings.Contains(line, "pvc-late-win/0 ") && strings.Contains(line, "set-gi") { + if strings.Contains(line, "pvc-late-win_00000 ") && strings.Contains(line, "set-gi") { t.Errorf("late-add seed re-stamped vol-0 GI (would wipe its live metadata): %s", line) } } @@ -3411,7 +3411,7 @@ func TestApplyLateAddedVolumeNonWinnerTakesSkipInitSync(t *testing.T) { var vol1LocalSlot string for _, line := range calls { - if strings.Contains(line, "pvc-late-lose/1 ") && strings.Contains(line, "set-gi --node-id 2 ") { + if strings.Contains(line, "pvc-late-lose_00001 ") && strings.Contains(line, "set-gi --node-id 2 ") { vol1LocalSlot = line } } @@ -3554,7 +3554,7 @@ func TestApplyFirstActivationSeedsGiBeforeAdjust(t *testing.T) { calls := fx.CommandLines() createMD := indexOfPrefix(calls, fmt.Sprintf("drbdadm create-md --force --max-peers=%d pvc-seed", drbd.MaxPeers-1)) - setGI := indexOfPrefix(calls, "drbdmeta --force pvc-seed/0 v09 ") + setGI := indexOfSetGI(calls, "pvc-seed") adjust := indexOfPrefix(calls, "drbdadm adjust pvc-seed") if createMD < 0 { @@ -3578,7 +3578,7 @@ func TestApplyFirstActivationSeedsGiBeforeAdjust(t *testing.T) { // current_uuid in BOTH current_uuid and bitmap_uuid slots, // stamped via `set-gi --node-id ` (DRBD 9.2+ per-peer // slot layout). - wantSetGI := "drbdmeta --force pvc-seed/0 v09 /dev/vg/pvc-seed_00000 internal set-gi --node-id 1 78A0DDDABCDEF000:78A0DDDABCDEF000:0:0" + wantSetGI := "drbdmeta --force - v09 /dev/vg/pvc-seed_00000 internal set-gi --node-id 1 78A0DDDABCDEF000:78A0DDDABCDEF000:0:0" if !slices.Contains(calls, wantSetGI) { t.Errorf("missing exact set-gi command %q in calls: %v", wantSetGI, calls) } @@ -4047,7 +4047,7 @@ func TestApplyFirstActivationSkipsInitialSyncOnThinOrZFS(t *testing.T) { calls := fx.CommandLines() createMD := indexOfPrefix(calls, fmt.Sprintf("drbdadm create-md --force --max-peers=%d pvc-zskip", drbd.MaxPeers-1)) - setGI := indexOfPrefix(calls, "drbdmeta --force pvc-zskip/0 v09 ") + setGI := indexOfSetGI(calls, "pvc-zskip") adjust := indexOfPrefix(calls, "drbdadm adjust pvc-zskip") if createMD < 0 { @@ -4077,7 +4077,7 @@ func TestApplyFirstActivationSkipsInitialSyncOnThinOrZFS(t *testing.T) { // the expected value in sync with // pkg/satellite/providerkind.go's day0GiFor(). day0 := satellite.Day0GiForTest("pvc-zskip", 0) - wantSetGI := fmt.Sprintf("drbdmeta --force pvc-zskip/0 v09 %s internal set-gi --node-id 1 %s:0:0:0:0:1", + wantSetGI := fmt.Sprintf("drbdmeta --force - v09 %s internal set-gi --node-id 1 %s:0:0:0:0:1", tc.wantDevice, day0) if !slices.Contains(calls, wantSetGI) { t.Errorf("missing exact day0 set-gi command %q in calls: %v", wantSetGI, calls) @@ -4157,8 +4157,8 @@ func TestApplyFirstActivationSeedsEveryPeerSlotConsistently(t *testing.T) { // emitted call sequence MUST follow GetPeers() iteration // (n2 → n3) so both satellites visit slots in the same order // (eliminates an FS-write ordering source of divergence). - wantN2 := fmt.Sprintf("drbdmeta --force pvc-race/0 v09 /dev/vg/pvc-race_00000 internal set-gi --node-id 1 %s:0:0:0:0:1", day0) - wantN3 := fmt.Sprintf("drbdmeta --force pvc-race/0 v09 /dev/vg/pvc-race_00000 internal set-gi --node-id 2 %s:0:0:0:0:1", day0) + wantN2 := fmt.Sprintf("drbdmeta --force - v09 /dev/vg/pvc-race_00000 internal set-gi --node-id 1 %s:0:0:0:0:1", day0) + wantN3 := fmt.Sprintf("drbdmeta --force - v09 /dev/vg/pvc-race_00000 internal set-gi --node-id 2 %s:0:0:0:0:1", day0) if !slices.Contains(calls, wantN2) { t.Errorf("missing per-peer set-gi for n2 (node-id 1): want %q in calls: %v", wantN2, calls) @@ -4269,7 +4269,7 @@ func TestApplyFirstActivationSeedsEveryMetadataSlotBlanket(t *testing.T) { // including slot 2 (n3, whose node-id was NOT allocated) and slots // 16..31 that no peer occupies. for nodeID := 0; nodeID <= drbd.NodeIDMax; nodeID++ { - want := fmt.Sprintf("drbdmeta --force pvc-blanket/0 v09 /dev/vg/pvc-blanket_00000 internal set-gi --node-id %d %s:0:0:0:0:1", + want := fmt.Sprintf("drbdmeta --force - v09 /dev/vg/pvc-blanket_00000 internal set-gi --node-id %d %s:0:0:0:0:1", nodeID, day0) if !slices.Contains(calls, want) { t.Errorf("missing blanket set-gi for node-id %d: want %q in calls", nodeID, want) @@ -4280,7 +4280,7 @@ func TestApplyFirstActivationSeedsEveryMetadataSlotBlanket(t *testing.T) { // fewer (no out-of-range node-id 32+, no skipped slot). var setGiCount int for _, c := range calls { - if strings.HasPrefix(c, "drbdmeta") && strings.Contains(c, "set-gi") && strings.Contains(c, "pvc-blanket/0") { + if strings.HasPrefix(c, "drbdmeta") && strings.Contains(c, "set-gi") && strings.Contains(c, "pvc-blanket_00000 ") { setGiCount++ } } @@ -4311,7 +4311,7 @@ func TestApplyFirstActivationSeedsEveryMetadataSlotBlanket(t *testing.T) { } for nodeID := 0; nodeID <= drbd.NodeIDMax; nodeID++ { - want := fmt.Sprintf("drbdmeta --force pvc-blanket/0 v09 /dev/vg/pvc-blanket_00000 internal set-gi --node-id %d %s:0:0:0:0:1", + want := fmt.Sprintf("drbdmeta --force - v09 /dev/vg/pvc-blanket_00000 internal set-gi --node-id %d %s:0:0:0:0:1", nodeID, day0) idx := slices.Index(calls, want) @@ -4397,7 +4397,7 @@ func TestApplyFirstActivationSeedsLocalSlotBug284(t *testing.T) { // Without this call the local current_uuid stays at the random // value drbdadm create-md generated → permanent unrelated-data on // every future handshake against a later-joining peer. - wantLocal := fmt.Sprintf("drbdmeta --force pvc-b284/0 v09 /dev/vg/pvc-b284_00000 internal set-gi --node-id 0 %s:0:0:0:0:1", + wantLocal := fmt.Sprintf("drbdmeta --force - v09 /dev/vg/pvc-b284_00000 internal set-gi --node-id 0 %s:0:0:0:0:1", day0) if !slices.Contains(calls, wantLocal) { t.Errorf("missing local-slot set-gi (Bug 284 fix): want %q in calls: %v", wantLocal, calls) @@ -7945,3 +7945,17 @@ func TestApplyAutoMkfsStorageOnlySkipsDisklessReplica(t *testing.T) { } } } + +// indexOfSetGI finds the first drbdmeta set-gi on a backing device of rd's +// first volume. drbdmeta is given `-` for the device, so the volume shows +// only in the backing device path. +func indexOfSetGI(calls []string, rd string) int { + for i, c := range calls { + if strings.HasPrefix(c, "drbdmeta --force - v09 ") && strings.Contains(c, rd+"_00000") && + strings.Contains(c, " internal set-gi ") { + return i + } + } + + return -1 +} diff --git a/pkg/satellite/reconciler_skip_init_sync_gate_test.go b/pkg/satellite/reconciler_skip_init_sync_gate_test.go index e29c7f18..db34ceab 100644 --- a/pkg/satellite/reconciler_skip_init_sync_gate_test.go +++ b/pkg/satellite/reconciler_skip_init_sync_gate_test.go @@ -193,7 +193,7 @@ func TestApplyElectsWinnerOnceSkipInitialSyncStampedTrue(t *testing.T) { var sawWinnerSeed bool for _, line := range calls { - if strings.HasPrefix(line, "drbdmeta --force pvc-skipgate/0 v09 ") && + if strings.HasPrefix(line, "drbdmeta --force - v09 ") && strings.Contains(line, "set-gi") && strings.HasSuffix(strings.TrimSpace(line), ":1:1") { sawWinnerSeed = true