diff --git a/CHANGELOG.md b/CHANGELOG.md index c856202..d1f1817 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,25 @@ +## v0.68.0 — S4.1: tier-aware restore-task deadline (unattended offsite restore-test) (2026-07-04) + +The offsite restore-test couldn't complete on the scheduler path because a WAN restore of a large +guest exceeds the restore-task wait's 10-minute default — the wait expired mid-restore, teardown +then fired against a still-restoring (not-yet-pool-associated) scratch guest, and it leaked. Make +the restore-task wait **tier-aware**. + +- **`internal/reconcile`**: `RestoreTestSpec.RestoreTaskTimeout` (0 → the 10m `WaitOptions` default); + the restore-task `WaitTask` now passes it. Local-tier restores are UNCHANGED (10m — a local + restore hanging that long is a genuine fault). +- **`internal/config`**: `BackupConfig.RestoreTestPBSRestoreTimeoutSeconds` + accessor + `RestoreTestPBSRestoreTimeout()` (positive as-is, else **120m** — for an unattended nightly test a + false timeout is worse than a slow pass; very large guests may need more). +- **`cmd/felhom-agent`**: `restoreTaskTimeout(cfg, tier)` sets the field to the configured PBS + timeout only when `SourceTier=="pbs"` (both the scheduler + selftest spec builds), else 0. +- Tests: tier-aware `WaitOptions.Timeout` (pbs→120m, local→0; red-proofed against `WaitOptions{}`) + + the accessor contract. The "grant scratch-band `VM.Allocate`" follow-up was diagnosed, not + blind-applied — the scratch guest is restored INTO `/pool/felhom` (whose ACL already grants + `VM.Allocate`), so the earlier teardown 403 was a *consequence* of the timeout (a not-yet-pooled, + still-restoring guest), not a missing grant. **No ACL/host-install change.** (Live confirmation of + the phantom in REPORT.) + ## v0.67.0 — S4: namespace-aware PBS client (per-customer offsite tenancy) (2026-07-04) Phase-1 live probe on felhom-hetzner proved the offsite tenancy path (backup/restore/list/isolation diff --git a/cmd/felhom-agent/main.go b/cmd/felhom-agent/main.go index e06fc96..616d23f 100644 --- a/cmd/felhom-agent/main.go +++ b/cmd/felhom-agent/main.go @@ -779,6 +779,16 @@ func storageTier(ctx context.Context, px *proxmox.Client, storageID string) stri return "local" } +// restoreTaskTimeout returns the tier-aware restore-task wait: the generous configured PBS timeout +// for a WAN (pbs-tier) restore, else 0 (→ WaitOptions' 10m default) for a local restore. S4.1: a +// too-short wait kills a WAN restore mid-flight → mid-restore teardown → leaked scratch. +func restoreTaskTimeout(cfg config.Config, tier string) time.Duration { + if tier == "pbs" { + return cfg.Backup.RestoreTestPBSRestoreTimeout() + } + return 0 +} + // readTrimmed reads a file and trims surrounding whitespace/newline (for the .pw secret). func readTrimmed(path string) (string, error) { b, err := os.ReadFile(path) @@ -811,12 +821,16 @@ func buildRestoreTestScheduler(cfg config.Config, px *proxmox.Client, engine *re Runner: engine, Pick: runner.PickRestoreCandidate, Store: store, - Spec: reconcile.RestoreTestSpec{ - RestoreStorage: cfg.Backup.RestoreStorage, - ScratchMin: min, - ScratchMax: max, - SourceTier: storageTier(context.Background(), px, target), - }, + Spec: func() reconcile.RestoreTestSpec { + tier := storageTier(context.Background(), px, target) + return reconcile.RestoreTestSpec{ + RestoreStorage: cfg.Backup.RestoreStorage, + ScratchMin: min, + ScratchMax: max, + SourceTier: tier, + RestoreTaskTimeout: restoreTaskTimeout(cfg, tier), + } + }(), Cadence: cadence, Logger: logger, }) @@ -1249,9 +1263,11 @@ 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) res := engine.RunRestoreTest(ctx, reconcile.RestoreTestSpec{ Archive: archive, RestoreStorage: cfg.Backup.RestoreStorage, - ScratchMin: min, ScratchMax: max, SourceTier: storageTier(ctx, px, target), + ScratchMin: min, ScratchMax: max, SourceTier: rtTier, + RestoreTaskTimeout: restoreTaskTimeout(cfg, rtTier), }) printJSON("restore-test record", backup.ToHubRestoreTest(res, time.Now().UTC())) if res.Skipped { diff --git a/internal/config/config.go b/internal/config/config.go index 59f5b8e..6f6ee1c 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -198,6 +198,12 @@ type BackupConfig struct { // always excluded. Defaults to 990000–990009. ScratchVMIDMin int `json:"scratch_vmid_min"` ScratchVMIDMax int `json:"scratch_vmid_max"` + // RestoreTestPBSRestoreTimeoutSeconds bounds the wait on a PBS-tier (offsite/WAN) restore-test + // restore task; 0 → default 120m. A large guest restored over a slow home uplink runs long, and + // for an UNATTENDED nightly test a false timeout (→ mid-restore teardown → leaked scratch) is + // worse than a slow pass. Very large guests may need a higher value. LOCAL-tier restores keep + // the 10m WaitOptions default (a local restore hanging 10m is a genuine fault). + RestoreTestPBSRestoreTimeoutSeconds int `json:"restore_test_pbs_restore_timeout_seconds"` // PBS (slice 6 Phase B). The verify maintenance loop runs on its own cadence (cheaper + // more frequent than the full restore-test); 0 → default (6h), negative → disabled. @@ -245,6 +251,14 @@ func (b BackupConfig) BackupCadence() time.Duration { return 24 * time.Hour } +// RestoreTestPBSRestoreTimeout returns the PBS-tier restore-task wait: positive as-is, else 120m. +func (b BackupConfig) RestoreTestPBSRestoreTimeout() time.Duration { + if b.RestoreTestPBSRestoreTimeoutSeconds > 0 { + return time.Duration(b.RestoreTestPBSRestoreTimeoutSeconds) * time.Second + } + return 120 * time.Minute +} + // defaultBackupTarget is the offsite PBS storage whole-guest backups land on by default. It is // SEPARATE HARDWARE from the guest's own disk (a PBS datastore on the DooPlex box), so a host // disk/hardware failure doesn't take the backups with it — that's what makes it real DR. Proven diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 613b741..7d21a73 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -5,8 +5,29 @@ import ( "path/filepath" "strings" "testing" + "time" ) +// TestRestoreTestPBSRestoreTimeout mirrors the BackupCadence accessor contract: positive as-is, +// 0 → default (120m), negative → default. +func TestRestoreTestPBSRestoreTimeout(t *testing.T) { + cases := []struct { + secs int + want time.Duration + }{ + {0, 120 * time.Minute}, + {-5, 120 * time.Minute}, + {1800, 30 * time.Minute}, + {7200, 120 * time.Minute}, + } + for _, c := range cases { + got := BackupConfig{RestoreTestPBSRestoreTimeoutSeconds: c.secs}.RestoreTestPBSRestoreTimeout() + if got != c.want { + t.Errorf("RestoreTestPBSRestoreTimeout(secs=%d) = %v, want %v", c.secs, got, c.want) + } + } +} + func TestRedactedMasksSecret(t *testing.T) { c := Default() c.Proxmox.Token = "felhom-agent@pve!agent=b6547d9d-08ec-4f22-beb8-a551dc2cd69d" diff --git a/internal/reconcile/engine_test.go b/internal/reconcile/engine_test.go index 9e441a7..b6f3e12 100644 --- a/internal/reconcile/engine_test.go +++ b/internal/reconcile/engine_test.go @@ -47,6 +47,7 @@ type fakeAPI struct { restores []proxmox.RestoreLXCOptions destroys []int waits []string + waitOpts []proxmox.WaitOptions // parallel to waits: the options each WaitTask was called with listErr error } @@ -151,9 +152,10 @@ func (f *fakeAPI) ResizeLXC(_ context.Context, vmid int, disk, size string) (str return f.resizeUPID, f.resizeErr } -func (f *fakeAPI) WaitTask(_ context.Context, upid string, _ proxmox.WaitOptions) (proxmox.TaskStatus, error) { +func (f *fakeAPI) WaitTask(_ context.Context, upid string, opts proxmox.WaitOptions) (proxmox.TaskStatus, error) { f.mu.Lock() f.waits = append(f.waits, upid) + f.waitOpts = append(f.waitOpts, opts) f.mu.Unlock() if f.waitFunc != nil { return f.waitFunc(upid) diff --git a/internal/reconcile/restoretest.go b/internal/reconcile/restoretest.go index 0673c20..dc5e930 100644 --- a/internal/reconcile/restoretest.go +++ b/internal/reconcile/restoretest.go @@ -35,6 +35,11 @@ type RestoreTestSpec struct { ScratchMin int // inclusive scratch VMID band (must be > 0) ScratchMax int // inclusive BootTimeout time.Duration // 0 → DefaultBootTimeout + // RestoreTaskTimeout bounds the wait on the restore (vzrestore) task. 0 → WaitOptions' 10m + // default (fine for a LOCAL restore). A WAN/pbs restore of a large guest runs long, so the + // caller sets this generously for the pbs tier — else the wait expires mid-restore, teardown + // fires against a still-restoring (not-yet-pool-associated) guest, and the scratch leaks. + RestoreTaskTimeout time.Duration } // RestoreTestResult is the reconcile-local outcome (the backup package maps it to the @@ -243,7 +248,7 @@ func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestS launched = true e.append(withUPID(base, upid, OpTaskRunning)) if upid != "" { - if _, err := e.api.WaitTask(ctx, upid, proxmox.WaitOptions{}); err != nil { + if _, err := e.api.WaitTask(ctx, upid, proxmox.WaitOptions{Timeout: spec.RestoreTaskTimeout}); err != nil { res.Err = fmt.Errorf("reconcile: restore-test restore task: %w", err) return } diff --git a/internal/reconcile/restoretest_test.go b/internal/reconcile/restoretest_test.go index c04cda7..50a5bb2 100644 --- a/internal/reconcile/restoretest_test.go +++ b/internal/reconcile/restoretest_test.go @@ -56,6 +56,49 @@ func TestRunRestoreTest_PassAndTeardown(t *testing.T) { } } +// TestRunRestoreTest_TierAwareRestoreTimeout (S4.1) pins that the restore-task wait carries the +// tier-derived timeout: the configured (generous) value for a pbs/WAN restore, and 0 (→ the 10m +// WaitOptions default, UNCHANGED) for a local restore. Red-proof: revert the L246 wait to +// WaitOptions{} → the pbs assertion (120m) fails. +func TestRunRestoreTest_TierAwareRestoreTimeout(t *testing.T) { + const restoreUPID = "UPID:node:1:2:3:4:vzrestore:990000:tok:" // async restore → the wait fires + + restoreWaitTimeout := func(api *fakeAPI) (time.Duration, bool) { + for i, u := range api.waits { + if u == restoreUPID { + return api.waitOpts[i].Timeout, true + } + } + return 0, false + } + + // pbs tier → the configured generous timeout is passed to WaitTask. + pbsAPI := &fakeAPI{cfg: map[int]proxmox.GuestConfig{990000: scratchCfg()}, restoreUPID: restoreUPID} + e, _, q := newEngine(t, pbsAPI, EmptyProvider{}) + defer q.Close() + e.RunRestoreTest(context.Background(), RestoreTestSpec{ + Archive: "felhom-offsite:backup/ct/9201/x", RestoreStorage: "local-lvm", + ScratchMin: 990000, ScratchMax: 990009, SourceTier: "pbs", + RestoreTaskTimeout: 120 * time.Minute, + }) + if to, ok := restoreWaitTimeout(pbsAPI); !ok || to != 120*time.Minute { + t.Errorf("pbs restore wait Timeout = %v (found=%v), want 120m", to, ok) + } + + // local tier → 0 (→ WaitOptions' 10m default preserved, UNCHANGED). + localAPI := &fakeAPI{cfg: map[int]proxmox.GuestConfig{990000: scratchCfg()}, restoreUPID: restoreUPID} + e2, _, q2 := newEngine(t, localAPI, EmptyProvider{}) + defer q2.Close() + e2.RunRestoreTest(context.Background(), RestoreTestSpec{ + Archive: "local:backup/x.tar.zst", RestoreStorage: "local-lvm", + ScratchMin: 990000, ScratchMax: 990009, SourceTier: "local", + RestoreTaskTimeout: 0, + }) + if to, ok := restoreWaitTimeout(localAPI); !ok || to != 0 { + t.Errorf("local restore wait Timeout = %v (found=%v), want 0 (→10m default)", to, ok) + } +} + // startWarnAPI builds a fakeAPI whose guest-start task exits "WARNINGS: 1" and whose start // task log contains the given warning lines. The guest reaches running (status default). func startWarnAPI(startUPID string, logLines []string) *fakeAPI {