v0.100.0 — R-82: the restore tier comes from the ARCHIVE, not the configured target
Found by the first real PBS restore round-trip, not by review. Restoring a felhom-pbs: archive on a box whose primary target is 'local' failed after exactly 600.76s — the 10-minute LOCAL wait — against a 14.46 GB WAN restore needing ~2 hours. The selftest derived its tier from cfg.Backup.BackupTarget() (the configured default), so restoreTaskTimeout correctly returned the local bound for a PBS archive. 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. 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 doc comment predicts the consequence exactly, and it happened: teardown fired at a still-restoring guest and was refused with HTTP 403 missing privilege VM.Allocate (PVE associates the pool only at restore COMPLETION, and the grant is on /pool/felhom not /vms). That 403 was load-bearing luck — the only reason a destructive teardown did not run against a half-restored guest. The restore completed unharmed. - restoreTierForArchive() derives the tier from the archive's own storage (archiveStorageID parses the volid prefix), falling back to the configured target only when there is no prefix. Recorded, NOT fixed here: the daemon's scheduled restore-test still only covers the PRIMARY tier (Pick uses a runner built on BackupTarget(); Spec is built once at construction, not per tick) — so the offsite tier is never automatically restore-tested. And the agent still cannot tear down a scratch guest until its restore completes; widening the token's privileges is deliberately not the fix. Full suite green (29 packages).
This commit is contained in:
@@ -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)
|
## 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
|
Implements two operator rulings of 2026-07-26. Both are behaviour changes on the multi-tier path
|
||||||
|
|||||||
@@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -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
|
// 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").
|
// 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 {
|
func storageTier(ctx context.Context, px *proxmox.Client, storageID string) string {
|
||||||
stores, err := px.ListStorage(ctx)
|
stores, err := px.ListStorage(ctx)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
@@ -1702,7 +1730,7 @@ func runSelftestRestoreTest(ctx context.Context, cfg config.Config, logger *slog
|
|||||||
}
|
}
|
||||||
min, max := cfg.Backup.ScratchBand()
|
min, max := cfg.Backup.ScratchBand()
|
||||||
fmt.Printf(" restoring %s into scratch band [%d,%d] on %s …\n", archive, min, max, cfg.Backup.RestoreStorage)
|
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{
|
res := engine.RunRestoreTest(ctx, reconcile.RestoreTestSpec{
|
||||||
Archive: archive, RestoreStorage: cfg.Backup.RestoreStorage,
|
Archive: archive, RestoreStorage: cfg.Backup.RestoreStorage,
|
||||||
ScratchMin: min, ScratchMax: max, SourceTier: rtTier,
|
ScratchMin: min, ScratchMax: max, SourceTier: rtTier,
|
||||||
|
|||||||
Reference in New Issue
Block a user