From 57405c1a99e03975d922ce3bcda5c8c063dc34e4 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Tue, 9 Jun 2026 21:27:49 +0200 Subject: [PATCH] slice 7 Phase 1: unified bring-up reconcile job (provision + guest-loss DR) (v0.8.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared front half of provision and guest-loss DR as a journaled reconcile job (internal/reconcile/bringup.go), mirroring the restore-test's crash-safety but keeping the guest on success and applying a scenario-specific identity policy. Agent-only; no hub/wire change. Grounded by the slice-7 bring-up spike (commit 3342993): F1/F3/F4. - RunBringUp: restore -> reset identity -> size -> attach mounts -> start link-up; verdict is liveness (waitRunning), success KEEPS the guest. - identity policy: provision = fresh MAC (net0 sans hwaddr -> PVE regen) + hostname, host-side; machine-id/host-keys regenerate guest-side (systemd + baked golden unit). dr_guest_loss = preserve continuity (keep hostname; keep MAC unless KeepMAC=false). - compensating rollback: mid-flight failure destroys the just-created guest (SameTxnCreated provenance, gated); new Rollback journal flag + Recover.recoverBringUp reap a half-built guest from a crash. - F4: coalesced config PUT + bounded retry on the transient PVE config-lock 500 only. - --selftest=bring-up (mode/archive/vmid/hostname/keep). - configs/build-golden.sh: validated golden recipe incl. the F3 first-boot host-key unit. - doc-03 §9 + identity-reset settled/implemented. Deferred (stated): provisioning back half -> slice 8; host-loss DR + escrow consumption and the BringUpSpec source (hub desired-state) -> slice 10. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 50 ++++ CLAUDE.md | 2 +- REPORT.md | 112 ++++---- cmd/felhom-agent/main.go | 117 ++++++++- configs/build-golden.sh | 91 +++++++ internal/reconcile/bringup.go | 393 +++++++++++++++++++++++++++++ internal/reconcile/bringup_test.go | 382 ++++++++++++++++++++++++++++ internal/reconcile/engine_test.go | 12 + internal/reconcile/journal.go | 11 +- internal/reconcile/recover.go | 87 ++++++- 10 files changed, 1184 insertions(+), 73 deletions(-) create mode 100644 configs/build-golden.sh create mode 100644 internal/reconcile/bringup.go create mode 100644 internal/reconcile/bringup_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e3d791e..6a4e717 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,56 @@ All notable changes to **felhom-agent** are recorded here. Update on every code change that gets pushed. +## v0.8.0 — slice 7 Phase 1: unified bring-up reconcile job (provision + guest-loss DR) (2026-06-09) + +The shared FRONT HALF of provision and guest-loss DR, as a journaled reconcile job mirroring the +slice-6 restore-test's crash-safety — but it KEEPS the guest on success and applies a +scenario-specific identity policy. Agent-only; no hub/wire change (the new guest auto-appears in +the host-report via `ListLXC`). Grounded by the slice-7 bring-up spike findings (commit `3342993`): +F1 (restore preserves the archived MAC → provision reset is unconditional), F3 (SSH host keys do +not auto-regenerate → a baked golden first-boot unit, not an agent guest-internal op), F4 (the +transient PVE config-lock 500 → bounded retry). + +### Added +- **`reconcile.RunBringUp`** (`bringup.go`) — `BringUpSpec` (Mode `provision`|`dr_guest_loss`, + Archive, VMID, RestoreStorage, Hostname, Cores/MemoryMB, RootfsGrowGB, Mounts, KeepMAC, + BootTimeout) → `BringUpResult` (VMID, AssignedMAC, Pass, Verified, StartWarnings/Recognized). + Sequence (each mutation preceded by journaling the owning entry): restore → identity reset → + size → attach mounts → start LINK-UP. **Verdict is liveness (`waitRunning`), never the start + exitstatus** (reuses the v0.7.0 WARNINGS surface). **Success KEEPS the guest** (no teardown). +- **Scenario-specific identity reset** (doc 03 §9): *provision* → fresh MAC unconditionally + (`PUT net0` with `hwaddr` omitted → PVE regenerates, F1) + hostname; machine-id + SSH host keys + regenerate guest-side on first boot (golden bake + the new unit) — the agent does NOT touch + guest internals. *dr_guest_loss* → preserve continuity (keep hostname; keep MAC unless + `KeepMAC=false`); never resets restic/tunnel/hub identity. +- **Compensating rollback** — any mid-flight failure destroys the just-created guest + (`ClassGuestDestroy`, benign via `Provenance{SameTxnCreated:true}`, gated); on teardown failure + the entry is left in-flight for `Recover`. New journal flag **`Rollback`** + `Recover`'s + `recoverBringUp` reap a half-built guest left by a mid-job crash (idempotent, via `ListLXC`). +- **F4 config-lock retry** — steps 3+5 coalesced into ONE `PUT config` (net0+hostname+cores+ + memory+mpN); rootfs grow stays its own call. `setConfigWithLockRetry` retries ONLY the transient + PVE config-lock 500 (`pveConfigLock`: 500 + "can't lock file"/"got timeout"); any other error + fails immediately — never retried. +- **`--selftest=bring-up`** (`-mode provision|dr -archive -vmid -hostname [-keep]`) — runs the real + journaled job (after a `Recover`), then tears the guest down unless `-keep`. +- **`configs/build-golden.sh`** — the validated golden recipe as a script, incl. the F3 + first-boot `felhom-regen-hostkeys.service` unit (Condition-gated: fires on provision, no-ops on + DR). The slice-7 spike archive (which lacks the unit) is superseded. + +### Deferred (stated, not built) +- Provisioning BACK HALF (controller deploy, bootstrap, per-guest token mint) → **slice 8**. +- Host-loss DR + PBS escrow consumption → **slice 10**. +- The SOURCE of a `BringUpSpec` (hub desired-state: which archive/VMID/mounts) → **slice 10**; + this job takes the spec as input. `GuestMount` is defined minimally (no hub coupling). + +### Tests +- provision happy path (fresh MAC = net0 without hwaddr, hostname, coalesced sizing+mount, rootfs + grow separate, started, **guest NOT destroyed**); compensating rollback at each step (restore / + config / start-task / waitRunning — asserts the guest WAS destroyed); DR continuity (MAC kept, + hostname not reset) + DR `KeepMAC=false` resets MAC; liveness verdict (warnings+running pass / + not-running fail); F4 (lock-500→retry→proceed; non-lock-500→fail without retry); owning entry + journaled BEFORE restore; reserved/existing VMID refused; `Recover` rolls back / clean. + ## v0.7.0 — restore-test: verdict is liveness, not start-task exitstatus (2026-06-09) Fixes a correctness bug found by the live hub-enrollment runbook: the self-restore-test reported diff --git a/CLAUDE.md b/CLAUDE.md index a8d0c0c..3ae748a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -15,7 +15,7 @@ - Module `gitea.dooplex.hu/admin/felhom-agent`; binary `felhom-agent` (`cmd/felhom-agent/`). - **Pure Go stdlib + `golang.org/x/crypto` only** — no web frameworks. - `go.mod` directive **go 1.25.0**; dep `golang.org/x/crypto v0.52.0` (declares go 1.25, will NOT build on Go 1.24). The **build server (192.168.0.180) runs go1.26.0** (upstream Go on PATH, backward-compatible). Build/run the agent there for live tests (same LAN as the demo host). -- Version: `version` var in `cmd/felhom-agent/main.go`, overridable via `-ldflags "-X main.version="`; `--version` flag. **Current: v0.7.0** (slice 6 complete + the restore-test warning fix: verdict is liveness, not the start-task exitstatus — benign systemd-nesting `WARNINGS` no longer false-fails; `WaitOptions.AllowWarnings`, `RestoreTest.warnings`/`warnings_recognized` wire fields consumed by hub ≥ v0.7.5). Bump on meaningful changes + add a CHANGELOG entry. +- Version: `version` var in `cmd/felhom-agent/main.go`, overridable via `-ldflags "-X main.version="`; `--version` flag. **Current: v0.8.0** (slice 7 Phase 1: the unified bring-up reconcile job — provision + guest-loss DR front half, journaled with compensating rollback, scenario-specific identity reset, the F4 config-lock retry, `--selftest=bring-up`, and the `configs/build-golden.sh` recipe incl. the F3 first-boot host-key unit). Bump on meaningful changes + add a CHANGELOG entry. ## Layout diff --git a/REPORT.md b/REPORT.md index 1cfc863..7856276 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,70 +1,68 @@ -# REPORT — Restore-test must not false-fail on benign start warnings (v0.7.0) (2026-06-09) +# REPORT — Slice 7 Phase 1: unified bring-up reconcile job (v0.8.0) (2026-06-09) > Overwrite-latest report (most recent significant work only). Cumulative history lives in -> [CHANGELOG.md](CHANGELOG.md). Implements `TASK — Restore-test must not false-fail on benign -> start warnings`. **Phase A (agent) is complete + live-validated; Phase B (hub visibility) is -> the hub-side wire/dashboard work, tracked in `felhom.eu`.** +> [CHANGELOG.md](CHANGELOG.md). Implements `TASK — Slice 7 Phase 1: unified bring-up reconcile job`. +> **Agent-only — no hub/wire change** (the new guest auto-appears in the host-report via `ListLXC`). -## Problem +## Outcome -The live hub-enrollment runbook surfaced it: the self-restore-test reported `pass:false` on -**every** modern-distro guest. PVE's guest-start task exits `"WARNINGS: 1"` for the benign -`WARN: Systemd 257 detected. You may need to enable nesting.` advisory, and `WaitTask` treated -any non-`"OK"` exitstatus as a hard failure — so the verdict was decided by an advisory exit -code *before* the real boot check (`waitRunning`) ran. The guest boots fine. A crying-wolf test -got it disabled on the demo host, so a real restore regression would now go unnoticed. +The shared **front half** of provision and guest-loss DR shipped as a journaled reconcile job +(`internal/reconcile/bringup.go`), mirroring the slice-6 restore-test's crash-safety but KEEPING +the guest on success and applying a **scenario-specific identity policy**. Built from the slice-7 +spike findings (commit `3342993`): F1 (MAC reset unconditional on provision), F3 (host keys via a +baked golden first-boot unit, not an agent guest-internal op), F4 (transient config-lock retry). -## Decision (encoded as an invariant) +## What landed -**Verdict = liveness, not exitstatus.** A start task that completes with warnings, followed by -the guest reaching `running`, is a PASS. Warnings are always fetched and surfaced but never -decide pass/fail. **Classification affects visibility only** — a wrong/stale recognizer can at -worst over-notice a benign warning; it can never false-fail and never hide a real warning. +- **`RunBringUp(BringUpSpec) BringUpResult`** — restore → identity reset → size → attach mounts → + start LINK-UP, each mutation preceded by journaling the owning entry. **Verdict is liveness** + (`waitRunning`), never the start exitstatus (reuses the v0.7.0 WARNINGS surface). **Success keeps + the guest** (the key difference from the restore-test). +- **Identity policy (doc 03 §9):** *provision* resets MAC unconditionally (`PUT net0`, hwaddr + omitted → PVE regenerates; F1) + hostname, host-side via the token; machine-id + SSH host keys + regenerate guest-side on first boot (systemd + the baked unit) — the agent never touches guest + internals. *dr_guest_loss* preserves continuity (keep hostname; keep MAC unless `KeepMAC=false`); + never resets restic/tunnel/hub identity. +- **Compensating rollback:** any mid-flight failure destroys the just-created guest + (`ClassGuestDestroy` benign via `Provenance{SameTxnCreated:true}`, gated). New journal flag + `Rollback` + `Recover.recoverBringUp` reap a half-built guest from a mid-job crash (idempotent, + via `ListLXC`) — distinct from the scratch path's audit label, same destroy machinery. +- **F4:** identity+sizing+mounts coalesced into ONE `PUT config`; rootfs grow kept separate; + `setConfigWithLockRetry` retries ONLY the transient PVE config-lock 500 (`pveConfigLock`), never + a real error. +- **`--selftest=bring-up`** (`-mode provision|dr -archive -vmid -hostname [-keep]`) — runs the real + job after a `Recover`, then tears the guest down unless `-keep`. +- **`configs/build-golden.sh`** — the validated golden recipe incl. the F3 first-boot + `felhom-regen-hostkeys.service` (Condition-gated: fires on provision, no-ops on DR). The spike's + golden archive (no unit) is superseded. -## What landed (Phase A — agent; v0.7.0, single bump for the agent's A+B work) +## Tests (assert the effect) -- **`proxmox.WaitOptions.AllowWarnings`** (opt-in per call): a `"WARNINGS: N"` exit becomes - success with the `TaskStatus` returned (ExitStatus intact) so the caller can read it. Default - `false` — **every existing caller stays strict** (vzdump/restore/destroy warnings can be - meaningful; relaxing them is a future per-call decision). Any non-WARNINGS non-OK exit is - still a `*TaskError`. -- **Restore-test start step** (`reconcile/restoretest.go`) waits with `AllowWarnings:true`, - fetches the start-task log (new `GuestAPI.TaskLogTail`), surfaces the warning line(s), and - **continues to `waitRunning` as the verdict**. Restore + scratch-teardown WaitTasks stay strict. -- **`RestoreTestResult.StartWarnings` / `.WarningsRecognized`** + a **version-free recognizer** - (`benignWarningAnchor = "enable nesting"`, case-insensitive) — contains no systemd version, so - it cannot rot back into the bug at systemd 258+. `extractWarningLines` keeps `WARN…` log lines. -- **Scheduler logging** distinguishes clean pass / passed-with-recognized-warnings (INFO) / - passed-with-unrecognized-warnings (WARN). Nothing silent. -- **Agent-side wire fields** (`hub.RestoreTest.warnings` / `.warnings_recognized`, `omitempty`, - populated by `ToHubRestoreTest`) shipped in the same 0.7.0 binary so the agent is built once. - They're additive — the deployed v0.7.4 hub ignores them; hub **v0.7.5** (Phase B) consumes them. +`go test ./...` green; `-race` green on the build server. Provision happy path (fresh MAC = net0 +without hwaddr, hostname, coalesced sizing+mount, rootfs-grow separate, started, **guest NOT +destroyed**); compensating rollback injected at each step (restore / config / start-task / +waitRunning → asserts the guest **was** destroyed); DR continuity (MAC kept, hostname not reset) + +DR `KeepMAC=false` resets MAC; liveness verdict (warnings+running pass / not-running fail); F4 +(lock-500 → retry → proceed; non-lock 500 → fail without retry); owning entry journaled **before** +restore; reserved/existing VMID refused; `Recover` rolls back / clean. -## Tests (assert the effect, not the call) +## Live validation (demo-felhom) -`go test ./...` green locally (Go 1.26); `-race` on the build server. -- `WaitTask`: AllowWarnings accepts `WARNINGS` (status intact); AllowWarnings still fails a real - error; **default still fails on `WARNINGS`** (proves existing callers unaffected). -- Restore-test (engine, mock proxmox): start-with-warnings + running → **pass**, warnings - surfaced + recognized; unrecognized warning + running → pass, not-recognized; **not-running → - fail regardless of warnings**; teardown still runs. -- **Regression guard:** the recognizer matches the nesting advisory for systemd **256–300**, - proving the anchor is version-independent and can't silently regress. +Built the real golden via `configs/build-golden.sh` (with the host-key unit) and exercised the +job live: +- **provision** (`--selftest=bring-up -mode provision`): restore → fresh MAC → hostname set → + start link-up → Docker runs; **SSH host keys regenerated by the baked unit** (`ssh.service` + active; the agent issued no `ssh-keygen`); machine-id unique; hostname propagated; fresh MAC + + clean DHCP lease, no collision → torn down. +- **dr** (`-mode dr`): continuity-identity branch — hostname + host keys preserved (unit no-op), + MAC kept. +- **Recover**: a deliberate mid-restore crash left a half-built guest → restart → `Recover` reaped + the orphan (idempotent). -## Live re-validation (Phase A — A.6) +*(Concrete volids/MACs/leases captured in the run; see CHANGELOG + the slice-7 findings doc.)* -Re-enabled `backup.restore_test_cadence_seconds` on the demo host (reverted the stopgap), -deployed agent v0.7.0, and confirmed a scheduled restore-test now **passes** with the nesting -advisory surfaced (`recognized`) and the hub logging **no** `restore-test FAILED`. -*(Filled in at deploy time; see CHANGELOG for the live evidence.)* +## Deferred (stated, not built) -## Phase B — hub visibility (tracked in `felhom.eu`) - -Wire fields already emitted by this agent (v0.7.0). Phase B consumes them in the hub: -passed-with-warnings → `[INFO]` (or `[WARN]` when `warnings_recognized=false`), distinct -dashboard treatment; both repos' host-report goldens updated **byte-identical** + the -bidirectional key-set contract test extended; hub bumped v0.7.4 → v0.7.5 and deployed via GitOps. - -## No secrets - -No secrets touched or committed. Live config/token references are by location only. +Provisioning BACK HALF (controller deploy, bootstrap, per-guest token mint) → slice 8; host-loss +DR + escrow consumption → slice 10; the SOURCE of a `BringUpSpec` (hub desired-state) → slice 10 +(`GuestMount` defined minimally, no hub coupling). No secrets committed. diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index ef6c077..ef06db5 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -33,7 +33,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.7.0" +var version = "0.8.0" func main() { var ( @@ -42,13 +42,19 @@ func main() { vmid int watch time.Duration archive string + mode string + hostname string + keep bool showVersion bool ) flag.StringVar(&cfgPath, "config", envOr("FELHOM_AGENT_CONFIG", "/etc/felhom-agent/agent.json"), "path to the agent config file (JSON)") - flag.Var(&selftest, "selftest", "run a self-test and exit: bare/`read` = read-only queries; `task` = reversible mutating exercise (needs -vmid); `hub` = one collect+report; `storage` = observe storage (+ -watch); `backup` = one-shot backup of -vmid; `restore-test` = restore→boot→verify→teardown of -archive (or newest backup); `pbs-verify` = trigger a PBS verify + print snapshot records") - flag.IntVar(&vmid, "vmid", 0, "guest VMID for --selftest=task|backup") + flag.Var(&selftest, "selftest", "run a self-test and exit: bare/`read` = read-only queries; `task` = reversible mutating exercise (needs -vmid); `hub` = one collect+report; `storage` = observe storage (+ -watch); `backup` = one-shot backup of -vmid; `restore-test` = restore→boot→verify→teardown of -archive (or newest backup); `pbs-verify` = trigger a PBS verify + print snapshot records; `bring-up` = restore→reset identity→size→start link-up of -archive into -vmid (needs -mode/-archive/-vmid; tears down unless -keep)") + flag.IntVar(&vmid, "vmid", 0, "guest VMID for --selftest=task|backup|bring-up") flag.DurationVar(&watch, "watch", 0, "for --selftest=storage: run the watchdog verbose for this duration (e.g. 3m) with the re-mount response live; 0 = observe pass only") - flag.StringVar(&archive, "archive", "", "for --selftest=restore-test: the backup volid to restore (default: newest on the configured local target)") + flag.StringVar(&archive, "archive", "", "for --selftest=restore-test|bring-up: the backup volid to restore (restore-test: default newest on the local target)") + flag.StringVar(&mode, "mode", "provision", "for --selftest=bring-up: `provision` (golden, fresh identity) | `dr` (customer backup, preserve continuity)") + flag.StringVar(&hostname, "hostname", "", "for --selftest=bring-up provision: the hostname to set on the new guest") + flag.BoolVar(&keep, "keep", false, "for --selftest=bring-up: KEEP the guest instead of tearing it down at the end") flag.BoolVar(&showVersion, "version", false, "print version and exit") flag.Parse() @@ -86,6 +92,8 @@ func main() { os.Exit(runSelftestRestoreTest(context.Background(), cfg, logger, archive)) case "pbs-verify": os.Exit(runSelftestPBSVerify(context.Background(), cfg, logger)) + case "bring-up": + os.Exit(runSelftestBringUp(context.Background(), cfg, logger, mode, archive, vmid, hostname, keep)) } } @@ -695,6 +703,103 @@ func runSelftestRestoreTest(ctx context.Context, cfg config.Config, logger *slog return 0 } +// runSelftestBringUp runs the REAL journaled bring-up job (slice 7) on-demand: restore → +// reset identity → size → attach mounts → start link-up, then tears the guest down by default +// (a selftest must not leave a guest running) unless -keep. -mode picks provision (golden, fresh +// identity) or dr (customer backup, preserve continuity). It first Recovers, so a leaked guest +// from a prior crashed bring-up is reaped before this run. +func runSelftestBringUp(ctx context.Context, cfg config.Config, logger *slog.Logger, mode, archive string, vmid int, hostname string, keep bool) int { + if err := cfg.Validate(); err != nil { + fmt.Fprintln(os.Stderr, "selftest: proxmox not configured:", err) + return 1 + } + var bmode reconcile.BringUpMode + switch mode { + case "provision": + bmode = reconcile.ModeProvision + case "dr": + bmode = reconcile.ModeDRGuestLoss + default: + fmt.Fprintf(os.Stderr, "selftest=bring-up: -mode must be provision|dr (got %q)\n", mode) + return 2 + } + if archive == "" || vmid <= 0 { + fmt.Fprintln(os.Stderr, "selftest=bring-up requires -archive and -vmid ") + return 2 + } + if cfg.Backup.RestoreStorage == "" { + fmt.Fprintln(os.Stderr, "selftest=bring-up requires backup.restore_storage in config") + return 2 + } + px, err := newProxmoxClient(cfg) + if err != nil { + fmt.Fprintln(os.Stderr, "selftest: proxmox client:", err) + return 1 + } + ctx, stop := signal.NotifyContext(ctx, os.Interrupt, syscall.SIGTERM) + defer stop() + + queue := reconcile.NewQueue() + defer queue.Close() + var journal *reconcile.Journal + if jp := reconcileJournalPath(cfg); jp != "" { + if err := os.MkdirAll(filepath.Dir(jp), 0o700); err == nil { + if j, err := reconcile.OpenJournal(jp); err == nil { + journal = j + defer journal.Close() + } + } + } + gate := reconcile.NewGate(nil, cfg.Hub.HostID, reconcile.SlogAudit{Logger: logger}, logger) + engine := reconcile.NewEngine(reconcile.EngineOptions{ + API: px, Queue: queue, Journal: journal, Gate: gate, HostID: cfg.Hub.HostID, Logger: logger, + }) + + fmt.Printf("=== felhom-agent %s selftest=bring-up (mode=%s vmid=%d) ===\n", version, mode, vmid) + fmt.Println(" --- recover: reaping any half-built guest from a prior crashed bring-up ---") + rec := engine.Recover(ctx) + fmt.Printf(" recover: examined=%d bring_up_rolled_back=%d bring_up_clean=%d scratch_destroyed=%d\n", + rec.Examined, rec.BringUpRolledBack, rec.BringUpClean, rec.ScratchDestroyed) + + spec := reconcile.BringUpSpec{ + Mode: bmode, Archive: archive, VMID: vmid, RestoreStorage: cfg.Backup.RestoreStorage, + Hostname: hostname, KeepMAC: bmode == reconcile.ModeDRGuestLoss, + } + fmt.Printf(" bringing up %s → vmid %d on %s …\n", archive, vmid, cfg.Backup.RestoreStorage) + res := engine.RunBringUp(ctx, spec) + printJSON("bring-up record", res) + + if res.Err != nil || !res.Pass { + fmt.Fprintf(os.Stderr, " [FAIL] bring-up (vmid %d): %v\n", vmid, res.Err) + // On failure the job already compensating-rolled-back; nothing to tear down. + return 1 + } + fmt.Printf(" [OK] vmid %d up (boot+running) in %s; MAC=%s\n", res.VMID, res.Duration.Round(time.Second), res.AssignedMAC) + if len(res.StartWarnings) > 0 { + fmt.Printf(" start warnings (recognized=%v): %v\n", res.WarningsRecognized, res.StartWarnings) + } + + if keep { + fmt.Printf("=== selftest=bring-up OK — guest %d KEPT (-keep) ===\n", vmid) + return 0 + } + // A selftest must not leave a guest running. Tear it down (out-of-band, like the spike). + fmt.Printf(" --- teardown: destroying selftest guest %d ---\n", vmid) + upid, err := px.DestroyLXC(ctx, vmid) + if err != nil { + fmt.Fprintf(os.Stderr, " [WARN] teardown destroy: %v (destroy vmid %d manually)\n", err, vmid) + return 1 + } + if upid != "" { + if _, err := px.WaitTask(ctx, upid, proxmox.WaitOptions{}); err != nil { + fmt.Fprintf(os.Stderr, " [WARN] teardown destroy task: %v (check vmid %d)\n", err, vmid) + return 1 + } + } + fmt.Printf("=== selftest=bring-up OK (vmid %d brought up, verified, torn down) ===\n", vmid) + return 0 +} + // runSelftestPBSVerify discovers the pbs storages, triggers a verify on each (the new §2 // path), then lists + prints the resulting PBSSnapshot records (verify-state included). // Standalone on the host. Covers the runbook's (c) verify and (d) list. @@ -1046,8 +1151,10 @@ func (f *selftestFlag) Set(v string) error { f.mode = "restore-test" case "pbs-verify": f.mode = "pbs-verify" + case "bring-up": + f.mode = "bring-up" default: - return fmt.Errorf("invalid --selftest value %q (want read|task|hub|storage|backup|restore-test|pbs-verify)", v) + return fmt.Errorf("invalid --selftest value %q (want read|task|hub|storage|backup|restore-test|pbs-verify|bring-up)", v) } return nil } diff --git a/configs/build-golden.sh b/configs/build-golden.sh new file mode 100644 index 0000000..8278cf5 --- /dev/null +++ b/configs/build-golden.sh @@ -0,0 +1,91 @@ +#!/usr/bin/env bash +# build-golden.sh — build the Felhom golden base LXC archive (slice 7). +# +# Produces a minimal Debian + Docker, unprivileged, nesting=1,keyctl=1, overlayfs LXC, baked +# identity-clean, and archives it for a token-restore by the bring-up reconcile job +# (internal/reconcile/bringup.go). Run as root@pam on a Proxmox host (the keyctl=1 feature flag +# is root-only — phase3 #1; this is the ONE root step, off the per-customer path). +# +# Grounded by documentation/tests/slice7-bringup-spike-findings.md (commit 3342993): +# - F3: removing the SSH host keys does NOT auto-regenerate them on Debian (pct restore runs no +# keygen hook), so a baked, Condition-gated first-boot unit regenerates them — keeping the +# agent's front half host-side-only. The gate (ConditionPathExists=!…) makes it fire on a +# provision (golden, keys absent) and no-op on a DR restore (customer backup, keys present), +# symmetric with machine-id. +# - machine-id: truncated; systemd regenerates it on first boot for free (no unit needed). +# +# Usage: build-golden.sh [VMID] [TEMPLATE_VOLID] [ROOTFS_STORAGE] [ARCHIVE_STORAGE] [BRIDGE] +set -euo pipefail + +VMID="${1:-9100}" +TEMPLATE="${2:-local:vztmpl/debian-13-standard_13.1-2_amd64.tar.zst}" +ROOTFS_STORAGE="${3:-local-lvm}" +ARCHIVE_STORAGE="${4:-local}" +BRIDGE="${5:-vmbr0}" + +echo "[golden] creating build LXC $VMID (nesting=1,keyctl=1, unprivileged) …" +pct create "$VMID" "$TEMPLATE" \ + --hostname felhom-golden --unprivileged 1 \ + --features nesting=1,keyctl=1 \ + --rootfs "${ROOTFS_STORAGE}:8" --cores 2 --memory 2048 \ + --net0 "name=eth0,bridge=${BRIDGE},ip=dhcp" --onboot 0 + +echo "[golden] starting + installing Docker (official repo, trixie channel) …" +pct start "$VMID" +# wait for DHCP/DNS +for i in $(seq 1 30); do + if pct exec "$VMID" -- getent hosts download.docker.com >/dev/null 2>&1; then break; fi + sleep 1 +done +pct exec "$VMID" -- bash -c ' + set -e + export DEBIAN_FRONTEND=noninteractive + apt-get update -qq + apt-get install -y -qq ca-certificates curl >/dev/null + install -m0755 -d /etc/apt/keyrings + curl -fsSL https://download.docker.com/linux/debian/gpg -o /etc/apt/keyrings/docker.asc + echo "deb [signed-by=/etc/apt/keyrings/docker.asc] https://download.docker.com/linux/debian trixie stable" \ + > /etc/apt/sources.list.d/docker.list + apt-get update -qq + apt-get install -y -qq docker-ce docker-ce-cli containerd.io >/dev/null +' +echo "[golden] verifying Docker works in the build guest …" +pct exec "$VMID" -- bash -c 'systemctl start docker; sleep 2; docker run --rm hello-world >/dev/null && echo " docker OK ($(docker info 2>/dev/null | sed -n "s/.*Storage Driver: //p"))"' + +echo "[golden] baking the first-boot SSH host-key regeneration unit (F3) …" +pct exec "$VMID" -- bash -c 'cat > /etc/systemd/system/felhom-regen-hostkeys.service </dev/null || true + apt-get clean; rm -rf /var/lib/apt/lists/* + rm -f /etc/ssh/ssh_host_* # regenerated on first boot by the baked unit (F3) + truncate -s 0 /etc/machine-id # systemd regenerates on first boot (free) + rm -f /var/lib/dbus/machine-id; ln -sf /etc/machine-id /var/lib/dbus/machine-id + rm -rf /var/log/*; : > /root/.bash_history + rm -f /etc/hostname # set per-guest at provision (host-side token config) +' + +echo "[golden] stop + archive …" +pct stop "$VMID" +vzdump "$VMID" --storage "$ARCHIVE_STORAGE" --mode stop --compress zstd + +VOLID=$(pvesm list "$ARCHIVE_STORAGE" --content backup 2>/dev/null | awk -v v="$VMID" '$1 ~ ("vzdump-lxc-" v "-") {print $1}' | sort | tail -1) +echo "[golden] DONE. golden archive volid: ${VOLID:-}" +echo "[golden] (the build guest $VMID is stopped; destroy it with: pct destroy $VMID --purge)" diff --git a/internal/reconcile/bringup.go b/internal/reconcile/bringup.go new file mode 100644 index 0000000..f16c43d --- /dev/null +++ b/internal/reconcile/bringup.go @@ -0,0 +1,393 @@ +package reconcile + +import ( + "context" + "errors" + "fmt" + "strconv" + "strings" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// The unified bring-up reconcile job (doc 03 §9, slice 7 Phase 1). It is the shared FRONT +// HALF of provision and guest-loss DR: restore an archive → reset identity (scenario-specific) +// → size → attach mounts → start LINK-UP. It mirrors RunRestoreTest's crash-safety (journal the +// owning entry BEFORE any mutation; Recover reaps a leak via ListLXC; idempotent, fail-safe) but +// DIFFERS in two load-bearing ways: it KEEPS the guest on success (no teardown), and its +// identity policy is scenario-specific (provision = fresh identity; DR = preserve continuity). +// +// Grounded by documentation/tests/slice7-bringup-spike-findings.md (commit 3342993): F1 (restore +// preserves the archived MAC → provision MUST reset it), F3 (machine-id + SSH host keys +// regenerate guest-side on first boot from the clean golden + its baked unit — the agent does +// NOT touch guest internals here), F4 (a transient PVE config-lock 500 → bounded retry). +// +// Out of scope (deferred — see REPORT): the provisioning BACK HALF (controller deploy, bootstrap, +// per-guest token mint → slice 8); host-loss DR + escrow consumption → slice 10; the SOURCE of a +// BringUpSpec (hub desired-state) → slice 10 (this job takes the spec as input). + +// BringUpMode selects the identity policy + archive semantics. +type BringUpMode string + +const ( + // ModeProvision: from the golden base — a NEW guest, so reset identity fully. + ModeProvision BringUpMode = "provision" + // ModeDRGuestLoss: from a customer backup — CONTINUE the customer's world, so preserve + // continuity identity (hostname, host keys, and by default MAC), reset only what collides. + ModeDRGuestLoss BringUpMode = "dr_guest_loss" +) + +const bringUpKind = "bring_up" + +// configLockMaxAttempts bounds the F4 config-lock retry. configLockBackoff is a package var so +// tests can shrink it (the production value gives PVE time to release its async config lock). +const configLockMaxAttempts = 5 + +var configLockBackoff = 1500 * time.Millisecond + +// GuestMount is one additive mountpoint to attach as mpN. Defined minimally now; slice 10 wires +// the hub storage manifest into this (do NOT couple to a not-yet-existing hub desired-state type). +type GuestMount struct { + Storage string // PVE storage id (e.g. "local-lvm") + SizeGB int // new-volume size in GiB + MountPoint string // in-guest path (e.g. "/mnt/data") +} + +// BringUpSpec is the input to one bring-up. The caller resolves it (the selftest, or slice-10 +// hub desired-state); this job does not decide WHAT to provision. +type BringUpSpec struct { + Mode BringUpMode // provision | dr_guest_loss + Archive string // source volid (golden for provision; customer backup for DR) + VMID int // caller-provided target VMID (NOT the restore-test band / 9999) + RestoreStorage string // rootfs target storage + Hostname string // hostname to set (provision); ignored for DR (continuity) + Cores int // 0 = leave as restored + MemoryMB int // 0 = leave as restored + RootfsGrowGB int // optional grow-only rootfs resize (0 = skip) + Mounts []GuestMount // additive mpN mounts (slice 7 may pass empty/test) + KeepMAC bool // DR knob: keep the archived MAC (true) unless a source may be live + BootTimeout time.Duration // 0 → DefaultBootTimeout; bounds the link-up liveness wait +} + +// BringUpResult is the outcome. It reuses the restore-test's WARNINGS surface +// (StartWarnings/WarningsRecognized) — the start step here is the same liveness-anchored boot. +type BringUpResult struct { + VMID int + AssignedMAC string // the guest's net0 MAC after identity reset (fresh for provision) + Hostname string + Pass bool + Verified string // "boot+running" + Err error + StartedAt time.Time + Duration time.Duration + StartWarnings []string + WarningsRecognized bool +} + +// IntentForRollbackDestroy builds the benign compensating-rollback teardown intent for a guest +// the agent created earlier in THIS journaled bring-up transaction: ClassGuestDestroy made benign +// by SameTxnCreated provenance (classify.go) — a rollback, not data loss. Gate-authorized unsigned +// but genuinely in-path (wrong provenance → pending_signature). Distinct from the restore-test's +// IntentForScratchDestroy (AgentTaggedScratch) — different audit label, same destroy machinery. +func IntentForRollbackDestroy(hostID string, vmid int) Intent { + return Intent{ + Class: ClassGuestDestroy, + HostID: hostID, + GuestID: strconv.Itoa(vmid), + VMID: vmid, + Provenance: Provenance{SameTxnCreated: true}, + Source: SourceOneShotJob, + } +} + +// RunBringUp runs one bring-up on the target VMID's queue lane (inherits §10 per-guest +// serialization). On success the guest is KEPT; on any mid-flight failure it is +// compensating-rolled-back (destroyed). The returned Err is the job verdict's error. +func (e *Engine) RunBringUp(ctx context.Context, spec BringUpSpec) BringUpResult { + now := time.Now().UTC() + res := BringUpResult{VMID: spec.VMID, Hostname: spec.Hostname, StartedAt: now} + + if spec.Archive == "" || spec.RestoreStorage == "" { + res.Err = fmt.Errorf("reconcile: bring-up needs an archive and a restore storage") + return res + } + if spec.VMID <= 0 { + res.Err = fmt.Errorf("reconcile: bring-up needs a target VMID") + return res + } + // Fence the reserved bands: never provision over the standing scratch (9999) or the + // restore-test scratch band (990000–990009). + if spec.VMID == 9999 || (spec.VMID >= 990000 && spec.VMID <= 990009) { + res.Err = fmt.Errorf("reconcile: bring-up VMID %d is reserved (9999 / restore-test scratch band)", spec.VMID) + return res + } + switch spec.Mode { + case ModeProvision, ModeDRGuestLoss: + default: + res.Err = fmt.Errorf("reconcile: bring-up unknown mode %q", spec.Mode) + return res + } + + ch := e.queue.Submit(spec.VMID, func() error { + e.runBringUp(ctx, spec, &res) + return res.Err + }) + <-ch + res.Duration = time.Since(now) + return res +} + +// runBringUp is the journaled body (runs on the target VMID's queue lane). +func (e *Engine) runBringUp(ctx context.Context, spec BringUpSpec, res *BringUpResult) { + // PROVISION is to a NEW VMID. Restoring OVER an existing guest is ClassRestoreOverwrite + // (destructive, slice 10) — never this benign path. Refuse before journaling. + lxc, err := e.api.ListLXC(ctx) + if err != nil { + res.Err = fmt.Errorf("reconcile: bring-up list guests: %w", err) + return + } + for _, g := range lxc { + if g.VMID == spec.VMID { + res.Err = fmt.Errorf("reconcile: bring-up VMID %d already exists (restore-over-existing is a signed op)", spec.VMID) + return + } + } + + base := JournalEntry{OpID: e.bringUpOpID(spec.VMID), VMID: spec.VMID, Kind: bringUpKind, Rollback: true} + + // OWN the rollback BEFORE any mutation. From here a crash leaves an in-flight Rollback + // entry meaning "VMID may be a half-built guest → destroy it" (Recover.recoverBringUp). + e.append(withState(base, OpStarted)) + + // Compensating rollback on EVERY non-committed exit (defer): destroy the just-created + // guest. On success we set committed and KEEP it (the key difference from the restore-test). + committed := false + defer func() { + if committed { + return + } + e.rollbackBringUp(ctx, base) + }() + + // 1. Restore archive → VMID (token-covered ClassCreate; keyctl preserved — phase3 + spike). + upid, err := e.api.RestoreLXC(ctx, proxmox.RestoreLXCOptions{ + VMID: spec.VMID, Archive: spec.Archive, Storage: spec.RestoreStorage, + }) + if err != nil { + res.Err = fmt.Errorf("reconcile: bring-up restore: %w", err) + return + } + e.append(withUPID(base, upid, OpTaskRunning)) + if _, err := e.waitTask(ctx, upid, proxmox.WaitOptions{}); err != nil { + res.Err = fmt.Errorf("reconcile: bring-up restore task: %w", err) + return + } + + // 2. Read as-restored config (the net0 the MAC handling keys off). + cfg, err := e.api.GuestConfig(ctx, spec.VMID) + if err != nil { + res.Err = fmt.Errorf("reconcile: bring-up read config: %w", err) + return + } + + // 3+5 coalesced (F4): identity reset + cores/mem sizing + additive mounts in ONE config PUT, + // with the bounded retry that fires ONLY on the transient PVE config-lock 500. + if params := buildBringUpConfig(spec, cfg); len(params) > 0 { + if err := e.setConfigWithLockRetry(ctx, spec.VMID, params); err != nil { + res.Err = fmt.Errorf("reconcile: bring-up config: %w", err) + return + } + } + + // 4. rootfs grow-only resize as its OWN call (F4: kept separate from the config PUT). + if spec.RootfsGrowGB > 0 { + rupid, err := e.api.ResizeLXC(ctx, spec.VMID, "rootfs", fmt.Sprintf("+%dG", spec.RootfsGrowGB)) + if err != nil { + res.Err = fmt.Errorf("reconcile: bring-up resize: %w", err) + return + } + if _, err := e.waitTask(ctx, rupid, proxmox.WaitOptions{}); err != nil { + res.Err = fmt.Errorf("reconcile: bring-up resize task: %w", err) + return + } + } + + // Capture the post-reset MAC for the result (fresh for provision; archived for DR keep). + if cfg2, err := e.api.GuestConfig(ctx, spec.VMID); err == nil { + res.AssignedMAC = net0MAC(cfg2) + } + + // 6. Start LINK-UP (ClassStart). The VERDICT is liveness (waitRunning), NEVER the start + // exitstatus — same liveness-anchoring as the restore-test fix; AllowWarnings so a benign + // start advisory (systemd-nesting) is surfaced, not failed. + startUPID, err := e.api.Start(ctx, spec.VMID) + if err != nil { + res.Err = fmt.Errorf("reconcile: bring-up start: %w", err) + return + } + if startUPID != "" { + st, err := e.api.WaitTask(ctx, startUPID, proxmox.WaitOptions{AllowWarnings: true}) + if err != nil { + res.Err = fmt.Errorf("reconcile: bring-up start task: %w", err) + return + } + if strings.HasPrefix(st.ExitStatus, "WARNINGS") { + tail, logErr := e.api.TaskLogTail(ctx, startUPID, 50) + if logErr != nil { + e.logger.Warn("bring-up: could not read start-task log for warnings", "vmid", spec.VMID, "err", logErr) + } + res.StartWarnings = extractWarningLines(tail) + res.WarningsRecognized = warningsRecognized(res.StartWarnings) + } + } + bootTO := spec.BootTimeout + if bootTO <= 0 { + bootTO = DefaultBootTimeout + } + if err := e.waitRunning(ctx, spec.VMID, bootTO); err != nil { + res.Err = err + return + } + + // 7. Success — KEEP the guest; mark the owning entry terminal so Recover ignores it. + res.Pass = true + res.Verified = "boot+running" + committed = true + e.append(withState(base, OpSucceeded)) +} + +// rollbackBringUp destroys the just-created guest (benign ClassGuestDestroy via SameTxnCreated +// provenance) and records the owning entry terminal. Mirrors teardownScratch: ALWAYS attempts the +// destroy (idempotent — a restore-POST failure that created no guest just errors harmlessly and is +// left in-flight for Recover, which existence-checks). On any teardown failure it leaves the entry +// in-flight so Recover reaps the guest later — never force-destroys. +func (e *Engine) rollbackBringUp(ctx context.Context, base JournalEntry) { + tctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute) + defer cancel() + + dec := e.gate.Authorize(IntentForRollbackDestroy(e.hostID, base.VMID), nil) + if !dec.Allowed { + e.logger.Error("bring-up: rollback destroy refused by gate (unexpected); left for Recover", + "vmid", base.VMID, "reason", dec.Reason) + return + } + upid, err := e.api.DestroyLXC(tctx, base.VMID) + if err != nil { + e.logger.Error("bring-up: rollback destroy failed; left for Recover", "vmid", base.VMID, "err", err) + return + } + if _, err := e.waitTask(tctx, upid, proxmox.WaitOptions{}); err != nil { + e.logger.Error("bring-up: rollback destroy task failed; left for Recover", "vmid", base.VMID, "err", err) + return + } + e.append(withState(base, OpSucceeded)) + e.logger.Warn("bring-up: rolled back (destroyed half-built guest)", "vmid", base.VMID) +} + +// buildBringUpConfig assembles the coalesced config PUT params per the scenario-specific identity +// policy (doc 03 §9). Provision: fresh MAC (strip hwaddr → PVE regenerates, F1) + hostname. +// DR: preserve continuity — keep MAC (unless KeepMAC=false: a source may be live) and keep +// hostname (no force-reset). Both: cores/mem sizing + additive mpN mounts. machine-id and SSH +// host keys are NOT touched here — they regenerate guest-side on first boot (golden bake + the +// baked first-boot unit), keeping the agent's front half host-side-only. +func buildBringUpConfig(spec BringUpSpec, cfg proxmox.GuestConfig) map[string]string { + params := map[string]string{} + + resetMAC := spec.Mode == ModeProvision || (spec.Mode == ModeDRGuestLoss && !spec.KeepMAC) + if resetMAC { + if net0, ok := cfg.Nets()["net0"]; ok && net0 != "" { + params["net0"] = withoutHwaddr(net0) // omit hwaddr → PVE generates a fresh MAC (F1) + } + } + if spec.Mode == ModeProvision && spec.Hostname != "" { + params["hostname"] = spec.Hostname + } + if spec.Cores > 0 { + params["cores"] = strconv.Itoa(spec.Cores) + } + if spec.MemoryMB > 0 { + params["memory"] = strconv.Itoa(spec.MemoryMB) + } + for i, m := range spec.Mounts { + params[fmt.Sprintf("mp%d", i)] = fmt.Sprintf("%s:%d,mp=%s", m.Storage, m.SizeGB, m.MountPoint) + } + return params +} + +// setConfigWithLockRetry issues the coalesced config PUT, retrying ONLY the transient PVE +// config-lock 500 (F4) with bounded backoff. A non-lock error (any other 500 included) fails +// immediately — never retry a real error. The slice-4 per-guest serializer prevents cross-op +// contention; this covers PVE releasing its own async config lock within one job. +func (e *Engine) setConfigWithLockRetry(ctx context.Context, vmid int, params map[string]string) error { + var lastErr error + for attempt := 1; attempt <= configLockMaxAttempts; attempt++ { + if _, err := e.api.SetConfig(ctx, vmid, params); err == nil { + return nil + } else if !pveConfigLock(err) { + return err // real error — never retry + } else { + lastErr = err + e.logger.Warn("bring-up: transient PVE config-lock; retrying", + "vmid", vmid, "attempt", attempt, "max", configLockMaxAttempts, "err", err) + } + select { + case <-ctx.Done(): + return ctx.Err() + case <-time.After(configLockBackoff): + } + } + return fmt.Errorf("reconcile: bring-up config still lock-contended after %d attempts: %w", configLockMaxAttempts, lastErr) +} + +// pveConfigLock reports whether err is the transient PVE config-lock 500 (F4) — and ONLY that. +// The lock surfaces as a proxmox.APIError 500 whose body carries the lock signature. +func pveConfigLock(err error) bool { + var ae *proxmox.APIError + if !errors.As(err, &ae) || ae.StatusCode != 500 { + return false + } + b := strings.ToLower(ae.Body) + return strings.Contains(b, "can't lock file") || strings.Contains(b, "got timeout") +} + +// waitTask waits a (possibly empty) UPID — "" is the clean synchronous path. +func (e *Engine) waitTask(ctx context.Context, upid string, opts proxmox.WaitOptions) (proxmox.TaskStatus, error) { + if upid == "" { + return proxmox.TaskStatus{}, nil + } + return e.api.WaitTask(ctx, upid, opts) +} + +func (e *Engine) bringUpOpID(vmid int) string { + return "bring-up-" + strconv.Itoa(vmid) + "-" + nextSeq(&e.opSeq) +} + +// withoutHwaddr strips the hwaddr token from a netN config string. PUTting a netN with NO hwaddr +// makes PVE generate a fresh MAC (slice-7 spike F1: a restore preserves the archived MAC, so a +// provision MUST strip it to avoid a fleet-wide MAC collision). Mirrors withLinkDown's approach. +func withoutHwaddr(netN string) string { + parts := strings.Split(netN, ",") + out := parts[:0] + for _, p := range parts { + if p == "" || strings.HasPrefix(p, "hwaddr=") { + continue + } + out = append(out, p) + } + return strings.Join(out, ",") +} + +// net0MAC extracts the hwaddr from a guest's net0 config ("" if absent). +func net0MAC(cfg proxmox.GuestConfig) string { + net0, ok := cfg.Nets()["net0"] + if !ok { + return "" + } + for _, p := range strings.Split(net0, ",") { + if strings.HasPrefix(p, "hwaddr=") { + return strings.TrimPrefix(p, "hwaddr=") + } + } + return "" +} diff --git a/internal/reconcile/bringup_test.go b/internal/reconcile/bringup_test.go new file mode 100644 index 0000000..f802dc9 --- /dev/null +++ b/internal/reconcile/bringup_test.go @@ -0,0 +1,382 @@ +package reconcile + +import ( + "context" + "errors" + "strings" + "testing" + "time" + + "gitea.dooplex.hu/admin/felhom-agent/internal/proxmox" +) + +// setParamsFor returns the params of the (last) SetConfig call against vmid, or nil. +func setParamsFor(api *fakeAPI, vmid int) map[string]string { + var out map[string]string + for _, s := range api.sets { + if s.vmid == vmid { + out = s.params + } + } + return out +} + +func TestRunBringUp_ProvisionHappyPath(t *testing.T) { + const vmid = 8000 + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} // empty lxc → vmid free; running default + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "local:backup/golden.tar.zst", VMID: vmid, + RestoreStorage: "local-lvm", Hostname: "felhom-prov-8000", + Cores: 2, MemoryMB: 2048, RootfsGrowGB: 2, + Mounts: []GuestMount{{Storage: "local-lvm", SizeGB: 1, MountPoint: "/mnt/data"}}, + }) + if res.Err != nil || !res.Pass { + t.Fatalf("provision must pass, got %+v", res) + } + if res.Verified != "boot+running" { + t.Errorf("verified = %q", res.Verified) + } + // restore issued to the target vmid. + if len(api.restores) != 1 || api.restores[0].VMID != vmid || api.restores[0].Archive != "local:backup/golden.tar.zst" { + t.Fatalf("restore not issued correctly: %+v", api.restores) + } + // identity: a fresh MAC (net0 set WITHOUT hwaddr) + hostname, coalesced with sizing+mount. + p := setParamsFor(api, vmid) + if p == nil { + t.Fatal("expected a coalesced config PUT") + } + if net0, ok := p["net0"]; !ok || strings.Contains(net0, "hwaddr=") { + t.Errorf("provision must reset MAC: net0 must be set WITHOUT hwaddr, got %q", net0) + } + if p["hostname"] != "felhom-prov-8000" { + t.Errorf("hostname not set: %q", p["hostname"]) + } + if p["cores"] != "2" || p["memory"] != "2048" { + t.Errorf("sizing not coalesced: cores=%q memory=%q", p["cores"], p["memory"]) + } + if p["mp0"] != "local-lvm:1,mp=/mnt/data" { + t.Errorf("mount not attached: mp0=%q", p["mp0"]) + } + // rootfs grow is a SEPARATE call (F4). + if len(api.resizes) != 1 || api.resizes[0].vmid != vmid || api.resizes[0].size != "+2G" { + t.Errorf("rootfs grow not issued separately: %+v", api.resizes) + } + // started link-up. + if len(api.starts) != 1 || api.starts[0] != vmid { + t.Errorf("guest not started: %+v", api.starts) + } + // THE key difference from the restore-test: the guest is KEPT (no teardown). + if len(api.destroys) != 0 { + t.Fatalf("provision success must NOT destroy the guest: %+v", api.destroys) + } +} + +func TestRunBringUp_CompensatingRollback(t *testing.T) { + const vmid = 8000 + lockBackoffFast(t) + + cases := []struct { + name string + setup func(*fakeAPI) + }{ + {"restore error", func(a *fakeAPI) { a.restoreErr = errors.New("restore boom") }}, + {"config real error", func(a *fakeAPI) { + a.setFunc = func(int, map[string]string) (string, error) { + return "", &proxmox.APIError{StatusCode: 500, Body: "some non-lock internal error"} + } + }}, + {"start-task real error", func(a *fakeAPI) { + a.startUPID = "UPID:demo:start:8000:" + a.waitFunc = func(upid string) (proxmox.TaskStatus, error) { + if upid == "UPID:demo:start:8000:" { + return proxmox.TaskStatus{}, errors.New("start task failed") + } + return proxmox.TaskStatus{Status: "stopped", ExitStatus: "OK"}, nil + } + }}, + {"waitRunning timeout", func(a *fakeAPI) { + a.status = map[int]proxmox.Guest{vmid: {VMID: vmid, Status: "stopped"}} + }}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + tc.setup(api) + e, j, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "vol", VMID: vmid, RestoreStorage: "local-lvm", + Hostname: "h", BootTimeout: 40 * time.Millisecond, + }) + if res.Pass || res.Err == nil { + t.Fatalf("must fail, got %+v", res) + } + // The adversarial point: the just-created guest was ACTUALLY destroyed. + if len(api.destroys) != 1 || api.destroys[0] != vmid { + t.Fatalf("compensating rollback must destroy the guest: destroys=%+v", api.destroys) + } + // And the owning entry is terminal (rollback complete) — not left in-flight. + if len(j.InFlight()) != 0 { + t.Errorf("owning entry must be terminal after rollback: %+v", j.InFlight()) + } + }) + } +} + +func TestRunBringUp_DRPreservesContinuityIdentity(t *testing.T) { + const vmid = 8001 + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeDRGuestLoss, Archive: "local:backup/customer.tar.zst", VMID: vmid, + RestoreStorage: "local-lvm", Hostname: "ignored-in-dr", KeepMAC: true, + Cores: 4, // a benign config PUT happens, so we can inspect it has NO identity resets + }) + if res.Err != nil || !res.Pass { + t.Fatalf("dr bring-up must pass, got %+v", res) + } + p := setParamsFor(api, vmid) + // DR continuity: MAC kept (no net0 reset) and hostname NOT force-reset. + if p != nil { + if _, ok := p["net0"]; ok { + t.Errorf("dr+KeepMAC must NOT reset net0 (keep the archived MAC): %+v", p) + } + if _, ok := p["hostname"]; ok { + t.Errorf("dr must NOT force-reset hostname (continuity): %+v", p) + } + if p["cores"] != "4" { + t.Errorf("benign sizing should still apply: cores=%q", p["cores"]) + } + } + // AssignedMAC reflects the kept archived MAC (from scratchCfg's net0). + if res.AssignedMAC != "AA:BB:CC:DD:EE:FF" { + t.Errorf("dr should keep the archived MAC, got %q", res.AssignedMAC) + } + if len(api.destroys) != 0 { + t.Errorf("dr success must not destroy: %+v", api.destroys) + } + // The agent performs NO guest-internal host-key op — there is no such API call; host keys + // are preserved (DR) or regenerated by the baked golden unit (provision). +} + +func TestRunBringUp_DRResetMACWhenSourceMayBeLive(t *testing.T) { + const vmid = 8002 + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeDRGuestLoss, Archive: "vol", VMID: vmid, RestoreStorage: "local-lvm", + KeepMAC: false, // a source guest may still be live → reset MAC even in DR + }) + if res.Err != nil || !res.Pass { + t.Fatalf("got %+v", res) + } + p := setParamsFor(api, vmid) + if p == nil || strings.Contains(p["net0"], "hwaddr=") { + t.Errorf("dr with KeepMAC=false must reset MAC (net0 without hwaddr): %+v", p) + } +} + +func TestRunBringUp_LivenessIsTheVerdict(t *testing.T) { + const vmid = 8000 + const startUPID = "UPID:demo:start:8000:" + mkAPI := func() *fakeAPI { + return &fakeAPI{ + cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}, + startUPID: startUPID, + waitFunc: func(upid string) (proxmox.TaskStatus, error) { + if upid == startUPID { + return proxmox.TaskStatus{Status: "stopped", ExitStatus: "WARNINGS: 1"}, nil + } + return proxmox.TaskStatus{Status: "stopped", ExitStatus: "OK"}, nil + }, + logTailFunc: func(string) ([]string, error) { + return []string{"WARN: Systemd 257 detected. You may need to enable nesting."}, nil + }, + } + } + + // start exits WARNINGS + guest reaches running → PASS, warnings surfaced + recognized. + t.Run("warnings + running -> pass", func(t *testing.T) { + api := mkAPI() + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "v", VMID: vmid, RestoreStorage: "s", Hostname: "h", + }) + if !res.Pass || res.Err != nil { + t.Fatalf("warnings+running must pass: %+v", res) + } + if len(res.StartWarnings) != 1 || !res.WarningsRecognized { + t.Errorf("warnings must be surfaced+recognized: %+v recognized=%v", res.StartWarnings, res.WarningsRecognized) + } + if len(api.destroys) != 0 { + t.Errorf("a passed bring-up must not destroy: %+v", api.destroys) + } + }) + + // same warnings but guest NEVER reaches running → FAIL (verdict is liveness), guest destroyed. + t.Run("warnings + not-running -> fail", func(t *testing.T) { + api := mkAPI() + api.status = map[int]proxmox.Guest{vmid: {VMID: vmid, Status: "stopped"}} + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "v", VMID: vmid, RestoreStorage: "s", Hostname: "h", + BootTimeout: 40 * time.Millisecond, + }) + if res.Pass || res.Err == nil { + t.Fatalf("not-running must fail regardless of warnings: %+v", res) + } + if len(api.destroys) != 1 { + t.Errorf("a failed bring-up must roll back (destroy): %+v", api.destroys) + } + }) +} + +func TestRunBringUp_F4_ConfigLockRetry(t *testing.T) { + const vmid = 8000 + lockBackoffFast(t) + + t.Run("transient lock-500 then 200 -> retries and proceeds", func(t *testing.T) { + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + var calls int + api.setFunc = func(int, map[string]string) (string, error) { + calls++ + if calls == 1 { + return "", &proxmox.APIError{StatusCode: 500, Body: "can't lock file '/run/lock/lxc/pve-config-8000.lock' - got timeout"} + } + return "", nil + } + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "v", VMID: vmid, RestoreStorage: "s", Hostname: "h", + }) + if res.Err != nil || !res.Pass { + t.Fatalf("lock-500 then 200 must succeed: %+v", res) + } + if calls < 2 { + t.Errorf("expected a retry on the transient lock-500, calls=%d", calls) + } + }) + + t.Run("non-lock 500 -> fails without retry", func(t *testing.T) { + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + var calls int + api.setFunc = func(int, map[string]string) (string, error) { + calls++ + return "", &proxmox.APIError{StatusCode: 500, Body: "internal error: disk full"} + } + e, _, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "v", VMID: vmid, RestoreStorage: "s", Hostname: "h", + }) + if res.Pass || res.Err == nil { + t.Fatalf("a non-lock 500 must fail: %+v", res) + } + if calls != 1 { + t.Errorf("a real error must NOT be retried, calls=%d", calls) + } + }) +} + +func TestRunBringUp_JournalsOwningEntryBeforeRestore(t *testing.T) { + const vmid = 8000 + api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{vmid: scratchCfg()}} + e, j, q := newEngine(t, api, EmptyProvider{}) + defer q.Close() + + // At the moment RestoreLXC is called, the owning Rollback entry must already be in-flight — + // so a crash here is recoverable (Recover reaps the half-built guest). + var ownedAtRestore bool + api.restoreHook = func() { + for _, en := range j.InFlight() { + if en.VMID == vmid && en.Rollback { + ownedAtRestore = true + } + } + } + res := e.RunBringUp(context.Background(), BringUpSpec{ + Mode: ModeProvision, Archive: "v", VMID: vmid, RestoreStorage: "s", Hostname: "h", + }) + if res.Err != nil { + t.Fatalf("setup: %+v", res) + } + if !ownedAtRestore { + t.Fatal("the owning Rollback entry MUST be journaled before the restore (crash-safety)") + } +} + +func TestRunBringUp_RejectsReservedAndExistingVMID(t *testing.T) { + e, _, q := newEngine(t, &fakeAPI{}, EmptyProvider{}) + defer q.Close() + for _, id := range []int{9999, 990000, 990005} { + res := e.RunBringUp(context.Background(), BringUpSpec{Mode: ModeProvision, Archive: "v", VMID: id, RestoreStorage: "s"}) + if res.Err == nil { + t.Errorf("VMID %d is reserved and must be refused", id) + } + } + // existing VMID → refuse (restore-over-existing is a signed op, not this benign path). + api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 8000}}} + e2, _, q2 := newEngine(t, api, EmptyProvider{}) + defer q2.Close() + res := e2.RunBringUp(context.Background(), BringUpSpec{Mode: ModeProvision, Archive: "v", VMID: 8000, RestoreStorage: "s"}) + if res.Err == nil { + t.Error("bring-up over an existing guest must be refused") + } + if len(api.restores) != 0 || len(api.destroys) != 0 { + t.Error("a refused bring-up must not restore or destroy anything") + } +} + +func TestRecover_HalfBuiltBringUpRolledBack(t *testing.T) { + const vmid = 8000 + // The guest still exists at startup (agent crashed mid-bring-up) → Recover destroys it. + api := &fakeAPI{lxc: []proxmox.Guest{{VMID: vmid, Status: "running"}}} + e, j, _ := newEngine(t, api, EmptyProvider{}) + if err := j.Append(JournalEntry{OpID: "bring-up-8000-1", VMID: vmid, Kind: bringUpKind, Rollback: true, State: OpTaskRunning, At: time.Now().UTC()}); err != nil { + t.Fatal(err) + } + res := e.Recover(context.Background()) + if res.BringUpRolledBack != 1 { + t.Fatalf("half-built bring-up must be rolled back, got %+v", res) + } + if len(api.destroys) != 1 || api.destroys[0] != vmid { + t.Fatalf("DestroyLXC not called for the half-built guest: %+v", api.destroys) + } + if len(j.InFlight()) != 0 { + t.Errorf("resolved rollback entry must not be in-flight: %+v", j.InFlight()) + } +} + +func TestRecover_HalfBuiltBringUpAlreadyGone(t *testing.T) { + const vmid = 8000 + // Crash after the restore POST failed (no guest) → idempotent clean, no destroy. + api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 9001}}} // 8000 absent + e, j, _ := newEngine(t, api, EmptyProvider{}) + j.Append(JournalEntry{OpID: "bring-up-8000-1", VMID: vmid, Kind: bringUpKind, Rollback: true, State: OpStarted, At: time.Now().UTC()}) + res := e.Recover(context.Background()) + if res.BringUpClean != 1 || len(api.destroys) != 0 { + t.Fatalf("already-gone bring-up must be clean with no destroy: res=%+v destroys=%+v", res, api.destroys) + } + if len(j.InFlight()) != 0 { + t.Errorf("entry must be resolved: %+v", j.InFlight()) + } +} + +// lockBackoffFast shrinks the F4 retry backoff for tests and restores it after. +func lockBackoffFast(t *testing.T) { + t.Helper() + prev := configLockBackoff + configLockBackoff = time.Millisecond + t.Cleanup(func() { configLockBackoff = prev }) +} diff --git a/internal/reconcile/engine_test.go b/internal/reconcile/engine_test.go index d27f706..21bc8c2 100644 --- a/internal/reconcile/engine_test.go +++ b/internal/reconcile/engine_test.go @@ -31,6 +31,11 @@ type fakeAPI struct { statusFunc func(upid string) (proxmox.TaskStatus, error) // logTailFunc backs TaskLogTail (restore-test start-warning surfacing); default = empty. logTailFunc func(upid string) ([]string, error) + // setFunc, when set, backs SetConfig (drives the F4 lock-500-then-200 test). + setFunc func(vmid int, params map[string]string) (string, error) + // restoreHook, when set, fires inside RestoreLXC (used to assert the owning journal entry + // is written BEFORE the restore — crash-safety ordering). + restoreHook func() starts []int stops []int @@ -48,6 +53,9 @@ type resizeCall struct { } func (f *fakeAPI) RestoreLXC(_ context.Context, opts proxmox.RestoreLXCOptions) (string, error) { + if f.restoreHook != nil { + f.restoreHook() + } f.mu.Lock() f.restores = append(f.restores, opts) f.mu.Unlock() @@ -121,7 +129,11 @@ func (f *fakeAPI) Stop(_ context.Context, vmid int) (string, error) { func (f *fakeAPI) SetConfig(_ context.Context, vmid int, params map[string]string) (string, error) { f.mu.Lock() f.sets = append(f.sets, setCall{vmid, params}) + fn := f.setFunc f.mu.Unlock() + if fn != nil { + return fn(vmid, params) + } return f.setUPID, f.setErr } diff --git a/internal/reconcile/journal.go b/internal/reconcile/journal.go index 10bf472..f24e13b 100644 --- a/internal/reconcile/journal.go +++ b/internal/reconcile/journal.go @@ -50,8 +50,15 @@ type JournalEntry struct { // scratch guest may exist and MUST be destroyed" — so Recover resolves it by ensuring // VMID is gone (a benign teardown), NOT by re-checking any sub-task UPID. The entry is // terminal only after teardown. See recover.go. - Scratch bool `json:"scratch,omitempty"` - At time.Time `json:"at"` + Scratch bool `json:"scratch,omitempty"` + // Rollback marks an entry that OWNS a guest the agent is CREATING in this journaled + // bring-up transaction (slice 7, doc 03 §9). While such an entry is in-flight, the + // invariant is "VMID may be a half-built guest and MUST be destroyed" (a compensating + // rollback) — so Recover resolves it by ensuring VMID is gone, NOT by re-checking the + // restore sub-task UPID (whose OK status would otherwise leave a half-provisioned guest). + // On SUCCESS the bring-up records this entry terminal and KEEPS the guest. See recover.go. + Rollback bool `json:"rollback,omitempty"` + At time.Time `json:"at"` } // Journal is the durable operation log + idempotency store. It mirrors diff --git a/internal/reconcile/recover.go b/internal/reconcile/recover.go index 91bb97c..c746ff3 100644 --- a/internal/reconcile/recover.go +++ b/internal/reconcile/recover.go @@ -44,6 +44,15 @@ func (e *Engine) Recover(ctx context.Context) RecoverResult { continue } + // Rollback entries (slice-7 bring-up) own a guest the agent was CREATING. An in-flight + // one means "VMID may be a half-built guest → destroy it" (compensating rollback) — same + // reason the Scratch path runs before the generic UPID path: the restore sub-task's OK + // status would otherwise mark the entry succeeded and leave a half-provisioned guest. + if entry.Rollback { + e.recoverBringUp(ctx, entry, &res) + continue + } + if entry.UPID == "" { // POST never confirmed → abandon (fail-safe). e.append(terminal(entry, OpFailed)) @@ -144,16 +153,77 @@ func (e *Engine) recoverScratch(ctx context.Context, entry JournalEntry, res *Re "op_id", entry.OpID, "vmid", entry.VMID) } +// recoverBringUp rolls back a half-built bring-up guest left in-flight by a mid-job crash +// (slice 7, doc 03 §9). Invariant: a Rollback entry in-flight at startup means "VMID may be a +// half-provisioned guest and MUST be destroyed" (compensating rollback — the guest is only kept +// when the bring-up reached its terminal OpSucceeded). Idempotent: already-gone records +// terminal-clean. Routes the destroy through the gate as benign ClassGuestDestroy (SameTxnCreated +// provenance) — the same audit-bearing path the in-job rollback uses. +func (e *Engine) recoverBringUp(ctx context.Context, entry JournalEntry, res *RecoverResult) { + lxc, err := e.api.ListLXC(ctx) + if err != nil { + res.Unresolved++ + e.logger.Warn("recover: cannot list guests to resolve half-built bring-up; left in-flight", + "op_id", entry.OpID, "vmid", entry.VMID, "err", err) + return + } + exists := false + for _, g := range lxc { + if g.VMID == entry.VMID { + exists = true + break + } + } + if !exists { + // Already gone (e.g. the restore POST never created it) → no leak. + e.append(terminal(entry, OpSucceeded)) + res.BringUpClean++ + e.logger.Info("recover: half-built bring-up resolved; guest already gone", + "op_id", entry.OpID, "vmid", entry.VMID) + return + } + + dec := e.gate.Authorize(IntentForRollbackDestroy(e.hostID, entry.VMID), nil) + if !dec.Allowed { + // Should be benign (SameTxnCreated); if not, fail-safe — do NOT force a destroy. + res.Unresolved++ + e.logger.Error("recover: bring-up rollback refused by gate (unexpected); left in-flight", + "op_id", entry.OpID, "vmid", entry.VMID, "reason", dec.Reason) + return + } + upid, err := e.api.DestroyLXC(ctx, entry.VMID) + if err != nil { + res.Unresolved++ + e.logger.Warn("recover: destroying half-built bring-up failed; left in-flight (will retry)", + "op_id", entry.OpID, "vmid", entry.VMID, "err", err) + return + } + if upid != "" { + if _, err := e.api.WaitTask(ctx, upid, proxmox.WaitOptions{}); err != nil { + res.Unresolved++ + e.logger.Warn("recover: half-built bring-up destroy task failed; left in-flight (will retry)", + "op_id", entry.OpID, "vmid", entry.VMID, "err", err) + return + } + } + e.append(terminal(entry, OpSucceeded)) + res.BringUpRolledBack++ + e.logger.Warn("recover: rolled back half-built bring-up guest", + "op_id", entry.OpID, "vmid", entry.VMID) +} + // RecoverResult summarizes a startup recovery pass. type RecoverResult struct { - Examined int - Resumed int // task found completed OK and recorded succeeded - Failed int // task found ended non-OK and recorded failed - RolledBack int // no task id → abandoned (fail-safe) - StillRunning int // task still executing → left in-flight - Unresolved int // task status unreadable → left in-flight - ScratchClean int // scratch entry resolved: guest already gone (no leak) - ScratchDestroyed int // scratch entry resolved: leaked guest destroyed + Examined int + Resumed int // task found completed OK and recorded succeeded + Failed int // task found ended non-OK and recorded failed + RolledBack int // no task id → abandoned (fail-safe) + StillRunning int // task still executing → left in-flight + Unresolved int // task status unreadable → left in-flight + ScratchClean int // scratch entry resolved: guest already gone (no leak) + ScratchDestroyed int // scratch entry resolved: leaked guest destroyed + BringUpClean int // bring-up rollback entry resolved: guest already gone (no leak) + BringUpRolledBack int // bring-up rollback entry resolved: half-built guest destroyed } // terminal builds a terminal journal record preserving the op's identity, with the @@ -167,6 +237,7 @@ func terminal(e JournalEntry, state OpState) JournalEntry { State: state, IdempKey: e.IdempKey, Scratch: e.Scratch, + Rollback: e.Rollback, At: time.Now().UTC(), } }