diff --git a/REUSE.md b/REUSE.md index cf130ce..13e2b1f 100644 --- a/REUSE.md +++ b/REUSE.md @@ -15,13 +15,14 @@ | `SudoHostOps.run` | internal/storage/hostops.go | `run(ctx, name, args...) error` | allowlisted exec with stderr-wrapped error | Every arg pre-validated via validate.go before this is called | | `Prober.Probe` | internal/capability/probe.go | `Probe(ctx) []Status` | live sudo-policy capability check (`sudo -n -l --`) | Needs a DIRECT runner (never the sudo-prefixing one — double-sudo); never executes probed cmds. v0.86.0: config-gated caps (`Capability.GatedBy` + `Prober.GateActive`) report `inactive`/"disabled by configuration" ONLY when healthy — broken plumbing stays degraded; the pbsdr-* gate answers from `pbsdr.Manager.DRConfigured` (marker-backed across restarts) | | ~~`stageTemp`~~ (REMOVED v0.146.0, R-861) | — | — | — | Nothing the agent writes is `install`ed where root reads it any more: use `felhom-priv-apply` (below) or ship a fixed file in the bundle | -| `felhom-priv-apply` (v0.146.0, R-861) | configs/felhom-priv-apply | `felhom-priv-apply unit \| dnsmasq \| wg \| sshd-config \| sshd-key \| controller-image ` (the last reads the ref on stdin, R-861 (a) A1) | ANY agent-rendered file a root program reads (systemd unit, dnsmasq drop-in, wg-quick conf, OOB sshd) — fixed source + destination, CONTENT checked against the agent's own renderers | A new renderer needs a verb + a contract test (`internal/privapplytest.Check`) feeding its REAL output; never a new `install` sudoers line | +| `felhom-priv-apply` (v0.146.0, R-861) | configs/felhom-priv-apply | `felhom-priv-apply unit \| unit-forget (v0.159.0, R-1124) \| dnsmasq \| wg \| sshd-config \| sshd-key \| controller-image ` (the last reads the ref on stdin, R-861 (a) A1) | ANY agent-rendered file a root program reads (systemd unit, dnsmasq drop-in, wg-quick conf, OOB sshd) — fixed source + destination, CONTENT checked against the agent's own renderers | A new renderer needs a verb + a contract test (`internal/privapplytest.Check`) feeding its REAL output; never a new `install` sudoers line | | `privapplytest.Check` | internal/privapplytest/check.go | `Check(t, verb, name, content) string` | the Go↔root-checker contract: a renderer's real output must read `OK` | Skips without python3; one call per rendered shape + one refused control | | `BUNDLE_FILES` + `Bundle` (mode `bundle`, `--install-bundle`; mode `agent_update` v0.146.0, R-861) | configs/felhom-os-apply | the ONE table of root-owned paths + the installer of them | ANY new root-owned file the installer writes (sudoers line, wrapper, unit) — add it to the table, never a new installer fetch (R-840) | The builder (`scripts/build-config-bundle.py`) and the installer read the same table; `test_every_root_file_the_installer_writes_is_in_the_bundle` fails on a path the bundle lacks. Trust files (`/etc/felhom/os-trust.json`, `operator-signers`) are NEVER bundle paths (R17) | | `osupdate.ConfigUpdateExecutor` | internal/osupdate/bundle.go | signed op `agent_config_update` {agent_version, bundle_sha256} | delivering the bundle to an installed box | a courier only: the root wrapper re-verifies signature, host, nonce and sha itself | | `osupdate.Leg.SendUnsent` / `lockPass` (v0.144.0, R-868) | internal/osupdate/unsent.go | `(ctx) int` | an OS-pass report the agent never sent (killed mid-pass): the wrapper keeps `report---apply.json` beside the plan; the agent deletes it once the hub has it | any new caller that runs an apply pass must hold `lockPass` (flock, across processes) — the sender must never take a running pass's copy | | `osupdate.LoadSavedBlock` (v0.144.0, R-866) | internal/osupdate/leg.go | `(planDir) (block, savedAt, ok)` | the hub's newest os_update block as the daemon last received it (`os-update-block.json`) | the debug pass uses it ONLY when the hub cannot be reached, and says so in its header; no saved block → no pass | | `dpkg_state()` / `DPKG_STATE_SCRIPT` (v0.145.0, R-876) | configs/felhom-os-apply | `audit, journal = self.dpkg_state()` | dpkg's state in ONE call: `--audit` AND the update journal | never gate a repair on `--audit` alone — a crash leaves only the journal (measured); keep it one call (R-845) | +| `thincheck.Checker.Check` + `Describe` (v0.159.0, R-1012) | internal/thincheck/thincheck.go | `Check(ctx, vmid) []Finding` | name an ORPHAN thin device (pool holds it, LVM does not) in a whole-guest backup failure — `BackupRunner.SetThinCheck` | READ-ONLY by design: the root helper `felhom-thin-check ` sends only reserve/release_metadata_snap; an orphan needs two reads; fails open (never stops a backup). The repair stays human | | `guesthook.SnippetReady` / `Register` (v0.146.0, R-861) | internal/guesthook/install.go | `SnippetReady(path) error` | is the pre-start hook (a FIXED file from the bundle) in place — register only then | The agent never installs the hook (Proxmox runs it as root); a missing hookscript stops a guest start, so never `Register` without `SnippetReady` | ### Disk / format safety (role gates, durable IDs, format guards) @@ -56,6 +57,7 @@ | `SudoHostOps.EnsureMount` | internal/storage/hostops.go | `EnsureMount(ctx, MountSpec) error` | persistent by-UUID systemd .mount | Validate→render→stage→`install`→`enable --now`; idempotent | | `SudoHostOps.Unmount` | internal/storage/hostops.go | `Unmount(ctx, where) error` | detach a mount unit | DESTRUCTIVE — caller MUST have gated it; does not self-authorize | | `SudoHostOps.ReassertEnrolledMounts` | internal/storage/hostops.go | `ReassertEnrolledMounts(ctx)` | reboot remount (re-resolve by UUID) | Re-asserts unless mounted AND enabled (`shouldReassertMount`); skips absent UUIDs | +| `SudoHostOps.ForgetRetiredDrives` (v0.159.0, R-1124) | internal/storage/forget_retired.go | `ForgetRetiredDrives(ctx, IntentReader) int` | drop the host mount unit of a DECOMMISSIONED drive that is GONE (start-up + the 20 s tick) | Only decommissioned + absent + unmounted; a present decommissioned drive keeps its unit (H3 re-enroll). The file goes only through `felhom-priv-apply unit-forget`, which re-checks all three as root | | `GuestBinder.AttachDrive` / `DetachDrive` | internal/localapi/intermediary.go | `AttachDrive(ctx, vmid, where) (guestPath, err)` | live drive hot-swap under `/mnt/felhom-drives` | Normalizes to EXACTLY ONE bind via `countHostMounts` (converges double-binds); force re-bind when guest can't see it | | `GuestBinder.EnsureSharedParent` | internal/localapi/intermediary.go | `EnsureSharedParent(ctx) error` | shared-parent bind + boot unit | make-private+make-shared ONLY on first bind — re-running orphans the guest's slave; F2-a: compares script AND unit for staleness | | `StablePathForRaw` / `DriveNameFromRaw` | internal/localapi/intermediary.go | `StablePathForRaw("/mnt/") string` | raw host mount → stable guest path | Single source of truth both repos derive from | diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index 6e84aee..7738d28 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -59,6 +59,7 @@ import ( "gitea.dooplex.hu/admin/felhom-agent/internal/selfupdate" "gitea.dooplex.hu/admin/felhom-agent/internal/signedjobs" "gitea.dooplex.hu/admin/felhom-agent/internal/storage" + "gitea.dooplex.hu/admin/felhom-agent/internal/thincheck" "gitea.dooplex.hu/admin/felhom-agent/internal/vzpartial" "gitea.dooplex.hu/admin/felhom-agent/internal/wgtunnel" ) @@ -1425,6 +1426,9 @@ func runDaemon(cfg config.Config, logger *slog.Logger, logRing *applog.Ring) int // re-assert below sees its unit. localSrv.EnsureInternalPlace(ctx) if mountReasserter != nil { + // R-1124: a decommissioned drive that is gone loses its host mount unit first (start-up and every tick), so + // no later boot waits for it and the re-assert below no longer lists it. + mountReasserter.ForgetRetiredDrives(ctx, intentReader) mountReasserter.ReassertEnrolledMounts(ctx) } // F9: on startup (the host's bring-up/reconcile trigger), re-assert any enrolled guest data-drive @@ -1463,6 +1467,7 @@ func runDaemon(cfg config.Config, logger *slog.Logger, logRing *applog.Ring) int // unit re-disabled at runtime), then re-assert the guest binds against the now-live mounts. localSrv.EnsureInternalPlace(ctx) // `16` §5: a record written since the last tick (the signed job) if mountReasserter != nil { + mountReasserter.ForgetRetiredDrives(ctx, intentReader) // R-1124: decommissioned + gone → unit removed mountReasserter.ReassertEnrolledMounts(ctx) } localSrv.ReassertGuestBinds(ctx) @@ -2061,8 +2066,12 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St }, Logger: logger, } + // R-1012: an orphan thin device (a full host root during a snapshot) is NAMED in the backup failure the controller + // mails to the operator, through the read-only root helper. Never removed — the repair stays a human step. + thinChecker := &thincheck.Checker{API: px, Runner: &proxmox.ExecRunner{Mode: gaMode, SudoPath: cfg.Privileged.SudoPath}, Logger: logger} for _, r := range tierRunners { r.SetBeforeBackup(func(ctx context.Context, storage string, vmid int) { cleaner.Clean(ctx, storage, vmid) }) + r.SetThinCheck(func(ctx context.Context, vmid int) string { return thincheck.Describe(thinChecker.Check(ctx, vmid)) }) } srv.SetPartialSweep(func(ctx context.Context, vmid int) { for _, t := range tierTargets { diff --git a/configs/felhom-agent.sudoers b/configs/felhom-agent.sudoers index 4b0c6ce..0f9d8a4 100644 --- a/configs/felhom-agent.sudoers +++ b/configs/felhom-agent.sudoers @@ -25,6 +25,7 @@ Cmnd_Alias FELHOM_MOUNT = \ /usr/local/sbin/felhom-priv-apply ^unit mnt-[A-Za-z0-9_.\\-]+\.(mount|automount)$, \ + /usr/local/sbin/felhom-priv-apply ^unit-forget mnt-[A-Za-z0-9_.\\-]+\.mount$, \ /usr/bin/systemctl daemon-reload, \ /usr/bin/systemctl ^enable --now -- mnt-[A-Za-z0-9_.\\-]+\.mount$, \ /usr/bin/systemctl ^disable -- mnt-[A-Za-z0-9_.\\-]+\.mount$, \ @@ -101,6 +102,11 @@ Cmnd_Alias FELHOM_VZPARTIAL = \ # ruled size rule and formats ONLY that volume, ONLY when it carries no signature. The agent calls it from the signed # `add_internal_place` job (an old box), always with 0 GiB planned: its guest's volumes already exist. `plan` changes # nothing. The installer runs the helper as root itself (a new box). +# R-1012 (agent v0.159.0): the READ-ONLY orphan-thin check before and after a whole-guest backup — lists the pool's thin +# device ids (through a metadata snapshot it reserves and releases) next to LVM's. It sends no other pool message; the +# repair stays a human step. Pinned by configs/test_felhom_thin_check.py. +Cmnd_Alias FELHOM_THINCHECK = \ + /usr/local/sbin/felhom-thin-check ^[A-Za-z0-9_+][A-Za-z0-9_.+-]{0\,63} [A-Za-z0-9_+][A-Za-z0-9_.+-]{0\,63}$ Cmnd_Alias FELHOM_INTERNALPLACE = \ /usr/local/sbin/felhom-internal-place ^(plan|create) 0$ @@ -336,4 +342,4 @@ Cmnd_Alias FELHOM_GUESTNET = \ /usr/sbin/pct ^exec [0-9]+ -- pgrep -x dhclient$, \ /usr/sbin/pct ^exec [0-9]+ -- dhclient -pf /run/dhclient\.eth0\.pid -lf /var/lib/dhcp/dhclient\.eth0\.leases eth0$ -felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP, FELHOM_STALELOCK, FELHOM_FSTRIM, FELHOM_NETMOUNT, FELHOM_WG, FELHOM_SELFUPDATE, FELHOM_SSHD, FELHOM_OOB, FELHOM_PBSDR, FELHOM_BACKUPTARGET, FELHOM_SELFHEAL, FELHOM_ESCROW, FELHOM_GUESTNET, FELHOM_SCRATCH_TEARDOWN, FELHOM_OSAPPLY, FELHOM_POWEROFF, FELHOM_VZPARTIAL, FELHOM_INTERNALPLACE +felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP, FELHOM_STALELOCK, FELHOM_FSTRIM, FELHOM_NETMOUNT, FELHOM_WG, FELHOM_SELFUPDATE, FELHOM_SSHD, FELHOM_OOB, FELHOM_PBSDR, FELHOM_BACKUPTARGET, FELHOM_SELFHEAL, FELHOM_ESCROW, FELHOM_GUESTNET, FELHOM_SCRATCH_TEARDOWN, FELHOM_OSAPPLY, FELHOM_POWEROFF, FELHOM_VZPARTIAL, FELHOM_INTERNALPLACE, FELHOM_THINCHECK diff --git a/configs/felhom-os-apply b/configs/felhom-os-apply index 8fd8fe9..81565fb 100755 --- a/configs/felhom-os-apply +++ b/configs/felhom-os-apply @@ -191,6 +191,8 @@ BUNDLE_FILES = [ ("/usr/local/sbin/felhom-backup-target-apply", "felhom-backup-target-apply", 0o755, "bash", "replace"), # R-1006 (agent v0.156.0): the only delete of backup files — a dead half-written vzdump, re-checked as root. ("/usr/local/sbin/felhom-vzdump-partial-clean", "felhom-vzdump-partial-clean", 0o755, "bash", "replace"), + # R-1012 (agent v0.159.0): the read-only orphan-thin check around a whole-guest backup (names an orphan, never removes). + ("/usr/local/sbin/felhom-thin-check", "felhom-thin-check", 0o755, "bash", "replace"), # felhom.eu architecture/16 §5 (agent v0.158.0): the internal data place — the only lvcreate/mkfs of a system-disk volume. ("/usr/local/sbin/felhom-internal-place", "felhom-internal-place", 0o755, "bash", "replace"), ("/usr/local/sbin/felhom-os-apply", "felhom-os-apply", 0o755, "python", "replace"), diff --git a/configs/felhom-priv-apply b/configs/felhom-priv-apply index 15c5d57..e21902e 100755 --- a/configs/felhom-priv-apply +++ b/configs/felhom-priv-apply @@ -13,6 +13,8 @@ longer name the destination: each verb has a fixed source and a fixed (or strict Verbs (each one sudoers line, exact-match pattern): unit /var/lib/felhom-agent/units/ -> /etc/systemd/system/ (.mount | .automount) + unit-forget REMOVES /etc/systemd/system/ (R-1124): only a felhom by-UUID drive .mount unit that is + disabled, not mounted, and whose drive is absent (its /dev/disk/by-uuid link is gone) dnsmasq /tmp/felhom-resolver-.conf -> /etc/dnsmasq.d/felhom-<...>.conf wg /var/lib/felhom-agent/wg/wg-felhom.conf -> /etc/wireguard/wg-felhom.conf (0600) sshd-config /var/lib/felhom-agent/felhom-sshd/sshd_config -> /etc/felhom-sshd/sshd_config @@ -41,7 +43,7 @@ WG_SRC, WG_DEST = STATE + "/wg/wg-felhom.conf", "/etc/wireguard/wg-felhom.conf" SSHD_SRC, SSHD_DEST = STATE + "/felhom-sshd/sshd_config", "/etc/felhom-sshd/sshd_config" KEY_SRC, KEY_DEST = STATE + "/felhom-sshd/authorized_keys.felhom-op", "/etc/felhom-sshd/authorized_keys/felhom-op" MAX_BYTES = 64 * 1024 -VERBS = ("unit", "dnsmasq", "wg", "sshd-config", "sshd-key", "controller-image") +VERBS = ("unit", "unit-forget", "dnsmasq", "wg", "sshd-config", "sshd-key", "controller-image") # R-861 (a) A1 (`09` §3 decision 165): the SAME pattern as the agent's controllerImageRe (internal/localapi/ # controllerswap.go) — a compromised agent cannot hand the guest's bootstrap any other image. Pinned by # configs/test_felhom_priv_apply.py ControllerImage. @@ -51,6 +53,11 @@ CONTROLLER_IMAGE_MAX = 256 VMID_RE = re.compile(r"^[0-9]{1,9}$") UNIT_NAME_RE = re.compile(r"^mnt-[A-Za-z0-9_.\\-]+\.(mount|automount)$") +DRIVE_UNIT_NAME_RE = re.compile(r"^mnt-[A-Za-z0-9_.\\-]+\.mount$") +DRIVE_UNIT_MARKER = "Managed by felhom-agent" +NET_UNIT_MARKER = "felhom network storage" +WANTS_DIR = UNIT_DIR + "/multi-user.target.wants" +BY_UUID_DIR = "/dev/disk/by-uuid" DNSMASQ_TMP_RE = re.compile(r"^/tmp/felhom-resolver-[0-9]+\.conf$") DNSMASQ_NAME_RE = re.compile(r"^felhom-[a-z0-9][a-z0-9._-]*\.conf$") SEG = r"[A-Za-z0-9_-][A-Za-z0-9_.-]*" @@ -135,6 +142,36 @@ class Host: def read_stdin(self, limit): return sys.stdin.buffer.read(limit + 1) + # ---- unit-forget (R-1124) ---- + def read_installed(self, path): + """The installed unit: None when absent; Refused when it is not a regular file (a symlink is never followed).""" + try: + fd = os.open(path, os.O_RDONLY | os.O_NOFOLLOW | os.O_CLOEXEC) + except FileNotFoundError: + return None + except OSError as e: + raise Refused("F1", f"cannot open {path}: {e.strerror}") + with os.fdopen(fd, "rb") as f: + if not stat.S_ISREG(os.fstat(f.fileno()).st_mode): + raise Refused("F1", f"{path} is not a regular file") + return f.read(MAX_BYTES + 1) + + def lexists(self, path): + return os.path.lexists(path) + + def mountpoints(self): + """Every mount point in /proc/self/mountinfo (field 5, octal escapes decoded).""" + out = set() + with open("/proc/self/mountinfo", "r", encoding="utf-8", errors="replace") as f: + for line in f: + parts = line.split(" ") + if len(parts) > 4: + out.add(re.sub(r"\\([0-7]{3})", lambda m: chr(int(m.group(1), 8)), parts[4])) + return out + + def remove(self, path): + os.remove(path) + def write_guest_image(self, vmid, data): """As root: `pct exec -- tee ` with the checked ref on stdin (no shell).""" r = subprocess.run(["/usr/sbin/pct", "exec", str(vmid), "--", "tee", CONTROLLER_IMAGE_FILE], @@ -424,6 +461,45 @@ def controller_image(rest, host): return 0 +def unit_forget(rest, host): + """R-1124: remove ONE felhom drive mount unit that the agent has already disabled, whose drive is gone. A drive that + is still present or still mounted keeps its unit — this verb can only forget, never unmount or take a drive away.""" + try: + if len(rest) != 1 or not DRIVE_UNIT_NAME_RE.match(rest[0]) or "/" in rest[0]: + raise Refused("A1", "usage: felhom-priv-apply unit-forget mnt-.mount") + name = rest[0] + path = os.path.join(UNIT_DIR, name) + data = host.read_installed(path) + if data is None: + host.log(f"felhom-priv-apply: ABSENT unit-forget {path} (nothing to remove)") + return 0 + text = text_of(data, path) + if DRIVE_UNIT_MARKER not in text or NET_UNIT_MARKER in text: + raise Refused("F2", f"{name} is not a felhom drive unit") + check_unit(name, text) + s = parse_ini(text, name) + what, where = s["Mount"].get("What", ""), s["Mount"]["Where"] + mu = re.match(r"^/dev/disk/by-uuid/(.+)$", what) + if not mu: + raise Refused("F2", f"{name} is not a by-UUID drive unit") + if host.lexists(os.path.join(WANTS_DIR, name)): + raise Refused("F3", f"{name} is still enabled (disable it first)") + if where in host.mountpoints(): + raise Refused("F4", f"{where} is mounted") + if host.lexists(os.path.join(BY_UUID_DIR, mu.group(1))): + raise Refused("F5", f"the drive of {name} is present (filesystem UUID {mu.group(1)})") + except Refused as e: + host.log(f"felhom-priv-apply: REFUSED [{e.rule}] unit-forget {' '.join(rest)[:120]}: {e.reason}") + return 2 if e.rule == "A1" else 3 + try: + host.remove(path) + except OSError as e: + host.log(f"felhom-priv-apply: FAILED unit-forget {path}: {e}") + return 4 + host.log(f"felhom-priv-apply: REMOVED unit-forget {path} (where={where})") + return 0 + + def main(argv, host=None): host = host or Host() if argv == ["--self-check"]: @@ -445,6 +521,8 @@ def main(argv, host=None): return 0 if argv and argv[0] == "controller-image": return controller_image(argv[1:], host) + if argv and argv[0] == "unit-forget": + return unit_forget(argv[1:], host) try: verb, src, dest, mode, checker = plan(argv) data = host.read_source(src) diff --git a/configs/felhom-thin-check b/configs/felhom-thin-check new file mode 100755 index 0000000..35bb761 --- /dev/null +++ b/configs/felhom-thin-check @@ -0,0 +1,74 @@ +#!/bin/bash +# felhom-thin-check — list the thin device ids a thin pool holds next to the ones LVM knows (R-1012). READ-ONLY: it +# names an orphan, it never removes one. Installed as /usr/local/sbin/felhom-thin-check (config bundle). +# +# felhom-thin-check +# +# A full host root while vzdump takes its snapshot can leave a thin snapshot inside the pool that LVM no longer knows +# (demo-hp 2026-10-14: `thin_ls` device 4, LVM only 1 and 2); from then on every snapshot backup of that guest fails +# („Creation of new snapshot 4 of device 2 failed") and nothing said why. The repair (`dmsetup message … delete `) +# stays a HUMAN step, decided by the operator — this helper only reads. +# +# Reading the live pool's device list needs a metadata snapshot: the kernel's documented way to read metadata while +# the pool is in use (`reserve_metadata_snap`, `thin_ls --metadata-snap`, `release_metadata_snap`). It changes no data +# and no mapping; it is held for the length of one `thin_ls -o DEV` (well under a second on these pools). Rules: +# 1. the arguments are an LVM name pair; / is a thin pool (lv_attr starts with `t`); +# 2. only one run at a time (a lock file); a metadata snapshot that someone ELSE holds is never touched — the run +# refuses (exit 3) and does not release it; +# 3. LVM's list is read before AND after the pool's, so a volume created in between is not reported as an orphan +# (the caller treats an id as an orphan only when it is in the pool and in neither LVM list). +# Output, one record per line: lvm-before | pool | lvm-after | ok +# Exit: 0 read, 1 refused (arguments / not a thin pool), 3 a metadata snapshot is already held or cannot be reserved, +# 4 the pool could not be read. Pinned by configs/test_felhom_thin_check.py. +set -u +LVS=/usr/sbin/lvs +DMSETUP=/usr/sbin/dmsetup +THIN_LS=/usr/sbin/thin_ls +LOCK=/run/lock/felhom-thin-check.lock +NAME_RE='^[A-Za-z0-9_+][A-Za-z0-9_.+-]{0,63}$' + +die() { echo "felhom-thin-check: REFUSED: $1" >&2; exit "${2:-1}"; } # $1 the reason, $2 the exit code + +lvm_ids() { # $1 tag → " " for every thin volume of this pool + "$LVS" --noheadings --separator , -o lv_name,thin_id --select "pool_lv=$POOL" -- "$VG" 2>/dev/null | + awk -F, -v tag="$1" '{ gsub(/ /, ""); if ($2 ~ /^[0-9]+$/) print tag, $2, $1 }' +} + +release() { "$DMSETUP" message "$TPOOL" 0 release_metadata_snap >/dev/null 2>&1; } + +main() { + [[ $# -eq 2 ]] || die "usage: felhom-thin-check " + VG=$1 POOL=$2 + local attr held ids + [[ "$VG" =~ $NAME_RE && "$POOL" =~ $NAME_RE ]] || die "not an LVM name pair" + + attr=$("$LVS" --noheadings -o lv_attr -- "$VG/$POOL" 2>/dev/null | tr -d ' ') + [[ "$attr" == t* ]] || die "$VG/$POOL is not a thin pool" + + DM="${VG//-/--}-${POOL//-/--}" + TPOOL="$DM-tpool" + TMETA="/dev/mapper/${DM}_tmeta" + + exec 9>"$LOCK" || die "cannot open the lock file" 4 + flock -w 30 9 || die "another check holds the lock" 3 + + # Field 7 of the thin-pool status is the held metadata root: "-" when no metadata snapshot exists. + held=$("$DMSETUP" status "$TPOOL" 2>/dev/null | awk '$3 == "thin-pool" { print $7 }') + [[ -n "$held" ]] || die "cannot read the status of $TPOOL" 4 + [[ "$held" == "-" ]] || die "a metadata snapshot of $TPOOL is already held (by someone else) — not touched" 3 + + lvm_ids lvm-before + "$DMSETUP" message "$TPOOL" 0 reserve_metadata_snap >/dev/null 2>&1 || die "could not reserve a metadata snapshot of $TPOOL" 3 + trap release EXIT + trap 'release; trap - EXIT; exit 4' INT TERM HUP + if ! ids=$("$THIN_LS" --metadata-snap --no-headers -o DEV "$TMETA" 2>/dev/null); then + die "thin_ls could not read $TMETA" 4 + fi + release + trap - EXIT INT TERM HUP + awk '$1 ~ /^[0-9]+$/ { print "pool", $1 }' <<<"$ids" + lvm_ids lvm-after + echo ok +} + +if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then main "$@"; fi diff --git a/configs/test_felhom_priv_apply.py b/configs/test_felhom_priv_apply.py index c9bd97d..f210ac4 100644 --- a/configs/test_felhom_priv_apply.py +++ b/configs/test_felhom_priv_apply.py @@ -376,5 +376,67 @@ class ControllerImage(unittest.TestCase): self.assertIn("controller-image", buf.getvalue()) +class UnitForget(unittest.TestCase): + """R-1124: the agent forgets a decommissioned drive whose disk is gone — the root half removes its unit file, and + ONLY a felhom by-UUID drive unit that is disabled, not mounted, and whose /dev/disk/by-uuid link is absent. + RED-PROOF: on the pre-R-1124 wrapper `unit-forget` is not a verb (A1 usage, rc 2) — the removal case fails.""" + NAME = "mnt-hdd_1.mount" + PATH = "/etc/systemd/system/mnt-hdd_1.mount" + UUID_LINK = "/dev/disk/by-uuid/91d2dc2d-2d28-4929-9bdd-3e11fa2f41ae" + + def host(self, text=LOCAL_UNIT, kind="file", links=(), mounted=()): + h = FakeHost() + h.installed = {} if text is None else {self.PATH: (kind, text.encode())} + h.links, h.mounted, h.removed = set(links), set(mounted), [] + h.read_installed = lambda p: (None if p not in h.installed else + (_ for _ in ()).throw(pa.Refused("F1", f"{p} is not a regular file")) + if h.installed[p][0] != "file" else h.installed[p][1]) + h.lexists = lambda p: p in h.links + h.mountpoints = lambda: set(h.mounted) + h.remove = lambda p: h.removed.append(p) + return h + + def test_an_absent_disabled_unmounted_drive_unit_is_removed(self): + h = self.host() + self.assertEqual(run(h, "unit-forget", self.NAME), 0, h.logs) + self.assertEqual(h.removed, [self.PATH]) + + def test_already_gone_is_fine(self): + h = self.host(text=None) + self.assertEqual(run(h, "unit-forget", self.NAME), 0, h.logs) + self.assertEqual(h.removed, []) + + def refused(self, h, rule, *argv): + rc = run(h, *(argv or ("unit-forget", self.NAME))) + self.assertIn(rc, (2, 3), h.logs) + self.assertEqual(h.removed, [], h.logs) + self.assertTrue(any(f"[{rule}]" in l for l in h.logs), h.logs) + + def test_F5_a_present_drive_keeps_its_unit(self): + self.refused(self.host(links=[self.UUID_LINK]), "F5") + + def test_F4_a_mounted_drive_keeps_its_unit(self): + self.refused(self.host(mounted=["/mnt/hdd_1"]), "F4") + + def test_F3_an_enabled_unit_is_not_removed(self): + self.refused(self.host(links=["/etc/systemd/system/multi-user.target.wants/" + self.NAME]), "F3") + + def test_F2_a_network_unit_or_a_foreign_unit_is_not_removed(self): + self.refused(self.host(text=LOCAL_UNIT.replace("Managed by felhom-agent", "Written by someone")), "F2") + net = LOCAL_UNIT.replace("# Managed by felhom-agent", "# Managed by felhom-agent felhom network storage") + self.refused(self.host(text=net), "F2") + + def test_F1_a_symlinked_unit_is_not_followed(self): + self.refused(self.host(kind="symlink"), "F1") + + def test_A1_names(self): + for argv in (("unit-forget", "mnt-hdd_1.automount"), ("unit-forget", "sshd.service"), + ("unit-forget", "../mnt-hdd_1.mount"), ("unit-forget",), ("unit-forget", self.NAME, "x")): + self.refused(self.host(), "A1", *argv) + + def test_U3_a_unit_whose_name_lies_about_where_is_not_removed(self): + self.refused(self.host(text=LOCAL_UNIT.replace("Where=/mnt/hdd_1", "Where=/mnt/other")), "U3") + + if __name__ == "__main__": unittest.main(verbosity=2) diff --git a/configs/test_felhom_thin_check.py b/configs/test_felhom_thin_check.py new file mode 100644 index 0000000..e3e46f8 --- /dev/null +++ b/configs/test_felhom_thin_check.py @@ -0,0 +1,118 @@ +#!/usr/bin/env python3 +"""Tests for configs/felhom-thin-check (R-1012): it lists the thin pool's device ids next to LVM's and NEVER removes +anything. Fake `lvs`, `dmsetup` and `thin_ls` record every call; nothing real is touched. Each refusal has a positive +control (the healthy read), so a test cannot pass because the helper refuses everything. +Run: python3 configs/test_felhom_thin_check.py (also run by internal/thincheck's Go test) +""" +import os, pathlib, re, subprocess, tempfile, unittest + +HELPER = pathlib.Path(__file__).resolve().parent / "felhom-thin-check" + + +class Box: + def __init__(self, pool_ids=(1, 2), lvm=(("vm-9201-disk-0", 1), ("vm-9201-disk-1", 2)), attr="twi-aotz--", + held="-", reserve_rc=0, thin_ls_rc=0): + self.t = tempfile.TemporaryDirectory() + r = pathlib.Path(self.t.name) + self.log = r / "calls" + self.log.write_text("") + lvm_lines = "\\n".join(f" {n},{i}" for n, i in lvm) + pool_lines = "\\n".join(str(i) for i in pool_ids) + self.bins = {} + for name, body in { + "lvs": f'''case "$*" in *lv_attr*) echo " {attr}";; *thin_id*) printf "{lvm_lines}\\n";; esac''', + "dmsetup": f'''case "$1" in + status) echo "0 112975872 thin-pool 585 6860/262144 517080/882624 {held} rw discard_passdown queue_if_no_space - 1024";; + message) [ "$4" = reserve_metadata_snap ] && exit {reserve_rc}; exit 0;; +esac''', + "thin_ls": f'''[ {thin_ls_rc} -ne 0 ] && exit {thin_ls_rc}; printf "{pool_lines}\\n"''', + }.items(): + p = r / name + p.write_text(f'#!/bin/bash\necho "{name} $*" >> "{self.log}"\n{body}\n') + p.chmod(0o755) + self.bins[name] = p + self.lock = r / "lock" + + def run(self, *args): + script = (f'source "{HELPER}"; LVS="{self.bins["lvs"]}"; DMSETUP="{self.bins["dmsetup"]}"; ' + f'THIN_LS="{self.bins["thin_ls"]}"; LOCK="{self.lock}"; main "$@"') + r = subprocess.run(["bash", "-c", script, "x", *args], capture_output=True, text=True) + return r.returncode, r.stdout, r.stderr + + def calls(self): + return self.log.read_text().splitlines() + + +def messages(calls): + return [c.split()[-1] for c in calls if c.startswith("dmsetup message")] + + +class Reads(unittest.TestCase): + def test_healthy_pool_lists_both_sides_and_releases(self): + b = Box() + rc, out, err = b.run("pve", "data") + self.assertEqual(rc, 0, err) + self.assertEqual(out.splitlines(), ["lvm-before 1 vm-9201-disk-0", "lvm-before 2 vm-9201-disk-1", + "pool 1", "pool 2", + "lvm-after 1 vm-9201-disk-0", "lvm-after 2 vm-9201-disk-1", "ok"]) + self.assertEqual(messages(b.calls()), ["reserve_metadata_snap", "release_metadata_snap"]) + self.assertIn("thin_ls --metadata-snap --no-headers -o DEV /dev/mapper/pve-data_tmeta", b.calls()) + + def test_the_2026_10_14_orphan_is_listed(self): + rc, out, _ = Box(pool_ids=(1, 2, 4)).run("pve", "data") + self.assertEqual(rc, 0) + self.assertIn("pool 4", out.splitlines()) + + def test_dashed_names_map_to_the_device_mapper_name(self): + b = Box() + rc, _, err = b.run("felhom-scratch", "thin-pool") + self.assertEqual(rc, 0, err) + self.assertIn("dmsetup status felhom--scratch-thin--pool-tpool", b.calls()) + + +class Refuses(unittest.TestCase): + def refused(self, box, rc_want, *args): + rc, out, err = box.run(*args) + self.assertEqual(rc, rc_want, (out, err)) + self.assertNotIn("ok", out.splitlines()) + return box.calls() + + def test_arguments(self): + for args in (("pve",), ("pve", "data", "x"), ("pve;id", "data"), ("-pve", "data"), ("pve", "../data"), + ("pve", "da ta")): + calls = self.refused(Box(), 1, *args) + self.assertEqual(calls, [], args) + + def test_not_a_thin_pool(self): + calls = self.refused(Box(attr="Vwi-aotz--"), 1, "pve", "vm-9201-disk-1") + self.assertEqual(messages(calls), []) + + def test_a_snapshot_someone_else_holds_is_never_touched(self): + b = Box(held="1234") + rc, _, err = b.run("pve", "data") + self.assertEqual(rc, 3) + self.assertEqual(messages(b.calls()), []) # neither reserve nor release + # the reason line ends with the reason, not the exit code (found live on demo-hp 2026-10-17) + self.assertTrue(err.strip().endswith("not touched"), err) + + def test_reserve_failure_does_not_release(self): + calls = self.refused(Box(reserve_rc=1), 3, "pve", "data") + self.assertEqual(messages(calls), ["reserve_metadata_snap"]) + + def test_thin_ls_failure_still_releases(self): + calls = self.refused(Box(thin_ls_rc=1), 4, "pve", "data") + self.assertEqual(messages(calls), ["reserve_metadata_snap", "release_metadata_snap"]) + + +class NeverDeletes(unittest.TestCase): + def test_the_only_pool_messages_are_reserve_and_release(self): + src = HELPER.read_text() + sent = re.findall(r'"\$DMSETUP" message "\$TPOOL" 0 (\S+)', src) + self.assertEqual(sorted(set(sent)), ["release_metadata_snap", "reserve_metadata_snap"]) + for word in ("delete", "lvremove", "thin_repair", "create_thin", "create_snap", " rm "): + code = "\n".join(l for l in src.splitlines() if not l.lstrip().startswith("#")) + self.assertNotIn(word, code, word) + + +if __name__ == "__main__": + unittest.main(verbosity=2) diff --git a/internal/backup/r1012_thincheck_test.go b/internal/backup/r1012_thincheck_test.go new file mode 100644 index 0000000..571d6bb --- /dev/null +++ b/internal/backup/r1012_thincheck_test.go @@ -0,0 +1,64 @@ +package backup + +import ( + "context" + "errors" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// R-1012: a snapshot backup that fails carries the orphan finding in its error — the text the controller mails to +// the operator (`whole_guest_backup_failed`). The check runs once before the run and once, FRESH, after the failure. +// RED-PROOF (P1/redproof-thincheck.txt): with explainFailure's append removed this test fails. +func TestBackup_SnapshotFailure_NamesTheOrphan(t *testing.T) { + const why = "probable cause: thin pool pve/data holds device 4 that LVM does not know" + for name, api := range map[string]*fakeBackupAPI{ + "task fails": {vzdumpUPID: "UPID:x", waitErr: errors.New("task failed: job errors")}, + "start fails": {vzdumpErr: errors.New("vzdump: 500")}, + } { + t.Run(name, func(t *testing.T) { + r := NewBackupRunner(api, "local", "", "", "", quiet()) + calls := 0 + r.SetThinCheck(func(context.Context, int) string { calls++; return why }) + rec, err := r.Backup(context.Background(), 9201) + if err == nil || rec.Success { + t.Fatalf("want a failure, got %+v", rec) + } + if !strings.Contains(rec.Error, why) { + t.Fatalf("the failure does not name the orphan: %q", rec.Error) + } + if calls != 2 { + t.Fatalf("thin check ran %d times, want 2 (before the run and after the failure)", calls) + } + }) + } +} + +// A successful backup keeps a clean record (the pre-check only logs); a stop-mode backup takes no snapshot, so the +// check never runs; an unwired runner behaves exactly as before. +func TestBackup_ThinCheck_SilentWhenNotApplicable(t *testing.T) { + ok := &fakeBackupAPI{vzdumpUPID: "UPID:x", + content: []proxmox.StorageContent{{VolID: "local:backup/vzdump-lxc-9201-x", Content: "backup", VMID: 9201, Size: 100, CTime: 1}}} + r := NewBackupRunner(ok, "local", "", "", "", quiet()) + calls := 0 + r.SetThinCheck(func(context.Context, int) string { calls++; return "probable cause: x" }) + rec, err := r.Backup(context.Background(), 9201) + if err != nil || !rec.Success || rec.Error != "" || calls != 1 { + t.Fatalf("success: err=%v rec=%+v calls=%d (want one pre-check, clean record)", err, rec, calls) + } + + stop := NewBackupRunner(&fakeBackupAPI{vzdumpUPID: "UPID:x", waitErr: errors.New("boom")}, "local", proxmox.ModeStop, "", "", quiet()) + calls = 0 + stop.SetThinCheck(func(context.Context, int) string { calls++; return "probable cause: x" }) + rec, _ = stop.Backup(context.Background(), 9201) + if calls != 0 || strings.Contains(rec.Error, "probable cause") { + t.Fatalf("stop mode: calls=%d error=%q", calls, rec.Error) + } + + plain := NewBackupRunner(&fakeBackupAPI{vzdumpUPID: "UPID:x", waitErr: errors.New("boom")}, "local", "", "", "", quiet()) + if rec, _ := plain.Backup(context.Background(), 9201); rec.Error != "boom" { + t.Fatalf("unwired: error = %q, want the task error unchanged", rec.Error) + } +} diff --git a/internal/backup/runner.go b/internal/backup/runner.go index 4726d13..9bc8f1e 100644 --- a/internal/backup/runner.go +++ b/internal/backup/runner.go @@ -61,8 +61,12 @@ type BackupRunner struct { // beforeBackup runs just before the space preflight (R-1006): it removes a dead half-written vzdump of this guest on // this target, so a leftover from a power cut cannot hold the space this backup needs. nil = nothing. beforeBackup func(ctx context.Context, storage string, vmid int) - logger *slog.Logger - now func() time.Time + // thinCheck (R-1012) names an orphan thin device in the guest's pools in plain words ("" = none / not checked). It + // runs before a snapshot-mode backup (a WARN in the log) and again when the vzdump fails, so the failure the + // controller mails to the operator says why. It never removes anything. nil = not wired. + thinCheck func(ctx context.Context, vmid int) string + logger *slog.Logger + now func() time.Time // rejected remembers the volids already announced by warnRejectedArchiveOnce, so an incomplete // archive is reported ONCE rather than on every 5-minute due-check. Bounded in practice: one // entry per aborted upload, and a process restart clears it. Guarded by rejectedMu because the @@ -110,6 +114,22 @@ func (r *BackupRunner) SetBeforeBackup(fn func(ctx context.Context, storage stri r.beforeBackup = fn } +// SetThinCheck wires the R-1012 orphan check (see BackupRunner.thinCheck). +func (r *BackupRunner) SetThinCheck(fn func(ctx context.Context, vmid int) string) { r.thinCheck = fn } + +// explainFailure appends the orphan finding to a failed snapshot-mode backup's error. A FRESH check (not the one from +// before the run): the failure itself may be what made the orphan. Bounded, so a stuck helper cannot hold the job. +func (r *BackupRunner) explainFailure(ctx context.Context, vmid int, rec *hub.Backup) { + if r.thinCheck == nil || r.mode != proxmox.ModeSnapshot { + return + } + cctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) + defer cancel() + if why := r.thinCheck(cctx, vmid); why != "" { + rec.Error = rec.Error + " — " + why + } +} + // localPruneSpec returns the `--prune-backups` spec to apply to THIS backup, or "" to skip pruning. It // applies the configured retention ONLY when the target is a non-PBS storage — PBS offsite retention is a // separate lifecycle and must never be pruned by the per-run flag (§9). Fail-safe: if the target's type @@ -201,12 +221,18 @@ func (r *BackupRunner) backup(ctx context.Context, vmid int, onSnapshot func()) return rec, fmt.Errorf("backup: %s", rec.Error) } + if r.thinCheck != nil && r.mode == proxmox.ModeSnapshot { + if why := r.thinCheck(ctx, vmid); why != "" { + r.logger.Warn("backup: an orphan thin device may make this snapshot backup fail (R-1012)", "vmid", vmid, "finding", why) + } + } upid, err := r.api.Vzdump(ctx, proxmox.VzdumpOptions{ VMID: vmid, Storage: r.target, Mode: r.mode, Notes: r.notes, PruneBackups: r.localPruneSpec(ctx), // local target → keep-last=N; PBS/unknown → "" (no prune) }) if err != nil { rec.Error = err.Error() + r.explainFailure(ctx, vmid, &rec) rec.DurationSeconds = time.Since(start).Seconds() return rec, fmt.Errorf("backup: vzdump vmid %d: %w", vmid, err) } @@ -220,6 +246,7 @@ func (r *BackupRunner) backup(ctx context.Context, vmid int, onSnapshot func()) } if _, err := r.api.WaitTask(ctx, upid, proxmox.WaitOptions{Timeout: r.waitTimeout}); err != nil { rec.Error = err.Error() + r.explainFailure(ctx, vmid, &rec) rec.DurationSeconds = time.Since(start).Seconds() return rec, fmt.Errorf("backup: vzdump task vmid %d: %w", vmid, err) } diff --git a/internal/capability/manifest.go b/internal/capability/manifest.go index 2e843a5..3f1db5a 100644 --- a/internal/capability/manifest.go +++ b/internal/capability/manifest.go @@ -91,9 +91,11 @@ var manifest = []Capability{ {"disk-mkfs-xfs", "guarded format (xfs)", "/usr/local/sbin/felhom-mkfs-guarded", []string{"/dev/sda", "xfs"}, false, ""}, {"disk-smart", "disk SMART health read", "/usr/sbin/smartctl", []string{"-a", "-j", "/dev/sda"}, false, ""}, {"disk-lvs", "thin-pool usage read", "/usr/sbin/lvs", []string{"--reportformat", "json", "--units", "b", "-o", "lv_name,data_percent,metadata_percent", "--", "pve/data"}, false, ""}, + {"thin-orphan-check", "orphan thin device check around a whole-guest backup (read-only, R-1012)", "/usr/local/sbin/felhom-thin-check", []string{"pve", "data"}, false, ""}, // ---- Storage mount units (watchdog re-mount) ---- {"mount-unit-install", "fs-UUID mount unit install (root content check, R-861)", "/usr/local/sbin/felhom-priv-apply", []string{"unit", "mnt-felhom\\x2dx.mount"}, false, ""}, + {"mount-unit-forget", "decommissioned-drive unit removal (root check, R-1124)", "/usr/local/sbin/felhom-priv-apply", []string{"unit-forget", "mnt-felhom\\x2dx.mount"}, false, ""}, {"mount-daemon-reload", "systemd reload after unit write", "/usr/bin/systemctl", []string{"daemon-reload"}, false, ""}, {"mount-unit-enable", "mount unit enable", "/usr/bin/systemctl", []string{"enable", "--now", "--", "mnt-felhom\\x2dx.mount"}, false, ""}, {"mount-unit-disable", "mount unit disable", "/usr/bin/systemctl", []string{"disable", "--", "mnt-felhom\\x2dx.mount"}, false, ""}, diff --git a/internal/storage/forget_retired.go b/internal/storage/forget_retired.go new file mode 100644 index 0000000..afda7f6 --- /dev/null +++ b/internal/storage/forget_retired.go @@ -0,0 +1,85 @@ +package storage + +import ( + "context" + "os" + "path/filepath" + "strings" +) + +// ForgetRetiredDrives (R-1124) removes the host mount unit of every DECOMMISSIONED drive that is physically gone, so a +// later host boot no longer waits for it and the agent stops listing it (RegistryKnownTargets and +// ReassertEnrolledMounts both enumerate the units — no unit, no entry, no „absent by UUID" line every 20 s). +// +// Only a drive that is BOTH decommissioned (the operator's or the customer's permanent removal) AND absent (its +// /dev/disk/by-uuid link does not resolve) AND not mounted is forgotten. A decommissioned drive that is still plugged in +// keeps its unit and its raw host mount — that is what makes the one-click re-enroll (H3) work, and decommission stays a +// logical retire (`03` §storage). The intent record stays `decommissioned`: a forgotten drive that comes back is a new +// candidate, never auto-mounted. The root half (`felhom-priv-apply unit-forget`) re-checks every one of these conditions +// and refuses otherwise. Called at agent start, on the 20-second storage tick, and right after a decommission. Best +// effort per unit; returns how many units it removed. Pinned by TestForgetRetiredDrives_*. +func (h *SudoHostOps) ForgetRetiredDrives(ctx context.Context, intent IntentReader) int { + if intent == nil { + return 0 // no intent store → we cannot know a drive was retired; forget nothing + } + entries, err := os.ReadDir(h.unitDir) + if err != nil { + h.logger.Warn("storage: forget retired drives — cannot read unit dir", "dir", h.unitDir, "err", err) + return 0 + } + mounted := map[string]bool{} + if ms, merr := h.host.Mounts(); merr == nil { + for _, m := range ms { + mounted[m.MountPoint] = true + } + } else { + h.logger.Warn("storage: forget retired drives — mount table unreadable; forgetting nothing", "err", merr) + return 0 + } + removed := 0 + for _, e := range entries { + name := e.Name() + if !strings.HasSuffix(name, ".mount") { + continue + } + data, rerr := os.ReadFile(filepath.Join(h.unitDir, name)) + if rerr != nil { + continue + } + spec, ok := parseFelhomMountUnit(string(data)) + if !ok { + continue // not one of our by-UUID drive units (network units are refused by the parser) + } + durableID := "uuid:" + spec.UUID + if intent.Get(durableID) != IntentDecommissioned { + continue + } + if mounted[spec.Where] { + h.logger.Debug("storage: decommissioned drive still mounted — keeping its unit", "where", spec.Where) + continue + } + if _, derr := ResolveStorageDevice(durableID); derr == nil { + h.logger.Debug("storage: decommissioned drive is present — keeping its unit (re-enroll stays one click)", "where", spec.Where) + continue + } + if h.unitEnabled(name) { + if err := h.run(ctx, h.bins.Systemctl, "disable", "--", name); err != nil { + h.logger.Warn("storage: forget retired drive — disable failed", "where", spec.Where, "unit", name, "err", err) + continue + } + } + if err := h.run(ctx, h.bins.PrivApply, "unit-forget", name); err != nil { + h.logger.Warn("storage: forget retired drive — unit removal refused or failed", "where", spec.Where, "unit", name, "err", err) + continue + } + removed++ + h.logger.Info("storage: forgot a decommissioned drive that is gone — its mount unit is removed", + "name", spec.Name, "where", spec.Where, "uuid", spec.UUID, "unit", name) + } + if removed > 0 { + if err := h.run(ctx, h.bins.Systemctl, "daemon-reload"); err != nil { + h.logger.Warn("storage: daemon-reload after forgetting retired drives failed", "err", err) + } + } + return removed +} diff --git a/internal/storage/forget_retired_test.go b/internal/storage/forget_retired_test.go new file mode 100644 index 0000000..ef33d65 --- /dev/null +++ b/internal/storage/forget_retired_test.go @@ -0,0 +1,155 @@ +package storage + +import ( + "context" + "io" + "os" + "path/filepath" + "reflect" + "testing" +) + +// forgetRunner records every privileged call and plays the root half of `felhom-priv-apply unit-forget`: the unit file +// disappears from the unit dir, so the CONSEQUENCE (the drive is no longer listed) is observable, not just the calls. +type forgetRunner struct { + unitDir string + calls [][]string +} + +func (r *forgetRunner) RunStdin(ctx context.Context, _ io.Reader, name string, args ...string) ([]byte, []byte, error) { + return r.Run(ctx, name, args...) +} + +func (r *forgetRunner) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + if len(args) == 2 && args[0] == "unit-forget" { + _ = os.Remove(filepath.Join(r.unitDir, args[1])) + } + return nil, nil, nil +} + +type forgetCase struct { + intent DriveIntent + present bool // the by-uuid link resolves + mounted bool + enabled bool // the wants-symlink exists + mountErr bool +} + +const forgetUUID = "4ebfd36d-87d3-4b63-839e-a5a710fc799a" + +func runForget(t *testing.T, c forgetCase, intent IntentReader) (int, *forgetRunner, *RegistryKnownTargets, string) { + t.Helper() + unitDir := t.TempDir() + unit := "mnt-media_a.mount" + writeUnit(t, unitDir, unit, renderMountUnit(MountSpec{Name: forgetUUID, UUID: forgetUUID, Where: "/mnt/media_a", FSType: "ext4"})) + if c.enabled { + if err := os.MkdirAll(filepath.Join(unitDir, "multi-user.target.wants"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(unitDir, unit), filepath.Join(unitDir, "multi-user.target.wants", unit)); err != nil { + t.Fatal(err) + } + } + devRoot := t.TempDir() + if err := os.MkdirAll(filepath.Join(devRoot, "by-uuid"), 0o755); err != nil { + t.Fatal(err) + } + if c.present { + dev := filepath.Join(devRoot, "sdz1") + if err := os.WriteFile(dev, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := os.Symlink(dev, filepath.Join(devRoot, "by-uuid", forgetUUID)); err != nil { + t.Fatal(err) + } + } + withDevDiskRoot(t, devRoot) + hr := &fakeHostReader{} + if c.mounted { + hr.mounts = []Mount{{MountPoint: "/mnt/media_a", FSType: "ext4"}} + } + if c.mountErr { + hr.mountsErr = os.ErrPermission + } + rr := &forgetRunner{unitDir: unitDir} + ops := NewSudoHostOps(SudoHostOpsConfig{Runner: rr, Bins: Binaries{}.withDefaults(), UnitDir: unitDir, + StageDir: t.TempDir(), Host: hr, Logger: quietLogger()}) + if intent == nil { + intent = &fakeIntent{m: map[string]DriveIntent{"uuid:" + forgetUUID: c.intent}} + } + n := ops.ForgetRetiredDrives(context.Background(), intent) + return n, rr, NewRegistryKnownTargets(unitDir, intent, quietLogger()), unitDir +} + +// The R-1124 case: decommissioned, then unplugged. The unit is disabled and removed through the root checker, systemd +// is reloaded, and the CONSEQUENCE — the agent no longer lists the drive — holds. RED-PROOF (P1/redproof-forget.txt): +// with ForgetRetiredDrives' body replaced by `return 0` this test fails (the drive is still listed). +func TestForgetRetiredDrives_DecommissionedAndGone_UnitRemovedAndNotListed(t *testing.T) { + n, rr, known, _ := runForget(t, forgetCase{intent: IntentDecommissioned, enabled: true}, nil) + if n != 1 { + t.Fatalf("removed = %d, want 1 (calls %v)", n, rr.calls) + } + pa := Binaries{}.withDefaults() + want := [][]string{ + {pa.Systemctl, "disable", "--", "mnt-media_a.mount"}, + {pa.PrivApply, "unit-forget", "mnt-media_a.mount"}, + {pa.Systemctl, "daemon-reload"}, + } + if !reflect.DeepEqual(rr.calls, want) { + t.Fatalf("calls = %v\nwant %v", rr.calls, want) + } + got, err := known.Known(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(got) != 0 { + t.Fatalf("the forgotten drive is still listed: %+v", got) + } +} + +// Already disabled (the Tester 1 shape after a manual disable): no disable call, the removal still runs. +func TestForgetRetiredDrives_AlreadyDisabled_NoDisableCall(t *testing.T) { + n, rr, _, _ := runForget(t, forgetCase{intent: IntentDecommissioned}, nil) + pa := Binaries{}.withDefaults() + want := [][]string{{pa.PrivApply, "unit-forget", "mnt-media_a.mount"}, {pa.Systemctl, "daemon-reload"}} + if n != 1 || !reflect.DeepEqual(rr.calls, want) { + t.Fatalf("n=%d calls=%v want %v", n, rr.calls, want) + } +} + +// Everything that must KEEP its unit: a decommissioned drive still plugged in (re-enroll is one click — H3), one still +// mounted, an enrolled or ejected drive that is merely absent (it comes back), a drive with no recorded intent, and an +// unreadable mount table. No privileged call at all, and the drive stays listed where it was listed. +// RED-PROOF: with the present-drive check removed, the "present" case calls unit-forget and fails. +func TestForgetRetiredDrives_KeepsEveryOtherUnit(t *testing.T) { + for name, c := range map[string]forgetCase{ + "decommissioned but present": {intent: IntentDecommissioned, present: true, enabled: true}, + "decommissioned but mounted": {intent: IntentDecommissioned, mounted: true}, + "enrolled and absent": {intent: IntentEnrolled}, + "ejected and absent": {intent: IntentEjected}, + "no intent": {intent: IntentNew}, + "mount table unreadable": {intent: IntentDecommissioned, mountErr: true}, + } { + t.Run(name, func(t *testing.T) { + n, rr, _, unitDir := runForget(t, c, nil) + if n != 0 || len(rr.calls) != 0 { + t.Fatalf("n=%d calls=%v — this unit must be kept", n, rr.calls) + } + if _, err := os.Stat(filepath.Join(unitDir, "mnt-media_a.mount")); err != nil { + t.Fatalf("unit file gone: %v", err) + } + }) + } +} + +// No intent store on the host (open failure): nothing is known to be retired, so nothing is forgotten. +func TestForgetRetiredDrives_NilIntent_ForgetsNothing(t *testing.T) { + unitDir := t.TempDir() + writeUnit(t, unitDir, "mnt-media_a.mount", renderMountUnit(MountSpec{Name: "x", UUID: forgetUUID, Where: "/mnt/media_a"})) + rr := &forgetRunner{unitDir: unitDir} + ops := NewSudoHostOps(SudoHostOpsConfig{Runner: rr, UnitDir: unitDir, StageDir: t.TempDir(), Host: &fakeHostReader{}, Logger: quietLogger()}) + if n := ops.ForgetRetiredDrives(context.Background(), nil); n != 0 || len(rr.calls) != 0 { + t.Fatalf("n=%d calls=%v", n, rr.calls) + } +} diff --git a/internal/thincheck/thincheck.go b/internal/thincheck/thincheck.go new file mode 100644 index 0000000..6037268 --- /dev/null +++ b/internal/thincheck/thincheck.go @@ -0,0 +1,243 @@ +// Package thincheck finds an ORPHAN thin device: one the LVM thin pool holds that LVM itself no longer knows (R-1012). +// +// A full host root while vzdump takes its storage snapshot can leave such a device behind (demo-hp 2026-10-14: the +// pool held devices 1, 2 and 4; LVM knew 1 and 2). From then on every snapshot backup of that guest fails at the +// snapshot step („Creation of new snapshot 4 of device 2 failed") and nothing in the product said why. This package +// NAMES the orphan, in plain words, in the backup's failure — it never removes one: the repair +// (`dmsetup message -tpool 0 "delete "`) stays a human step, decided by the operator (`03` whole-guest +// backup; runbook `whole-guest-backup-orphan-thin.md`). +// +// The device list comes from the root helper `felhom-thin-check ` (config bundle), which reads the pool +// through a metadata snapshot and LVM's list before and after. An id is an orphan only when the pool holds it and +// NEITHER LVM list names it, on two reads a few seconds apart — so a volume being created at that moment is never +// reported. +package thincheck + +import ( + "context" + "fmt" + "log/slog" + "sort" + "strconv" + "strings" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// HelperPath is where the config bundle installs the root helper. +const HelperPath = "/usr/local/sbin/felhom-thin-check" + +// Pool names one LVM thin pool. +type Pool struct{ VG, Pool string } + +func (p Pool) String() string { return p.VG + "/" + p.Pool } + +// Reading is one helper run, parsed. +type Reading struct { + PoolIDs []int + LVMBefore map[int]string + LVMAfter map[int]string +} + +// Parse reads the helper's output. ok=false unless the closing "ok" line is there (a cut-off run is no reading). +func Parse(out string) (Reading, bool) { + r := Reading{LVMBefore: map[int]string{}, LVMAfter: map[int]string{}} + done := false + for _, line := range strings.Split(out, "\n") { + f := strings.Fields(line) + switch { + case len(f) == 1 && f[0] == "ok": + done = true + case len(f) == 2 && f[0] == "pool": + if id, err := strconv.Atoi(f[1]); err == nil { + r.PoolIDs = append(r.PoolIDs, id) + } + case len(f) == 3 && (f[0] == "lvm-before" || f[0] == "lvm-after"): + id, err := strconv.Atoi(f[1]) + if err != nil { + continue + } + if f[0] == "lvm-before" { + r.LVMBefore[id] = f[2] + } else { + r.LVMAfter[id] = f[2] + } + } + } + return r, done +} + +// Orphans returns the ids the pool holds that neither LVM list names, sorted. +func (r Reading) Orphans() []int { + var out []int + for _, id := range r.PoolIDs { + _, b := r.LVMBefore[id] + _, a := r.LVMAfter[id] + if !b && !a { + out = append(out, id) + } + } + sort.Ints(out) + return out +} + +// Runner is the privileged exec surface (proxmox.ExecRunner in sudo mode). +type Runner interface { + Run(ctx context.Context, name string, args ...string) ([]byte, []byte, error) +} + +// StorageLister is the read the checker needs to map a guest's volumes to their thin pools (*proxmox.Client). +type StorageLister interface { + GuestConfig(ctx context.Context, vmid int) (proxmox.GuestConfig, error) + ListStorage(ctx context.Context) ([]proxmox.Storage, error) +} + +// Checker runs the check for one guest's pools. +type Checker struct { + API StorageLister + Runner Runner + Helper string // "" → HelperPath + Pause time.Duration // between the two reads when the first finds an orphan; 0 → 3 s + Logger *slog.Logger +} + +// Finding is one pool with orphans. +type Finding struct { + Pool Pool + Orphans []int +} + +// PoolsOf maps the guest's volume-backed disks (rootfs, mpN) to the lvmthin pools they live on. +func (c *Checker) PoolsOf(ctx context.Context, vmid int) ([]Pool, error) { + cfg, err := c.API.GuestConfig(ctx, vmid) + if err != nil { + return nil, err + } + stores, err := c.API.ListStorage(ctx) + if err != nil { + return nil, err + } + thin := map[string]Pool{} + for _, s := range stores { + if s.Type == "lvmthin" && s.VGName != "" && s.ThinPool != "" { + thin[s.Storage] = Pool{VG: s.VGName, Pool: s.ThinPool} + } + } + vols := []string{cfg.RootFS} + mps := cfg.MountPoints() + keys := make([]string, 0, len(mps)) + for k := range mps { + keys = append(keys, k) + } + sort.Strings(keys) + for _, k := range keys { + vols = append(vols, mps[k]) + } + seen := map[Pool]bool{} + var out []Pool + for _, v := range vols { + store, _, ok := strings.Cut(strings.Split(v, ",")[0], ":") + if !ok || strings.HasPrefix(store, "/") { + continue // a bind mount (a path) is no volume + } + if p, isThin := thin[store]; isThin && !seen[p] { + seen[p] = true + out = append(out, p) + } + } + return out, nil +} + +// read runs the helper once. +func (c *Checker) read(ctx context.Context, p Pool) (Reading, error) { + helper := c.Helper + if helper == "" { + helper = HelperPath + } + out, stderr, err := c.Runner.Run(ctx, helper, p.VG, p.Pool) + if err != nil { + return Reading{}, fmt.Errorf("felhom-thin-check %s: %w: %s", p, err, strings.TrimSpace(string(stderr))) + } + r, ok := Parse(string(out)) + if !ok { + return Reading{}, fmt.Errorf("felhom-thin-check %s: the output has no closing ok line", p) + } + return r, nil +} + +// Check returns the pools of vmid that hold an orphan (two reads, intersected). A pool that cannot be read is logged +// and left out: the check FAILS OPEN — it explains a failure, it never stops a backup. +func (c *Checker) Check(ctx context.Context, vmid int) []Finding { + logger := c.Logger + if logger == nil { + logger = slog.Default() + } + pools, err := c.PoolsOf(ctx, vmid) + if err != nil { + logger.Warn("thincheck: cannot map the guest's disks to thin pools — not checked", "vmid", vmid, "err", err) + return nil + } + pause := c.Pause + if pause <= 0 { + pause = 3 * time.Second + } + var found []Finding + for _, p := range pools { + first, err := c.read(ctx, p) + if err != nil { + logger.Warn("thincheck: thin pool not read — not checked", "vmid", vmid, "pool", p.String(), "err", err) + continue + } + o1 := first.Orphans() + if len(o1) == 0 { + logger.Debug("thincheck: thin pool and LVM agree", "vmid", vmid, "pool", p.String(), "devices", len(first.PoolIDs)) + continue + } + select { + case <-ctx.Done(): + return found + case <-time.After(pause): + } + second, err := c.read(ctx, p) + if err != nil { + logger.Warn("thincheck: second read failed — not reported", "vmid", vmid, "pool", p.String(), "err", err) + continue + } + in2 := map[int]bool{} + for _, id := range second.Orphans() { + in2[id] = true + } + var both []int + for _, id := range o1 { + if in2[id] { + both = append(both, id) + } + } + if len(both) > 0 { + found = append(found, Finding{Pool: p, Orphans: both}) + logger.Warn("thincheck: ORPHAN thin device(s) in the pool — LVM does not know them; NOT removed (a human step)", + "vmid", vmid, "pool", p.String(), "orphans", both) + } + } + return found +} + +// Describe is the operator sentence for a backup failure. Plain words; names the pool and the ids; says nothing was +// removed and where the repair is written down. +func Describe(f []Finding) string { + if len(f) == 0 { + return "" + } + var parts []string + for _, x := range f { + ids := make([]string, len(x.Orphans)) + for i, id := range x.Orphans { + ids[i] = strconv.Itoa(id) + } + parts = append(parts, fmt.Sprintf("thin pool %s holds device %s that LVM does not know", x.Pool, strings.Join(ids, ", "))) + } + return "probable cause: " + strings.Join(parts, "; ") + + " (an orphan snapshot, usually left when the host disk filled during a backup). Snapshot backups on this pool " + + "fail until an operator removes it; Felhom did not remove anything. Repair: runbook whole-guest-backup-orphan-thin.md" +} diff --git a/internal/thincheck/thincheck_test.go b/internal/thincheck/thincheck_test.go new file mode 100644 index 0000000..cedc6ff --- /dev/null +++ b/internal/thincheck/thincheck_test.go @@ -0,0 +1,184 @@ +package thincheck + +import ( + "context" + "encoding/json" + "errors" + "io" + "log/slog" + "os/exec" + "path/filepath" + "reflect" + "runtime" + "strings" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// The helper's output on demo-hp 2026-10-14, before the repair: the pool held 1, 2 and 4; LVM knew 1 and 2 +// (audits/power-cut-fixes-2026-10-14/E/thin-ls.txt). +const orphanOut = `lvm-before 1 vm-9201-disk-0 +lvm-before 2 vm-9201-disk-1 +pool 1 +pool 2 +pool 4 +lvm-after 1 vm-9201-disk-0 +lvm-after 2 vm-9201-disk-1 +ok +` + +const healthyOut = `lvm-before 1 vm-9201-disk-0 +lvm-before 2 vm-9201-disk-1 +pool 1 +pool 2 +lvm-after 1 vm-9201-disk-0 +lvm-after 2 vm-9201-disk-1 +ok +` + +func quiet() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } + +func TestParse_TheOrphanOf20261014(t *testing.T) { + r, ok := Parse(orphanOut) + if !ok { + t.Fatal("a complete reading was not accepted") + } + if got := r.Orphans(); !reflect.DeepEqual(got, []int{4}) { + t.Fatalf("orphans = %v, want [4]", got) + } + h, _ := Parse(healthyOut) + if got := h.Orphans(); len(got) != 0 { + t.Fatalf("a healthy pool reported orphans %v", got) + } +} + +// A volume LVM created WHILE the pool was read appears only in the after-list: never an orphan. +func TestOrphans_AVolumeCreatedDuringTheReadIsNotAnOrphan(t *testing.T) { + r, _ := Parse(strings.Replace(orphanOut, "lvm-after 2 vm-9201-disk-1\n", "lvm-after 2 vm-9201-disk-1\nlvm-after 4 snap_vm-9201-disk-1_vzdump\n", 1)) + if got := r.Orphans(); len(got) != 0 { + t.Fatalf("orphans = %v, want none", got) + } +} + +// A run cut off before its closing "ok" (the helper died, the output is partial) is no reading at all. +func TestParse_NoClosingLineIsNoReading(t *testing.T) { + if _, ok := Parse(strings.TrimSuffix(orphanOut, "ok\n")); ok { + t.Fatal("a cut-off run was accepted as a reading") + } +} + +type fakeAPI struct { + cfg proxmox.GuestConfig + stores []proxmox.Storage +} + +func (f *fakeAPI) GuestConfig(context.Context, int) (proxmox.GuestConfig, error) { return f.cfg, nil } +func (f *fakeAPI) ListStorage(context.Context) ([]proxmox.Storage, error) { return f.stores, nil } + +func guest9201() *fakeAPI { + extra := map[string]json.RawMessage{ + "mp0": json.RawMessage(`"local-lvm:vm-9201-disk-1,mp=/var/lib/felhom,backup=1,size=20G"`), + "mp8": json.RawMessage(`"/mnt/felhom-drives,mp=/mnt/felhom-drives"`), + "mp9": json.RawMessage(`"local:9201/vm-9201-disk-2.raw,mp=/x"`), + } + return &fakeAPI{ + cfg: proxmox.GuestConfig{RootFS: "local-lvm:vm-9201-disk-0,size=8G", Extra: extra}, + stores: []proxmox.Storage{ + {Storage: "local", Type: "dir", Path: "/var/lib/vz"}, + {Storage: "local-lvm", Type: "lvmthin", VGName: "pve", ThinPool: "data"}, + {Storage: "felhom-pbs", Type: "pbs"}, + }, + } +} + +type seqRunner struct { + outs []string + errs []error + calls [][]string +} + +func (r *seqRunner) Run(_ context.Context, name string, args ...string) ([]byte, []byte, error) { + r.calls = append(r.calls, append([]string{name}, args...)) + i := len(r.calls) - 1 + var err error + if i < len(r.errs) { + err = r.errs[i] + } + if i < len(r.outs) { + return []byte(r.outs[i]), nil, err + } + return nil, nil, err +} + +func TestPoolsOf_RootfsAndVolumeMountpointsOnly(t *testing.T) { + c := &Checker{API: guest9201(), Runner: &seqRunner{}, Logger: quiet()} + pools, err := c.PoolsOf(context.Background(), 9201) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(pools, []Pool{{VG: "pve", Pool: "data"}}) { + t.Fatalf("pools = %v, want [pve/data] once (bind mounts and dir volumes are not thin)", pools) + } +} + +// The consequence: an orphan seen on BOTH reads becomes a finding whose sentence names the pool and the id, and says +// nothing was removed. The only command ever run is the read-only helper with . +// RED-PROOF (P1/redproof-thincheck.txt): with Orphans() returning nil this test fails. +func TestCheck_OrphanOnBothReads_IsNamed(t *testing.T) { + rr := &seqRunner{outs: []string{orphanOut, orphanOut}} + c := &Checker{API: guest9201(), Runner: rr, Pause: 1, Logger: quiet()} + f := c.Check(context.Background(), 9201) + if len(f) != 1 || f[0].Pool.String() != "pve/data" || !reflect.DeepEqual(f[0].Orphans, []int{4}) { + t.Fatalf("findings = %+v", f) + } + want := [][]string{{HelperPath, "pve", "data"}, {HelperPath, "pve", "data"}} + if !reflect.DeepEqual(rr.calls, want) { + t.Fatalf("calls = %v, want only the read-only helper twice", rr.calls) + } + d := Describe(f) + for _, s := range []string{"thin pool pve/data holds device 4 that LVM does not know", "Felhom did not remove anything", + "whole-guest-backup-orphan-thin.md"} { + if !strings.Contains(d, s) { + t.Fatalf("Describe = %q, missing %q", d, s) + } + } +} + +// An id that is gone on the second read (a volume mid-creation) is not reported; a healthy pool is read ONCE. +func TestCheck_TransientOrHealthy_NothingReported(t *testing.T) { + rr := &seqRunner{outs: []string{orphanOut, healthyOut}} + if f := (&Checker{API: guest9201(), Runner: rr, Pause: 1, Logger: quiet()}).Check(context.Background(), 9201); len(f) != 0 { + t.Fatalf("a transient id was reported: %+v", f) + } + rr = &seqRunner{outs: []string{healthyOut}} + if f := (&Checker{API: guest9201(), Runner: rr, Logger: quiet()}).Check(context.Background(), 9201); len(f) != 0 || len(rr.calls) != 1 { + t.Fatalf("healthy: findings %+v, calls %v", f, rr.calls) + } + if Describe(nil) != "" { + t.Fatal("no finding must describe as empty") + } +} + +// The helper refusing (a metadata snapshot held by someone else, exit 3) or failing: no finding, no panic — the check +// fails open; it explains a failure, it never stops a backup. +func TestCheck_HelperRefuses_FailsOpen(t *testing.T) { + rr := &seqRunner{errs: []error{errors.New("exit status 3")}} + if f := (&Checker{API: guest9201(), Runner: rr, Logger: quiet()}).Check(context.Background(), 9201); len(f) != 0 { + t.Fatalf("findings = %+v", f) + } +} + +// The root helper's own tests (configs/test_felhom_thin_check.py) run with the Go suite. +func TestRootHelperPythonSuite(t *testing.T) { + py, err := exec.LookPath("python3") + if err != nil { + t.Skip("python3 not found") + } + _, file, _, _ := runtime.Caller(0) + script := filepath.Join(filepath.Dir(file), "..", "..", "configs", "test_felhom_thin_check.py") + out, err := exec.Command(py, script).CombinedOutput() + if err != nil { + t.Fatalf("helper suite failed: %v\n%s", err, out) + } +}