F-REBOOT + F-LEAK: the agent's authority over guest lifecycle (v0.107.0)
F-REBOOT — a guest rebooted mid-backup never came back (fault 11: 9m47s of total appliance outage, no lock, nothing retrying). The existing stale-lock recovery is correct but missed it two ways: its predicate needs a stale vzdump lock and that guest was unlocked, and it runs only at agent startup. New periodic guest-power watchdog acts on 'should be running, is not, is not locked'. onboot is the should-be-running signal, not invented here: stalelock.go already uses it for this same decision, it is 0 on scratch/golden, and pve-guests uses it at host boot. Guards: onboot:0 never touched (Scenario B), a locked guest is left to the stale-lock path, a guest with a vzdump in flight is left stopped, unprovable ownership acts on nothing, unconfirmable backup state fails safe. Bounded retry 3x at 1/2/4m then ERROR (Scenario C) — a healthy start takes ~25s. F-LEAK — a failed restore-test could not destroy its scratch (403 VM.Allocate). It is pool membership, not privsep: VM.Allocate is granted at /pool/felhom only, and a failed restore never completes the --pool association. Fix needs NO new grant — Pool.Allocate is already held, so the teardown adopts the stranded scratch into the pool and retries the destroy. Guarded by scratchAdoptAllowed: scratch provenance AND the numeric band, both required (Scenario E). Six red-proofs across both fixes, all observed failing.
This commit is contained in:
@@ -0,0 +1,238 @@
|
||||
package localapi
|
||||
|
||||
import (
|
||||
"context"
|
||||
"sync"
|
||||
"time"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-agent/internal/proxmox"
|
||||
)
|
||||
|
||||
// F-REBOOT (Campaign 8 fault 11) — a guest that should be running and is not.
|
||||
//
|
||||
// THE OUTAGE THIS EXISTS TO KILL. A `pct reboot` issued while a vzdump was in flight completed its
|
||||
// SHUTDOWN half and never issued the start. The guest was found `stopped` with 0 containers, no
|
||||
// lock, and nothing retrying; it stayed down 9m47s until a human ran `pct start`. The backup itself
|
||||
// SUCCEEDED — so every alarm the appliance has was silent, because nothing was broken except that
|
||||
// the customer's entire appliance was off.
|
||||
//
|
||||
// WHY THE EXISTING RECOVERY MISSED IT. `RecoverStaleLockedGuests` (stalelock.go) already does
|
||||
// unlock → delete dangling snapshot → start iff onboot, and it is CORRECT. It missed this by two
|
||||
// gaps, both narrow:
|
||||
// - its predicate acts only on a guest holding a stale vzdump lock (`backup`/`snapshot-delete`);
|
||||
// fault 11's guest was stopped and UNLOCKED, so it returned early;
|
||||
// - it runs ONCE at agent startup, on the load-bearing invariant that a backup lock present then
|
||||
// is stale by definition. A guest that goes down while the agent is already up is never
|
||||
// re-examined.
|
||||
//
|
||||
// This watchdog closes exactly those two gaps and nothing more: it is periodic, and it acts on
|
||||
// "should be running, is not, and is not locked".
|
||||
//
|
||||
// ── THE TRAP, WHICH IS THE SAME SHAPE AS F-CRIT-1's ──────────────────────────────────────────
|
||||
//
|
||||
// A guest the operator deliberately stopped must NOT be auto-started. Fighting the operator makes
|
||||
// maintenance impossible and is worse than the outage — the same over-correction that F-CRIT-1's fix
|
||||
// had to avoid when it stopped whitelisting StateStopped.
|
||||
//
|
||||
// The distinction used is `onboot`, and it is deliberately NOT invented here:
|
||||
// - it is ALREADY the distinction stalelock.go uses for exactly this decision
|
||||
// (`if onboot && g.Status != "running"`), so the two paths cannot disagree;
|
||||
// - it is 1 on customer guests and 0 on scratch/golden guests (agent v0.101.0 sets scratch to 0);
|
||||
// - it is the same flag `pve-guests` itself consults at host boot, so the agent AGREES WITH THE
|
||||
// PLATFORM rather than maintaining a second, private definition of "should be running".
|
||||
//
|
||||
// The hub's desired-state `Run` (internal/desired) is a stronger signal and is wired, but it is
|
||||
// hub-dependent. `onboot` keeps working on a box that has lost hub contact — which is precisely when
|
||||
// an unattended appliance most needs to come back up.
|
||||
|
||||
const (
|
||||
// guestPowerInterval is how often the watchdog looks. Matches the guestnet watchdog's cadence so
|
||||
// the two guest-facing sweeps stay in step, and is far below the 9m47s outage the finding recorded.
|
||||
guestPowerInterval = 60 * time.Second
|
||||
|
||||
// guestPowerMaxAttempts bounds the retry. A guest that will not start must not be started in a
|
||||
// loop forever (Scenario C) — after this many failures the watchdog stops trying and raises it.
|
||||
guestPowerMaxAttempts = 3
|
||||
)
|
||||
|
||||
// guestPowerBackoff is the delay before each retry: 1m, 2m, 4m.
|
||||
//
|
||||
// Measured, not picked round: a healthy `pct start` of guest 9201 completed in ~25 s (observed twice
|
||||
// on 2026-07-28), so even the first 1-minute wait carries 2.4x headroom over a normal start. Three
|
||||
// attempts bound the disruption at roughly 7 minutes — inside the 9m47s outage this fixes — while
|
||||
// never becoming an unbounded loop.
|
||||
var guestPowerBackoff = []time.Duration{time.Minute, 2 * time.Minute, 4 * time.Minute}
|
||||
|
||||
// guestPowerState is one guest's recovery attempt record. In-memory on purpose, like the R-88
|
||||
// breaker: an agent restart re-attempts immediately, which is the cheap direction to fail — a
|
||||
// forgotten backoff costs one extra start attempt, whereas persisting it could carry a stale
|
||||
// "this guest won't start" verdict across the restart that fixed it.
|
||||
type guestPowerState struct {
|
||||
attempts int
|
||||
nextAt time.Time
|
||||
raised bool // the give-up fault has already been raised for this run
|
||||
}
|
||||
|
||||
// WatchGuestPower runs the guest-power sweep every guestPowerInterval until ctx is done. No-op when
|
||||
// the stale-lock controller is not wired (it supplies the ownership-proven guest list).
|
||||
func (s *Server) WatchGuestPower(ctx context.Context) {
|
||||
if s.staleLock == nil {
|
||||
return
|
||||
}
|
||||
s.logger.Info("guest-power: watchdog started", "interval", guestPowerInterval.String(),
|
||||
"max_attempts", guestPowerMaxAttempts)
|
||||
t := time.NewTicker(guestPowerInterval)
|
||||
defer t.Stop()
|
||||
for {
|
||||
select {
|
||||
case <-ctx.Done():
|
||||
return
|
||||
case <-t.C:
|
||||
s.GuestPowerTick(ctx)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// GuestPowerTick performs one sweep. Exported so a test (and a live check) can drive exactly one
|
||||
// cycle instead of waiting on the ticker.
|
||||
func (s *Server) GuestPowerTick(ctx context.Context) {
|
||||
if s.staleLock == nil {
|
||||
return
|
||||
}
|
||||
guests, err := s.staleLock.Guests(ctx)
|
||||
if err != nil {
|
||||
// Unknown ownership ⇒ do nothing. Never fall back to an unfiltered list: starting a
|
||||
// co-tenant's guest would be worse than leaving ours down.
|
||||
s.logger.Warn("guest-power: guest list unavailable — skipping sweep (ownership unproven)", "err", err)
|
||||
return
|
||||
}
|
||||
for _, g := range guests {
|
||||
if ctx.Err() != nil {
|
||||
return
|
||||
}
|
||||
s.recoverOneStoppedGuest(ctx, g)
|
||||
}
|
||||
}
|
||||
|
||||
// recoverOneStoppedGuest starts a single guest that should be running and is not.
|
||||
func (s *Server) recoverOneStoppedGuest(ctx context.Context, g proxmox.Guest) {
|
||||
if g.Status == "running" {
|
||||
s.forgetGuestPower(g.VMID) // healthy again: clear any attempt history
|
||||
return
|
||||
}
|
||||
|
||||
lock, onboot, err := s.staleLock.Lock(ctx, g.VMID)
|
||||
if err != nil {
|
||||
s.logger.Warn("guest-power: read guest config failed — skipping", "vmid", g.VMID, "err", err)
|
||||
return
|
||||
}
|
||||
|
||||
// SCENARIO B — a deliberately stopped guest is left alone, forever. onboot:0 means the operator
|
||||
// (or the golden-image provisioning) does not want this guest running.
|
||||
if !onboot {
|
||||
return
|
||||
}
|
||||
|
||||
// A locked guest belongs to another operation, mid-flight or stale. The stale-lock recovery owns
|
||||
// that case and knows how to prove a lock is stale; this watchdog must not race it or start a
|
||||
// guest whose lock means "a restore is writing my disks right now".
|
||||
if lock != "" {
|
||||
s.logger.Info("guest-power: guest is stopped but LOCKED — leaving it to the stale-lock path",
|
||||
"vmid", g.VMID, "lock", lock)
|
||||
return
|
||||
}
|
||||
|
||||
// Never start a guest while a vzdump is genuinely in flight for it — a stop-mode backup stops the
|
||||
// guest ON PURPOSE and starting it underneath would corrupt the backup. Fail safe on doubt.
|
||||
running, err := s.staleLock.BackupRunning(ctx, g.VMID)
|
||||
if err != nil {
|
||||
s.logger.Warn("guest-power: could not confirm no backup is running — NOT starting (fail-safe)",
|
||||
"vmid", g.VMID, "err", err)
|
||||
return
|
||||
}
|
||||
if running {
|
||||
s.logger.Info("guest-power: a vzdump is in flight — leaving the guest stopped until it finishes",
|
||||
"vmid", g.VMID)
|
||||
return
|
||||
}
|
||||
|
||||
st, due := s.guestPowerDue(g.VMID)
|
||||
if !due {
|
||||
return
|
||||
}
|
||||
if st.attempts >= guestPowerMaxAttempts {
|
||||
// SCENARIO C — bounded. Raise it ONCE and stop retrying; an infinite silent retry loop is the
|
||||
// over-correction here, and a guest that has refused three starts needs a human, not a fourth.
|
||||
if !st.raised {
|
||||
s.markGuestPowerRaised(g.VMID)
|
||||
s.logger.Error("guest-power: GIVING UP — guest should be running (onboot) but failed to start after repeated attempts; it needs operator attention",
|
||||
"vmid", g.VMID, "attempts", st.attempts, "status", g.Status)
|
||||
}
|
||||
return
|
||||
}
|
||||
|
||||
s.logger.Warn("guest-power: guest should be running (onboot) but is stopped and unlocked — starting it",
|
||||
"vmid", g.VMID, "status", g.Status, "attempt", st.attempts+1, "of", guestPowerMaxAttempts)
|
||||
if err := s.staleLock.Start(ctx, g.VMID); err != nil {
|
||||
s.noteGuestPowerFailure(g.VMID)
|
||||
s.logger.Error("guest-power: start failed", "vmid", g.VMID, "attempt", st.attempts+1, "err", err)
|
||||
return
|
||||
}
|
||||
s.forgetGuestPower(g.VMID)
|
||||
s.logger.Warn("guest-power: STARTED a guest that should have been running", "vmid", g.VMID)
|
||||
}
|
||||
|
||||
// ---- attempt bookkeeping (guarded by its own mutex; independent of the jobs lock) ----------
|
||||
|
||||
var guestPowerMu sync.Mutex
|
||||
|
||||
// guestPowerDue reports the guest's attempt state and whether a new attempt is due now.
|
||||
func (s *Server) guestPowerDue(vmid int) (guestPowerState, bool) {
|
||||
guestPowerMu.Lock()
|
||||
defer guestPowerMu.Unlock()
|
||||
if s.guestPower == nil {
|
||||
s.guestPower = map[int]guestPowerState{}
|
||||
}
|
||||
st := s.guestPower[vmid]
|
||||
if st.nextAt.IsZero() || !s.now().Before(st.nextAt) {
|
||||
return st, true
|
||||
}
|
||||
return st, false
|
||||
}
|
||||
|
||||
// noteGuestPowerFailure records a failed start and arms the next backoff.
|
||||
func (s *Server) noteGuestPowerFailure(vmid int) {
|
||||
guestPowerMu.Lock()
|
||||
defer guestPowerMu.Unlock()
|
||||
if s.guestPower == nil {
|
||||
s.guestPower = map[int]guestPowerState{}
|
||||
}
|
||||
st := s.guestPower[vmid]
|
||||
st.attempts++
|
||||
i := st.attempts - 1
|
||||
if i >= len(guestPowerBackoff) {
|
||||
i = len(guestPowerBackoff) - 1
|
||||
}
|
||||
st.nextAt = s.now().Add(guestPowerBackoff[i])
|
||||
s.guestPower[vmid] = st
|
||||
}
|
||||
|
||||
// markGuestPowerRaised records that the give-up fault has been raised, so it is logged once.
|
||||
func (s *Server) markGuestPowerRaised(vmid int) {
|
||||
guestPowerMu.Lock()
|
||||
defer guestPowerMu.Unlock()
|
||||
st := s.guestPower[vmid]
|
||||
st.raised = true
|
||||
s.guestPower[vmid] = st
|
||||
}
|
||||
|
||||
// forgetGuestPower clears a guest's attempt history — called when it is running again, so a guest
|
||||
// that recovers does not carry its old failures into the next incident.
|
||||
func (s *Server) forgetGuestPower(vmid int) {
|
||||
guestPowerMu.Lock()
|
||||
defer guestPowerMu.Unlock()
|
||||
if s.guestPower == nil {
|
||||
return
|
||||
}
|
||||
delete(s.guestPower, vmid)
|
||||
}
|
||||
@@ -0,0 +1,239 @@
|
||||
package localapi
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"log/slog"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"gitea.dooplex.hu/admin/felhom-agent/internal/proxmox"
|
||||
)
|
||||
|
||||
// F-REBOOT (Campaign 8 fault 11): a guest rebooted mid-backup never came back — stopped, unlocked,
|
||||
// nothing retrying, 9m47s of total appliance outage.
|
||||
//
|
||||
// Scenario A (a crashed guest is restarted), B (a deliberately stopped guest is left alone) and
|
||||
// C (bounded retry, then escalate). B and C are what make A safe.
|
||||
|
||||
type fakeGuestPowerCtl struct {
|
||||
guests []proxmox.Guest
|
||||
guestsErr error
|
||||
locks map[int]string // vmid -> lock ("" = unlocked)
|
||||
onboot map[int]bool
|
||||
backupRun map[int]bool
|
||||
backupErr error
|
||||
started []int
|
||||
startErr error
|
||||
}
|
||||
|
||||
func (f *fakeGuestPowerCtl) Guests(context.Context) ([]proxmox.Guest, error) {
|
||||
return f.guests, f.guestsErr
|
||||
}
|
||||
func (f *fakeGuestPowerCtl) Lock(_ context.Context, vmid int) (string, bool, error) {
|
||||
return f.locks[vmid], f.onboot[vmid], nil
|
||||
}
|
||||
func (f *fakeGuestPowerCtl) BackupRunning(_ context.Context, vmid int) (bool, error) {
|
||||
return f.backupRun[vmid], f.backupErr
|
||||
}
|
||||
func (f *fakeGuestPowerCtl) HasVzdumpSnapshot(context.Context, int) (bool, error) { return false, nil }
|
||||
func (f *fakeGuestPowerCtl) Unlock(context.Context, int) error { return nil }
|
||||
func (f *fakeGuestPowerCtl) DeleteVzdumpSnapshot(context.Context, int) error { return nil }
|
||||
func (f *fakeGuestPowerCtl) Start(_ context.Context, vmid int) error {
|
||||
f.started = append(f.started, vmid)
|
||||
return f.startErr
|
||||
}
|
||||
|
||||
func gpServer(t *testing.T, ctl StaleLockController, now func() time.Time) *Server {
|
||||
t.Helper()
|
||||
s := &Server{staleLock: ctl, logger: slog.New(slog.NewTextHandler(discardW{}, nil))}
|
||||
if now != nil {
|
||||
s.now = now
|
||||
} else {
|
||||
s.now = func() time.Time { return time.Now().UTC() }
|
||||
}
|
||||
return s
|
||||
}
|
||||
|
||||
type discardW struct{}
|
||||
|
||||
func (discardW) Write(p []byte) (int, error) { return len(p), nil }
|
||||
|
||||
// Scenario A — a guest that should be running (onboot) and is stopped-and-unlocked IS started.
|
||||
//
|
||||
// RED-PROOF: delete the `s.staleLock.Start(...)` call in recoverOneStoppedGuest (or make the whole
|
||||
// function return before it) → started is empty and this fails with
|
||||
// "guest 9201 was NOT started — this is F-REBOOT".
|
||||
func TestGuestPower_StoppedOnbootGuestIsStarted(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
|
||||
locks: map[int]string{9201: ""},
|
||||
onboot: map[int]bool{9201: true},
|
||||
}
|
||||
s := gpServer(t, ctl, nil)
|
||||
|
||||
s.GuestPowerTick(context.Background())
|
||||
|
||||
if len(ctl.started) != 1 || ctl.started[0] != 9201 {
|
||||
t.Fatalf("guest 9201 was NOT started — this is F-REBOOT (started=%v)", ctl.started)
|
||||
}
|
||||
}
|
||||
|
||||
// Scenario B — a DELIBERATELY stopped guest (onboot:0) is never started. This is the trap: fighting
|
||||
// the operator makes maintenance impossible and is worse than the outage being fixed.
|
||||
//
|
||||
// RED-PROOF: remove the `if !onboot { return }` guard → the golden/scratch guest is started and this
|
||||
// fails with "a deliberately stopped guest (onboot:0) was started".
|
||||
func TestGuestPower_DeliberatelyStoppedGuestIsLeftAlone(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9100, Status: "stopped"}, {VMID: 990000, Status: "stopped"}},
|
||||
locks: map[int]string{9100: "", 990000: ""},
|
||||
onboot: map[int]bool{9100: false, 990000: false}, // golden + scratch
|
||||
}
|
||||
s := gpServer(t, ctl, nil)
|
||||
|
||||
s.GuestPowerTick(context.Background())
|
||||
|
||||
if len(ctl.started) != 0 {
|
||||
t.Errorf("a deliberately stopped guest (onboot:0) was started: %v", ctl.started)
|
||||
}
|
||||
}
|
||||
|
||||
// A LOCKED stopped guest belongs to the stale-lock path, which knows how to prove a lock is stale.
|
||||
// This watchdog must not race it.
|
||||
func TestGuestPower_LockedGuestIsLeftToTheStaleLockPath(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
|
||||
locks: map[int]string{9201: "snapshot-delete"},
|
||||
onboot: map[int]bool{9201: true},
|
||||
}
|
||||
s := gpServer(t, ctl, nil)
|
||||
s.GuestPowerTick(context.Background())
|
||||
if len(ctl.started) != 0 {
|
||||
t.Errorf("started a LOCKED guest: %v — that races the stale-lock recovery", ctl.started)
|
||||
}
|
||||
}
|
||||
|
||||
// A guest whose vzdump is genuinely in flight must be left stopped — a stop-mode backup stops the
|
||||
// guest ON PURPOSE, and starting it underneath would corrupt the backup.
|
||||
func TestGuestPower_InFlightBackupIsNotDisturbed(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
|
||||
locks: map[int]string{9201: ""},
|
||||
onboot: map[int]bool{9201: true},
|
||||
backupRun: map[int]bool{9201: true},
|
||||
}
|
||||
s := gpServer(t, ctl, nil)
|
||||
s.GuestPowerTick(context.Background())
|
||||
if len(ctl.started) != 0 {
|
||||
t.Errorf("started a guest with a vzdump in flight: %v", ctl.started)
|
||||
}
|
||||
}
|
||||
|
||||
// Fail-safe: if we cannot confirm no backup is running, do NOT start.
|
||||
func TestGuestPower_UnconfirmableBackupFailsSafe(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
|
||||
locks: map[int]string{9201: ""},
|
||||
onboot: map[int]bool{9201: true},
|
||||
backupErr: errors.New("task list unavailable"),
|
||||
}
|
||||
s := gpServer(t, ctl, nil)
|
||||
s.GuestPowerTick(context.Background())
|
||||
if len(ctl.started) != 0 {
|
||||
t.Errorf("started despite being unable to confirm no backup is running: %v", ctl.started)
|
||||
}
|
||||
}
|
||||
|
||||
// Unknown ownership ⇒ act on nothing. Starting a co-tenant's guest is worse than leaving ours down.
|
||||
func TestGuestPower_UnprovenOwnershipActsOnNothing(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{guestsErr: errors.New("pool read failed")}
|
||||
s := gpServer(t, ctl, nil)
|
||||
s.GuestPowerTick(context.Background())
|
||||
if len(ctl.started) != 0 {
|
||||
t.Errorf("acted with unproven ownership: %v", ctl.started)
|
||||
}
|
||||
}
|
||||
|
||||
// Scenario C — bounded retry, then escalate. A guest that will not start must NOT be retried forever.
|
||||
//
|
||||
// RED-PROOF: remove the `if st.attempts >= guestPowerMaxAttempts` branch → the sweep keeps starting
|
||||
// on every tick and this fails with "start attempted N times, want at most 3 — infinite retry loop".
|
||||
func TestGuestPower_RetryIsBoundedThenEscalates(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
|
||||
locks: map[int]string{9201: ""},
|
||||
onboot: map[int]bool{9201: true},
|
||||
startErr: errors.New("cannot start: storage offline"),
|
||||
}
|
||||
base := time.Now().UTC()
|
||||
now := base
|
||||
s := gpServer(t, ctl, func() time.Time { return now })
|
||||
|
||||
// Drive many ticks, advancing well past every backoff each time.
|
||||
for i := 0; i < 12; i++ {
|
||||
s.GuestPowerTick(context.Background())
|
||||
now = now.Add(10 * time.Minute)
|
||||
}
|
||||
|
||||
if len(ctl.started) > guestPowerMaxAttempts {
|
||||
t.Errorf("start attempted %d times, want at most %d — this is an infinite retry loop",
|
||||
len(ctl.started), guestPowerMaxAttempts)
|
||||
}
|
||||
if len(ctl.started) != guestPowerMaxAttempts {
|
||||
t.Errorf("start attempted %d times, want exactly %d before giving up", len(ctl.started), guestPowerMaxAttempts)
|
||||
}
|
||||
}
|
||||
|
||||
// The backoff must actually hold a retry back — otherwise "bounded" is only bounded by luck.
|
||||
func TestGuestPower_BackoffDefersTheNextAttempt(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
|
||||
locks: map[int]string{9201: ""},
|
||||
onboot: map[int]bool{9201: true},
|
||||
startErr: errors.New("boom"),
|
||||
}
|
||||
base := time.Now().UTC()
|
||||
now := base
|
||||
s := gpServer(t, ctl, func() time.Time { return now })
|
||||
|
||||
s.GuestPowerTick(context.Background()) // attempt 1, arms a 1m backoff
|
||||
if len(ctl.started) != 1 {
|
||||
t.Fatalf("precondition: want 1 attempt, got %d", len(ctl.started))
|
||||
}
|
||||
now = base.Add(30 * time.Second) // still inside the 1m backoff
|
||||
s.GuestPowerTick(context.Background())
|
||||
if len(ctl.started) != 1 {
|
||||
t.Errorf("retried inside the backoff window (%d attempts) — the bound is not being honoured", len(ctl.started))
|
||||
}
|
||||
now = base.Add(90 * time.Second) // past it
|
||||
s.GuestPowerTick(context.Background())
|
||||
if len(ctl.started) != 2 {
|
||||
t.Errorf("did not retry after the backoff lapsed (%d attempts)", len(ctl.started))
|
||||
}
|
||||
}
|
||||
|
||||
// A guest that comes back healthy must lose its attempt history, so it does not carry old failures
|
||||
// into the next incident.
|
||||
func TestGuestPower_RunningGuestClearsAttemptHistory(t *testing.T) {
|
||||
ctl := &fakeGuestPowerCtl{
|
||||
guests: []proxmox.Guest{{VMID: 9201, Status: "stopped"}},
|
||||
locks: map[int]string{9201: ""},
|
||||
onboot: map[int]bool{9201: true},
|
||||
startErr: errors.New("boom"),
|
||||
}
|
||||
base := time.Now().UTC()
|
||||
now := base
|
||||
s := gpServer(t, ctl, func() time.Time { return now })
|
||||
s.GuestPowerTick(context.Background())
|
||||
|
||||
if _, due := s.guestPowerDue(9201); due {
|
||||
t.Error("precondition: a backoff should be armed after a failed start")
|
||||
}
|
||||
// it comes back up
|
||||
ctl.guests = []proxmox.Guest{{VMID: 9201, Status: "running"}}
|
||||
s.GuestPowerTick(context.Background())
|
||||
if _, due := s.guestPowerDue(9201); !due {
|
||||
t.Error("attempt history survived the guest coming back healthy")
|
||||
}
|
||||
}
|
||||
@@ -263,7 +263,10 @@ type Server struct {
|
||||
guestBinds *GuestBindStore // F9 startup bind re-assert record (optional)
|
||||
formatJobs *FormatJobStore // F20-BUG3 detached-format job record (optional)
|
||||
staleLock StaleLockController // F2-b startup stale-lock recovery (optional)
|
||||
host storage.HostReader // role classification source (optional; defaults to ProcHostReader)
|
||||
// guestPower (F-REBOOT) is per-guest start-attempt state for the guest-power watchdog.
|
||||
// Guarded by guestPowerMu in guestpower.go; in-memory on purpose (see guestPowerState).
|
||||
guestPower map[int]guestPowerState
|
||||
host storage.HostReader // role classification source (optional; defaults to ProcHostReader)
|
||||
|
||||
hostMetrics HostMetricsProvider // slice 9 (optional)
|
||||
hostID string // slice 10B: for the data-bearing-format pending-op hint
|
||||
|
||||
@@ -198,7 +198,7 @@ func (e *Engine) runScratchTest(ctx context.Context, vmid int, spec RestoreTestS
|
||||
launched := false
|
||||
defer func() {
|
||||
if launched {
|
||||
e.teardownScratch(ctx, base)
|
||||
e.teardownScratch(ctx, base, spec.ScratchMin, spec.ScratchMax)
|
||||
return
|
||||
}
|
||||
e.append(withState(base, OpFailed))
|
||||
@@ -444,9 +444,44 @@ func sizeToGB(s string) int {
|
||||
return gb
|
||||
}
|
||||
|
||||
// scratchAdoptAllowed decides whether a stranded guest may be adopted into the felhom pool so the
|
||||
// pool-scoped token can destroy it (F-LEAK). PURE, so the refusal is unit-testable without PVE.
|
||||
//
|
||||
// TWO INDEPENDENT GUARDS, both required. This function is the only thing standing between "clean up
|
||||
// my own scratch" and "co-opt an arbitrary guest into the pool and delete it", so it does not rely
|
||||
// on either check alone:
|
||||
// - PROVENANCE: the journal entry must be one the agent itself created as a restore-test scratch.
|
||||
// - NUMERIC BAND: the VMID must be inside the configured scratch band (scratch_vmid_min..max).
|
||||
//
|
||||
// A guest failing either is refused, loudly. Adopting a customer guest into the pool would hand the
|
||||
// token destroy rights over it, which is a far worse outcome than a leaked scratch.
|
||||
func scratchAdoptAllowed(vmid, min, max int, scratch bool) (bool, string) {
|
||||
if !scratch {
|
||||
return false, "journal entry is not agent-created scratch provenance"
|
||||
}
|
||||
if min <= 0 || max < min {
|
||||
return false, fmt.Sprintf("scratch band [%d,%d] is not configured", min, max)
|
||||
}
|
||||
if vmid < min || vmid > max {
|
||||
return false, fmt.Sprintf("vmid %d is outside the scratch band [%d,%d]", vmid, min, max)
|
||||
}
|
||||
return true, ""
|
||||
}
|
||||
|
||||
// teardownScratch destroys the scratch guest (benign, gated) and records the entry terminal.
|
||||
// On any teardown failure it leaves the entry in-flight so Recover reaps the guest later.
|
||||
func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry) {
|
||||
//
|
||||
// F-LEAK (Campaign 8): a restore-test whose RESTORE FAILED left a scratch guest the agent could not
|
||||
// destroy — `DELETE /nodes/x/lxc/990000` returned 403 "missing privilege VM.Allocate". The cause is
|
||||
// pool membership, not privsep: VM.Allocate is granted at /pool/felhom ONLY (never at /), and a
|
||||
// failed restore never completes the `--pool felhom` association, so the guest's own path resolves
|
||||
// to / where the token holds nothing. Verified live: /vms/<non-member> grants only
|
||||
// Datastore.Audit+SDN.Use+Sys.Audit, while /pool/felhom grants VM.Allocate AND Pool.Allocate.
|
||||
//
|
||||
// So the recovery needs NO new privilege: Pool.Allocate is already held, so we adopt the stranded
|
||||
// scratch into the pool and retry the destroy, which then authorizes via /pool/felhom. Guarded by
|
||||
// scratchAdoptAllowed — see there for why two guards rather than one.
|
||||
func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry, scratchMin, scratchMax int) {
|
||||
// Cancel-immune + bounded, so a shutdown mid-test still tears down.
|
||||
tctx, cancel := context.WithTimeout(context.WithoutCancel(ctx), 2*time.Minute)
|
||||
defer cancel()
|
||||
@@ -459,8 +494,28 @@ func (e *Engine) teardownScratch(ctx context.Context, base JournalEntry) {
|
||||
}
|
||||
upid, err := e.api.DestroyLXC(tctx, base.VMID)
|
||||
if err != nil {
|
||||
e.logger.Error("restore-test: scratch teardown failed; left for Recover", "vmid", base.VMID, "err", err)
|
||||
return
|
||||
// F-LEAK: the destroy may have 403'd because a FAILED restore never completed pool
|
||||
// membership, leaving the guest outside /pool/felhom where the token's VM.Allocate lives.
|
||||
// Adopt it into the pool (Pool.Allocate, already granted) and retry ONCE. Any other error
|
||||
// falls through to the original behaviour.
|
||||
if ok, why := scratchAdoptAllowed(base.VMID, scratchMin, scratchMax, base.Scratch); !ok {
|
||||
e.logger.Error("restore-test: scratch teardown failed and adoption REFUSED; left for Recover",
|
||||
"vmid", base.VMID, "refused_because", why, "err", err)
|
||||
return
|
||||
}
|
||||
e.logger.Warn("restore-test: scratch teardown failed — adopting the stranded scratch into the pool and retrying once",
|
||||
"vmid", base.VMID, "pool", DefaultPool, "err", err)
|
||||
if perr := e.api.PoolAddVMID(tctx, DefaultPool, base.VMID); perr != nil {
|
||||
e.logger.Error("restore-test: pool adoption failed; left for Recover", "vmid", base.VMID, "err", perr)
|
||||
return
|
||||
}
|
||||
upid, err = e.api.DestroyLXC(tctx, base.VMID)
|
||||
if err != nil {
|
||||
e.logger.Error("restore-test: scratch teardown failed even after pool adoption; left for Recover",
|
||||
"vmid", base.VMID, "err", err)
|
||||
return
|
||||
}
|
||||
e.logger.Info("restore-test: stranded scratch adopted into the pool and destroyed", "vmid", base.VMID)
|
||||
}
|
||||
if upid != "" {
|
||||
if _, err := e.api.WaitTask(tctx, upid, proxmox.WaitOptions{}); err != nil {
|
||||
|
||||
@@ -0,0 +1,71 @@
|
||||
package reconcile
|
||||
|
||||
import "testing"
|
||||
|
||||
// F-LEAK (Campaign 8): a restore-test whose RESTORE FAILED could not destroy its own scratch guest —
|
||||
// `DELETE /nodes/x/lxc/990000` → 403 "missing privilege VM.Allocate". The cause is pool membership,
|
||||
// not privsep: VM.Allocate is granted at /pool/felhom only, and a failed restore never completes the
|
||||
// `--pool felhom` association, so the guest's own path resolves to / where the token holds nothing.
|
||||
//
|
||||
// The fix adopts the stranded scratch into the pool (Pool.Allocate — already granted) and retries the
|
||||
// destroy. scratchAdoptAllowed is the guard that keeps that from becoming "co-opt any guest into the
|
||||
// pool and delete it", and this file is its mirror test.
|
||||
//
|
||||
// Scenario D (the scratch is destroyed) is the live replay; Scenario E — the agent still cannot reach
|
||||
// a non-scratch guest this way — is HERE, because it must hold as a pure property and not depend on
|
||||
// what PVE happens to refuse.
|
||||
|
||||
// Scenario E — the adoption path REFUSES anything outside the scratch band.
|
||||
//
|
||||
// RED-PROOF: replace the band check with `return true, ""` → every case below reports allowed and the
|
||||
// test fails with "adoption ALLOWED for vmid 9201 — that would let the agent co-opt a customer guest
|
||||
// into the pool and destroy it".
|
||||
func TestScratchAdoptAllowed_RefusesNonScratch(t *testing.T) {
|
||||
const min, max = 990000, 990009
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
vmid int
|
||||
scratch bool
|
||||
}{
|
||||
{"a live customer guest", 9201, true}, // scratch-flagged but OUTSIDE the band
|
||||
{"the golden image", 9100, true}, // ditto
|
||||
{"just below the band", min - 1, true}, // off-by-one
|
||||
{"just above the band", max + 1, true}, // off-by-one
|
||||
{"in-band but NOT scratch provenance", min, false},
|
||||
{"neither", 100, false},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
ok, why := scratchAdoptAllowed(tc.vmid, min, max, tc.scratch)
|
||||
if ok {
|
||||
t.Errorf("adoption ALLOWED for vmid %d — that would let the agent co-opt a non-scratch guest into the pool and destroy it", tc.vmid)
|
||||
}
|
||||
if why == "" {
|
||||
t.Error("refusal carried no reason — a silent refusal is unreviewable")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// The in-band, scratch-provenanced case IS allowed — otherwise the fix does nothing and the leak stays.
|
||||
//
|
||||
// RED-PROOF: make scratchAdoptAllowed always return false → this fails with "adoption refused for a
|
||||
// genuine in-band scratch guest", i.e. F-LEAK is not fixed at all.
|
||||
func TestScratchAdoptAllowed_AllowsGenuineScratch(t *testing.T) {
|
||||
const min, max = 990000, 990009
|
||||
for _, vmid := range []int{min, min + 5, max} {
|
||||
ok, why := scratchAdoptAllowed(vmid, min, max, true)
|
||||
if !ok {
|
||||
t.Errorf("adoption refused for a genuine in-band scratch guest %d: %s", vmid, why)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// An unconfigured band must refuse everything rather than defaulting to something permissive — a
|
||||
// zero-valued band is a wiring bug, and the safe reading of a wiring bug is "do nothing".
|
||||
func TestScratchAdoptAllowed_UnconfiguredBandRefuses(t *testing.T) {
|
||||
for _, tc := range []struct{ min, max int }{{0, 0}, {0, 990009}, {990009, 990000}, {-1, 5}} {
|
||||
if ok, _ := scratchAdoptAllowed(990000, tc.min, tc.max, true); ok {
|
||||
t.Errorf("adoption allowed with an unconfigured band [%d,%d]", tc.min, tc.max)
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user