From b89bab0212da583712d6804fe2b967cd902556b0 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Wed, 23 Sep 2026 23:55:18 +0100 Subject: [PATCH] Stop copying the resource registry on every read-state lookup GetUnifiedReadStateOrSnapshot built a full unified state view only to return its readState, which for a store-backed monitor is the live store itself. Building the view deep-clones every resource through GetAll, and the host-continuity wrapper clones them again, so each call copied the whole estate twice and discarded both copies. Alert evaluation reaches this once per resource per poll: the default 5-minute CPU evaluation window sends every CPU-bearing resource through metricWindowPoints and MetricsTargetForResource, so poll cost grew with the square of the estate. Measured per call on main: 2.6 ms and 1.7 MB at 130 resources, 32 ms and 19 MB at 1,040, 85 ms and 56 MB at 2,080. At 1,040 resources that is roughly 22 CPU-seconds per 10-second poll. Returning the store directly costs 2.5-25 us and 9 allocations at every size. Mock mode and monitors without a ReadState store keep the view path. The new test holds the lookup's allocations flat from 8 to 1,000 resources; before this change it measured 545 and 66,019. Refs #2199 Contract-Neutral: Read-state accessor performance only; callers receive the same live store, no monitoring or agent-lifecycle contract change --- internal/monitoring/monitor.go | 13 ++++ .../unified_read_state_clone_test.go | 76 +++++++++++++++++++ 2 files changed, 89 insertions(+) create mode 100644 internal/monitoring/unified_read_state_clone_test.go diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 358f66f909..1f33ab3d3c 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -5027,6 +5027,19 @@ func (m *Monitor) GetUnifiedReadState() unifiedresources.ReadState { // ephemeral snapshot-backed adapter to preserve read access without exposing // direct state reads to consumer packages. func (m *Monitor) GetUnifiedReadStateOrSnapshot() unifiedresources.ReadState { + if m == nil { + return nil + } + // A store that is itself the read state needs no view. Building one + // deep-clones every resource, twice, only for this caller to discard the + // copies, and alert evaluation reaches here once per resource per poll + // through the default CPU evaluation window, which made every poll cost + // grow with the square of the estate (#2199). + if !mock.IsMockEnabled() { + if readState := m.GetUnifiedReadState(); readState != nil { + return m.readStateWithStandaloneHostContinuity(readState) + } + } return m.currentUnifiedStateView().readState } diff --git a/internal/monitoring/unified_read_state_clone_test.go b/internal/monitoring/unified_read_state_clone_test.go new file mode 100644 index 0000000000..06e72b0b1c --- /dev/null +++ b/internal/monitoring/unified_read_state_clone_test.go @@ -0,0 +1,76 @@ +package monitoring + +import ( + "fmt" + "testing" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +// Alert evaluation resolves a metrics target once per resource per poll, so a +// read-state lookup that copies the registry makes every poll quadratic in +// estate size (#2199). These tests pin the lookup to the live store. + +func newReadStateCloneTestMonitor(t testing.TB, vms int) (*Monitor, *unifiedresources.MonitorAdapter, string) { + t.Helper() + now := time.Now().UTC() + snapshot := models.StateSnapshot{LastUpdate: now} + for i := 0; i < vms; i++ { + snapshot.VMs = append(snapshot.VMs, models.VM{ + ID: fmt.Sprintf("lab:pve-%d:%d", i%8, 100+i), + VMID: 100 + i, + Name: fmt.Sprintf("vm-%d", i), + Node: fmt.Sprintf("pve-%d", i%8), + Instance: "lab", + Status: "running", + Type: "qemu", + CPU: 0.1, + Memory: models.Memory{Total: 4 << 30, Used: 1 << 30}, + LastSeen: now, + }) + } + registry := unifiedresources.NewRegistry(nil) + registry.IngestSnapshot(snapshot) + adapter := unifiedresources.NewMonitorAdapter(registry) + m := &Monitor{state: models.NewState(), resourceStore: adapter} + + vmID := "" + for _, resource := range adapter.GetAll() { + if resource.Type == unifiedresources.ResourceTypeVM { + vmID = resource.ID + break + } + } + if vmID == "" { + t.Fatal("fixture produced no VM resource") + } + return m, adapter, vmID +} + +func TestGetUnifiedReadStateOrSnapshotReturnsLiveStore(t *testing.T) { + m, adapter, _ := newReadStateCloneTestMonitor(t, 4) + + readState := m.GetUnifiedReadStateOrSnapshot() + got, ok := readState.(*unifiedresources.MonitorAdapter) + if !ok || got != adapter { + t.Fatalf("read state = %T %p, want the configured store %p", readState, got, adapter) + } +} + +func TestMetricsTargetForResourceDoesNotCopyRegistry(t *testing.T) { + small, _, smallID := newReadStateCloneTestMonitor(t, 8) + large, _, largeID := newReadStateCloneTestMonitor(t, 1000) + if small.MetricsTargetForResource(smallID) == nil || large.MetricsTargetForResource(largeID) == nil { + t.Fatal("expected metrics targets for fixture VMs") + } + + smallAllocs := testing.AllocsPerRun(20, func() { _ = small.MetricsTargetForResource(smallID) }) + largeAllocs := testing.AllocsPerRun(20, func() { _ = large.MetricsTargetForResource(largeID) }) + // A registry copy costs several allocations per resource, so any growth + // from 8 to 1,000 resources means the lookup is copying the estate again. + if largeAllocs > smallAllocs+8 { + t.Fatalf("MetricsTargetForResource allocations grew with estate size: %.0f at 8 VMs, %.0f at 1000 VMs", smallAllocs, largeAllocs) + } +}