From cefc032a3779eacf90022625f567a5bdf740460f Mon Sep 17 00:00:00 2001 From: rcourtman Date: Sat, 4 Jul 2026 18:37:59 +0100 Subject: [PATCH] Fix patch-release follow-up regressions Refs #1510 Refs #1501 Refs #1507 Refs #1442 - persist scoped workloads status filters across platform navigation - derive host memory pressure from available memory - reapply system settings after every monitor reload path - bound PBS backup snapshot polling workers during large backup scans --- .../v6/internal/subsystems/monitoring.md | 10 +++ .../subsystems/performance-and-scalability.md | 4 + .../useWorkloadsControlsState.test.ts | 43 ++++++++++ .../__tests__/workloadUrlSyncModel.test.ts | 4 +- .../Workloads/useWorkloadsControlsState.ts | 37 +++++++-- internal/hostmetrics/collector.go | 31 ++++--- internal/hostmetrics/collector_test.go | 40 ++++++++++ internal/monitoring/monitor_backups.go | 80 ++++++++++++------- .../monitor_backups_readstate_test.go | 73 +++++++++++++++++ pkg/server/server.go | 19 +++-- 10 files changed, 283 insertions(+), 58 deletions(-) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 13d61ed5c..9db2f4d1c 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -27,6 +27,11 @@ Monitoring owns source freshness cadence for Proxmox, PBS, and PMG resources: the stale threshold is derived from the configured polling interval with a minimum floor, so API-facing resource status must not degrade merely because a healthy source is between normal poll cycles. +PBS backup snapshot refresh is a bounded monitoring hot path: group-level +snapshot fetches must run through the fixed worker pool in +`internal/monitoring/monitor_backups.go`, reuse cached snapshots on per-group +fetch failures, and must not allocate one goroutine or buffered result slot per +backup group in large PBS datastores. Removed host-agent reconnect blocks are identity-scoped: matching may use the canonical host ID or token-qualified machine/hostname continuity, but must never block a distinct live host by hostname alone. @@ -205,6 +210,11 @@ threshold value. guest slices as the primary source of truth. Clustered PVE snapshot polling must therefore see guests collected earlier in the same cycle before calling the Proxmox guest snapshot APIs. + PBS backup snapshot refreshes in that same file must stay bounded by the + package worker-pool constant and stream requests through workers instead of + creating one goroutine per backup group; per-group API failures may reuse + cached snapshots, but the polling loop must keep memory proportional to the + worker count rather than datastore cardinality. 12. Add or change agentless availability monitoring only through the poll-provider path. `internal/monitoring/availability_poller.go` owns ICMP, TCP, and HTTP probes, provider health, scheduler task construction, and diff --git a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md index 3611132f1..d480f6bd3 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -208,6 +208,10 @@ regression protection. URL from already-loaded row metadata and must not add per-row metadata fetches, Discovery probes, or page-local external-link logic on the table hot path. + Workload status filters remain URL-canonical. Scoped local persistence may + seed a missing `status` parameter when a platform page is revisited, but the + restore path must write the URL once with `replace` and must not force row + filtering through a separate page-local state channel. 4. Keep shared auth gating in `internal/api/router.go` cheap and local: pre-auth quick-setup and recovery routing may short-circuit on loopback/session/token checks, but they must not trigger chart, metrics, or broad persistence fan-out on the protected request hot path. Reading mutable auth configuration for CSRF bootstrap and login checks must stay a short in-memory snapshot under `config.Mu.RLock()`: local diff --git a/frontend-modern/src/components/Workloads/__tests__/useWorkloadsControlsState.test.ts b/frontend-modern/src/components/Workloads/__tests__/useWorkloadsControlsState.test.ts index ca1d7930f..bb89f2650 100644 --- a/frontend-modern/src/components/Workloads/__tests__/useWorkloadsControlsState.test.ts +++ b/frontend-modern/src/components/Workloads/__tests__/useWorkloadsControlsState.test.ts @@ -42,6 +42,7 @@ describe('useWorkloadsControlsState', () => { localStorage.clear(); setMockRouterSearch(''); navigateSpy.mockClear(); + window.history.replaceState(null, '', '/workloads'); setWideViewport(); }); @@ -92,6 +93,48 @@ describe('useWorkloadsControlsState', () => { }); }); + it('persists scoped workload status so platform pages restore it when revisited', async () => { + createRoot((dispose) => { + try { + const [showFilters, setShowFilters] = createSignal(false); + const state = useWorkloadsControlsState({ + viewMode: () => 'all' as ViewMode, + showFilters, + setShowFilters, + statusModeStorageScope: 'proxmox', + }); + + state.setStatusMode('running'); + + expect(mockRouterSearch()).toBe('?status=running'); + expect(localStorage.getItem('workloadsStatusMode:proxmox')).toBe('running'); + } finally { + dispose(); + } + }); + + setMockRouterSearch(''); + window.history.replaceState(null, '', '/workloads'); + navigateSpy.mockClear(); + + const disposeRestore = createRoot((dispose) => { + const [showFilters, setShowFilters] = createSignal(false); + useWorkloadsControlsState({ + viewMode: () => 'all' as ViewMode, + showFilters, + setShowFilters, + statusModeStorageScope: 'proxmox', + }); + return dispose; + }); + + await Promise.resolve(); + + expect(navigateSpy).toHaveBeenCalledWith('/workloads?status=running', { replace: true }); + expect(mockRouterSearch()).toBe('?status=running'); + disposeRestore(); + }); + it('lets Docker scope use a container-native column profile without hiding disk globally', () => { createRoot((dispose) => { try { diff --git a/frontend-modern/src/components/Workloads/__tests__/workloadUrlSyncModel.test.ts b/frontend-modern/src/components/Workloads/__tests__/workloadUrlSyncModel.test.ts index 244091d48..e8a5de58f 100644 --- a/frontend-modern/src/components/Workloads/__tests__/workloadUrlSyncModel.test.ts +++ b/frontend-modern/src/components/Workloads/__tests__/workloadUrlSyncModel.test.ts @@ -93,7 +93,7 @@ describe('workloadUrlSyncModel', () => { expect( resolveWorkloadsManagedWorkloadsNavigateTarget({ currentPathname: '/proxmox/overview', - currentSearch: '?resource=guest-1&type=vm&agent=node-a', + currentSearch: '?resource=guest-1&status=running&type=vm&agent=node-a', viewMode: 'pod', effectiveViewMode: 'pod', containerRuntime: 'docker', @@ -105,7 +105,7 @@ describe('workloadUrlSyncModel', () => { selectedHostHint: null, }), ).toBe( - '/proxmox/overview?resource=guest-1&type=pod&platform=kubernetes&context=prod&namespace=default', + '/proxmox/overview?resource=guest-1&status=running&type=pod&platform=kubernetes&context=prod&namespace=default', ); expect( diff --git a/frontend-modern/src/components/Workloads/useWorkloadsControlsState.ts b/frontend-modern/src/components/Workloads/useWorkloadsControlsState.ts index 1b7451b8e..8cbaea829 100644 --- a/frontend-modern/src/components/Workloads/useWorkloadsControlsState.ts +++ b/frontend-modern/src/components/Workloads/useWorkloadsControlsState.ts @@ -56,6 +56,33 @@ const parseWorkloadsStatusMode = (raw: string | null | undefined): WorkloadsStat ? (raw as WorkloadsStatusMode) : DEFAULT_WORKLOADS_STATUS_MODE; +const workloadsStatusModeStorageKey = (scope: string | undefined): string => { + const trimmedScope = (scope || '').trim(); + return trimmedScope ? `workloadsStatusMode:${trimmedScope}` : 'workloadsStatusMode'; +}; + +const saveWorkloadsStatusMode = ( + scope: string | undefined, + value: WorkloadsStatusMode, +): void => { + if (typeof window === 'undefined') return; + try { + window.localStorage.setItem(workloadsStatusModeStorageKey(scope), value); + } catch { + // Ignore storage failures; the URL remains the canonical live state. + } +}; + +const readSavedWorkloadsStatusMode = (scope: string | undefined): WorkloadsStatusMode => { + if (typeof window === 'undefined') return DEFAULT_WORKLOADS_STATUS_MODE; + try { + const raw = window.localStorage.getItem(workloadsStatusModeStorageKey(scope)); + return parseWorkloadsStatusMode(raw); + } catch { + return DEFAULT_WORKLOADS_STATUS_MODE; + } +}; + export function useWorkloadsControlsState(options: WorkloadsControlsStateOptions) { const location = useLocation(); const navigate = useNavigate(); @@ -86,6 +113,7 @@ export function useWorkloadsControlsState(options: WorkloadsControlsStateOptions const statusMode: Accessor = () => parseWorkloadsStatusMode(new URLSearchParams(location.search).get('status')); const setStatusMode = (value: WorkloadsStatusMode): void => { + saveWorkloadsStatusMode(options.statusModeStorageScope, value); updateSearchParam((params) => { if (value === DEFAULT_WORKLOADS_STATUS_MODE) { params.delete('status'); @@ -99,12 +127,9 @@ export function useWorkloadsControlsState(options: WorkloadsControlsStateOptions if (typeof window === 'undefined') return; const params = new URLSearchParams(window.location.search); if (params.has('status')) return; - const scope = (options.statusModeStorageScope || '').trim(); - const legacyKey = scope ? `workloadsStatusMode:${scope}` : 'workloadsStatusMode'; - const legacy = window.localStorage.getItem(legacyKey); - const parsed = parseWorkloadsStatusMode(legacy); - if (parsed !== DEFAULT_WORKLOADS_STATUS_MODE && legacy === parsed) { - params.set('status', parsed); + const saved = readSavedWorkloadsStatusMode(options.statusModeStorageScope); + if (saved !== DEFAULT_WORKLOADS_STATUS_MODE) { + params.set('status', saved); navigate(`${window.location.pathname}?${params.toString()}`, { replace: true }); } }); diff --git a/internal/hostmetrics/collector.go b/internal/hostmetrics/collector.go index 5c89e967c..16c275a91 100644 --- a/internal/hostmetrics/collector.go +++ b/internal/hostmetrics/collector.go @@ -73,10 +73,15 @@ func Collect(ctx context.Context, diskExclude []string) (Snapshot, error) { return Snapshot{}, fmt.Errorf("memory stats: %w", err) } - usedBytes := memStats.Used freeBytes := memStats.Free + usedBytes := memStats.Used usedPercent := memStats.UsedPercent + if memStats.Total > 0 && memStats.Available > 0 && memStats.Available <= memStats.Total { + usedBytes = memStats.Total - memStats.Available + usedPercent = memoryUsagePercent(usedBytes, memStats.Total) + } + // Reclaimable page cache: Available counts the pages the kernel would hand // back under pressure on top of truly-free ones, so the gap is buff/cache. // Reported separately so the memory bar can show used | cache | free. @@ -94,15 +99,7 @@ func Collect(ctx context.Context, diskExclude []string) (Snapshot, error) { } else { usedBytes = 0 } - if memStats.Total > 0 { - usedPercent = float64(usedBytes) / float64(memStats.Total) * 100.0 - if usedPercent < 0 { - usedPercent = 0 - } - if usedPercent > 100 { - usedPercent = 100 - } - } + usedPercent = memoryUsagePercent(usedBytes, memStats.Total) // Recompute free so used + cache + free still covers the total after // the ARC pages move out of used. if memStats.Total >= usedBytes+cacheBytes { @@ -135,6 +132,20 @@ func Collect(ctx context.Context, diskExclude []string) (Snapshot, error) { return snapshot, nil } +func memoryUsagePercent(usedBytes, totalBytes uint64) float64 { + if totalBytes == 0 { + return 0 + } + percent := float64(usedBytes) / float64(totalBytes) * 100.0 + if percent < 0 { + return 0 + } + if percent > 100 { + return 100 + } + return percent +} + func collectCPUUsage(ctx context.Context) (float64, error) { percentages, err := cpuPercent(ctx, time.Second, false) if err != nil { diff --git a/internal/hostmetrics/collector_test.go b/internal/hostmetrics/collector_test.go index 4d5d3eeab..5e73a08ec 100644 --- a/internal/hostmetrics/collector_test.go +++ b/internal/hostmetrics/collector_test.go @@ -148,3 +148,43 @@ func TestCollectSplitsReclaimableCache(t *testing.T) { t.Fatalf("used+cache+free = %d exceeds total %d", sum, snapshot.Memory.TotalBytes) } } + +func TestCollectDerivesMemoryPressureFromAvailableMemory(t *testing.T) { + origVirtualMemory := virtualMemory + t.Cleanup(func() { virtualMemory = origVirtualMemory }) + + const gib = uint64(1024 * 1024 * 1024) + virtualMemory = func(ctx context.Context) (*gomem.VirtualMemoryStat, error) { + return &gomem.VirtualMemoryStat{ + Total: 16 * gib, + // Some Linux/gopsutil paths can report Used as cache-inclusive. The + // operator-facing pressure value should follow MemAvailable instead. + Used: 15 * gib, + Free: 1 * gib, + Available: 8 * gib, + UsedPercent: 93.75, + }, nil + } + + snapshot, err := Collect(context.Background(), nil) + if err != nil { + t.Fatalf("Collect failed: %v", err) + } + + if got, want := snapshot.Memory.UsedBytes, int64(8*gib); got != want { + t.Fatalf("UsedBytes = %d, want %d", got, want) + } + if got, want := snapshot.Memory.CacheBytes, int64(7*gib); got != want { + t.Fatalf("CacheBytes = %d, want %d", got, want) + } + if got, want := snapshot.Memory.FreeBytes, int64(1*gib); got != want { + t.Fatalf("FreeBytes = %d, want %d", got, want) + } + if got, want := snapshot.Memory.Usage, 50.0; got != want { + t.Fatalf("Usage = %.2f, want %.2f", got, want) + } + sum := snapshot.Memory.UsedBytes + snapshot.Memory.CacheBytes + snapshot.Memory.FreeBytes + if got, want := sum, snapshot.Memory.TotalBytes; got != want { + t.Fatalf("used+cache+free = %d, want total %d", got, want) + } +} diff --git a/internal/monitoring/monitor_backups.go b/internal/monitoring/monitor_backups.go index e3c1da293..0a99082e9 100644 --- a/internal/monitoring/monitor_backups.go +++ b/internal/monitoring/monitor_backups.go @@ -20,6 +20,8 @@ import ( "github.com/rs/zerolog/log" ) +const pbsBackupSnapshotFetchWorkers = 5 + func pveBackupTemplateSubjectKey(instance, guestType, node string, vmid int) string { return alerts.BuildBackupPVETemplateSubjectKey(instance, guestType, node, vmid) } @@ -1630,52 +1632,70 @@ func (m *Monitor) fetchPBSBackupSnapshots(ctx context.Context, client *pbs.Clien return nil } - results := make(chan []models.PBSBackup, len(requests)) - var wg sync.WaitGroup - sem := make(chan struct{}, 5) + workerCount := pbsBackupSnapshotFetchWorkers + if len(requests) < workerCount { + workerCount = len(requests) + } - for _, req := range requests { - req := req + jobs := make(chan pbsBackupFetchRequest) + results := make(chan []models.PBSBackup, workerCount) + var wg sync.WaitGroup + + for i := 0; i < workerCount; i++ { wg.Add(1) go func() { defer wg.Done() - select { - case sem <- struct{}{}: - case <-ctx.Done(): - return - } - defer func() { <-sem }() - - log.Debug(). - Str("instance", instanceName). - Str("datastore", req.datastore). - Str("namespace", req.namespace). - Str("type", req.group.BackupType). - Str("id", req.group.BackupID). - Msg("Refreshing PBS backup group") - - snapshots, err := client.ListBackupSnapshots(ctx, req.datastore, req.namespace, req.group.BackupType, req.group.BackupID) - if err != nil { - log.Error(). - Err(err). + for req := range jobs { + log.Debug(). Str("instance", instanceName). Str("datastore", req.datastore). Str("namespace", req.namespace). Str("type", req.group.BackupType). Str("id", req.group.BackupID). - Msg("Failed to list PBS backup snapshots") + Msg("Refreshing PBS backup group") - if len(req.cached.snapshots) > 0 { - results <- req.cached.snapshots + snapshots, err := client.ListBackupSnapshots(ctx, req.datastore, req.namespace, req.group.BackupType, req.group.BackupID) + if err != nil { + log.Error(). + Err(err). + Str("instance", instanceName). + Str("datastore", req.datastore). + Str("namespace", req.namespace). + Str("type", req.group.BackupType). + Str("id", req.group.BackupID). + Msg("Failed to list PBS backup snapshots") + + if len(req.cached.snapshots) > 0 { + select { + case results <- req.cached.snapshots: + case <-ctx.Done(): + } + } + continue } - return - } - results <- convertPBSSnapshots(instanceName, req.datastore, req.namespace, snapshots) + backups := convertPBSSnapshots(instanceName, req.datastore, req.namespace, snapshots) + select { + case results <- backups: + case <-ctx.Done(): + return + } + } }() } + go func() { + defer close(jobs) + for _, req := range requests { + select { + case jobs <- req: + case <-ctx.Done(): + return + } + } + }() + go func() { wg.Wait() close(results) diff --git a/internal/monitoring/monitor_backups_readstate_test.go b/internal/monitoring/monitor_backups_readstate_test.go index 205170fe0..748eaa642 100644 --- a/internal/monitoring/monitor_backups_readstate_test.go +++ b/internal/monitoring/monitor_backups_readstate_test.go @@ -3,12 +3,18 @@ package monitoring import ( "context" "fmt" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "sync/atomic" "testing" "time" "github.com/rcourtman/pulse-go-rewrite/internal/models" proxmoxmapper "github.com/rcourtman/pulse-go-rewrite/internal/recovery/mapper/proxmox" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" + "github.com/rcourtman/pulse-go-rewrite/pkg/pbs" pveapi "github.com/rcourtman/pulse-go-rewrite/pkg/proxmox" ) @@ -157,6 +163,73 @@ func TestMonitorCalculateBackupOperationTimeout_UsesCanonicalReadState(t *testin } } +func TestFetchPBSBackupSnapshotsUsesBoundedWorkerPool(t *testing.T) { + t.Parallel() + + const requestCount = 40 + + var active int64 + var maxActive int64 + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.Contains(r.URL.Path, "/admin/datastore/archive/snapshots") { + current := atomic.AddInt64(&active, 1) + defer atomic.AddInt64(&active, -1) + + for { + previous := atomic.LoadInt64(&maxActive) + if current <= previous || atomic.CompareAndSwapInt64(&maxActive, previous, current) { + break + } + } + + time.Sleep(10 * time.Millisecond) + + backupID := r.URL.Query().Get("backup-id") + _, _ = w.Write([]byte(fmt.Sprintf( + `{"data":[{"backup-type":"vm","backup-id":%q,"backup-time":1700000000}]}`, + backupID, + ))) + return + } + http.NotFound(w, r) + })) + defer server.Close() + + client, err := pbs.NewClient(pbs.ClientConfig{ + Host: server.URL, + TokenName: "root@pam!token", + TokenValue: "secret", + }) + if err != nil { + t.Fatalf("failed to create PBS client: %v", err) + } + + requests := make([]pbsBackupFetchRequest, 0, requestCount) + for i := 0; i < requestCount; i++ { + backupID := strconv.Itoa(1000 + i) + requests = append(requests, pbsBackupFetchRequest{ + datastore: "archive", + group: pbs.BackupGroup{ + BackupType: "vm", + BackupID: backupID, + LastBackup: 1700000000, + BackupCount: 1, + }, + }) + } + + m := &Monitor{} + backups := m.fetchPBSBackupSnapshots(context.Background(), client, "pbs1", requests) + if len(backups) != requestCount { + t.Fatalf("expected %d fetched backups, got %d", requestCount, len(backups)) + } + + if got := atomic.LoadInt64(&maxActive); got > int64(pbsBackupSnapshotFetchWorkers) { + t.Fatalf("expected at most %d concurrent PBS snapshot fetches, saw %d", pbsBackupSnapshotFetchWorkers, got) + } +} + func TestMonitorPollGuestSnapshots_UsesCanonicalReadState(t *testing.T) { m := &Monitor{ state: models.NewState(), diff --git a/pkg/server/server.go b/pkg/server/server.go index 7c04df48b..de4764ff2 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -402,10 +402,7 @@ func Run(ctx context.Context, version string) error { // Initialize API server with reload function var router *api.Router - reloadFunc := func() error { - if err := reloadableMonitor.Reload(); err != nil { - return err - } + refreshRouterAfterMonitorReload := func() { if router != nil { router.SetMonitor(reloadableMonitor.GetMonitor()) router.SetMultiTenantMonitor(reloadableMonitor.GetMultiTenantMonitor()) @@ -418,6 +415,12 @@ func Run(ctx context.Context, version string) error { // are lost on every monitor reload triggered by auto-registration. router.ReloadSystemSettings() } + } + reloadFunc := func() error { + if err := reloadableMonitor.Reload(); err != nil { + return err + } + refreshRouterAfterMonitorReload() return nil } router = api.NewRouter(cfg, reloadableMonitor.GetMonitor(), reloadableMonitor.GetMultiTenantMonitor(), wsHub, reloadFunc, version) @@ -627,12 +630,8 @@ func Run(ctx context.Context, version string) error { log.Info().Msg(".env mock settings changed, reloading monitor") if err := reloadableMonitor.Reload(); err != nil { log.Error().Err(err).Msg("Failed to reload monitor after .env mock setting change") - } else if router != nil { - router.SetMonitor(reloadableMonitor.GetMonitor()) - router.SetMultiTenantMonitor(reloadableMonitor.GetMultiTenantMonitor()) - if cfg := reloadableMonitor.GetConfig(); cfg != nil { - router.SetConfig(cfg) - } + } else { + refreshRouterAfterMonitorReload() } })