admin/felhom-agent
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

v0.60.0: proof-of-launch destroy gating (F1a/b/c) + restore-test band-advance (F2)

Browse Source 
Campaign pool-effects F1 (HIGH): the bring-up compensating rollback and the
restore-test teardown destroyed the target vmid even when RestoreLXC failed
synchronously without creating anything — destroying a guest the transaction
never made (only the pool ACL 403 contained it). A RestoreLXC UPID is now the
sole destroy authorization in all three destroy paths (in-process bring-up
defer, in-process restore-test teardown, Recover). F2: the restore-test
advances past an 'already exists' band vmid (invisible squatter) instead of
failing + false-alerting; a fully-occupied band Skips.

Red-proof verified: with the gates reverted, the four new tests fail with the
innocent-guest destroy. go build/vet/test clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
authentik Default Admin
2026-07-02 10:18:51 +02:00
parent 55ade9e254
commit b9356d60ab
8 changed files with 400 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files
internal/reconcile/restoretest_test.go
+141 -11
View File
@@ -210,7 +210,12 @@ func TestRunRestoreTest_TeardownOnFailedVerify(t *testing.T) {
}
}
func TestRunRestoreTest_RestoreFailureStillTearsDown(t *testing.T) {
// TestRunRestoreTest_RestoreNoLaunchNoTeardown is the F1b red-proof (campaign pool-effects): a
// restore that fails SYNCHRONOUSLY (no UPID — nothing created) must NOT run teardown. The old
// behavior destroyed the picked vmid anyway — the exact destroy-innocent-guest bug when a
// pool-invisible squatter occupied the band vmid. Red-proof: revert the `launched` gate in
// runScratchTest and this test fails with destroys=[990000].
func TestRunRestoreTest_RestoreNoLaunchNoTeardown(t *testing.T) {
api := &fakeAPI{restoreErr: errors.New("restore boom")}
e, j, q := newEngine(t, api, EmptyProvider{})
defer q.Close()
@@ -221,16 +226,112 @@ func TestRunRestoreTest_RestoreFailureStillTearsDown(t *testing.T) {
if res.Pass || res.Err == nil {
t.Fatalf("expected restore failure, got %+v", res)
}
// Even though restore failed, the scratch entry was journaled BEFORE the restore, so
// teardown runs (idempotent — destroys the maybe-partial guest).
if len(api.destroys) != 1 {
t.Fatalf("teardown must run after a restore failure: %+v", api.destroys)
// THE point: no teardown — the txn created nothing at that vmid.
if len(api.destroys) != 0 {
t.Fatalf("a no-launch restore failure must NOT destroy the vmid: %+v", api.destroys)
}
// The entry is closed terminal in-process (not left for Recover).
if len(j.InFlight()) != 0 {
t.Errorf("scratch entry must be terminal: %+v", j.InFlight())
}
}
// TestRunRestoreTest_LaunchedTaskFailureStillTearsDown (no-regression, Scenario B): the restore
// POST was accepted (UPID) but the task then fails → we own the maybe-partial guest → teardown
// MUST still run.
func TestRunRestoreTest_LaunchedTaskFailureStillTearsDown(t *testing.T) {
const restoreUPID = "UPID:demo:restore:990000:"
api := &fakeAPI{
restoreUPID: restoreUPID,
waitFunc: func(upid string) (proxmox.TaskStatus, error) {
if upid == restoreUPID {
return proxmox.TaskStatus{}, errors.New("restore task failed")
}
return proxmox.TaskStatus{Status: "stopped", ExitStatus: "OK"}, nil
},
}
e, j, q := newEngine(t, api, EmptyProvider{})
defer q.Close()
res := e.RunRestoreTest(context.Background(), RestoreTestSpec{
Archive: "vol", RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990009,
})
if res.Pass || res.Err == nil {
t.Fatalf("expected restore-task failure, got %+v", res)
}
if len(api.destroys) != 1 || api.destroys[0] != 990000 {
t.Fatalf("teardown must run after a LAUNCHED restore fails: %+v", api.destroys)
}
if len(j.InFlight()) != 0 {
t.Errorf("scratch entry must be terminal: %+v", j.InFlight())
}
}
// TestRunRestoreTest_AdvancesPastOccupiedBandVMID is the F2 test (campaign pool-effects): the
// first band vmid is refused "already exists" (an invisible squatter) → the test ADVANCES to the
// next band vmid and PASSES there; the squatter is never destroyed; no FAIL, no false alert.
func TestRunRestoreTest_AdvancesPastOccupiedBandVMID(t *testing.T) {
squatterRefusal := &proxmox.APIError{
StatusCode: 500, Method: "POST", Path: "/nodes/x/lxc",
Body: `{"message":"CT 990000 already exists on node 'x'\n","data":null}`,
}
api := &fakeAPI{cfg: map[int]proxmox.GuestConfig{990001: scratchCfg()}}
api.restoreFunc = func(opts proxmox.RestoreLXCOptions) (string, error) {
if opts.VMID == 990000 {
return "", squatterRefusal
}
return "", nil // 990001 restores fine (synchronous OK path)
}
e, j, q := newEngine(t, api, EmptyProvider{})
defer q.Close()
res := e.RunRestoreTest(context.Background(), RestoreTestSpec{
Archive: "vol", RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990009,
})
if res.Skipped || !res.Pass || res.Err != nil {
t.Fatalf("must advance past the squatter and PASS, got %+v", res)
}
if res.ScratchVMID != 990001 {
t.Fatalf("must have advanced to 990001, got %d", res.ScratchVMID)
}
// The squatter at 990000 must NEVER be destroyed; only the own scratch at 990001 is.
for _, d := range api.destroys {
if d == 990000 {
t.Fatalf("the squatter at 990000 must never be destroyed: destroys=%+v", api.destroys)
}
}
if len(api.destroys) != 1 || api.destroys[0] != 990001 {
t.Fatalf("own scratch teardown expected at 990001: %+v", api.destroys)
}
if len(j.InFlight()) != 0 {
t.Errorf("all entries must be terminal: %+v", j.InFlight())
}
}
// TestRunRestoreTest_BandFullOfSquattersSkips (F2): every band vmid is refused "already exists"
// → Skipped (NOT a FAIL — no false "backup unrestorable" alert), nothing destroyed, bounded.
func TestRunRestoreTest_BandFullOfSquattersSkips(t *testing.T) {
api := &fakeAPI{}
api.restoreFunc = func(opts proxmox.RestoreLXCOptions) (string, error) {
return "", &proxmox.APIError{StatusCode: 500, Body: "CT already exists on node 'x'"}
}
e, _, q := newEngine(t, api, EmptyProvider{})
defer q.Close()
res := e.RunRestoreTest(context.Background(), RestoreTestSpec{
Archive: "vol", RestoreStorage: "local-lvm", ScratchMin: 990000, ScratchMax: 990002,
})
if !res.Skipped || res.Err != nil || res.Pass {
t.Fatalf("a squatter-full band must SKIP (not fail), got %+v", res)
}
if len(api.destroys) != 0 {
t.Fatalf("nothing may be destroyed: %+v", api.destroys)
}
if len(api.restores) != 3 {
t.Errorf("must have tried each band vmid exactly once (bounded), got %d", len(api.restores))
}
}
func TestRunRestoreTest_FullBandSkips(t *testing.T) {
// Whole band occupied → skipped, never run / out-of-band.
var guests []proxmox.Guest
@@ -263,15 +364,23 @@ func TestRunRestoreTest_InvalidBandErrors(t *testing.T) {
func TestPickScratchVMID(t *testing.T) {
// excludes 9999 and in-use; lowest free.
got, ok := pickScratchVMID([]proxmox.Guest{{VMID: 990000}}, 990000, 990009)
got, ok := pickScratchVMID([]proxmox.Guest{{VMID: 990000}}, 990000, 990009, nil)
if !ok || got != 990001 {
t.Errorf("pick = %d,%v want 990001,true", got, ok)
}
// full band.
full := []proxmox.Guest{{VMID: 990000}, {VMID: 990001}}
if _, ok := pickScratchVMID(full, 990000, 990001); ok {
if _, ok := pickScratchVMID(full, 990000, 990001, nil); ok {
t.Error("full band must return ok=false")
}
// the exclude set (F2 band-advance: vmids PVE reported occupied) is honored.
got, ok = pickScratchVMID(nil, 990000, 990009, map[int]bool{990000: true, 990001: true})
if !ok || got != 990002 {
t.Errorf("exclude-set pick = %d,%v want 990002,true", got, ok)
}
if _, ok := pickScratchVMID(nil, 990000, 990001, map[int]bool{990000: true, 990001: true}); ok {
t.Error("a fully-excluded band must return ok=false")
}
}
func TestWithLinkDown(t *testing.T) {
@@ -289,10 +398,11 @@ func TestWithLinkDown(t *testing.T) {
// --- recover the leaked scratch guest (the headline crash-safety test) ---
func TestRecover_LeakedScratchDestroyed(t *testing.T) {
// The scratch guest still exists at startup (agent crashed mid-test) → Recover destroys it.
// The scratch guest still exists at startup (agent crashed mid-test AFTER the restore
// launched — the entry carries the UPID = the launch proof) → Recover destroys it.
api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 990000, Status: "running"}}}
e, j, _ := newEngine(t, api, EmptyProvider{})
if err := j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpTaskRunning, At: time.Now().UTC()}); err != nil {
if err := j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, UPID: "UPID:demo:restore:990000:", State: OpTaskRunning, At: time.Now().UTC()}); err != nil {
t.Fatal(err)
}
res := e.Recover(context.Background())
@@ -312,7 +422,7 @@ func TestRecover_LeakedScratchAlreadyGone(t *testing.T) {
// idempotent clean (no destroy issued).
api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 9001, Status: "stopped"}}} // 990000 absent
e, j, _ := newEngine(t, api, EmptyProvider{})
j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpTaskRunning, At: time.Now().UTC()})
j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, UPID: "UPID:demo:restore:990000:", State: OpTaskRunning, At: time.Now().UTC()})
res := e.Recover(context.Background())
if res.ScratchClean != 1 || len(api.destroys) != 0 {
t.Fatalf("already-gone scratch must be clean with no destroy, got res=%+v destroys=%+v", res, api.destroys)
@@ -325,7 +435,7 @@ func TestRecover_LeakedScratchAlreadyGone(t *testing.T) {
func TestRecover_LeakedScratchListUnreadable(t *testing.T) {
api := &fakeAPI{listErr: errors.New("api down")}
e, j, _ := newEngine(t, api, EmptyProvider{})
j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpTaskRunning, At: time.Now().UTC()})
j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, UPID: "UPID:demo:restore:990000:", State: OpTaskRunning, At: time.Now().UTC()})
res := e.Recover(context.Background())
if res.Unresolved != 1 || len(j.InFlight()) != 1 {
t.Fatalf("unreadable list must leave the scratch in-flight for a later Recover, got res=%+v inflight=%d", res, len(j.InFlight()))
@@ -335,6 +445,26 @@ func TestRecover_LeakedScratchListUnreadable(t *testing.T) {
}
}
// TestRecover_ScratchNoUPIDAbandoned (F1c, scratch twin of the bring-up test): a crash left a
// Scratch entry with NO UPID while a guest sits at the band vmid (an invisible squatter, or a
// pre-existing guest under a broad token). Recover must ABANDON, never destroy — no journaled
// UPID ⇒ the restore-test created nothing there.
func TestRecover_ScratchNoUPIDAbandoned(t *testing.T) {
api := &fakeAPI{lxc: []proxmox.Guest{{VMID: 990000, Status: "stopped"}}} // a guest IS at the vmid
e, j, _ := newEngine(t, api, EmptyProvider{})
j.Append(JournalEntry{OpID: "scratch-990000-1", VMID: 990000, Kind: scratchKind, Scratch: true, State: OpStarted, At: time.Now().UTC()})
res := e.Recover(context.Background())
if len(api.destroys) != 0 {
t.Fatalf("a no-UPID Scratch entry must NEVER destroy the vmid: destroys=%+v", api.destroys)
}
if res.RolledBack != 1 || res.ScratchDestroyed != 0 {
t.Fatalf("entry must be abandoned via the no-UPID fail-safe path, got %+v", res)
}
if len(j.InFlight()) != 0 {
t.Errorf("abandoned entry must be terminal: %+v", j.InFlight())
}
}
// small string helpers (avoid importing strings in the test for one call).
func contains2(s, sub string) bool { return indexOf(s, sub) >= 0 }
func count(s, sub string) int {