From 5134b39800e2b0fa0a728e397ed1306e76c6c7ca Mon Sep 17 00:00:00 2001 From: rcourtman Date: Thu, 4 Jun 2026 09:00:30 +0100 Subject: [PATCH] Keep snapshot polling independent of backup scans (#1437) Back-port v5 fix 0dca8a037 to v6. Extract pollPVEBackupsAndSnapshots: the two backup-inventory scans share a bounded backupCtx, which is then cancelled, and pollGuestSnapshots runs on the parent context. Because the parent has no deadline, pollGuestSnapshots establishes its own 60s-4min budget instead of inheriting an already-exhausted backup deadline and skipping entirely (the v6 early-return at monitor_backups.go made this strictly worse). Adds a regression test proving snapshots still poll after the storage scan exhausts its budget. --- internal/monitoring/monitor_backups.go | 45 ++++++++--- internal/monitoring/monitor_snapshot_test.go | 78 ++++++++++++++++++++ 2 files changed, 112 insertions(+), 11 deletions(-) diff --git a/internal/monitoring/monitor_backups.go b/internal/monitoring/monitor_backups.go index 857739bf0..2546edc11 100644 --- a/internal/monitoring/monitor_backups.go +++ b/internal/monitoring/monitor_backups.go @@ -698,6 +698,39 @@ func (m *Monitor) calculateBackupOperationTimeout(instanceName string) time.Dura return timeout } +// pollPVEBackupsAndSnapshots runs the two backup-inventory scans under a shared +// bounded budget, then polls guest snapshots on the parent context so they get +// their own independent budget. A slow storage/backup scan can no longer starve +// snapshot discovery by exhausting the shared timeout before snapshots run. +func (m *Monitor) pollPVEBackupsAndSnapshots(parentCtx context.Context, instanceName string, client PVEClientInterface, nodes []proxmox.Node, nodeEffectiveStatus map[string]string, timeout time.Duration) { + if parentCtx == nil { + parentCtx = context.Background() + } + + backupCtx, cancel := context.WithTimeout(parentCtx, timeout) + + // Poll backup tasks + m.pollBackupTasks(backupCtx, instanceName, client) + + // Poll storage backups - pass nodes to avoid duplicate API calls + m.pollStorageBackupsWithNodes(backupCtx, instanceName, client, nodes, nodeEffectiveStatus) + + backupErr := backupCtx.Err() + cancel() + + if backupErr != nil && parentCtx.Err() == nil { + log.Warn(). + Str("instance", instanceName). + Err(backupErr). + Msg("Backup storage polling budget was exhausted before guest snapshot polling; continuing snapshots with their own bounded poll budget") + } + + // Snapshots are independent backup inventory. Passing parentCtx (no deadline) + // lets pollGuestSnapshots establish its own bounded budget instead of + // inheriting an already-exhausted backup deadline and skipping entirely. + m.pollGuestSnapshots(parentCtx, instanceName, client) +} + // pollGuestSnapshots polls snapshots for all VMs and containers func (m *Monitor) pollGuestSnapshots(ctx context.Context, instanceName string, client PVEClientInterface) { log.Debug().Str("instance", instanceName).Msg("polling guest snapshots") @@ -1776,17 +1809,7 @@ func (m *Monitor) pollPVEBackupsAsync( parentCtx = context.Background() } - backupCtx, cancel := context.WithTimeout(parentCtx, timeout) - defer cancel() - - // Poll backup tasks - m.pollBackupTasks(backupCtx, inst, pveClient) - - // Poll storage backups - pass nodes to avoid duplicate API calls - m.pollStorageBackupsWithNodes(backupCtx, inst, pveClient, nodes, nodeEffectiveStatus) - - // Poll guest snapshots - m.pollGuestSnapshots(backupCtx, inst, pveClient) + m.pollPVEBackupsAndSnapshots(parentCtx, inst, pveClient, nodes, nodeEffectiveStatus, timeout) duration := time.Since(startTime) log.Info(). diff --git a/internal/monitoring/monitor_snapshot_test.go b/internal/monitoring/monitor_snapshot_test.go index 7396f51aa..fdc7f2bab 100644 --- a/internal/monitoring/monitor_snapshot_test.go +++ b/internal/monitoring/monitor_snapshot_test.go @@ -55,6 +55,84 @@ func (m *mockPVEClientSnapshots) trackSnapshotConcurrency(ctx context.Context) { } } +type backupStorageTimeoutSnapshotClient struct { + mockPVEClientExtra + snapshots []proxmox.Snapshot + snapshotCalls int + storageCalls int +} + +func (m *backupStorageTimeoutSnapshotClient) GetBackupTasks(ctx context.Context) ([]proxmox.Task, error) { + return nil, nil +} + +func (m *backupStorageTimeoutSnapshotClient) GetStorage(ctx context.Context, node string) ([]proxmox.Storage, error) { + m.storageCalls++ + if m.storageCalls > 1 { + return nil, nil + } + + <-ctx.Done() + return nil, fmt.Errorf("storage scan exceeded backup inventory budget") +} + +func (m *backupStorageTimeoutSnapshotClient) GetVMSnapshots(ctx context.Context, node string, vmid int) ([]proxmox.Snapshot, error) { + m.snapshotCalls++ + if err := ctx.Err(); err != nil { + return nil, err + } + return m.snapshots, nil +} + +func (m *backupStorageTimeoutSnapshotClient) GetContainerSnapshots(ctx context.Context, node string, vmid int) ([]proxmox.Snapshot, error) { + if err := ctx.Err(); err != nil { + return nil, err + } + return nil, nil +} + +func TestMonitor_PollPVEBackupsAndSnapshots_DoesNotStarveSnapshotsAfterStorageTimeout(t *testing.T) { + m := &Monitor{state: models.NewState()} + + m.state.UpdateVMsForInstance("pve1", []models.VM{{ + ID: "qemu/100", + VMID: 100, + Node: "node1", + Instance: "pve1", + Name: "vm100", + Template: false, + }}) + + client := &backupStorageTimeoutSnapshotClient{ + snapshots: []proxmox.Snapshot{{ + Name: "snap_after_storage_timeout", + SnapTime: 4000, + Description: "created while storage scan was slow", + }}, + } + + m.pollPVEBackupsAndSnapshots( + context.Background(), + "pve1", + client, + []proxmox.Node{{Node: "node1", Status: "online"}}, + map[string]string{"node1": "online"}, + time.Millisecond, + ) + + if client.snapshotCalls == 0 { + t.Fatal("expected guest snapshot polling to run even after storage backup polling exhausted its budget") + } + + got := m.state.GetSnapshot().PVEBackups.GuestSnapshots + if len(got) != 1 { + t.Fatalf("expected one guest snapshot after storage timeout, got %#v", got) + } + if got[0].Name != "snap_after_storage_timeout" { + t.Fatalf("expected fresh snapshot after storage timeout, got %#v", got[0]) + } +} + func TestMonitor_PollGuestSnapshots_Coverage(t *testing.T) { m := &Monitor{ state: models.NewState(),