diff --git a/pkg/satellite/controllers/storagepool_replacement_test.go b/pkg/satellite/controllers/storagepool_replacement_test.go index a55663ce5..d9768e465 100644 --- a/pkg/satellite/controllers/storagepool_replacement_test.go +++ b/pkg/satellite/controllers/storagepool_replacement_test.go @@ -115,7 +115,7 @@ const ( // private and pulling it in via a helper export would widen the // public API just for this test. const drvReplacementLvsCmd = "lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } " + - "--noheadings --separator | -o lv_size,data_percent --units k --nosuffix " + + "--noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix " + drvReplacementVG + "/" + drvReplacementThin func newDrvReplacementPool() *blockstoriov1alpha1.StoragePool { @@ -159,12 +159,12 @@ func reconcileDrvReplacement(t *testing.T, cli client.Client, fx *storage.FakeEx } // expectLvsOutput stubs the LVM thin pool's `lvs` invocation to -// return a synthetic size + 0% used line. The reconciler's +// return a synthetic size + 0% used line for an active pool. The reconciler's // `writeCapacity` therefore observes `TotalCapacityKib==sizeKib` // and `FreeCapacityKib==sizeKib`. func expectLvsOutput(fx *storage.FakeExec, sizeKib int64) { fx.Expect(drvReplacementLvsCmd, storage.FakeResponse{ - Stdout: []byte(fmt.Sprintf("%d.00|0.00\n", sizeKib)), + Stdout: []byte(fmt.Sprintf("%d.00|0.00|twi-aotz--\n", sizeKib)), }) } diff --git a/pkg/storage/lvm/lvm_thin.go b/pkg/storage/lvm/lvm_thin.go index eb8218d0e..c9de5a6da 100644 --- a/pkg/storage/lvm/lvm_thin.go +++ b/pkg/storage/lvm/lvm_thin.go @@ -159,28 +159,13 @@ func (t *Thin) PoolStatus(ctx context.Context) (storage.PoolStatus, error) { ctx, cancel := withProbeTimeout(ctx) defer cancel() - out, err := t.exec.Run(ctx, "lvs", - Args("--noheadings", - "--separator", "|", - "-o", "lv_size,data_percent", - "--units", "k", - "--nosuffix", - t.cfg.VolumeGroup+"/"+t.cfg.ThinPool)...) + line, err := t.poolLine(ctx) if err != nil { - return storage.PoolStatus{}, errors.Wrap(err, "lvs (pool)") - } - - line := strings.TrimSpace(string(out)) - if line == "" { - // Bug 282: tag with ErrPoolGone so writeCapacity flips - // PoolMissing=true. Transient lvs errors (timeout, vg locked) - // MUST NOT carry this tag. - return storage.PoolStatus{}, errors.Wrapf(storage.ErrPoolGone, - "thin pool %s/%s not found", t.cfg.VolumeGroup, t.cfg.ThinPool) + return storage.PoolStatus{}, err } - parts := strings.SplitN(line, "|", lvsCols) - if len(parts) != lvsCols { + parts := strings.SplitN(line, "|", poolStatusCols) + if len(parts) != poolStatusCols { return storage.PoolStatus{}, errors.Errorf("lvs (pool): unexpected line %q", line) } @@ -560,8 +545,101 @@ const ( mibPerKib = 1024 pctMax = 100.0 lvsCols = 2 + // poolStatusCols are the fields PoolStatus reads: lv_size, + // data_percent and lv_attr. The attribute is what separates a pool + // that is merely inactive, and whose data_percent lvs leaves empty, + // from one that is gone. + poolStatusCols = 3 + // lvAttrActive is the position of the state character in lv_attr, + // which is 'a' for an active volume. An inactive thin pool reports + // no data_percent at all, because the kernel is not carrying it. + lvAttrActive = 4 ) +// poolLine reads the thin pool's lvs line, activating the pool first if +// it is there but not active. +// +// A thin pool survives a reboot as metadata, not as a running device: +// until something activates it, lvs reports its lv_size but leaves +// data_percent empty, because the kernel is not carrying the pool and +// has no usage to report. Nothing else in the satellite activates it -- +// pools are created once, and the volume paths assume a pool that is +// already running -- so a node that reboots comes back with a pool that +// reports no capacity at all. +// +// Left alone, that reads as a destroyed pool: PoolStatus fails to parse +// the empty data_percent, writeCapacity treats any non-transient error +// as the operator having removed the backing pool, and zeroes the +// capacity. The placer then skips every rebooted node, and volume +// creation fails cluster-wide with "Add more nodes or storage pools", +// while the StoragePool objects still look present and the nodes are +// online. The only trace is a per-probe "pool disappeared" log line. +// +// Activating here keeps that recovery where the condition is first +// observed. It is deliberately not a blind `lvchange -ay` on every +// probe: the attribute says whether the pool is already active, so the +// steady state stays a single lvs. +func (t *Thin) poolLine(ctx context.Context) (string, error) { + line, err := t.poolProbe(ctx) + if err != nil || !poolInactive(line) { + return line, err + } + + vgLV := t.cfg.VolumeGroup + "/" + t.cfg.ThinPool + + _, err = t.exec.Run(ctx, "lvchange", Args("--activate", "y", vgLV)...) + if err != nil { + return "", errors.Wrapf(err, "lvchange --activate y %s", vgLV) + } + + // Re-probe once. A pool that still reports nothing after a + // successful activation is a real fault rather than a cold one, and + // the caller's parse error is the right thing to surface. + return t.poolProbe(ctx) +} + +// poolProbe is one lvs call for the pool's size, usage and attributes. +func (t *Thin) poolProbe(ctx context.Context) (string, error) { + out, err := t.exec.Run(ctx, "lvs", + Args("--noheadings", + "--separator", "|", + "-o", "lv_size,data_percent,lv_attr", + "--units", "k", + "--nosuffix", + t.cfg.VolumeGroup+"/"+t.cfg.ThinPool)...) + if err != nil { + return "", errors.Wrap(err, "lvs (pool)") + } + + line := strings.TrimSpace(string(out)) + if line == "" { + // Bug 282: tag with ErrPoolGone so writeCapacity flips + // PoolMissing=true. Transient lvs errors (timeout, vg locked) + // MUST NOT carry this tag. + return "", errors.Wrapf(storage.ErrPoolGone, + "thin pool %s/%s not found", t.cfg.VolumeGroup, t.cfg.ThinPool) + } + + return line, nil +} + +// poolInactive reports whether the line describes a pool that exists but +// is not active: no usage reported, and lv_attr says so. +func poolInactive(line string) bool { + parts := strings.SplitN(line, "|", poolStatusCols) + if len(parts) != poolStatusCols { + return false + } + + if strings.TrimSpace(parts[1]) != "" { + return false + } + + attr := strings.TrimSpace(parts[2]) + + return len(attr) > lvAttrActive && attr[lvAttrActive] != 'a' +} + // parseFloatToInt64 parses a numeric string LVM emits with `--units k // --nosuffix`, which is e.g. "104857600.00". func parseFloatToInt64(raw string) (int64, error) { diff --git a/pkg/storage/lvm/lvm_thin_test.go b/pkg/storage/lvm/lvm_thin_test.go index 8d794f8ba..2d72a20d9 100644 --- a/pkg/storage/lvm/lvm_thin_test.go +++ b/pkg/storage/lvm/lvm_thin_test.go @@ -254,8 +254,8 @@ func TestThinVolumeStatusMissing(t *testing.T) { // TestThinPoolStatusParsesVgsLvs: PoolStatus reports free + total + can-snap. func TestThinPoolStatusParsesVgsLvs(t *testing.T) { fx := storage.NewFakeExec() - fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent --units k --nosuffix vg/thinpool", - storage.FakeResponse{Stdout: []byte("104857600.00|25.00\n")}) + fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/thinpool", + storage.FakeResponse{Stdout: []byte("104857600.00|25.00|twi-aotz--\n")}) p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "thinpool"}, fx) @@ -295,8 +295,8 @@ func TestThinPoolStatusNearFull(t *testing.T) { fx := storage.NewFakeExec() // 13 GiB thinpool data LV (13631488 KiB) at 99.25% allocated — // the shape `lvs --units k --nosuffix` actually emits. - fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent --units k --nosuffix vg/thinpool", - storage.FakeResponse{Stdout: []byte("13631488.00|99.25\n")}) + fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/thinpool", + storage.FakeResponse{Stdout: []byte("13631488.00|99.25|twi-aotz--\n")}) p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "thinpool"}, fx) @@ -475,7 +475,7 @@ func TestThinDeleteSnapshot(t *testing.T) { // "pool absent" signal that flips Status.PoolMissing=true. func TestThinPoolStatusEmptyOutput(t *testing.T) { fx := storage.NewFakeExec() - fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent --units k --nosuffix vg/missing", + fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/missing", storage.FakeResponse{Stdout: []byte("")}) p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "missing"}, fx) @@ -495,7 +495,7 @@ func TestThinPoolStatusEmptyOutput(t *testing.T) { // error, not a slice-out-of-bounds panic. func TestThinPoolStatusBadColumns(t *testing.T) { fx := storage.NewFakeExec() - fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent --units k --nosuffix vg/tp", + fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/tp", storage.FakeResponse{Stdout: []byte("only-one-col\n")}) p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "tp"}, fx) @@ -512,8 +512,8 @@ func TestThinPoolStatusBadColumns(t *testing.T) { // panic on float coercion. func TestThinPoolStatusBadDataPercent(t *testing.T) { fx := storage.NewFakeExec() - fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent --units k --nosuffix vg/tp", - storage.FakeResponse{Stdout: []byte("104857600.00|inactive\n")}) + fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/tp", + storage.FakeResponse{Stdout: []byte("104857600.00|inactive|twi-aotz--\n")}) p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "tp"}, fx) @@ -531,8 +531,8 @@ func TestThinPoolStatusBadDataPercent(t *testing.T) { // (which previously sat at 75%). func TestThinPoolStatusGarbageFromLvs(t *testing.T) { fx := storage.NewFakeExec() - fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent --units k --nosuffix vg/thinpool", - storage.FakeResponse{Stdout: []byte("not-a-number|25.00\n")}) + fx.Expect("lvs --config devices { filter=['r|^/dev/drbd|','r|^/dev/zd|'] } --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/thinpool", + storage.FakeResponse{Stdout: []byte("not-a-number|25.00|twi-aotz--\n")}) p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "thinpool"}, fx) diff --git a/pkg/storage/lvm/pool_inactive_test.go b/pkg/storage/lvm/pool_inactive_test.go new file mode 100644 index 000000000..8f68363f7 --- /dev/null +++ b/pkg/storage/lvm/pool_inactive_test.go @@ -0,0 +1,181 @@ +// 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 lvm_test + +import ( + "context" + "io" + "strings" + "sync" + "testing" + + "github.com/cozystack/blockstor/pkg/storage/lvm" +) + +// A thin pool survives a reboot as metadata rather than as a running +// device, and until something activates it lvs reports its lv_size but +// leaves data_percent empty. Read as a destroyed pool, that zeroes the +// StoragePool's capacity, the placer then skips the node, and volume +// creation fails across the cluster with "Add more nodes or storage +// pools" while every node is online and every pool object still +// present. These tests pin the recovery: an inactive pool is activated +// and re-probed, and an active one costs a single lvs. + +const ( + poolKey = "lvs --config " + lvm.ConfigFilter + + " --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/thinpool" + activateKey = "lvchange --config " + lvm.ConfigFilter + " --activate y vg/thinpool" +) + +// scriptedExec answers each command from a script and records the order +// in which they were called. +type scriptedExec struct { + mu sync.Mutex + calls []string + // replies are keyed by command line; a key may hold several + // answers, returned in turn, so a re-probe can differ. + replies map[string][][]byte +} + +func newScriptedExec() *scriptedExec { + return &scriptedExec{replies: map[string][][]byte{}} +} + +func (f *scriptedExec) Run(ctx context.Context, name string, args ...string) ([]byte, error) { + return f.RunWithStdin(ctx, nil, name, args...) +} + +func (f *scriptedExec) RunWithStdin( + _ context.Context, + _ io.Reader, + name string, + args ...string, +) ([]byte, error) { + f.mu.Lock() + defer f.mu.Unlock() + + key := name + if len(args) > 0 { + key += " " + strings.Join(args, " ") + } + f.calls = append(f.calls, key) + + queued := f.replies[key] + if len(queued) == 0 { + return nil, nil + } + out := queued[0] + if len(queued) > 1 { + f.replies[key] = queued[1:] + } + + return out, nil +} + +func (f *scriptedExec) ran(key string) int { + f.mu.Lock() + defer f.mu.Unlock() + + var n int + for _, c := range f.calls { + if c == key { + n++ + } + } + + return n +} + +func TestPoolStatusActivatesAnInactivePool(t *testing.T) { + t.Parallel() + + fx := newScriptedExec() + // First probe: the pool is there, with no usage reported and the + // state character of lv_attr unset. Second: active, as lvchange + // leaves it. + fx.replies[poolKey] = [][]byte{ + []byte("104857600.00||twi---tz--\n"), + []byte("104857600.00|10.00|twi-aotz--\n"), + } + + p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "thinpool"}, fx) + + st, err := p.PoolStatus(context.Background()) + if err != nil { + t.Fatalf("PoolStatus on an inactive pool: %v", err) + } + + if fx.ran(activateKey) != 1 { + t.Errorf("the pool must be activated exactly once, ran %d times", fx.ran(activateKey)) + } + + if fx.ran(poolKey) != 2 { + t.Errorf("the pool must be probed again after activation, probed %d times", fx.ran(poolKey)) + } + + if st.TotalCapacityKib != 104857600 { + t.Errorf("TotalCapacityKib %d, want 104857600", st.TotalCapacityKib) + } + + // 10% of 104857600 used. + if st.FreeCapacityKib != 94371840 { + t.Errorf("FreeCapacityKib %d, want 94371840", st.FreeCapacityKib) + } +} + +func TestPoolStatusLeavesAnActivePoolAlone(t *testing.T) { + t.Parallel() + + fx := newScriptedExec() + fx.replies[poolKey] = [][]byte{[]byte("104857600.00|10.00|twi-aotz--\n")} + + p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "thinpool"}, fx) + + if _, err := p.PoolStatus(context.Background()); err != nil { + t.Fatalf("PoolStatus: %v", err) + } + + if fx.ran(activateKey) != 0 { + t.Errorf("an active pool must not be activated; lvchange ran %d times", fx.ran(activateKey)) + } + + if fx.ran(poolKey) != 1 { + t.Errorf("an active pool costs one probe, took %d", fx.ran(poolKey)) + } +} + +// A pool that reports nothing at all is gone, not cold: that distinction +// is what keeps "the operator removed the backing pool" working, so it +// must not be swallowed by the activation path. +func TestPoolStatusStillReportsAMissingPool(t *testing.T) { + t.Parallel() + + fx := newScriptedExec() + fx.replies[poolKey] = [][]byte{[]byte("\n")} + + p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "thinpool"}, fx) + + if _, err := p.PoolStatus(context.Background()); err == nil { + t.Fatal("a missing pool must be an error") + } + + if fx.ran(activateKey) != 0 { + t.Errorf("a missing pool must not be activated; lvchange ran %d times", fx.ran(activateKey)) + } +} diff --git a/pkg/storage/lvm/probe_timeout_bug_270_test.go b/pkg/storage/lvm/probe_timeout_bug_270_test.go index 25a905986..f5219e506 100644 --- a/pkg/storage/lvm/probe_timeout_bug_270_test.go +++ b/pkg/storage/lvm/probe_timeout_bug_270_test.go @@ -86,7 +86,7 @@ func TestPoolStatusBoundsBackendCall_Thin(t *testing.T) { t.Parallel() fx := newDeadlineRecorderExec() - fx.stdout["lvs --config "+lvm.ConfigFilter+" --noheadings --separator | -o lv_size,data_percent --units k --nosuffix vg/thinpool"] = []byte("104857600.00|10.00\n") + fx.stdout["lvs --config "+lvm.ConfigFilter+" --noheadings --separator | -o lv_size,data_percent,lv_attr --units k --nosuffix vg/thinpool"] = []byte("104857600.00|10.00|twi-aotz--\n") p := lvm.NewThin(lvm.ThinConfig{VolumeGroup: "vg", ThinPool: "thinpool"}, fx)