diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ca31e4..514808c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,16 +18,21 @@ already correct (by-UUID), but nothing re-asserted the unit at startup. - `storage.parseFelhomMountUnit` — pure inverse of `renderMountUnit` (Name/UUID/Where/Type/Options) keyed on a `Managed by felhom-agent` marker; ignores any foreign `.mount` unit. - `(*SudoHostOps).ReassertEnrolledMounts(ctx)` — at startup (BEFORE binding into the guest) and on the - periodic 20s tick: for each enrolled `.mount` unit not currently mounted (per `/proc/mounts`), re-resolve - by UUID and re-run `EnsureMount` (idempotent `systemctl enable --now`) — re-enables a disabled unit AND - mounts the CURRENT device by UUID, so a `/dev/sdX` reshuffle is a no-op. Already-mounted drives are - skipped (no daemon-reload churn); an absent UUID is skipped (re-asserts on a later tick). + periodic 20s tick: for each enrolled `.mount` unit, re-resolve by UUID and re-run `EnsureMount` + (idempotent `systemctl enable --now`) — re-enables a disabled unit AND mounts the CURRENT device by + UUID, so a `/dev/sdX` reshuffle is a no-op. Skips ONLY the durable steady state (mounted AND enabled), + via the pure `shouldReassertMount`; a **mounted-but-DISABLED** unit (the exact live felhom-usb bug — it + serves now but a reboot would not auto-mount it) is still re-asserted to re-create the wants-symlink. + Enabled-state is read with a privilege-free `os.Lstat` of the `multi-user.target.wants` symlink + (`unitEnabled`) — no `systemctl is-enabled` subprocess, no new sudoers entry. An absent UUID is skipped + (re-asserts on a later tick). - Wired in `main.go` ahead of `ReassertGuestBinds` so mounts are live before the guest binds re-assert. - Tests (Linux, seam the device-resolution): `TestResolveStorageDevice_ToleratesDeviceLetterMove` (UUID symlink moved sdb→sdc → resolves sdc; companion asserts the cached enroll-time node differs from the freshly-resolved one — a node-based remount would target the wrong device), `..._AbsentAndScheme` (absent UUID errors; only the `uuid:` scheme resolvable), `TestParseFelhomMountUnit` (render→parse - round-trip + rejects a foreign unit). + round-trip + rejects a foreign unit), `TestShouldReassertMount` (the four mounted/enabled combos — pins + the mounted-but-disabled re-assert), `TestUnitEnabled` (wants-symlink detection). **TASK A2 — verdict: enrolling a NEW drive does NOT need an LXC restart.** The enroll path lands on the live intermediary-mount `AttachDrive` (`/disks/guest-attach` → `handleDiskGuestAttach` → `AttachDrive`, diff --git a/REPORT.md b/REPORT.md index bd6b5e6..8038e69 100644 --- a/REPORT.md +++ b/REPORT.md @@ -28,10 +28,13 @@ rewrite the unit. errors when the UUID is absent so a caller skips a gone drive instead of fail-mounting. - `storage.parseFelhomMountUnit` (`mountunit.go`) — pure inverse of `renderMountUnit`, gated on a `Managed by felhom-agent` marker; extracts Name/UUID/Where/Type/Options, ignores foreign units. -- `(*SudoHostOps).ReassertEnrolledMounts(ctx)` (`hostops.go`) — for each enrolled `.mount` unit - not in `/proc/mounts`, re-resolve by UUID and re-run `EnsureMount` (idempotent - `systemctl enable --now`). Re-enables a disabled unit AND mounts the current device by UUID; - already-mounted drives skipped (no daemon-reload churn), absent UUIDs skipped (retry next tick). +- `(*SudoHostOps).ReassertEnrolledMounts(ctx)` (`hostops.go`) — for each enrolled `.mount` unit, + re-resolve by UUID and re-run `EnsureMount` (idempotent `systemctl enable --now`). Skips ONLY the + durable steady state (mounted AND enabled), via the pure `shouldReassertMount`; a + **mounted-but-DISABLED** unit (the exact live felhom-usb state — serves now, but a reboot would not + auto-mount it) is still re-asserted so the wants-symlink is re-created. Enabled-state read with a + privilege-free `os.Lstat` of the `multi-user.target.wants` symlink (`unitEnabled`) — no + `systemctl is-enabled` subprocess / no new sudoers entry. Absent UUIDs skipped (retry next tick). - `main.go` — runs `ReassertEnrolledMounts` at startup **before** `ReassertGuestBinds`, and again ahead of the bind re-assert on the periodic 20s tick, so host mounts are live before the guest binds re-establish. @@ -46,6 +49,9 @@ rewrite the unit. (`/dev/sdb1`, `store:`, `byid:`, `""` all rejected). - `TestParseFelhomMountUnit` — `renderMountUnit`→`parseFelhomMountUnit` round-trip + rejects a non-felhom unit. +- `TestShouldReassertMount` — the four mounted/enabled combinations; pins that mounted-but-disabled + (the live bug) re-asserts and only mounted+enabled is skipped. +- `TestUnitEnabled` — wants-symlink presence ⇒ enabled (privilege-free `Lstat`). ## Task A2 — verdict: enrolling a NEW drive does NOT need an LXC restart diff --git a/internal/storage/hostops.go b/internal/storage/hostops.go index 8445af9..4364206 100644 --- a/internal/storage/hostops.go +++ b/internal/storage/hostops.go @@ -250,8 +250,15 @@ func (h *SudoHostOps) ReassertEnrolledMounts(ctx context.Context) { continue } spec, ok := parseFelhomMountUnit(string(data)) - if !ok || mounted[spec.Where] { - continue // not ours, or already mounted → nothing to do (no churn) + if !ok { + continue // not one of ours + } + // Re-assert unless the drive is BOTH mounted AND its unit enabled (the durable steady state). + // A mounted-but-DISABLED unit (the live felhom-usb bug: a prior detach left it disabled, so it + // would NOT auto-mount on the next host reboot) is re-asserted too — EnsureMount's enable --now + // re-creates the wants-symlink. Skipping a mounted-but-disabled unit would leave the reboot fragile. + if !shouldReassertMount(mounted[spec.Where], h.unitEnabled(e.Name())) { + continue // mounted + enabled → no churn } dev, derr := ResolveStorageDevice("uuid:" + spec.UUID) if derr != nil { @@ -262,10 +269,27 @@ func (h *SudoHostOps) ReassertEnrolledMounts(ctx context.Context) { h.logger.Warn("storage: re-assert enrolled mount failed", "name", spec.Name, "where", spec.Where, "err", err) continue } - h.logger.Info("storage: re-asserted enrolled mount by UUID (enable --now)", "name", spec.Name, "where", spec.Where, "uuid", spec.UUID, "device", dev) + h.logger.Info("storage: re-asserted enrolled mount by UUID (enable --now)", "name", spec.Name, "where", spec.Where, "uuid", spec.UUID, "device", dev, "wasMounted", mounted[spec.Where]) } } +// shouldReassertMount decides whether an enrolled mount needs re-asserting. Re-assert unless it is +// BOTH currently mounted AND its unit enabled — the durable steady state. The mounted-but-disabled +// case is the load-bearing one: the drive serves now but its unit has no wants-symlink, so a host +// reboot would not auto-mount it; re-asserting re-enables it. Pure → unit-tested. +func shouldReassertMount(mounted, enabled bool) bool { + return !(mounted && enabled) +} + +// unitEnabled reports whether a WantedBy=multi-user.target unit is enabled, by checking for its +// wants-symlink. A privilege-free os.Lstat (the systemd dirs are world-readable) — no subprocess and +// no sudoers entry, consistent with the durable-id reads. A missing symlink (or any stat error) == +// not enabled, so the re-assert errs toward re-enabling rather than leaving a reboot fragile. +func (h *SudoHostOps) unitEnabled(unitName string) bool { + _, err := os.Lstat(filepath.Join(h.unitDir, "multi-user.target.wants", unitName)) + return err == nil +} + // mountedSet returns the set of currently-active mountpoints from /proc/mounts (mountpoint is field 2). // Best-effort: a read failure yields an empty set (every enrolled drive is then considered for // re-assert, which EnsureMount makes idempotent). diff --git a/internal/storage/hostops_test.go b/internal/storage/hostops_test.go index b88b76e..1a989dc 100644 --- a/internal/storage/hostops_test.go +++ b/internal/storage/hostops_test.go @@ -192,3 +192,52 @@ type errExitT int func (e errExitT) Error() string { return "exit status nonzero" } func errExit(code int) error { return errExitT(code) } + +// TestShouldReassertMount pins the host-reboot re-assert decision. The load-bearing case is +// mounted-but-DISABLED (the live felhom-usb bug): the drive serves now, but with no wants-symlink a +// host reboot would not auto-mount it, so it MUST still be re-asserted (enable --now re-creates the +// symlink). Only mounted+enabled is the durable steady state we skip. +func TestShouldReassertMount(t *testing.T) { + cases := []struct { + mounted, enabled, want bool + name string + }{ + {true, true, false, "mounted+enabled → durable, skip"}, + {true, false, true, "mounted+DISABLED → re-assert (the live bug: reboot would not auto-mount)"}, + {false, true, true, "unmounted+enabled → re-assert (mount it now)"}, + {false, false, true, "unmounted+disabled → re-assert (enable + mount)"}, + } + for _, c := range cases { + if got := shouldReassertMount(c.mounted, c.enabled); got != c.want { + t.Errorf("%s: shouldReassertMount(%v,%v)=%v want %v", c.name, c.mounted, c.enabled, got, c.want) + } + } +} + +// TestUnitEnabled detects the WantedBy=multi-user.target wants-symlink with a privilege-free Lstat +// (no systemctl subprocess), so the re-assert can tell a disabled unit from an enabled one. +func TestUnitEnabled(t *testing.T) { + unitDir := t.TempDir() + ops := NewSudoHostOps(SudoHostOpsConfig{ + Runner: &recordingRunner{}, + Bins: Binaries{Systemctl: "/usr/bin/systemctl", Install: "/usr/bin/install"}, + UnitDir: unitDir, + StageDir: t.TempDir(), + Logger: quietLogger(), + }) + const unit = "mnt-felhom-usb.mount" + if ops.unitEnabled(unit) { + t.Fatal("a unit with no wants-symlink must report NOT enabled (the felhom-usb disabled state)") + } + wants := filepath.Join(unitDir, "multi-user.target.wants") + if err := os.MkdirAll(wants, 0o755); err != nil { + t.Fatal(err) + } + // systemd points the wants-symlink at the real unit file; the target need not exist for Lstat. + if err := os.Symlink(filepath.Join(unitDir, unit), filepath.Join(wants, unit)); err != nil { + t.Skipf("symlink unsupported here (Windows privilege): %v", err) + } + if !ops.unitEnabled(unit) { + t.Fatal("a unit WITH a wants-symlink must report enabled") + } +}