diff --git a/cmd/installer/install/blockdev.go b/cmd/installer/install/blockdev.go index 7ef054d9c..9c9038874 100644 --- a/cmd/installer/install/blockdev.go +++ b/cmd/installer/install/blockdev.go @@ -45,6 +45,7 @@ var ( sysClassBlockDir = "/sys/class/block" devByIDDir = "/dev/disk/by-id" procMountsPath = "/proc/mounts" + procSwapsPath = "/proc/swaps" ) // ListDisks enumerates install-candidate disks from /sys/block. It reads sysfs @@ -132,6 +133,144 @@ func partitionPath(disk string, n int) string { return fmt.Sprintf("%s%d", disk, n) } +// readKernelPartitions returns the partition device names the kernel currently +// publishes for a disk, sorted. +// +// It reads sysfs rather than /dev deliberately. A partition device node +// outlives the table that created it, so a stale node is indistinguishable +// from a fresh one by existence alone — which is how a disk whose old table +// the kernel never dropped passes every check and then gets formatted at the +// previous layout's offsets. +func readKernelPartitions(disk string) ([]string, error) { + name := filepath.Base(disk) + entries, err := os.ReadDir(filepath.Join(sysBlockDir, name)) + if err != nil { + return nil, fmt.Errorf("read the kernel's partition list for %s: %w", disk, err) + } + var out []string + for _, e := range entries { + // Partitions are the child directories carrying a "partition" attribute. + // "queue", "device" and the plain attribute files sit alongside them. + if _, err := os.Stat(filepath.Join(sysBlockDir, name, e.Name(), "partition")); err != nil { + continue + } + out = append(out, e.Name()) + } + slices.Sort(out) + return out, nil +} + +// kernelPartitions is indirected so tests can drive the partition-table +// assertions without a real block device. +var kernelPartitions = readKernelPartitions + +// partitionDevice maps a kernel partition name back to its device node, +// resolved alongside the disk it belongs to rather than assuming /dev. +func partitionDevice(disk, part string) string { + return filepath.Join(filepath.Dir(disk), part) +} + +// diskHolders names the md, LVM or device-mapper devices claiming this disk or +// one of its partitions. A holder is the usual reason the kernel refuses to +// re-read a partition table, so it is what the operator needs told when the +// installer gives up on a disk. +func diskHolders(disk string) []string { + name := filepath.Base(disk) + dirs := []string{filepath.Join(sysBlockDir, name, "holders")} + parts, _ := kernelPartitions(disk) + for _, p := range parts { + dirs = append(dirs, filepath.Join(sysBlockDir, name, p, "holders")) + } + + seen := map[string]bool{} + var out []string + for _, dir := range dirs { + entries, err := os.ReadDir(dir) + if err != nil { + continue + } + for _, e := range entries { + if seen[e.Name()] { + continue + } + seen[e.Name()] = true + out = append(out, e.Name()) + } + } + slices.Sort(out) + return out +} + +// diskMount is one filesystem mounted from a disk the installer is about to +// erase. +type diskMount struct { + Device string + Mountpoint string +} + +// devicesOnDisk is the set of device paths belonging to a disk: the whole disk +// and every partition the kernel currently publishes. Matching /proc tables +// against a set rather than a string prefix keeps /dev/nvme0n1 from claiming +// /dev/nvme0n11. +func devicesOnDisk(disk string) map[string]bool { + out := map[string]bool{disk: true} + parts, _ := kernelPartitions(disk) + for _, p := range parts { + out[partitionDevice(disk, p)] = true + } + return out +} + +// mountsOnDisk returns the filesystems mounted from a disk, deepest mountpoint +// first so a nested mount is released before the one it sits inside. +func mountsOnDisk(disk string) []diskMount { + devs := devicesOnDisk(disk) + var out []diskMount + for _, fields := range procTable(procMountsPath, 0) { + if len(fields) >= 2 && devs[fields[0]] { + out = append(out, diskMount{Device: fields[0], Mountpoint: fields[1]}) + } + } + slices.SortFunc(out, func(a, b diskMount) int { + return strings.Count(b.Mountpoint, "/") - strings.Count(a.Mountpoint, "/") + }) + return out +} + +// swapsOnDisk returns the active swap areas backed by a disk. +func swapsOnDisk(disk string) []string { + devs := devicesOnDisk(disk) + var out []string + // The first line of /proc/swaps is a column header, not an entry. + for _, fields := range procTable(procSwapsPath, 1) { + if len(fields) >= 1 && devs[fields[0]] { + out = append(out, fields[0]) + } + } + return out +} + +// procTable splits a whitespace-delimited /proc table into fields per line, +// dropping the first skip lines and any blank ones. +func procTable(path string, skip int) [][]string { + data, err := os.ReadFile(path) + if err != nil { + return nil + } + var out [][]string + n := 0 + for line := range strings.SplitSeq(string(data), "\n") { + n++ + if n <= skip { + continue + } + if fields := strings.Fields(line); len(fields) > 0 { + out = append(out, fields) + } + } + return out +} + // byIDPreference orders the /dev/disk/by-id/ namespaces we prefer to name a // pool member by. wwn- ids are last: they are stable but opaque, so a degraded // pool reports a number nobody can map to a drive bay. diff --git a/cmd/installer/install/blockdev_test.go b/cmd/installer/install/blockdev_test.go index 3f2f514a1..b40669735 100644 --- a/cmd/installer/install/blockdev_test.go +++ b/cmd/installer/install/blockdev_test.go @@ -5,6 +5,7 @@ import ( "path/filepath" "slices" "strconv" + "strings" "testing" ) @@ -40,6 +41,7 @@ func newFakeSysfs(t *testing.T, disks ...fakeDisk) *fakeSysfs { swap(&sysClassBlockDir, filepath.Join(f.root, "sys/class/block")) swap(&devByIDDir, filepath.Join(f.root, "dev/disk/by-id")) swap(&procMountsPath, filepath.Join(f.root, "proc/mounts")) + swap(&procSwapsPath, filepath.Join(f.root, "proc/swaps")) f.mkdirAll(sysBlockDir) f.mkdirAll(sysClassBlockDir) @@ -113,6 +115,18 @@ func (f *fakeSysfs) setMounts(content string) { f.write(procMountsPath, content) } +func (f *fakeSysfs) setSwaps(content string) { + f.t.Helper() + f.write(procSwapsPath, content) +} + +// addHolder marks a partition as claimed by md, LVM or device-mapper — the +// state that makes the kernel refuse to re-read the disk's partition table. +func (f *fakeSysfs) addHolder(disk, part, holder string) { + f.t.Helper() + f.mkdirAll(filepath.Join(sysBlockDir, disk, part, "holders", holder)) +} + func boolAttr(b bool) string { if b { return "1\n" @@ -352,3 +366,119 @@ func TestHasAnyPrefix(t *testing.T) { t.Error("sda should not match") } } + +func TestReadKernelPartitionsListsOnlyPartitions(t *testing.T) { + newFakeSysfs(t, fakeDisk{ + name: "nvme0n1", sectors: 1 << 22, + partitions: []string{"nvme0n1p3", "nvme0n1p1", "nvme0n1p2"}, + }) + + got, err := readKernelPartitions("/dev/nvme0n1") + if err != nil { + t.Fatalf("readKernelPartitions: %v", err) + } + // Sorted, and "queue" and "device" alongside them are not partitions. + want := []string{"nvme0n1p1", "nvme0n1p2", "nvme0n1p3"} + if !slices.Equal(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestReadKernelPartitionsIsEmptyForAnUnpartitionedDisk(t *testing.T) { + newFakeSysfs(t, fakeDisk{name: "nvme2n1", sectors: 1 << 22}) + + got, err := readKernelPartitions("/dev/nvme2n1") + if err != nil { + t.Fatalf("readKernelPartitions: %v", err) + } + if len(got) != 0 { + t.Errorf("got %v, want none", got) + } +} + +func TestReadKernelPartitionsFailsOnAnUnknownDisk(t *testing.T) { + newFakeSysfs(t) + + if _, err := readKernelPartitions("/dev/nvme9n1"); err == nil { + t.Fatal("expected an error for a disk with no sysfs entry") + } +} + +func TestDiskHoldersNamesEveryClaimant(t *testing.T) { + f := newFakeSysfs(t, fakeDisk{ + name: "sda", sectors: 1 << 22, partitions: []string{"sda1", "sda2"}, + }) + f.addHolder("sda", "sda1", "md0") + f.addHolder("sda", "sda2", "dm-0") + + if got, want := diskHolders("/dev/sda"), []string{"dm-0", "md0"}; !slices.Equal(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestDiskHoldersIsEmptyWhenNothingClaimsTheDisk(t *testing.T) { + newFakeSysfs(t, fakeDisk{name: "sda", sectors: 1 << 22, partitions: []string{"sda1"}}) + + if got := diskHolders("/dev/sda"); len(got) != 0 { + t.Errorf("got %v, want none", got) + } +} + +func TestMountsOnDiskFindsOnlyThisDiskDeepestFirst(t *testing.T) { + f := newFakeSysfs(t, + fakeDisk{name: "sda", sectors: 1 << 22, partitions: []string{"sda1", "sda2"}}, + fakeDisk{name: "sdb", sectors: 1 << 22, partitions: []string{"sdb1"}}, + ) + f.setMounts(strings.Join([]string{ + "/dev/sda1 /oldroot ext4 rw 0 0", + "/dev/sda2 /oldroot/boot/efi vfat rw 0 0", + "/dev/sdb1 /other ext4 rw 0 0", + "proc /proc proc rw 0 0", + }, "\n") + "\n") + + got := mountsOnDisk("/dev/sda") + want := []diskMount{ + {Device: "/dev/sda2", Mountpoint: "/oldroot/boot/efi"}, + {Device: "/dev/sda1", Mountpoint: "/oldroot"}, + } + if !slices.Equal(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +// /dev/nvme0n1 must not claim /dev/nvme0n11, which a string-prefix match would. +func TestMountsOnDiskDoesNotMatchASiblingNamespace(t *testing.T) { + f := newFakeSysfs(t, + fakeDisk{name: "nvme0n1", sectors: 1 << 22, partitions: []string{"nvme0n1p1"}}, + fakeDisk{name: "nvme0n11", sectors: 1 << 22, partitions: []string{"nvme0n11p1"}}, + ) + f.setMounts("/dev/nvme0n11p1 /elsewhere ext4 rw 0 0\n") + + if got := mountsOnDisk("/dev/nvme0n1"); len(got) != 0 { + t.Errorf("got %v, want none", got) + } +} + +func TestSwapsOnDiskSkipsTheHeaderAndOtherDisks(t *testing.T) { + f := newFakeSysfs(t, + fakeDisk{name: "sda", sectors: 1 << 22, partitions: []string{"sda1"}}, + fakeDisk{name: "sdb", sectors: 1 << 22, partitions: []string{"sdb1"}}, + ) + f.setSwaps(strings.Join([]string{ + "Filename\t\t\t\tType\t\tSize\tUsed\tPriority", + "/dev/sda1 partition 8388604 0 -2", + "/dev/sdb1 partition 8388604 0 -3", + }, "\n") + "\n") + + if got, want := swapsOnDisk("/dev/sda"), []string{"/dev/sda1"}; !slices.Equal(got, want) { + t.Errorf("got %v, want %v", got, want) + } +} + +func TestSwapsOnDiskIsEmptyWithoutASwapsTable(t *testing.T) { + newFakeSysfs(t, fakeDisk{name: "sda", sectors: 1 << 22, partitions: []string{"sda1"}}) + + if got := swapsOnDisk("/dev/sda"); len(got) != 0 { + t.Errorf("got %v, want none", got) + } +} diff --git a/cmd/installer/install/disk.go b/cmd/installer/install/disk.go index a7d8f5ac8..df16816ca 100644 --- a/cmd/installer/install/disk.go +++ b/cmd/installer/install/disk.go @@ -4,6 +4,9 @@ import ( "fmt" "log/slog" "os" + "path/filepath" + "slices" + "strings" "time" ) @@ -47,6 +50,95 @@ const ( tailReserveMiB = 1024 ) +// partitionSettleTimeout bounds the wait for the kernel and udev to publish a +// disk's new partition table. A var so tests do not pay it. +var partitionSettleTimeout = 15 * time.Second + +// releaseDisk detaches whatever the live environment has attached to a disk +// before it is erased. A mounted filesystem or an active swap area makes +// BLKRRPART return EBUSY, and the kernel then keeps serving the disk's old +// partition table while sgdisk writes the new one and reports success. +func releaseDisk(d Disk) { + for _, m := range mountsOnDisk(d.Path) { + err := runQuiet("umount", m.Device) + if err != nil { + // Lazy detach: a busy mount must not stall an install that is about to + // erase the filesystem underneath it anyway. + err = runQuiet("umount", "-l", m.Device) + } + if err != nil { + slog.Warn("could not unmount before erase", "device", m.Device, "mountpoint", m.Mountpoint, "err", err) + continue + } + slog.Info("released mount before erase", "device", m.Device, "mountpoint", m.Mountpoint) + } + for _, s := range swapsOnDisk(d.Path) { + if err := runQuiet("swapoff", s); err != nil { + slog.Warn("could not disable swap before erase", "device", s, "err", err) + continue + } + slog.Info("disabled swap before erase", "device", s) + } +} + +// settlePartitions forces the kernel to re-read a disk's partition table and +// blocks until its view matches want, naming the partitions by number. +// +// An empty want asserts the disk has no partitions at all. That is the only +// state no leftover can satisfy: a disk whose previous table had the same +// partition numbers as the one being written — an old OS install is p1, p2, p3, +// exactly what the boot layout writes — passes an "are the expected partitions +// there" check on stale nodes alone. +func settlePartitions(d Disk, want ...int) error { + if err := runQuiet("partprobe", d.Path); err != nil { + // partprobe rescans the whole disk and fails if it cannot open any part of + // it; the ioctl on its own sometimes still gets through. + if err := runQuiet("blockdev", "--rereadpt", d.Path); err != nil { + slog.Warn("could not force a partition table re-read", "disk", d.Path, "err", err) + } + } + _ = runQuiet("udevadm", "settle", "--timeout=10") + + wanted := make([]string, 0, len(want)) + for _, n := range want { + wanted = append(wanted, filepath.Base(d.PartitionPath(n))) + } + slices.Sort(wanted) + + deadline := time.Now().Add(partitionSettleTimeout) + for { + got, err := kernelPartitions(d.Path) + if err != nil { + return err + } + if slices.Equal(got, wanted) { + return nil + } + if time.Now().After(deadline) { + return staleTableError(d, wanted, got) + } + time.Sleep(200 * time.Millisecond) + } +} + +// staleTableError explains a kernel partition view that never caught up. The +// erase case gets its own wording because it is the dangerous one — proceeding +// would format the previous layout — and because the holder is usually the +// whole diagnosis. +func staleTableError(d Disk, wanted, got []string) error { + if len(wanted) == 0 { + msg := fmt.Sprintf("%s still shows partitions [%s] after its table was erased — the kernel is using the old table, so formatting would write at the previous layout's offsets", + d.Path, strings.Join(got, ", ")) + if h := diskHolders(d.Path); len(h) > 0 { + return fmt.Errorf("%s; it is held by %s, which must be stopped before this disk can be installed to", + msg, strings.Join(h, ", ")) + } + return fmt.Errorf("%s; something in the live environment is holding it open", msg) + } + return fmt.Errorf("%s: the kernel shows partitions [%s] but the new table has [%s] — the table was written but never picked up", + d.Path, strings.Join(got, ", "), strings.Join(wanted, ", ")) +} + // espSizeMiB picks the ESP size for a disk. func espSizeMiB(d Disk) int { if d.Bytes >= espLargeDiskGiB<<30 { @@ -98,7 +190,7 @@ func partitionDataDisk(rm RoleMount) error { if err := run("sgdisk", "-G", rm.Disk.Path); err != nil { slog.Warn("could not randomise GPT GUIDs", "disk", rm.Disk.Path, "err", err) } - return nil + return settlePartitions(rm.Disk, dataPartNum) } // partitionOne writes the GPT for a single disk. @@ -129,25 +221,15 @@ func partitionOne(d Disk, rootType string) error { if err := run("sgdisk", "-G", d.Path); err != nil { slog.Warn("could not randomise GPT GUIDs", "disk", d.Path, "err", err) } - return nil + return settlePartitions(d, biosPartNum, espPartNum, rootPartNum) } // waitForPartitions ensures every partition device node the installer is about -// to use exists. Trixie's udev is slow enough after a table rewrite that mkfs -// races it and fails with ENOENT on a device the kernel has already accepted. +// to use exists. The kernel's own view was already asserted per disk as each +// table was written; udev publishes the /dev nodes and by-id symlinks after +// that, and Trixie is slow enough about it that mkfs races them. func waitForPartitions(cfg DiskConfig) error { - for _, d := range cfg.Disks { - // Best-effort: partprobe failing is not fatal, since udev may still act - // on the BLKRRPART ioctl sgdisk issued itself. - if err := run("partprobe", d.Path); err != nil { - slog.Warn("partprobe failed, continuing", "disk", d.Path, "err", err) - } - } - if err := run("udevadm", "settle", "--timeout=10"); err != nil { - slog.Warn("udevadm settle failed, continuing", "err", err) - } - - deadline := time.Now().Add(15 * time.Second) + deadline := time.Now().Add(partitionSettleTimeout) for _, d := range cfg.bootDisks() { wanted := []string{d.PartitionPath(espPartNum), d.PartitionPath(rootPartNum)} // The pool is built from by-id paths, so their symlinks have to exist diff --git a/cmd/installer/install/steps_test.go b/cmd/installer/install/steps_test.go index c922f5a94..41363d18e 100644 --- a/cmd/installer/install/steps_test.go +++ b/cmd/installer/install/steps_test.go @@ -5,6 +5,8 @@ import ( "fmt" "os" "path/filepath" + "slices" + "strconv" "strings" "testing" "time" @@ -16,17 +18,69 @@ import ( type fakeExec struct { calls []string fail func(name string, args []string) error + + // parts is the kernel's partition view per disk. Seeded by the test and then + // moved by the sgdisk calls the installer makes, so the partition-table + // assertions can run without a block device. + parts map[string][]string + + // stale marks disks whose view never moves however many tables are written + // to them: something holds the disk open, BLKRRPART returns EBUSY, and the + // kernel keeps serving the old table while sgdisk reports success. + stale map[string]bool +} + +// holdOpen makes a disk keep its current partition view forever. +func (f *fakeExec) holdOpen(disk string) { f.stale[disk] = true } + +// seedPartitions gives a disk a pre-existing partition table — the state a +// drive is in when the node has been installed on before. +func (f *fakeExec) seedPartitions(disk string, nums ...int) { + for _, n := range nums { + f.parts[disk] = append(f.parts[disk], filepath.Base(partitionPath(disk, n))) + } +} + +// applySgdisk moves the fake kernel view in step with a table rewrite, the way +// a kernel that honoured every BLKRRPART would: -Z empties it, each -n adds the +// partition it creates. +func (f *fakeExec) applySgdisk(args []string) { + if len(args) == 0 { + return + } + disk := args[len(args)-1] + if f.stale[disk] { + return + } + for i, a := range args { + switch { + case a == "-Z": + f.parts[disk] = nil + case a == "-n" && i+1 < len(args): + num, _, _ := strings.Cut(args[i+1], ":") + n, err := strconv.Atoi(num) + if err != nil { + continue + } + f.parts[disk] = append(f.parts[disk], filepath.Base(partitionPath(disk, n))) + } + } } func fakeCommands(t *testing.T) *fakeExec { t.Helper() - f := &fakeExec{} - prevRun, prevQuiet, prevEnv := run, runQuiet, runEnv + f := &fakeExec{parts: map[string][]string{}, stale: map[string]bool{}} + prevRun, prevQuiet, prevEnv, prevParts := run, runQuiet, runEnv, kernelPartitions record := func(name string, args ...string) error { f.calls = append(f.calls, strings.TrimSpace(name+" "+strings.Join(args, " "))) if f.fail != nil { - return f.fail(name, args) + if err := f.fail(name, args); err != nil { + return err + } + } + if name == "sgdisk" { + f.applySgdisk(args) } return nil } @@ -35,8 +89,21 @@ func fakeCommands(t *testing.T) *fakeExec { runEnv = func(env []string, name string, args ...string) error { return record(name, append(env, args...)...) } + kernelPartitions = func(disk string) ([]string, error) { + got := slices.Clone(f.parts[disk]) + slices.Sort(got) + return got, nil + } - t.Cleanup(func() { run, runQuiet, runEnv = prevRun, prevQuiet, prevEnv }) + // A short settle so a test asserting the stale-table failure does not wait + // out the install's real budget. + prevTimeout := partitionSettleTimeout + partitionSettleTimeout = 10 * time.Millisecond + + t.Cleanup(func() { + run, runQuiet, runEnv, kernelPartitions = prevRun, prevQuiet, prevEnv, prevParts + partitionSettleTimeout = prevTimeout + }) return f } @@ -114,6 +181,24 @@ func targetSkeleton(t *testing.T, root string) { // tempDisks builds disks whose device nodes are real files, so the steps that // wait for partition nodes to appear can complete. +// fakeProcTables points the mount and swap readers at fixture tables for the +// duration of a test. +func fakeProcTables(t *testing.T, mounts, swaps string) { + t.Helper() + dir := t.TempDir() + point := func(dst *string, name, content string) { + prev := *dst + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", p, err) + } + *dst = p + t.Cleanup(func() { *dst = prev }) + } + point(&procMountsPath, "mounts", mounts) + point(&procSwapsPath, "swaps", swaps) +} + func tempDisks(t *testing.T, n int, gib int64) []Disk { t.Helper() dir := t.TempDir() @@ -237,21 +322,154 @@ func TestEspSizeMiB(t *testing.T) { } } -func TestClearDiskOnlyLabelclearsAnExistingPartition(t *testing.T) { +// A drive arriving with a previous install's layout must have every partition +// cleared, not just the one the root filesystem used to be on: a data drive's +// partition is p1, so a p3-only labelclear leaves a former pool member's ZFS +// label in place at both ends of the device. +func TestClearDiskWipesEveryPartitionItFinds(t *testing.T) { f := fakeCommands(t) - disks := tempDisks(t, 1, 40) - if err := clearDisk(disks[0]); err != nil { + d := tempDisks(t, 1, 40)[0] + f.seedPartitions(d.Path, biosPartNum, espPartNum, rootPartNum) + + if err := clearDisk(d); err != nil { t.Fatalf("clearDisk: %v", err) } - f.mustRun(t, "zpool labelclear -f "+disks[0].PartitionPath(rootPartNum)) + for _, n := range []int{biosPartNum, espPartNum, rootPartNum} { + f.mustRun(t, "zpool labelclear -f "+d.PartitionPath(n)) + f.mustRun(t, "wipefs -a "+d.PartitionPath(n)) + } + f.mustRun(t, "wipefs -a "+d.Path) + f.mustRun(t, "sgdisk -Z "+d.Path) +} - f2 := fakeCommands(t) +func TestClearDiskProbesNoPartitionsOnABlankDrive(t *testing.T) { + f := fakeCommands(t) blank := Disk{Path: filepath.Join(t.TempDir(), "sdb")} + if err := clearDisk(blank); err != nil { t.Fatalf("clearDisk: %v", err) } - f2.mustNotRun(t, "labelclear") - f2.mustRun(t, "wipefs -a "+blank.Path) + f.mustNotRun(t, "labelclear") + f.mustRun(t, "wipefs -a "+blank.Path) +} + +// The reported field failure: a drive whose old table the kernel never dropped. +// Proceeding would format the previous layout's offsets, so the install must +// stop, and the message must say the table is stale rather than blaming the +// device node that did not appear. +func TestClearDiskFailsWhenTheKernelKeepsTheOldTable(t *testing.T) { + f := fakeCommands(t) + d := tempDisks(t, 1, 40)[0] + f.seedPartitions(d.Path, biosPartNum, espPartNum, rootPartNum) + // sgdisk writes to the platters and exits 0, but BLKRRPART returned EBUSY, + // so the kernel's view never changes. + f.holdOpen(d.Path) + + err := clearDisk(d) + if err == nil { + t.Fatal("clearDisk succeeded against a stale partition table") + } + for _, want := range []string{d.Path, "still shows partitions", "previous layout"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("err = %q, want it to mention %q", err, want) + } + } +} + +func TestClearDiskReleasesMountsAndSwapFirst(t *testing.T) { + f := fakeCommands(t) + d := tempDisks(t, 1, 40)[0] + f.seedPartitions(d.Path, biosPartNum, espPartNum, rootPartNum) + + root, esp := d.PartitionPath(rootPartNum), d.PartitionPath(espPartNum) + swapDev := d.PartitionPath(biosPartNum) + fakeProcTables(t, + fmt.Sprintf("%s /oldroot ext4 rw 0 0\n%s /oldroot/boot/efi vfat rw 0 0\n", root, esp), + fmt.Sprintf("Filename\t\t\t\tType\t\tSize\tUsed\tPriority\n%s partition 1000 0 -2\n", swapDev), + ) + + if err := clearDisk(d); err != nil { + t.Fatalf("clearDisk: %v", err) + } + f.mustRun(t, "swapoff "+swapDev) + + // Deepest mountpoint first: unmounting /oldroot while /oldroot/boot/efi is + // still attached fails with EBUSY and leaves the disk held. + var order []string + for _, c := range f.calls { + if c == "umount "+root || c == "umount "+esp { + order = append(order, c) + } + } + if want := []string{"umount " + esp, "umount " + root}; !slices.Equal(order, want) { + t.Errorf("unmount order:\n got %v\nwant %v", order, want) + } +} + +// A drive held by md or LVM cannot be released — the ISO ships neither tool — +// so the operator has to be told what is holding it. +func TestStaleTableErrorNamesTheHolder(t *testing.T) { + f := newFakeSysfs(t, fakeDisk{name: "sda", sectors: 1 << 20, partitions: []string{"sda1"}}) + f.addHolder("sda", "sda1", "md0") + + prev := kernelPartitions + kernelPartitions = readKernelPartitions + t.Cleanup(func() { kernelPartitions = prev }) + + err := staleTableError(Disk{Path: "/dev/sda"}, nil, []string{"sda1"}) + if err == nil || !strings.Contains(err.Error(), "held by md0") { + t.Fatalf("err = %v, want it to name the md0 holder", err) + } +} + +// The reported field failure as a unit test: two drives arrive carrying a +// previous install's partitions and the third has never been partitioned. All +// three must be taken over, and the blank one must not be the only one to get +// a new table. +func TestPartitionDisksTakesOverDrivesThatAlreadyHavePartitions(t *testing.T) { + f := fakeCommands(t) + nodes := tempDisks(t, 3, 200) + f.seedPartitions(nodes[0].Path, biosPartNum, espPartNum, rootPartNum) // held an OS + f.seedPartitions(nodes[1].Path, dataPartNum) // held data + // nodes[2] arrives blank. + + cfg := DiskConfig{FS: FSExt4}.WithRoles([]RoleMount{ + {Role: RoleOS, Disk: nodes[0]}, + {Role: RoleSpinifex, Disk: nodes[1]}, + {Role: RolePredastore, Disk: nodes[2]}, + }) + if err := partitionDisks(cfg); err != nil { + t.Fatalf("partitionDisks: %v", err) + } + for _, d := range nodes { + f.mustRun(t, "sgdisk -Z "+d.Path) + } + f.mustRun(t, "sgdisk", nodes[0].Path, "-n 3:0:-1024M") + for _, d := range nodes[1:] { + f.mustRun(t, "sgdisk", d.Path, "-n 1:1M:-1024M", "-t 1:8300") + } +} + +// A previous OS install leaves p1, p2, p3 — exactly the numbering the boot +// layout writes. Asserting the expected partitions exist would pass on those +// stale nodes and the install would format the old offsets, so the erase-stage +// assertion on an empty table is what has to catch it. +func TestPartitionOneStopsWhenAnOldTableHasTheSameLayout(t *testing.T) { + f := fakeCommands(t) + d := tempDisks(t, 1, 200)[0] + f.seedPartitions(d.Path, biosPartNum, espPartNum, rootPartNum) + f.holdOpen(d.Path) + + err := partitionOne(d, typeLinuxFS) + if err == nil { + t.Fatal("partitionOne succeeded against a stale partition table") + } + // Specifically the erase-stage message: reaching the post-write check would + // mean the stale nodes had already been accepted. + if !strings.Contains(err.Error(), "still shows partitions") { + t.Errorf("err = %q, want the erase-stage stale-table message", err) + } + f.mustNotRun(t, "sgdisk -n") } func TestFormatESPsLabelsEveryMember(t *testing.T) { diff --git a/cmd/installer/install/zfs.go b/cmd/installer/install/zfs.go index fa2fecfad..b3fa65c56 100644 --- a/cmd/installer/install/zfs.go +++ b/cmd/installer/install/zfs.go @@ -340,23 +340,35 @@ func exportPool() { } } -// clearDisk removes any prior filesystem, partition table and ZFS label from a -// disk so a retry after a partial install is not rejected for stale metadata — -// a failure that otherwise reads as mysterious rather than as leftover debris. +// clearDisk takes a disk over unconditionally, whatever it holds: everything +// attached to it is released, every signature on it is wiped, its partition +// table is erased, and the kernel is made to agree the disk is now empty. +// +// The last part is the point. sgdisk writes to the platters and exits 0 even +// when the kernel refuses to drop the old table, so without the assertion an +// install proceeds against partition nodes describing the previous layout. // // The probes are quiet because a blank disk makes all of them fail, and their // stderr on the install console reads like the install is going wrong. func clearDisk(d Disk) error { - // ZFS labels live at both ends of the device and wipefs does not know about - // them, so an old pool member has to be cleared before its partition goes. - if old := d.PartitionPath(rootPartNum); exists(old) { - _ = runQuiet("zpool", "labelclear", "-f", old) - } - _ = runQuiet("wipefs", "-a", d.Path) - return runQuiet("sgdisk", "-Z", d.Path) -} + releaseDisk(d) -func exists(path string) bool { - _, err := os.Stat(path) - return err == nil + // Every partition, not just the root slice. ZFS labels live at both ends of + // a device and wipefs does not know about them, so a data drive that was + // once a pool member keeps its label and fails a later zpool create. + parts, err := kernelPartitions(d.Path) + if err != nil { + return err + } + for _, p := range parts { + dev := partitionDevice(d.Path, p) + _ = runQuiet("zpool", "labelclear", "-f", dev) + _ = runQuiet("wipefs", "-a", dev) + } + + _ = runQuiet("wipefs", "-a", d.Path) + if err := runQuiet("sgdisk", "-Z", d.Path); err != nil { + return fmt.Errorf("erase partition table: %w", err) + } + return settlePartitions(d) }