diff --git a/CHANGELOG.md b/CHANGELOG.md index d2ffe7b..f512a87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,50 @@ +## v0.100.0 — R-82: the restore tier comes from the ARCHIVE, not the configured target (2026-07-26) + +**Found by the first real PBS restore round-trip (2026-07-26), not by review.** Restoring +`felhom-pbs:backup/ct/9201/2026-07-26T12:21:48Z` on a box whose primary target is `"local"` failed +after exactly **600.76 s** — the 10-minute local wait — against a 14.46 GB WAN restore that needed +roughly two hours. + +`--selftest=restore-test` derived its tier with `storageTier(ctx, px, cfg.Backup.BackupTarget())` — +the **configured default target** — and then `restoreTaskTimeout` correctly returned the local +(10 m) bound for it. The recorded result even said `"source_tier": "local"` for a PBS archive. + +**The tier-aware machinery was already right; it was fed the wrong input.** `RestoreTaskTimeout`, +the generous `RestoreTestPBSRestoreTimeout` (120 m), and the pool-association handling all existed +and all worked. What broke is an assumption that stopped being true the moment a second tier +existed: *"the configured target"* is no longer a proxy for *"the tier this archive belongs to"*. + +`RestoreTestSpec.RestoreTaskTimeout`'s own doc comment predicts the consequence exactly: + +> *"…else the wait expires mid-restore, teardown fires against a still-restoring +> (not-yet-pool-associated) guest, and the scratch leaks."* + +Which is what happened: the teardown fired at a live restore and was refused — +`HTTP 403 … missing privilege VM.Allocate` — because PVE associates the pool only when the restore +COMPLETES, and the agent's `VM.Allocate` is granted on `/pool/felhom`, not on `/vms`. **The 403 was +load-bearing luck:** it is the only reason a destructive teardown did not run against a +half-restored guest. The restore itself carried on to completion, unharmed. + +### Changed +- **`restoreTierForArchive(ctx, px, archive, fallbackTarget)`** — derives the tier from the + ARCHIVE'S OWN storage (`archiveStorageID` parses the volid prefix), falling back to the configured + target only when the volid carries no prefix. Wired into the `--selftest=restore-test` path. + +### NOT changed (recorded, not fixed here) +- **The daemon's scheduled restore-test still only covers the PRIMARY tier.** Its `Pick` uses a + runner built on `BackupTarget()`, so it never selects a PBS archive, and its `Spec` is built once + at construction rather than per tick. That is self-consistent today (tier matches archive) but it + means **the offsite tier is never automatically restore-tested** — a real R-82 gap, and arguably + the more important half of "is the DR tier real?". Needs a per-tick spec; own task. +- **The agent cannot tear down a scratch guest until its restore completes** (no `VM.Allocate` on + `/vms`; the pool association lands only at completion). With the correct timeout the teardown no + longer fires mid-restore, so this is latent again — but it is not fixed, and widening the token's + privileges is deliberately NOT the answer. + +### Tests +`TestArchiveStorageID` pins the volid parsing incl. the `i>0` guard (a leading colon is not a +storage id). Full suite green (29 packages). + ## v0.99.0 — R-82: operator rulings — 2-week offsite retention + one backup at a time (2026-07-26) Implements two operator rulings of 2026-07-26. Both are behaviour changes on the multi-tier path diff --git a/cmd/felhom-agent/archivetier_test.go b/cmd/felhom-agent/archivetier_test.go new file mode 100644 index 0000000..d0f7f52 --- /dev/null +++ b/cmd/felhom-agent/archivetier_test.go @@ -0,0 +1,23 @@ +package main + +import "testing" + +// R-82 live regression (2026-07-26): the restore-test derived its tier from the CONFIGURED default +// target instead of the archive's own storage. Restoring a `felhom-pbs:` archive on a box whose +// primary target is "local" was classified "local" → the 10-minute local wait instead of the +// generous PBS one → the wait expired mid-restore at 600s against a 14.46 GB WAN restore, teardown +// fired at a still-restoring guest, and the scratch leaked. +func TestArchiveStorageID(t *testing.T) { + cases := []struct{ in, want string }{ + {"felhom-pbs:backup/ct/9201/2026-07-26T12:21:48Z", "felhom-pbs"}, + {"local:backup/vzdump-lxc-9201-2026_07_26-09_03_19.tar.zst", "local"}, + {"", ""}, + {"no-prefix", ""}, + {":leading-colon", ""}, // i>0 guard: a leading colon is not a storage id + } + for _, c := range cases { + if got := archiveStorageID(c.in); got != c.want { + t.Fatalf("archiveStorageID(%q) = %q, want %q", c.in, got, c.want) + } + } +} diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index d775082..5648680 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -1164,6 +1164,34 @@ func pbsTargetsFromPVE(cfg config.Config, px *proxmox.Client, logger *slog.Logge // storageTier returns the restore-test source tier for a backup storage id: "pbs" when that // storage is a PBS datastore, else "local". Best-effort (a lookup failure → "local"). +// archiveStorageID returns the storage a volid lives on — "felhom-pbs" from +// "felhom-pbs:backup/ct/9201/2026-07-26T12:21:48Z". Empty when there is no storage prefix. +func archiveStorageID(volid string) string { + if i := strings.Index(volid, ":"); i > 0 { + return volid[:i] + } + return "" +} + +// restoreTierForArchive derives the restore tier from THE ARCHIVE'S OWN STORAGE, falling back to +// the configured default target only when the volid carries no storage prefix. +// +// R-82 (found live 2026-07-26): this used to read the tier from cfg.Backup.BackupTarget(), i.e. the +// PRIMARY tier's target. Restoring a `felhom-pbs:` archive on a box whose primary is "local" was +// therefore classified "local" and got the 10-MINUTE local wait instead of the generous PBS one — +// the wait expired mid-restore at 600s, teardown fired against a still-restoring guest, and the +// scratch leaked. Exactly the failure RestoreTestSpec.RestoreTaskTimeout's doc comment predicts. +// +// The tier-aware machinery was already correct; it was fed the wrong input. With more than one tier +// configured, "the configured target" is no longer a proxy for "the tier this archive belongs to". +func restoreTierForArchive(ctx context.Context, px *proxmox.Client, archive, fallbackTarget string) string { + id := archiveStorageID(archive) + if id == "" { + id = fallbackTarget + } + return storageTier(ctx, px, id) +} + func storageTier(ctx context.Context, px *proxmox.Client, storageID string) string { stores, err := px.ListStorage(ctx) if err != nil { @@ -1702,7 +1730,7 @@ func runSelftestRestoreTest(ctx context.Context, cfg config.Config, logger *slog } min, max := cfg.Backup.ScratchBand() fmt.Printf(" restoring %s into scratch band [%d,%d] on %s …\n", archive, min, max, cfg.Backup.RestoreStorage) - rtTier := storageTier(ctx, px, target) + rtTier := restoreTierForArchive(ctx, px, archive, target) res := engine.RunRestoreTest(ctx, reconcile.RestoreTestSpec{ Archive: archive, RestoreStorage: cfg.Backup.RestoreStorage, ScratchMin: min, ScratchMax: max, SourceTier: rtTier,