diff --git a/CHANGELOG.md b/CHANGELOG.md index 2217eb2..60db1b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,39 @@ +## v0.49.0 — reboot-during-backup stale-lock recovery (F2-b) + shared-parent script redeploy fix (F2-a) (2026-06-30) + +Closes the two host-reboot findings from `TESTRUN-fullstack-2026-06-29.md`. + +- **F2-b — startup stale-lock recovery (`internal/localapi/stalelock.go`, NEW).** A host reboot DURING a + vzdump backup leaves the guest with a `snapshot-delete`/`backup` lock + a dangling `vzdump` snapshot; + `onboot:1` then can't start the locked CT → the customer box stays DOWN until a human runs `pct unlock`. + The agent now self-heals at startup (`Server.RecoverStaleLockedGuests`, called alongside + `ReassertGuestBinds`/`RecoverFormatJob`): for each guest carrying a backup lock, **only when no vzdump is + genuinely in-flight** (the load-bearing invariant — at startup the agent's own backup loop hasn't run, so + the lock is stale; the guard fails SAFE if it can't confirm), it `pct unlock`s → deletes the dangling + `vzdump` snapshot (API + WaitTask) → starts the CT **iff** `onboot` and not already running. Scope is + strictly the two vzdump locks; `migrate`/`disk`/`create`/… are left untouched. Idempotent. + - **`internal/proxmox`:** new reads `GuestConfig.Lock()`/`OnBoot()`, `Client.ListSnapshots`, + `Client.ListRunningTasks`, and the `Snapshot` type. Reads + snapshot-delete + start go through the API + token; only `pct unlock` shells out (no API equivalent). + - **sudoers + capability manifest:** new narrow grant `FELHOM_STALELOCK = /usr/sbin/pct unlock [0-9]*` + and Critical capability `stalelock-unlock` (a stuck-locked guest = customer box down). `visudo -cf` clean; + covered by the manifest↔sudoers build gate. + - Tests: recovery sequence + companions — no-lock touches nothing; non-backup lock left alone; onboot=0 + unlocked-but-not-started; delsnapshot only when a snapshot exists; **invariant guard** (live backup → + not cleared; unconfirmable → fail-safe); already-running → not restarted. + +- **F2-a — shared-parent boot script never redeployed (`internal/localapi/intermediary.go`).** The host's + `/mnt/felhom-drives` was still in root's `shared:1` peer group (so every drive bind DOUBLED) because the + live boot script predated the v0.36.6 `make-private` fix. Root cause: `EnsureSharedParent` gated the + (re)install on the **unit** file only, so a script-only change never deployed. Fixed: the new + `sharedParentInstallStale` helper compares **both** the script and the unit (missing or differing → + reinstall). Boot-time-only — it rewrites the on-disk script; it does NOT churn the live mount (the live + bind/make-private/make-shared stays guarded on `!isHostMountpoint`). Verified empirically on the host: + the correct `bind → make-private → make-shared` sequence gives the parent its own group + no doubling. + - Tests: a stale-script/current-unit case triggers reinstall (the F2-a regression); both-current is a + no-op; missing files are stale; a content guard asserts the shipped script keeps `make-private`. + +- Version `0.48.0 → 0.49.0`. + ## v0.48.0 — report the served local-API leaf fingerprint (hub-side re-key detection, Part A) (2026-06-29) The agent now rides its **served leaf fingerprint** on every host report so the hub can detect an diff --git a/REPORT.md b/REPORT.md index fab1e2b..0860399 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,31 +1,61 @@ -# REPORT — agent reports served leaf fingerprint (hub re-key detection, Part A), v0.48.0 +# REPORT — felhom-agent v0.49.0 -**Task:** the last self-health leg — the agent reports its served local-API **leaf fingerprint** so the -hub can detect an agent re-key fleet-wide (`host_leaf_changed`, hub v0.22.x). This repo = **Part A**. +Closes the two host-reboot findings (F2-a, F2-b) from +`felhom.eu/documentation/audits/TESTRUN-fullstack-2026-06-29.md`. Diagnose-before-fix on both; both root +causes confirmed live on `felhom-pve` before any code change. -**Baseline:** agent `main` @ `3844df7` (v0.47.0) → **v0.48.0** @ `bf8e3be`, sha `a3dad050…`. +## F2-b — reboot-during-backup stale-lock recovery (Part B) -## Change -- **`internal/hub/report.go`:** new `HostReport.LeafFingerprint string` (`json:"leaf_fingerprint"`) — - the SHA-256 of the leaf the agent currently serves. Empty when the local API is disabled (no leaf) → - the hub treats "" as unknown, never an alert. Not a secret (the fp is public; the token is never - reported). -- **`internal/hub/collect.go` + `cmd/felhom-agent/main.go`:** `Collector.SetLeafFingerprint(fp)` threads - the `fp` from `EnsureLeaf` (the SAME value the LOADED/REGENERATED log reports) into every report, next - to `Capabilities`. +**Diagnosis (B.0, confirmed live).** An interrupted vzdump leaves the guest config with a backup lock +(`lock: snapshot-delete` after a snapshot-mode backup's cleanup, or `lock: backup`) + a dangling `vzdump` +snapshot. `onboot:1` then fails: *"Starting CT … failed: CT is locked (snapshot-delete)"* — the box stays +DOWN. **Invariant:** at agent startup the agent's own backup loop has not run, so a backup lock present then +is stale by definition — *unless* a vzdump is genuinely in-flight (external, or one that outlived a bare +agent restart). That is the one case clearing would corrupt, so the recovery confirms no vzdump is running +and **fails safe** (leaves the lock) if it can't. -## Tests (green: `go build/vet/test ./...`) -`TestCollect_LeafFingerprint`: the report carries the fp when set, and `""` when unset (local API -disabled) — companion proving the threading is what populates it. The golden + contract + field-names -tests updated; the cross-repo golden mirrors `leaf_fingerprint` (byte-identical with the hub copy). +**Fix.** `internal/localapi/stalelock.go` (NEW) + `Server.RecoverStaleLockedGuests`, run at startup next to +`ReassertGuestBinds`/`RecoverFormatJob`. Per guest with a backup lock and no in-flight vzdump: `pct unlock` +→ delete the dangling `vzdump` snapshot (API + WaitTask, only when one exists) → start **iff** `onboot` and +not already running. Reads (`GuestConfig.Lock()`/`OnBoot()`, `ListSnapshots`, `ListRunningTasks`), +snapshot-delete and start go through the API token; only `pct unlock` shells out (no API equivalent). -## Live validation (felhom-pve) — PASS -Deployed 0.48.0 (`leaf LOADED` fp `60b5974d…`, capabilities 45/45). The hub received the report with -`leaf_fingerprint: 60b5974d…` (confirmed in the hub DB), then — during the end-to-end re-key test — a -regenerated leaf (`82078fab…`) and its restore (`60b5974d…`) both rode the report stream and the hub -raised `host_leaf_changed` on the change (see felhom.eu/REPORT.md). Final state: leaf == pin -`60b5974d…`, capabilities 45/45, channel recovered. +**Grants.** New narrow `FELHOM_STALELOCK = /usr/sbin/pct unlock [0-9]*` + Critical capability +`stalelock-unlock`. `visudo -cf` clean; the manifest↔sudoers build gate covers it. -## NOT changed -The pin/transport, the token store, the sudoers/capability surface, the swap, the channel-health logic. -Report contract + collector threading only. +**Tests.** Sequence + companions: no-lock → nothing called; `migrate` lock → left alone; onboot=0 → +unlocked-but-not-started; delsnapshot only when a snapshot exists; **invariant guard** (live backup → not +cleared; unconfirmable → fail-safe); already-running → not restarted; nil controller → no-op. + +**Live (B.3).** Simulated stale state on a scratch guest (lock + dangling `vzdump` snapshot) → agent +restart auto-recovered (unlock + delsnapshot + start). The real reboot-during-backup is the §D supervised +step. + +## F2-a — shared-parent boot script never redeployed (Part C) + +**Diagnosis (C.0, confirmed live).** `/mnt/felhom-drives` was in root's `shared:1` peer group, so each +drive bind appeared twice. Reproduced exactly in a scratch dir at `/mnt`: the **stale** sequence (`bind + +make-shared`, no `make-private`) leaves the self-bind in `shared:1` → sub-bind count 2 (doubling); the +**correct** sequence (`bind → make-private → make-shared`) gives an own group → count 1. The live boot +script was the pre-v0.36.6 body (no `make-private`). Root cause: `EnsureSharedParent` gated the (re)install +on the **unit** file only, so the v0.36.6 *script-only* fix never deployed to a host whose unit was current. + +**Fix (provably safe, boot-time-only).** New `sharedParentInstallStale` compares **both** the script and +unit (missing/differing → reinstall). It rewrites the on-disk script; it does **not** touch the live mount +(the live bind/make-private/make-shared stays guarded on `!isHostMountpoint`), so it can't orphan the +guest's slave bind. The doubling persists live until the next boot, when the corrected script gives the +parent its own peer group. + +**Tests.** Stale-script/current-unit → reinstall (the F2-a regression); both-current → no-op; missing → +stale; content guard that the shipped script keeps `make-private`. + +**Red-proof (C.1).** Requires a reboot — the §D supervised step: parent must come up as its own group +(not `shared:1`) AND the guest must still see both drives (`bound_under_parent=true`). + +## Green gate + +`go build ./... && go vet ./... && go test ./...` — all green (full suite, 0 failures). Version +`0.48.0 → 0.49.0`. + +Secrets-safe: no tokens/keys logged or written. Leaf fingerprints are not secret; the operator email is not +a secret. diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index cb3f67b..3cc8c1a 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -45,7 +45,7 @@ import ( // version is the agent version. Overridable at build time with // -ldflags "-X main.version="; defaults to the in-repo CHANGELOG version. -var version = "0.48.0" +var version = "0.49.0" // runGuestHook is the PVE pre-start hook body (`felhom-agent guest-hook `). On the // pre-start phase it creates placeholder dirs for any absent bind-mount source so the guest always boots @@ -581,6 +581,10 @@ func runDaemon(cfg config.Config, logger *slog.Logger) int { }() // F20-BUG3: complete a disk format that an agent restart interrupted (durable-id-bound, re-resolved). localSrv.RecoverFormatJob(ctx) + // F2-b: recover any guest left with a stale vzdump lock by a reboot-during-backup (unlock → delete + // the dangling snapshot → start iff onboot). Runs BEFORE the backup loop starts, so a present + // backup lock is stale by definition (guarded by a no-vzdump-running check; fail-safe otherwise). + localSrv.RecoverStaleLockedGuests(ctx) go func() { errc <- localSrv.Run(ctx) }() } if lanLoop != nil { @@ -748,6 +752,9 @@ func buildLocalAPIServer(cfg config.Config, px *proxmox.Client, store *backup.St Guests2: px, GuestAttach: guestBinder, // slice 10 P2: bind enrolled data drives into the guest ControllerSwap: guestBinder, // Phase 1: agentic controller update — in-guest image swap + // F2-b: recover a guest left with a stale vzdump lock by a reboot-during-backup. Reads + start + // go through the API client; the `pct unlock` is the one fenced root-CLI op (no API equivalent). + StaleLock: localapi.NewStaleLockController(px, &proxmox.ExecRunner{Mode: gaMode, SudoPath: cfg.Privileged.SudoPath}), Intent: intent, // slice 10 P3: record enroll/eject intent for self-heal GuestBinds: guestBinds, // F9: per-guest bind record for the startup re-assert FormatJobs: formatJobs, // F20-BUG3: detached-format job record + restart recovery diff --git a/configs/felhom-agent.sudoers b/configs/felhom-agent.sudoers index ae1eaf6..9a03eff 100644 --- a/configs/felhom-agent.sudoers +++ b/configs/felhom-agent.sudoers @@ -119,4 +119,12 @@ Cmnd_Alias FELHOM_CONTROLLERSWAP = \ /usr/sbin/pct exec [0-9]* -- systemctl restart felhom-controller-bootstrap.service, \ /usr/sbin/pct exec [0-9]* -- tee /etc/felhom-controller-image -felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP +# Stale-lock recovery (F2-b, v0.49.0). A host reboot DURING a vzdump backup leaves the guest with a +# `snapshot-delete`/`backup` lock + `onboot:1` then can't start it → the customer box stays DOWN. The +# agent clears the STALE lock at startup (only when no vzdump is in-flight). `pct unlock` is the one op +# with no API equivalent (snapshot-delete + start go through the API token); the agent fine-validates the +# vmid (numeric) before exec — the `[0-9]*` is the coarse allowlist. +Cmnd_Alias FELHOM_STALELOCK = \ + /usr/sbin/pct unlock [0-9]* + +felhom-agent ALL=(root) NOPASSWD: FELHOM_MOUNT, FELHOM_DISK, FELHOM_PROVISION, FELHOM_FORMAT, FELHOM_DNSMASQ, FELHOM_GUESTHOOK, FELHOM_INTERMEDIARY, FELHOM_CONTROLLERSWAP, FELHOM_STALELOCK diff --git a/internal/capability/manifest.go b/internal/capability/manifest.go index 582d61d..c279ee8 100644 --- a/internal/capability/manifest.go +++ b/internal/capability/manifest.go @@ -97,4 +97,8 @@ var manifest = []Capability{ {"controllerswap-inspect", "controller-swap / managed auto-update", "/usr/sbin/pct", []string{"exec", "9201", "--", "docker", "inspect", "-f", "{{.State.Running}}", "felhom-controller"}, true}, {"controllerswap-restart", "controller-swap / managed auto-update", "/usr/sbin/pct", []string{"exec", "9201", "--", "systemctl", "restart", "felhom-controller-bootstrap.service"}, true}, {"controllerswap-write", "controller-swap / managed auto-update", "/usr/sbin/pct", []string{"exec", "9201", "--", "tee", "/etc/felhom-controller-image"}, true}, + + // ---- Stale-lock recovery (FELHOM_STALELOCK, v0.49.0; Critical: a guest stuck behind a stale + // reboot-during-backup lock can't start → the customer box stays DOWN until this clears it) ---- + {"stalelock-unlock", "reboot-during-backup stale-lock recovery", "/usr/sbin/pct", []string{"unlock", "9201"}, true}, } diff --git a/internal/localapi/intermediary.go b/internal/localapi/intermediary.go index ad33ccd..5912d7f 100644 --- a/internal/localapi/intermediary.go +++ b/internal/localapi/intermediary.go @@ -114,17 +114,35 @@ func (b *GuestBinder) EnsureSharedParent(ctx context.Context) error { return fmt.Errorf("shared-parent: make-shared: %w", err) } } - // Install the boot-persistence unit only when missing OR its content differs from what we ship (so a - // unit-template fix deploys) — EnsureSharedParent runs on a periodic reconcile, and re-writing files + - // daemon-reload every tick would be wasteful, so the common case (unchanged) is a cheap read. - if cur, err := os.ReadFile(sharedParentUnitPath); err != nil || string(cur) != sharedParentUnit { + // (Re)install the boot-persistence files only when the on-disk SCRIPT or UNIT differs from what we + // ship (or is missing) — EnsureSharedParent runs on a periodic reconcile, so re-writing files + + // daemon-reload every tick would be wasteful; the common case (both current) is two cheap reads. + // + // F2-a: this MUST compare the SCRIPT too, not just the unit. The v0.36.6 make-private fix changed + // only the script (the unit was unchanged), so the earlier unit-only gate never redeployed it — + // leaving hosts running the pre-fix script (no make-private), whose self-bind stays in root's shared + // peer group and DOUBLES every drive bind. Comparing both files closes that deploy gap. + if sharedParentInstallStale(sharedParentUnitPath, sharedParentScriptPath) { if ierr := b.installSharedParentUnit(ctx); ierr != nil { - b.logger.Warn("shared-parent: boot-persistence unit install failed (live setup OK; survives until host reboot)", "err", ierr) + b.logger.Warn("shared-parent: boot-persistence (re)install failed (live setup OK; survives until host reboot)", "err", ierr) } } return nil } +// sharedParentInstallStale reports whether the on-disk boot script OR unit is missing or differs from +// what this build ships — the trigger to (re)install both. Comparing BOTH (not the unit alone) is the +// F2-a fix: a script-only change must still redeploy. Pure (path args) so it is unit-testable. +func sharedParentInstallStale(unitPath, scriptPath string) bool { + if cur, err := os.ReadFile(unitPath); err != nil || string(cur) != sharedParentUnit { + return true + } + if cur, err := os.ReadFile(scriptPath); err != nil || string(cur) != sharedParentScript { + return true + } + return false +} + // installSharedParentUnit writes the script + unit (from agent-written temps) and enables the unit so the // shared parent is re-established on every host boot before pve-guests. Idempotent. func (b *GuestBinder) installSharedParentUnit(ctx context.Context) error { diff --git a/internal/localapi/intermediary_gate_test.go b/internal/localapi/intermediary_gate_test.go new file mode 100644 index 0000000..a598864 --- /dev/null +++ b/internal/localapi/intermediary_gate_test.go @@ -0,0 +1,77 @@ +package localapi + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestSharedParentInstallStale_ScriptOnlyChangeRedeploys is the F2-a regression proof: when the on-disk +// UNIT is current but the SCRIPT differs (exactly the v0.36.6 make-private situation), the gate MUST +// report stale so the corrected script redeploys. The earlier unit-only gate returned false here and +// left hosts running the pre-fix script that doubled drive binds. +func TestSharedParentInstallStale_ScriptOnlyChangeRedeploys(t *testing.T) { + dir := t.TempDir() + unitPath := filepath.Join(dir, "felhom-shared-parent.service") + scriptPath := filepath.Join(dir, "felhom-shared-parent.sh") + + // Unit current, script STALE (a pre-make-private body). + if err := os.WriteFile(unitPath, []byte(sharedParentUnit), 0o644); err != nil { + t.Fatal(err) + } + staleScript := "#!/bin/sh\nset -e\nmkdir -p /mnt/felhom-drives\nmount --make-shared /mnt/felhom-drives\n" + if err := os.WriteFile(scriptPath, []byte(staleScript), 0o755); err != nil { + t.Fatal(err) + } + + if !sharedParentInstallStale(unitPath, scriptPath) { + t.Fatal("a stale SCRIPT with a current unit must trigger reinstall (F2-a) — gate returned not-stale") + } +} + +// TestSharedParentInstallStale_BothCurrentIsNoop: when both files match what we ship, the gate reports +// not-stale (the cheap common case — no churn on every reconcile). +func TestSharedParentInstallStale_BothCurrentIsNoop(t *testing.T) { + dir := t.TempDir() + unitPath := filepath.Join(dir, "felhom-shared-parent.service") + scriptPath := filepath.Join(dir, "felhom-shared-parent.sh") + if err := os.WriteFile(unitPath, []byte(sharedParentUnit), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(scriptPath, []byte(sharedParentScript), 0o755); err != nil { + t.Fatal(err) + } + + if sharedParentInstallStale(unitPath, scriptPath) { + t.Fatal("both files current must be a no-op; gate reported stale") + } +} + +// TestSharedParentInstallStale_MissingFilesAreStale: a missing unit or script (fresh host) is stale. +func TestSharedParentInstallStale_MissingFilesAreStale(t *testing.T) { + dir := t.TempDir() + unitPath := filepath.Join(dir, "felhom-shared-parent.service") + scriptPath := filepath.Join(dir, "felhom-shared-parent.sh") + + if !sharedParentInstallStale(unitPath, scriptPath) { + t.Fatal("missing files must be stale (fresh install)") + } + + // Unit present + current, script missing → still stale. + if err := os.WriteFile(unitPath, []byte(sharedParentUnit), 0o644); err != nil { + t.Fatal(err) + } + if !sharedParentInstallStale(unitPath, scriptPath) { + t.Fatal("a missing script with a current unit must be stale (F2-a)") + } +} + +// TestSharedParentScript_HasMakePrivate is a content guard: the shipped boot script MUST contain the +// make-private step (the actual fix for the doubling). If a refactor drops it, the parent re-joins +// root's shared group on boot and binds double again. +func TestSharedParentScript_HasMakePrivate(t *testing.T) { + if want := "mount --make-private " + StableParentDir; !strings.Contains(sharedParentScript, want) { + t.Fatalf("shipped boot script missing %q — the doubling fix would regress", want) + } +} diff --git a/internal/localapi/server.go b/internal/localapi/server.go index 4965e8b..6d6c213 100644 --- a/internal/localapi/server.go +++ b/internal/localapi/server.go @@ -87,6 +87,9 @@ type Options struct { // ControllerSwap runs guest commands (pct exec) for the agentic controller-update swap (Phase 1). // OPTIONAL — when nil, POST /controller/swap reports "not configured". Satisfied by *GuestBinder. ControllerSwap GuestExecutor + // StaleLock recovers a guest left with a stale vzdump lock by a reboot-during-backup (F2-b), run at + // startup by RecoverStaleLockedGuests. OPTIONAL — when nil, the recovery is a no-op. + StaleLock StaleLockController // ControllerSwapStateDir holds the per-guest swap state file (crash-safety). "" → /var/lib/felhom-agent. ControllerSwapStateDir string // Intent records drive enroll/eject intent for the self-heal watchdog (slice 10 P3). OPTIONAL — @@ -158,6 +161,7 @@ type Server struct { intent IntentRecorder // slice 10 P3 (optional) guestBinds *GuestBindStore // F9 startup bind re-assert record (optional) formatJobs *FormatJobStore // F20-BUG3 detached-format job record (optional) + staleLock StaleLockController // F2-b startup stale-lock recovery (optional) host storage.HostReader // role classification source (optional; defaults to ProcHostReader) hostMetrics HostMetricsProvider // slice 9 (optional) @@ -224,6 +228,7 @@ func NewServer(o Options) (*Server, error) { intent: o.Intent, guestBinds: o.GuestBinds, formatJobs: o.FormatJobs, + staleLock: o.StaleLock, host: o.HostReader, hostMetrics: o.HostMetrics, hostID: o.HostID, diff --git a/internal/localapi/stalelock.go b/internal/localapi/stalelock.go new file mode 100644 index 0000000..ff5b597 --- /dev/null +++ b/internal/localapi/stalelock.go @@ -0,0 +1,245 @@ +package localapi + +import ( + "context" + "strconv" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// Stale-lock recovery (TESTRUN finding F2-b). A host reboot DURING a vzdump backup leaves the guest +// with a backup lock (`lock: snapshot-delete` after an interrupted snapshot-mode backup, or `lock: +// backup`) and possibly a dangling `vzdump` snapshot. With `onboot: 1`, pve-guests then FAILS to start +// the locked CT ("CT is locked (snapshot-delete)") — the customer box stays DOWN until a human runs +// `pct unlock`. This makes the agent self-heal it at startup. +// +// The load-bearing invariant (B.0): at AGENT STARTUP the agent's own backup loop has not run yet, so a +// backup lock present then is STALE BY DEFINITION — UNLESS a vzdump is genuinely in-flight (an external +// backup, or one that outlived a bare `systemctl restart felhom-agent`). The recovery therefore clears a +// lock ONLY after confirming no vzdump task is running for that guest. If that confirmation can't be +// made, it FAILS SAFE (leaves the lock) — a wrongly-cleared live backup would corrupt state. +// +// Scope is deliberately narrow: only the two vzdump-related locks are ever touched. migrate/disk/create/ +// rollback/mounted/snapshot locks are left alone (they belong to a different operation, mid-flight or +// needing different handling). The recovery is idempotent and never touches a guest without a stale lock. + +// staleBackupLocks are the lock values an interrupted vzdump can leave. ONLY these are cleared. +var staleBackupLocks = map[string]bool{ + "backup": true, // interrupted suspend/stop-mode backup + "snapshot-delete": true, // interrupted snapshot-mode backup's cleanup (the F2-b repro) +} + +// vzdumpSnapshotName is the snapshot a vzdump snapshot-mode backup creates then deletes; an interrupted +// one leaves it dangling under exactly this name. +const vzdumpSnapshotName = "vzdump" + +// StaleLockController is the seam the recovery composes over. Reads go through the Proxmox API; the +// unlock is the one op with no API equivalent (`pct unlock`, the fenced root-CLI runner). Satisfied in +// production by *staleLockController (over *proxmox.Client + a proxmox.Runner). +type StaleLockController interface { + // Guests lists the node's guests (vmid + running status). + Guests(ctx context.Context) ([]proxmox.Guest, error) + // Lock returns the guest's current lock ("" if unlocked) and its onboot flag. + Lock(ctx context.Context, vmid int) (lock string, onboot bool, err error) + // BackupRunning reports whether a vzdump task is genuinely in-flight for vmid (the invariant guard). + BackupRunning(ctx context.Context, vmid int) (bool, error) + // HasVzdumpSnapshot reports whether a dangling `vzdump` snapshot exists for vmid. + HasVzdumpSnapshot(ctx context.Context, vmid int) (bool, error) + // Unlock clears the guest's lock (`pct unlock` — no API equivalent). + Unlock(ctx context.Context, vmid int) error + // DeleteVzdumpSnapshot removes the dangling `vzdump` snapshot (API + WaitTask). + DeleteVzdumpSnapshot(ctx context.Context, vmid int) error + // Start starts the guest (API + WaitTask). + Start(ctx context.Context, vmid int) error +} + +// RecoverStaleLockedGuests scans every guest at startup and clears a stale vzdump lock (unlock → +// delete the dangling snapshot → start iff onboot). No-op when the controller is not wired. +func (s *Server) RecoverStaleLockedGuests(ctx context.Context) { + if s.staleLock == nil { + return + } + guests, err := s.staleLock.Guests(ctx) + if err != nil { + s.logger.Warn("stale-lock: guest list unavailable — skipping recovery", "err", err) + return + } + for _, g := range guests { + s.recoverOneStaleLock(ctx, g) + } +} + +// recoverOneStaleLock applies the recovery to a single guest. It acts ONLY on a stale backup lock and +// only when no backup is in-flight; every branch is logged so an operator can see what was (or wasn't) +// cleared. +func (s *Server) recoverOneStaleLock(ctx context.Context, g proxmox.Guest) { + lock, onboot, err := s.staleLock.Lock(ctx, g.VMID) + if err != nil { + s.logger.Warn("stale-lock: read guest config failed — skipping", "vmid", g.VMID, "err", err) + return + } + if !staleBackupLocks[lock] { + return // unlocked, or a non-backup lock we deliberately leave alone (the overwhelming common case) + } + + // INVARIANT GUARD: a backup lock is only STALE when no vzdump is genuinely running. Confirm before + // clearing; on any doubt, FAIL SAFE and leave the lock (clearing a live backup's lock corrupts it). + running, err := s.staleLock.BackupRunning(ctx, g.VMID) + if err != nil { + s.logger.Warn("stale-lock: could not confirm no backup is running — NOT clearing (fail-safe)", + "vmid", g.VMID, "lock", lock, "err", err) + return + } + if running { + s.logger.Warn("stale-lock: a vzdump backup is genuinely in-flight — leaving the lock (NOT stale)", + "vmid", g.VMID, "lock", lock) + return + } + + s.logger.Warn("stale-lock: clearing a stale backup lock left by an interrupted backup", + "vmid", g.VMID, "lock", lock, "onboot", onboot, "status", g.Status) + if err := s.staleLock.Unlock(ctx, g.VMID); err != nil { + s.logger.Error("stale-lock: pct unlock failed", "vmid", g.VMID, "err", err) + return + } + + // Remove the dangling vzdump snapshot, if any (an interrupted snapshot-mode backup leaves it). Done + // only when one actually exists, so a stop/suspend-mode interruption (no snapshot) doesn't no-op-fail. + has, err := s.staleLock.HasVzdumpSnapshot(ctx, g.VMID) + if err != nil { + s.logger.Warn("stale-lock: snapshot list failed — skipping snapshot cleanup", "vmid", g.VMID, "err", err) + } else if has { + if err := s.staleLock.DeleteVzdumpSnapshot(ctx, g.VMID); err != nil { + s.logger.Error("stale-lock: delete dangling vzdump snapshot failed", "vmid", g.VMID, "err", err) + } else { + s.logger.Info("stale-lock: removed dangling vzdump snapshot", "vmid", g.VMID) + } + } + + // Start ONLY a guest that is configured to auto-start AND is not already running. A deliberately- + // stopped guest (onboot:0, e.g. the golden) is unlocked but never started; an already-running guest + // (a bare agent restart found it up) is left as-is. + if onboot && g.Status != "running" { + if err := s.staleLock.Start(ctx, g.VMID); err != nil { + s.logger.Error("stale-lock: start after unlock failed", "vmid", g.VMID, "err", err) + return + } + s.logger.Warn("stale-lock: started CT after clearing the stale lock (onboot)", "vmid", g.VMID) + } +} + +// staleLockController is the production StaleLockController over the Proxmox API client + the fenced +// root-CLI runner. Reads (guest list, config, snapshots, running tasks), snapshot-delete and start go +// through the API (token-authed, WaitTask-asserted); only `pct unlock` shells out (no API equivalent). +type staleLockController struct { + px staleLockAPI + runner proxmox.Runner +} + +// staleLockAPI is the subset of *proxmox.Client the controller uses (kept narrow for clarity/testing). +type staleLockAPI interface { + ListLXC(ctx context.Context) ([]proxmox.Guest, error) + GuestConfig(ctx context.Context, vmid int) (proxmox.GuestConfig, error) + ListSnapshots(ctx context.Context, vmid int) ([]proxmox.Snapshot, error) + ListRunningTasks(ctx context.Context) ([]proxmox.TaskStatus, error) + DeleteSnapshot(ctx context.Context, vmid int, snapname string) (string, error) + Start(ctx context.Context, vmid int) (string, error) + WaitTask(ctx context.Context, upid string, opts proxmox.WaitOptions) (proxmox.TaskStatus, error) +} + +// NewStaleLockController builds the production controller. Returns nil if px or runner is nil (the +// feature then stays unwired and RecoverStaleLockedGuests is a no-op). +func NewStaleLockController(px staleLockAPI, runner proxmox.Runner) StaleLockController { + if px == nil || runner == nil { + return nil + } + return &staleLockController{px: px, runner: runner} +} + +func (c *staleLockController) Guests(ctx context.Context) ([]proxmox.Guest, error) { + return c.px.ListLXC(ctx) +} + +func (c *staleLockController) Lock(ctx context.Context, vmid int) (string, bool, error) { + cfg, err := c.px.GuestConfig(ctx, vmid) + if err != nil { + return "", false, err + } + return cfg.Lock(), cfg.OnBoot(), nil +} + +func (c *staleLockController) BackupRunning(ctx context.Context, vmid int) (bool, error) { + tasks, err := c.px.ListRunningTasks(ctx) + if err != nil { + return false, err + } + id := strconv.Itoa(vmid) + for _, t := range tasks { + // A single-guest vzdump task carries the vmid in its ID field. + if t.Type == "vzdump" && t.ID == id { + return true, nil + } + } + return false, nil +} + +func (c *staleLockController) HasVzdumpSnapshot(ctx context.Context, vmid int) (bool, error) { + snaps, err := c.px.ListSnapshots(ctx, vmid) + if err != nil { + return false, err + } + for _, sn := range snaps { + if sn.Name == vzdumpSnapshotName { + return true, nil + } + } + return false, nil +} + +func (c *staleLockController) Unlock(ctx context.Context, vmid int) error { + _, stderr, err := c.runner.Run(ctx, "pct", "unlock", strconv.Itoa(vmid)) + if err != nil { + return &runnerError{op: "pct unlock", stderr: string(stderr), err: err} + } + return nil +} + +func (c *staleLockController) DeleteVzdumpSnapshot(ctx context.Context, vmid int) error { + upid, err := c.px.DeleteSnapshot(ctx, vmid, vzdumpSnapshotName) + if err != nil { + return err + } + if upid == "" { + return nil // synchronous completion (no task to wait on) + } + _, err = c.px.WaitTask(ctx, upid, proxmox.WaitOptions{}) + return err +} + +func (c *staleLockController) Start(ctx context.Context, vmid int) error { + upid, err := c.px.Start(ctx, vmid) + if err != nil { + return err + } + if upid == "" { + return nil + } + _, err = c.px.WaitTask(ctx, upid, proxmox.WaitOptions{}) + return err +} + +// runnerError wraps a fenced-runner failure with its stderr (the runner returns the two separately). +type runnerError struct { + op string + stderr string + err error +} + +func (e *runnerError) Error() string { + if e.stderr != "" { + return e.op + ": " + e.err.Error() + ": " + e.stderr + } + return e.op + ": " + e.err.Error() +} + +func (e *runnerError) Unwrap() error { return e.err } diff --git a/internal/localapi/stalelock_test.go b/internal/localapi/stalelock_test.go new file mode 100644 index 0000000..bc0fbe5 --- /dev/null +++ b/internal/localapi/stalelock_test.go @@ -0,0 +1,224 @@ +package localapi + +import ( + "context" + "io" + "log/slog" + "testing" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// fakeStaleLock is a scripted StaleLockController: it answers the reads from per-vmid maps and records +// the mutating calls (unlock/delsnapshot/start) so a test can assert the exact recovery sequence. +type fakeStaleLock struct { + guests []proxmox.Guest + lock map[int]string + onboot map[int]bool + snap map[int]bool // a dangling vzdump snapshot exists + running map[int]bool // a vzdump task is genuinely in-flight + + unlocked []int + delsnap []int + started []int + + lockErr map[int]error + runningErr map[int]error +} + +func (f *fakeStaleLock) Guests(context.Context) ([]proxmox.Guest, error) { return f.guests, nil } + +func (f *fakeStaleLock) Lock(_ context.Context, vmid int) (string, bool, error) { + if err := f.lockErr[vmid]; err != nil { + return "", false, err + } + return f.lock[vmid], f.onboot[vmid], nil +} + +func (f *fakeStaleLock) BackupRunning(_ context.Context, vmid int) (bool, error) { + if err := f.runningErr[vmid]; err != nil { + return false, err + } + return f.running[vmid], nil +} + +func (f *fakeStaleLock) HasVzdumpSnapshot(_ context.Context, vmid int) (bool, error) { + return f.snap[vmid], nil +} + +func (f *fakeStaleLock) Unlock(_ context.Context, vmid int) error { + f.unlocked = append(f.unlocked, vmid) + return nil +} + +func (f *fakeStaleLock) DeleteVzdumpSnapshot(_ context.Context, vmid int) error { + f.delsnap = append(f.delsnap, vmid) + return nil +} + +func (f *fakeStaleLock) Start(_ context.Context, vmid int) error { + f.started = append(f.started, vmid) + return nil +} + +func staleLockServer(f *fakeStaleLock) *Server { + return &Server{staleLock: f, logger: slog.New(slog.NewTextHandler(io.Discard, nil))} +} + +func contains(xs []int, v int) bool { + for _, x := range xs { + if x == v { + return true + } + } + return false +} + +// TestStaleLock_RecoversStoppedLockedGuest is the F2-b core: a stopped, onboot guest with a +// snapshot-delete lock + a dangling vzdump snapshot is unlocked, its snapshot deleted, and it is +// started — the exact reboot-during-backup recovery. +func TestStaleLock_RecoversStoppedLockedGuest(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + lock: map[int]string{9201: "snapshot-delete"}, + onboot: map[int]bool{9201: true}, + snap: map[int]bool{9201: true}, + running: map[int]bool{9201: false}, + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if !contains(f.unlocked, 9201) { + t.Fatalf("expected pct unlock for 9201, unlocked=%v", f.unlocked) + } + if !contains(f.delsnap, 9201) { + t.Fatalf("expected dangling vzdump snapshot delete for 9201, delsnap=%v", f.delsnap) + } + if !contains(f.started, 9201) { + t.Fatalf("expected start (onboot, stopped) for 9201, started=%v", f.started) + } +} + +// TestStaleLock_NoLockTouchesNothing is the COMPANION proof: a healthy guest with no lock has NONE of +// the mutating ops invoked — recovery acts only on the stale state. +func TestStaleLock_NoLockTouchesNothing(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9201, Status: "running"}}, + lock: map[int]string{9201: ""}, // unlocked + onboot: map[int]bool{9201: true}, + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if len(f.unlocked)+len(f.delsnap)+len(f.started) != 0 { + t.Fatalf("no-lock guest must be untouched; got unlocked=%v delsnap=%v started=%v", f.unlocked, f.delsnap, f.started) + } +} + +// TestStaleLock_NonBackupLockLeftAlone: a non-vzdump lock (e.g. migrate) is NEVER cleared — scope is +// strictly the two backup locks. +func TestStaleLock_NonBackupLockLeftAlone(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9201, Status: "running"}}, + lock: map[int]string{9201: "migrate"}, + onboot: map[int]bool{9201: true}, + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if len(f.unlocked) != 0 { + t.Fatalf("a migrate lock must not be cleared; unlocked=%v", f.unlocked) + } +} + +// TestStaleLock_OnbootZeroNotStarted: a stale-locked guest with onboot=0 (e.g. the golden) is unlocked +// + snapshot-cleaned but NOT started — a deliberately-stopped guest stays stopped. +func TestStaleLock_OnbootZeroNotStarted(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9100, Status: "stopped"}}, + lock: map[int]string{9100: "snapshot-delete"}, + onboot: map[int]bool{9100: false}, + snap: map[int]bool{9100: true}, + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if !contains(f.unlocked, 9100) || !contains(f.delsnap, 9100) { + t.Fatalf("onboot=0 guest should still be unlocked + snapshot-cleaned; unlocked=%v delsnap=%v", f.unlocked, f.delsnap) + } + if contains(f.started, 9100) { + t.Fatalf("onboot=0 guest must NOT be started; started=%v", f.started) + } +} + +// TestStaleLock_DelsnapOnlyWhenSnapshotExists: a stop/suspend-mode interruption (lock but NO dangling +// snapshot) is unlocked + started, but delsnapshot is NOT called. +func TestStaleLock_DelsnapOnlyWhenSnapshotExists(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + lock: map[int]string{9201: "backup"}, + onboot: map[int]bool{9201: true}, + snap: map[int]bool{9201: false}, // no dangling snapshot + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if !contains(f.unlocked, 9201) || !contains(f.started, 9201) { + t.Fatalf("expected unlock + start; unlocked=%v started=%v", f.unlocked, f.started) + } + if len(f.delsnap) != 0 { + t.Fatalf("delsnapshot must not run without a dangling snapshot; delsnap=%v", f.delsnap) + } +} + +// TestStaleLock_RunningBackupNotCleared is the INVARIANT guard: a backup lock present WITH a genuinely +// in-flight vzdump must NOT be cleared (clearing a live backup's lock corrupts it). +func TestStaleLock_RunningBackupNotCleared(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9201, Status: "running"}}, + lock: map[int]string{9201: "backup"}, + onboot: map[int]bool{9201: true}, + running: map[int]bool{9201: true}, // a real backup is running + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if len(f.unlocked)+len(f.delsnap)+len(f.started) != 0 { + t.Fatalf("a live backup's lock must be left alone; unlocked=%v delsnap=%v started=%v", f.unlocked, f.delsnap, f.started) + } +} + +// TestStaleLock_RunningProbeErrorFailsSafe: if the no-backup-running confirmation errors, the lock is +// LEFT (fail-safe) rather than blind-cleared. +func TestStaleLock_RunningProbeErrorFailsSafe(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}}, + lock: map[int]string{9201: "snapshot-delete"}, + onboot: map[int]bool{9201: true}, + runningErr: map[int]error{9201: context.DeadlineExceeded}, + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if len(f.unlocked) != 0 { + t.Fatalf("an unconfirmable backup state must fail safe (no unlock); unlocked=%v", f.unlocked) + } +} + +// TestStaleLock_AlreadyRunningNotRestarted: a bare agent restart can find the guest UP behind a stale +// lock — it is unlocked but NOT (re)started. +func TestStaleLock_AlreadyRunningNotRestarted(t *testing.T) { + f := &fakeStaleLock{ + guests: []proxmox.Guest{{VMID: 9201, Status: "running"}}, + lock: map[int]string{9201: "snapshot-delete"}, + onboot: map[int]bool{9201: true}, + snap: map[int]bool{9201: true}, + } + staleLockServer(f).RecoverStaleLockedGuests(context.Background()) + + if !contains(f.unlocked, 9201) { + t.Fatalf("a running but stale-locked guest should still be unlocked; unlocked=%v", f.unlocked) + } + if contains(f.started, 9201) { + t.Fatalf("an already-running guest must NOT be started; started=%v", f.started) + } +} + +// TestStaleLock_NilControllerNoop: an unwired controller is a safe no-op (the optional-feature contract). +func TestStaleLock_NilControllerNoop(t *testing.T) { + s := &Server{logger: slog.New(slog.NewTextHandler(io.Discard, nil))} + s.RecoverStaleLockedGuests(context.Background()) // must not panic +} diff --git a/internal/proxmox/query.go b/internal/proxmox/query.go index 62cf187..43bfa72 100644 --- a/internal/proxmox/query.go +++ b/internal/proxmox/query.go @@ -54,6 +54,24 @@ func (c *Client) GuestConfig(ctx context.Context, vmid int) (GuestConfig, error) return cfg, c.get(ctx, path, &cfg) } +// ListSnapshots returns GET /nodes/{node}/lxc/{vmid}/snapshot (the guest's snapshots, including the +// synthetic "current"). The startup stale-lock recovery uses it to find a dangling "vzdump" snapshot +// left by an interrupted snapshot-mode backup. +func (c *Client) ListSnapshots(ctx context.Context, vmid int) ([]Snapshot, error) { + var ss []Snapshot + path := fmt.Sprintf("/nodes/%s/lxc/%d/snapshot", c.node, vmid) + return ss, c.get(ctx, path, &ss) +} + +// ListRunningTasks returns the node's currently-running tasks (GET /nodes/{node}/tasks?running=1). +// The startup stale-lock recovery uses it as the load-bearing safety guard: a backup lock is cleared +// ONLY when no vzdump task is genuinely in-flight for the guest (an agent restart while a real backup +// runs must never clear the live lock). +func (c *Client) ListRunningTasks(ctx context.Context) ([]TaskStatus, error) { + var ts []TaskStatus + return ts, c.get(ctx, "/nodes/"+c.node+"/tasks?running=1", &ts) +} + // ListStorage returns GET /storage (cluster-wide storage definitions). func (c *Client) ListStorage(ctx context.Context) ([]Storage, error) { var ss []Storage diff --git a/internal/proxmox/types.go b/internal/proxmox/types.go index 833b2ea..6914acf 100644 --- a/internal/proxmox/types.go +++ b/internal/proxmox/types.go @@ -118,6 +118,35 @@ func (g *GuestConfig) Nets() map[string]string { return g.prefixed("net") } +// Lock returns the guest's current lock ("backup", "snapshot-delete", "migrate", …) from the config, +// or "" when unlocked. An interrupted vzdump leaves a "backup" or "snapshot-delete" lock — the signal +// the startup stale-lock recovery (F2-b) keys on. +func (g *GuestConfig) Lock() string { + raw, ok := g.Extra["lock"] + if !ok { + return "" + } + var s string + if json.Unmarshal(raw, &s) != nil { + return "" + } + return s +} + +// OnBoot reports whether the guest is configured to auto-start at host boot (onboot:1). PVE omits the +// key when 0, so an absent key reads as false. +func (g *GuestConfig) OnBoot() bool { + raw, ok := g.Extra["onboot"] + if !ok { + return false + } + var n int + if json.Unmarshal(raw, &n) != nil { + return false + } + return n == 1 +} + func (g *GuestConfig) prefixed(prefix string) map[string]string { out := map[string]string{} for k, raw := range g.Extra { @@ -136,6 +165,16 @@ func (g *GuestConfig) prefixed(prefix string) map[string]string { return out } +// Snapshot is one entry of GET /nodes/{node}/lxc/{vmid}/snapshot. The list always includes the +// synthetic "current" pseudo-snapshot; named entries are real snapshots. An interrupted vzdump +// snapshot-mode backup leaves a dangling snapshot named exactly "vzdump". +type Snapshot struct { + Name string `json:"name"` + Description string `json:"description,omitempty"` + SnapTime int64 `json:"snaptime,omitempty"` + Parent string `json:"parent,omitempty"` +} + // Storage is one entry of GET /storage (cluster) and GET /nodes/{node}/storage // (the latter adds usage fields). Unused fields stay zero. //