From 8823440f33b5dcecf6804f8d3a8d3e58ced53ec0 Mon Sep 17 00:00:00 2001 From: rcourtman <8825017+rcourtman@users.noreply.github.com> Date: Sat, 10 Oct 2026 12:34:19 +0100 Subject: [PATCH] Stop mock-mode callers that read identity rebuilding the estate view In mock mode the unified view is keyed on the fixture data version, which advances on every metric tick, and building it for the default 1,776-resource estate costs about half a second of CPU (many seconds on a starved process). #2778 moved the metrics-target lookup onto the fixture structure revision; the other callers that read only what the estate lists and how it connects still rebuilt the whole view after each tick. At startup they made 42 view requests, all misses, and with no client connected the monitor tick alone rebuilt the estate about every nine seconds. Callers that read identity and topology now share the view built for the current structure revision and operator links (currentStructureUnifiedStateView, GetUnifiedStructureReadState): the Docker-alert prune, the workload chart prewarm, the demo Patrol resolver, and the host-agent evaluation, which also lists nodes only when an offline host names a linked node. The connection-degraded alert feed keeps only platform rows, so it no longer reads the hosts, their desired-config fingerprints or the PBS instances that only feed agent rows and a host alias its snapshot drops. The mock storage chart is not one of them: a linked pool takes its capacity from whichever source was seen last, which a tick can move without a structure change. It keeps the data-version view, read once per request instead of once per uncached pool. The per-node polling lookups of a monitor that keeps real polling in mock mode (linkedHostForNode, getHostAgentTemperatureForNode, carriedTemperatureOutlivesAgentLease) read a host's status, disks and sensors, which a tick moves, so they stay on the data-version view. On pulse-dev (one pinned core, host load 30 to 110, probe builds): a start built the estate view twice before the listener answered with the wiring change alone and once with this; with no client connected the process built the estate 33 times in 299 s and used 12.3 s of CPU over a 180 s window, against twice and 3.3 s with this change. --- .../v6/internal/subsystems/agent-lifecycle.md | 17 ++ .../v6/internal/subsystems/api-contracts.md | 19 +++ .../v6/internal/subsystems/monitoring.md | 71 +++++++-- .../subsystems/performance-and-scalability.md | 15 +- .../internal/subsystems/security-privacy.md | 15 ++ .../internal/subsystems/storage-recovery.md | 10 ++ internal/api/connections_alerts.go | 20 ++- internal/api/router.go | 2 +- .../api/runtime_inventory_sources_test.go | 147 ++++++++++++++++++ .../monitoring/metrics_target_resolution.go | 14 +- internal/monitoring/mock_chart_history.go | 16 +- internal/monitoring/monitor.go | 5 +- internal/monitoring/monitor_agents.go | 23 ++- internal/monitoring/monitor_alert_sync.go | 3 +- .../monitoring/monitor_host_agents_test.go | 145 +++++++++++++++++ internal/monitoring/monitor_metrics.go | 12 +- .../monitoring/monitor_metrics_slo_test.go | 30 ++++ internal/monitoring/unified_structure_view.go | 78 ++++++++++ 18 files changed, 595 insertions(+), 47 deletions(-) create mode 100644 internal/monitoring/unified_structure_view.go diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index 2625c04957..8847722e10 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -9459,3 +9459,20 @@ pair the operator has decided about since. The agents API's unlink is unchanged. Registration, enrolment, install, update, removal, token binding and report identity are unchanged. The unified-resources contract records the split's rules. + +### Host-agent evaluation lists nodes only when an offline host needs them + +`Monitor.evaluateHostAgents` (`internal/monitoring/monitor_agents.go`) runs on +every monitor tick. It used to list every Proxmox node through +`NodesSnapshot` before looking at a single host, only to give an offline host +its shared-system alert correlation (`sharedSystemAlertCorrelationForHost`, +which reads node ID, linked agent and instance). It now lists the nodes when +the first offline host that names a linked node asks, through +`structureNodes`, which in mock mode serves the view built for the current +fixture structure instead of rebuilding the estate after each metric tick. The +liveness window, the online and offline transitions, the host lifecycle, the +telemetry expiry and the alert correlation itself are unchanged, and no agent +report, enrollment, lease or command path is touched +(`TestSharedSystemAlertCorrelationForHostLazyListsNodesOnDemand` and +`TestMockStructureConsumersDoNotRebuildTheViewAfterFixtureTicks` in +`internal/monitoring/monitor_host_agents_test.go`). diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 3ea9c617e3..79ab1face9 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -12080,3 +12080,22 @@ it. The request, status codes, response shape and the meaning of an empty decision is added: `exclusions` still counts one link pair each. `TestResourceReportMergeSourceFilterDetachesChainMembers` reports a chain through the handler and the REST listing. + +### The connection-degraded alert feed reads platform rows only + +`buildAlertConnectionSnapshotsWithRuntimeSources` (`internal/api/connections_alerts.go`) +feeds the connection-degraded alert, and `snapshotConnectionsForAlerts` keeps +only the PVE, PBS, PMG, VMware and TrueNAS rows. It now builds the aggregator's +inputs with `aggregatorRuntimeSources.platformRowsOnly`, which leaves out the +monitor's hosts, their desired-config fingerprints and the PBS reported node +names: they only feed agent rows, which that snapshot drops, and a PBS host +alias, which it does not carry. The connections endpoint, diagnostics and the +runtime inventory sources still read them. No route, request, response, status +code, header or payload changes, and a platform row is the same with and +without those reads (`TestAlertConnectionSnapshotsMatchTheFullAggregatorOnPlatformRows` +and `TestAlertConnectionSnapshotsKeepConfiguredPlatformRowsWithoutTheHosts` in +`internal/api/runtime_inventory_sources_test.go`). The demo Patrol resolver +(`patrolResourceOperatorStateProvider` in `internal/api/router.go`) resolves a +finding's resource reference through `Monitor.GetUnifiedStructureReadState`; in +mock mode that is the view built for the current fixture structure, outside it +the read state it used before. diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index d7ee52daa5..53043e47c1 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -3634,9 +3634,34 @@ being reconfigured or a link change rebuilds the view on the next lookup; as before, two lookups that miss together can each build one, and a cold or invalidated cache builds even for an ID the estate does not list. Once the view is current an unknown ID resolves to nothing and falls to the raw -resource store. The view stays keyed on the data version for every other -consumer (broadcast, `/api/state`, `GetUnifiedReadStateOrSnapshot`), which -still rebuild it on a data-version miss. +resource store. The view stays keyed on the data version for the consumers +that read what a tick moves (broadcast, `/api/state`, +`GetUnifiedReadStateOrSnapshot`), which still rebuild it on a data-version +miss. + +Callers that read the estate's identity and topology only share the +structure-keyed view instead (`currentStructureUnifiedStateView`, with +`GetUnifiedStructureReadState` and `currentModeStructureReadState` over it in +`internal/monitoring/unified_structure_view.go`): the Docker-alert prune +(`pruneStaleDockerAlerts`, host IDs), the host-agent evaluation +(`evaluateHostAgents`, which runs on every monitor tick and lists nodes through +`structureNodes`, for their IDs and links, only when an offline host names a +linked node), the chart cache prewarm (metric IDs) and the demo patrol's +operator-state resolver (`patrolResourceOperatorStateProvider`, canonical ID +resolution). Such a caller can be handed a view several ticks old, so it must +not read status, metric values, timestamps, sensors or anything a link merges by +freshness, and a new consumer that does belongs on the data-version view. The +mock storage chart history (`resolveMockStorageTotal`) is one of those: a linked +pool takes its capacity from whichever source was seen last, which moves +without a structure change, so it totals each pool whose chart is not cached +from the data-version view, and `GetStorageMetricsForChartBatch` reads that view +once for all the pools of a request instead of once per pool +(`TestMockStorageChartReadsTheEstateOnlyForAnUncachedChart`). The connection-degraded alert feed +(`buildAlertConnectionSnapshotsWithRuntimeSources`) reads neither the hosts nor +the PBS instances: it keeps the platform rows and drops every agent row, and a +PBS row's reported node name only becomes a host alias that the alert snapshot +does not carry, so it asks the aggregator for platform rows only +(`aggregatorRuntimeSources.platformRowsOnly`). The revision is read before the data version when a view is cached, and the fixture advances its data version before its revision @@ -3658,16 +3683,36 @@ busy loops missed the view cache on 675 of 929 requests at a 16-second average build and had not served by 606 s; with this change it served at 148 s (13 s on the core alone). This was not a regression: builds from 1a094646d7 and d2ac0c5112 stall identically under equal contention and both start in 14 to -16 seconds at normal load. The remaining time on a starved host is the other -consumers of the shared view, which rebuild it whenever they find it stale (on -the starved run, 42 view requests in a start of 194 s were all misses, from the -Docker-alert prune, the connection snapshots and the host-agent evaluation), -and the monitor's repeated full resource-store passes at startup. Loops that -still reach the data-version view once per item are unchanged: the mock storage -chart history (`resolveMockStorageTotal` per uncached pool), the demo patrol's -finding adds (`patrolResourceOperatorStateProvider`) and, with -`PULSE_MOCK_KEEP_REAL_POLLING`, the per-node polling lookups -(`linkedHostForNode`, `getHostAgentTemperatureForNode`). +16 seconds at normal load. The remaining time on a starved host was the other +consumers of the shared view, which rebuilt it whenever they found it stale (on +the starved run, 42 view requests in a start of 194 s were all misses: the +Docker-alert prune 18, the connection snapshots 13, the host-agent evaluation 8, +the chart prewarm 2 and the metrics-target lookup 1), and the monitor's repeated full resource-store +passes at startup. The consumers above now ride the structure view or skip the +read, and the passes are one (see "Store and provider wiring publishes the +estate once, and only when it changed"). The loops that still reach the +data-version view once per item are the per-node polling lookups of a monitor +that keeps real polling in mock mode (`PULSE_MOCK_KEEP_REAL_POLLING`): +`linkedHostForNode`, `getHostAgentTemperatureForNode` and +`carriedTemperatureOutlivesAgentLease`. They read a host's status, disks, +sensors and report lease, which a tick moves, so the structure view cannot +answer them; hoisting the read out of the per-node loop would need the read +state threaded through the node poller, and they run in the polling goroutines +after startup, not before the listener opens. + +Measured on pulse-dev (single runs on a shared host at load 50 to 90, the +process pinned to one core with GOMAXPROCS=1, probe builds counting estate +builds): before the listener answered, a start of the previous build made 19 +store passes and 25 estate view builds; with the wiring change alone it made one +pass and two builds (the first metrics-target lookup and the Docker-alert +prune); with the structure view as well it makes one pass and one build. With no +client connected, a mock-mode process kept building the estate about once every +nine seconds (33 builds in the 299-second life of a run with the wiring change +alone) and used 12.3 seconds of CPU over a 180-second window; with the +structure view it built the estate twice in all, both at startup, and used 3.3 +seconds over the same window. The previous build used 9.0 seconds over that +window in a separate run, so read the 12.3 against 3.3 as the comparison, not as +exact percentages. `TestMockMetricsTargetLookupsDoNotRebuildTheViewAfterFixtureTicks` and `TestMockMetricsTargetForUnknownIDDoesNotRebuildTheViewAfterFixtureTicks` fail on 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 a7372f932d..eeb88823d2 100644 --- a/docs/release-control/v6/internal/subsystems/performance-and-scalability.md +++ b/docs/release-control/v6/internal/subsystems/performance-and-scalability.md @@ -3748,9 +3748,18 @@ alert loop, which is what the one-off measurement above showed. Run both benchmarks with `-benchtime=3x`, since the registry case rebuilds its ingest outside the timer. `TestMockMetricsTargetLookupsDoNotRebuildTheViewAfterFixtureTicks` is the guard that fails on a rebuild per lookup. The remaining startup cost -under starvation is the monitor's repeated full resource-store passes (one per -supplemental provider registration) and the other consumers of the shared view, -which rebuild it whenever they find it stale; neither is changed here. +under starvation was the monitor's repeated full resource-store passes (one per +supplemental provider registration, 19 before the listener answered) and the +other consumers of the shared view, which rebuilt it whenever they found it +stale. A start now makes one pass and one estate build, and a mock-mode process +with no client connected no longer builds the estate on every monitor tick (the +data-version consumers, broadcast and `/api/state`, still rebuild it after a +tick for a connected client). The callers that read +identity and topology only share the view built for the current fixture +structure, and the connection-degraded alert feed no longer asks the monitor for +hosts or PBS instances it throws away (see "Mock-mode metrics-target lookups +ride the fixture structure revision" in the monitoring contract for the callers +and the measurements). ### Update evidence reuses the bounded node observation diff --git a/docs/release-control/v6/internal/subsystems/security-privacy.md b/docs/release-control/v6/internal/subsystems/security-privacy.md index 39579156bc..268ff47963 100644 --- a/docs/release-control/v6/internal/subsystems/security-privacy.md +++ b/docs/release-control/v6/internal/subsystems/security-privacy.md @@ -3168,3 +3168,18 @@ that node's row in the same monitor. This only withholds or substitutes values the caller could already read; the new `NodeView` and `DockerHostView` `SourceStatus` accessors expose no field beyond the delivery state the registry already records on the resource. + +### Patrol operator-state resolution keeps its org scoping + +`patrolResourceOperatorStateProvider` (`internal/api/router.go`) resolves a +finding's resource reference to a canonical ID before it reads the operator +state, and now reads the monitor's structure read state +(`Monitor.GetUnifiedStructureReadState`) for that. The org is resolved by +`resolveMonitorForOrg` as before, the operator-state store is the org's own, and +the resolver is identity-only. In mock mode the read state can be several metric +ticks old; the fixture moves no pod between nodes and re-IDs nothing on a tick +(pod node names are set when the estate is built, and they feed the platform +aliases a reference can resolve through), so a reference resolves to the same +canonical ID for as long as the fixture structure holds. Outside mock mode it +is the read state the resolver used before. No authentication, authorization, token or +tenant-isolation path is touched. diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index a1b9dc254c..ab402ed7fe 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -6784,3 +6784,13 @@ excludes the links that join a member carrying the named source to the rest of the merged resource. That changes which pairs the report excludes, through the same exclusion write as before; no schema, backup, retention, migration or recovery path is added or moved. + +### Mock-mode structure views open no storage or recovery path + +`internal/api/connections_alerts.go` and `internal/api/router.go` now ask the +monitor for less in mock mode: the connection-degraded alert feed no longer +reads the hosts and PBS instances it discards, and the demo Patrol resolver +reads the structure view (see "Mock-mode metrics-target lookups ride the +fixture structure revision" in the monitoring contract). Real mode, its stores, +backups, retention, snapshots, migrations and recovery paths are unchanged, and +none is added or moved. diff --git a/internal/api/connections_alerts.go b/internal/api/connections_alerts.go index 0a8dbdbf41..acd8c948d3 100644 --- a/internal/api/connections_alerts.go +++ b/internal/api/connections_alerts.go @@ -15,6 +15,12 @@ type aggregatorRuntimeSources struct { orgID string vmwarePoller *monitoring.VMwarePoller truenasPoller *monitoring.TrueNASPoller + // platformRowsOnly leaves out what only the agent rows and the PBS host + // aliases read: the monitor's hosts and PBS instances. A caller that keeps + // the platform rows alone (the connection-degraded alert feed) would + // otherwise ask the monitor for both, and in mock mode each ask rebuilds + // the whole estate view whenever a metric tick has made it stale. + platformRowsOnly bool } func buildAggregatorInputsWithRuntimeSources( @@ -61,11 +67,15 @@ func buildAggregatorInputsWithRuntimeSources( } if monitor != nil { - inputs.hosts = monitor.HostsSnapshot() - inputs.agentDesiredConfigs = connectionAgentDesiredConfigFingerprints(monitor, inputs.hosts, inputs.apiTokens) + if runtime.platformRowsOnly { + inputs.hosts = []models.Host{} + } else { + inputs.hosts = monitor.HostsSnapshot() + inputs.agentDesiredConfigs = connectionAgentDesiredConfigFingerprints(monitor, inputs.hosts, inputs.apiTokens) + inputs.pbsReportedNodeNames = pbsReportedNodeNamesByInstance(monitor.PBSInstancesSnapshot()) + } inputs.instanceHealth = instanceHealthByKey(monitor.SchedulerHealth()) inputs.availabilityStatuses = monitor.AvailabilityStatusSnapshot() - inputs.pbsReportedNodeNames = pbsReportedNodeNamesByInstance(monitor.PBSInstancesSnapshot()) // A non-default org's cfg is its monitor's detached config copy // (#1619), which a settings save does not update: saved PBS and PMG // intervals reach the monitor only as runtime overrides. Scale the @@ -155,6 +165,10 @@ func buildAlertConnectionSnapshotsWithRuntimeSources( monitor *monitoring.Monitor, runtime aggregatorRuntimeSources, ) []alerts.ConnectionSnapshot { + // snapshotConnectionsForAlerts keeps the platform rows (PVE, PBS, PMG, + // VMware, TrueNAS) and drops every agent row, and a PBS row's reported node + // name only becomes a host alias that the snapshot does not carry. + runtime.platformRowsOnly = true inputs := buildAggregatorInputsWithRuntimeSources(ctx, cfg, persistence, monitor, runtime) return snapshotConnectionsForAlerts(buildConnections(inputs)) } diff --git a/internal/api/router.go b/internal/api/router.go index 30c3971cb5..fe2766d9e5 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -2242,7 +2242,7 @@ func (r *Router) patrolResourceOperatorStateProvider(orgID string) ai.ResourceOp return ai.ResourceOperatorStateProjection{}, false } canonicalID := resourceRef - if resolver, ok := r.resolveMonitorForOrg(orgID).GetUnifiedReadStateOrSnapshot().(canonicalResourceIDResolver); ok { + if resolver, ok := r.resolveMonitorForOrg(orgID).GetUnifiedStructureReadState().(canonicalResourceIDResolver); ok { if resolvedID, resolved := resolver.ResolveCanonicalResourceID(resourceRef); resolved { canonicalID = resolvedID } diff --git a/internal/api/runtime_inventory_sources_test.go b/internal/api/runtime_inventory_sources_test.go index cdbf40953f..5b56c83c24 100644 --- a/internal/api/runtime_inventory_sources_test.go +++ b/internal/api/runtime_inventory_sources_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/mock" "github.com/rcourtman/pulse-go-rewrite/internal/monitoring" ) @@ -314,3 +315,149 @@ func TestRuntimeInventorySourcesHandlerRejectsNonGET(t *testing.T) { t.Fatalf("status = %d, want 405 (%s)", rec.Code, rec.Body.String()) } } + +// mockMonitorWithStaleView returns a mock-mode monitor whose estate view was +// built before the fixture last ticked, and the structure read state it holds. +func mockMonitorWithStaleView(t *testing.T) (*monitoring.Monitor, *config.Config) { + t.Helper() + previous := mock.IsMockEnabled() + previousConfig := mock.GetConfig() + estate := mock.DefaultConfig + estate.NodeCount = 4 + estate.UpdateInterval = time.Second + mock.SetMockConfig(estate) + if err := mock.SetEnabled(true); err != nil { + t.Fatalf("enable mock mode: %v", err) + } + t.Cleanup(func() { + _ = mock.SetEnabled(false) + mock.SetMockConfig(previousConfig) + _ = mock.SetEnabled(previous) + }) + + cfg := &config.Config{DataPath: t.TempDir()} + monitor, err := monitoring.New(cfg) + if err != nil { + t.Fatalf("new monitor: %v", err) + } + t.Cleanup(monitor.Stop) + + monitor.GetUnifiedReadStateOrSnapshot() + tick := mock.FixtureDataVersion() + deadline := time.Now().Add(5 * time.Minute) + for mock.FixtureDataVersion() <= tick { + if time.Now().After(deadline) { + t.Fatal("the mock fixture did not tick") + } + time.Sleep(20 * time.Millisecond) + } + return monitor, cfg +} + +// The connection-degraded alert feed keeps the platform rows (PVE, PBS, PMG, +// VMware, TrueNAS) and drops every agent row, so it has no use for the +// monitor's hosts or PBS instances. In mock mode each of those reads rebuilds +// the whole estate view once a metric tick has made it stale, and the feed +// runs on startup's critical path. +func TestAlertConnectionSnapshotsDoNotRebuildTheMockViewAfterFixtureTicks(t *testing.T) { + monitor, cfg := mockMonitorWithStaleView(t) + warm := monitor.GetUnifiedStructureReadState() + + buildAlertConnectionSnapshotsWithRuntimeSources(context.Background(), cfg, nil, monitor, aggregatorRuntimeSources{orgID: "default"}) + if monitor.GetUnifiedStructureReadState() != warm { + t.Fatal("the alert connection feed rebuilt the unified view after a metric tick; it reads no host or PBS instance") + } + + // Control: the connections endpoint does read them, and the test would + // pass vacuously if a rebuild left no trace. + buildAggregatorInputsWithRuntimeSources(context.Background(), cfg, nil, monitor, aggregatorRuntimeSources{orgID: "default"}) + if monitor.GetUnifiedStructureReadState() == warm { + t.Fatal("reading the hosts did not rebuild the stale view, so this test cannot see a rebuild") + } +} + +// Leaving the hosts and PBS instances out must not change a platform row. The +// lean inputs are the full ones with exactly what platformRowsOnly omits taken +// away, so a supplemental fixture tick between two reads cannot make the rows +// differ for a reason other than the omission. +func TestAlertConnectionSnapshotsMatchTheFullAggregatorOnPlatformRows(t *testing.T) { + monitor, cfg := mockMonitorWithStaleView(t) + + full := buildAggregatorInputsWithRuntimeSources(context.Background(), cfg, nil, monitor, aggregatorRuntimeSources{orgID: "default"}) + lean := buildAggregatorInputsWithRuntimeSources(context.Background(), cfg, nil, monitor, aggregatorRuntimeSources{orgID: "default", platformRowsOnly: true}) + + if len(full.hosts) == 0 { + t.Fatal("the mock estate lists no host, so the comparison proves nothing") + } + if len(lean.hosts) != 0 || lean.agentDesiredConfigs != nil || lean.pbsReportedNodeNames != nil { + t.Fatalf("platformRowsOnly kept hosts=%d desiredConfigs=%v pbsNodeNames=%v", len(lean.hosts), lean.agentDesiredConfigs, lean.pbsReportedNodeNames) + } + + stripped := full + stripped.hosts = lean.hosts + stripped.agentDesiredConfigs = nil + stripped.pbsReportedNodeNames = nil + want := snapshotConnectionsForAlerts(buildConnections(full)) + got := snapshotConnectionsForAlerts(buildConnections(stripped)) + if len(want) == 0 { + t.Fatal("the mock estate yields no platform connection") + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("platform connections changed without the hosts:\n got %+v\nwant %+v", got, want) + } +} + +// The patrol resolver turns a finding's resource reference into a canonical ID. +// It reads identity only, so in mock mode it must not rebuild the estate view +// after a metric tick (it runs once per finding). +func TestPatrolOperatorStateResolverDoesNotRebuildTheMockViewAfterFixtureTicks(t *testing.T) { + monitor, cfg := mockMonitorWithStaleView(t) + warm := monitor.GetUnifiedStructureReadState() + + router := &Router{ + resourceHandlers: NewResourceHandlers(&config.Config{DataPath: cfg.DataPath}), + monitor: monitor, + } + t.Cleanup(router.ShutdownResourceStores) + provider := router.patrolResourceOperatorStateProvider("default") + provider.OperatorStateProjection("some-finding-resource", time.Now()) + + if monitor.GetUnifiedStructureReadState() != warm { + t.Fatal("the patrol operator-state resolver rebuilt the unified view after a metric tick; it only resolves identity") + } +} + +// Mock mode drops the configured PVE, PBS and PMG instances from the ledger, so +// their rows are checked here with mock mode off: the platform rows an alert +// is raised from are the same with and without the hosts and PBS reads. +func TestAlertConnectionSnapshotsKeepConfiguredPlatformRowsWithoutTheHosts(t *testing.T) { + previousMock := mock.IsMockEnabled() + if err := mock.SetEnabled(false); err != nil { + t.Fatalf("disable mock mode: %v", err) + } + t.Cleanup(func() { _ = mock.SetEnabled(previousMock) }) + monitor, err := monitoring.New(&config.Config{DataPath: t.TempDir()}) + if err != nil { + t.Fatalf("new monitor: %v", err) + } + t.Cleanup(monitor.Stop) + cfg := &config.Config{ + DataPath: t.TempDir(), + PVEInstances: []config.PVEInstance{{Name: "pve-lab", Host: "https://pve-lab.example:8006"}}, + PBSInstances: []config.PBSInstance{{Name: "pbs-lab", Host: "https://pbs-lab.example:8007"}}, + PMGInstances: []config.PMGInstance{{Name: "pmg-lab", Host: "https://pmg-lab.example:8006"}}, + } + + full := buildAggregatorInputsWithRuntimeSources(context.Background(), cfg, nil, monitor, aggregatorRuntimeSources{orgID: "default"}) + lean := buildAggregatorInputsWithRuntimeSources(context.Background(), cfg, nil, monitor, aggregatorRuntimeSources{orgID: "default", platformRowsOnly: true}) + lean.now = full.now + + want := snapshotConnectionsForAlerts(buildConnections(full)) + got := snapshotConnectionsForAlerts(buildConnections(lean)) + if len(want) != 3 { + t.Fatalf("expected the PVE, PBS and PMG rows, got %+v", want) + } + if !reflect.DeepEqual(got, want) { + t.Fatalf("platform connections changed without the hosts:\n got %+v\nwant %+v", got, want) + } +} diff --git a/internal/monitoring/metrics_target_resolution.go b/internal/monitoring/metrics_target_resolution.go index 6d81f731a3..8d6f57f4b3 100644 --- a/internal/monitoring/metrics_target_resolution.go +++ b/internal/monitoring/metrics_target_resolution.go @@ -43,19 +43,7 @@ func (m *Monitor) mockMetricsTarget(resourceID string) (*unifiedresources.Metric return nil, false } - structure := mock.FixtureStructureRevision() - links := m.resourceStoreManualLinks() - - m.mockUnifiedViewMu.Lock() - view := m.mockUnifiedView - reusable := m.mockUnifiedViewValid && - m.mockUnifiedViewStructure == structure && - sameManualLinks(m.mockUnifiedViewLinks, links) - m.mockUnifiedViewMu.Unlock() - - if !reusable { - view = m.currentUnifiedStateView() - } + view := m.currentStructureUnifiedStateView() if view.metricsTargets == nil { return nil, true } diff --git a/internal/monitoring/mock_chart_history.go b/internal/monitoring/mock_chart_history.go index 507cd858ba..500e1ed165 100644 --- a/internal/monitoring/mock_chart_history.go +++ b/internal/monitoring/mock_chart_history.go @@ -6,6 +6,7 @@ import ( "time" "github.com/rcourtman/pulse-go-rewrite/internal/mock" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" ) var ( @@ -171,9 +172,14 @@ func latestMetricPoint(points []MetricPoint) (MetricPoint, bool) { return latest, true } -func (m *Monitor) resolveMockStorageTotal(storageID string, inMemory map[string][]MetricPoint) float64 { - readState := m.GetUnifiedReadStateOrSnapshot() - if readState != nil { +// resolveMockStorageTotal totals one pool for a chart. readState lists the pools +// and is read here only if the pool's chart is not cached, so a request over +// many pools reads the estate view once (see GetStorageMetricsForChartBatch) +// and not once per pool, which on a starved process rebuilt the view for each. +// It is not the structure view: a linked pool takes its capacity from whichever +// source was seen last, and that moves without a structure change. +func (m *Monitor) resolveMockStorageTotal(storageID string, inMemory map[string][]MetricPoint, readState func() unifiedresources.ReadState) float64 { + if readState := readState(); readState != nil { for _, pool := range readState.StoragePools() { if pool == nil || strings.TrimSpace(pool.SourceID()) != strings.TrimSpace(storageID) { continue @@ -191,8 +197,8 @@ func (m *Monitor) resolveMockStorageTotal(storageID string, inMemory map[string] return 0 } -func (m *Monitor) mockStorageMetricsForChart(storageID string, duration time.Duration, inMemory map[string][]MetricPoint) map[string][]MetricPoint { - total := m.resolveMockStorageTotal(storageID, inMemory) +func (m *Monitor) mockStorageMetricsForChart(storageID string, duration time.Duration, inMemory map[string][]MetricPoint, readState func() unifiedresources.ReadState) map[string][]MetricPoint { + total := m.resolveMockStorageTotal(storageID, inMemory, readState) if total <= 0 { return cloneMetricPointMap(inMemory) } diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 72aff4182a..a3daa80544 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -3610,7 +3610,10 @@ func (m *Monitor) NodesSnapshot() []models.Node { if m == nil { return nil } - readState := m.GetUnifiedReadStateOrSnapshot() + return nodesSnapshotFromReadState(m.GetUnifiedReadStateOrSnapshot()) +} + +func nodesSnapshotFromReadState(readState unifiedresources.ReadState) []models.Node { if readState == nil { return nil } diff --git a/internal/monitoring/monitor_agents.go b/internal/monitoring/monitor_agents.go index 0d93930105..7a391fc4a7 100644 --- a/internal/monitoring/monitor_agents.go +++ b/internal/monitoring/monitor_agents.go @@ -5227,7 +5227,15 @@ func hostAgentHealthWindow(interval int) time.Duration { // evaluateHostAgents updates health for host agents based on last report time. func (m *Monitor) evaluateHostAgents(now time.Time) { hosts := m.state.GetHosts() - nodes := m.NodesSnapshot() + // Only an offline host that names a linked node asks which node it is + // linked to, and only for the node's identity, so the nodes are listed when + // the first one does, from the structure view: this runs on every monitor + // tick, and in mock mode a listing of the data-version view rebuilds the + // whole estate after each metric tick. + nodes := sync.OnceValue(m.structureNodes) + offlineCorrelation := func(host models.Host) *alerts.AlertCorrelation { + return sharedSystemAlertCorrelationForHostLazy(host, nodes) + } liveHostIDs := make(map[string]struct{}, len(hosts)) resourceRefreshNeeded := false for _, host := range hosts { @@ -5279,7 +5287,7 @@ func (m *Monitor) evaluateHostAgents(now time.Time) { if m.alertManager != nil { m.alertManager.HandleHostOfflineWithCorrelation( hostCopy, - sharedSystemAlertCorrelationForHost(hostCopy, nodes), + offlineCorrelation(hostCopy), ) } } @@ -5304,7 +5312,7 @@ func (m *Monitor) evaluateHostAgents(now time.Time) { if m.alertManager != nil { m.alertManager.HandleHostOfflineWithCorrelation( host, - sharedSystemAlertCorrelationForHost(host, nodes), + offlineCorrelation(host), ) } } @@ -5314,6 +5322,15 @@ func (m *Monitor) evaluateHostAgents(now time.Time) { } } +// sharedSystemAlertCorrelationForHostLazy is sharedSystemAlertCorrelationForHost +// for a caller that lists the nodes only when the host names a linked node. +func sharedSystemAlertCorrelationForHostLazy(host models.Host, nodes func() []models.Node) *alerts.AlertCorrelation { + if strings.TrimSpace(host.LinkedNodeID) == "" || strings.TrimSpace(host.ID) == "" { + return nil + } + return sharedSystemAlertCorrelationForHost(host, nodes()) +} + func sharedSystemAlertCorrelationForHost(host models.Host, nodes []models.Node) *alerts.AlertCorrelation { linkedNodeID := strings.TrimSpace(host.LinkedNodeID) hostID := strings.TrimSpace(host.ID) diff --git a/internal/monitoring/monitor_alert_sync.go b/internal/monitoring/monitor_alert_sync.go index a53f30a6e6..eb55d9aa5d 100644 --- a/internal/monitoring/monitor_alert_sync.go +++ b/internal/monitoring/monitor_alert_sync.go @@ -175,8 +175,9 @@ func (m *Monitor) pruneStaleDockerAlerts() bool { // The inventory is the fixture read view in mock mode. A prune that read // it must not remove Docker alerts the other mode raised after a switch. + // Only the hosts' IDs matter, so the view may predate the last metric ticks. scope := m.mockModeFence.begin() - readState := m.currentModeReadState() + readState := m.currentModeStructureReadState() if readState == nil { return false } diff --git a/internal/monitoring/monitor_host_agents_test.go b/internal/monitoring/monitor_host_agents_test.go index 46cf04cea2..a859ed65f4 100644 --- a/internal/monitoring/monitor_host_agents_test.go +++ b/internal/monitoring/monitor_host_agents_test.go @@ -6173,6 +6173,151 @@ func TestMockMetricsTargetRebuildsWhenTheLinkListMoves(t *testing.T) { } } +// The estate view is keyed on the fixture data version, which advances on +// every metric tick, and a build costs about half a second of CPU on an idle +// core (several seconds on a starved process). Callers that read the estate's +// identity and topology only share the view built for the current fixture +// structure instead (currentStructureUnifiedStateView); startup runs every one +// of them before the listener opens, and on a starved process each used to +// build its own estate. +func TestMockStructureConsumersDoNotRebuildTheViewAfterFixtureTicks(t *testing.T) { + useMockEstate(t, 4, time.Second) + m := &Monitor{ + state: models.NewState(), + resourceStore: unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(unifiedresources.NewMemoryStore())), + alertManager: alerts.NewManager(), + } + t.Cleanup(m.alertManager.Stop) + + warm := m.currentUnifiedStateView() + waitForFixtureTicks(t, 2) + + consumers := []struct { + name string + run func() + }{ + {"structure read state", func() { m.GetUnifiedStructureReadState() }}, + {"mode structure read state", func() { m.currentModeStructureReadState() }}, + {"structure node listing", func() { m.structureNodes() }}, + {"docker alert prune", func() { m.pruneStaleDockerAlerts() }}, + {"host agent evaluation", func() { m.evaluateHostAgents(time.Now()) }}, + {"chart cache prewarm", func() { m.prewarmMockDashboardChartCaches() }}, + {"host agent evaluation with an offline linked host", func() { + m.state.UpsertHost(models.Host{ID: "offline-agent", Hostname: "offline-agent", LinkedNodeID: "pve1-node", Status: "online", LastSeen: time.Now().Add(-24 * time.Hour)}) + m.evaluateHostAgents(time.Now()) + }}, + } + for _, consumer := range consumers { + consumer.run() + after, ok := publishedMockView(m) + if !ok || after.readState != warm.readState { + t.Fatalf("%s rebuilt the unified view after a metric tick; it only reads identity and topology, which the view built for the current fixture structure holds", consumer.name) + } + } +} + +// The host-agent evaluation lists nodes only for a host that names a linked +// node, and only when it has to. +func TestSharedSystemAlertCorrelationForHostLazyListsNodesOnDemand(t *testing.T) { + listed := 0 + nodes := func() []models.Node { + listed++ + return []models.Node{{ID: "node-1", Instance: "lab", LinkedAgentID: "host-1"}} + } + + if got := sharedSystemAlertCorrelationForHostLazy(models.Host{ID: "host-1"}, nodes); got != nil || listed != 0 { + t.Fatalf("an unlinked host listed nodes %d times and correlated %v", listed, got) + } + if got := sharedSystemAlertCorrelationForHostLazy(models.Host{LinkedNodeID: "node-1"}, nodes); got != nil || listed != 0 { + t.Fatalf("a host without an ID listed nodes %d times and correlated %v", listed, got) + } + got := sharedSystemAlertCorrelationForHostLazy(models.Host{ID: "host-1", LinkedNodeID: "node-1"}, nodes) + if listed != 1 { + t.Fatalf("a linked host listed nodes %d times, want once", listed) + } + want := sharedSystemAlertCorrelationForHost(models.Host{ID: "host-1", LinkedNodeID: "node-1"}, nodes()) + if got == nil || want == nil || got.Key != want.Key { + t.Fatalf("a linked host correlated %+v, want %+v", got, want) + } +} + +// The data-version view must keep serving the callers that read values: a +// consumer of statuses, sensors and metrics must see the tick, so the +// structure view cannot have replaced it. +func TestMockDataVersionViewStillRebuildsAfterFixtureTicks(t *testing.T) { + useMockEstate(t, 3, time.Second) + m := newMockEstateMonitor() + + warm := m.currentUnifiedStateView() + waitForFixtureTicks(t, 1) + + if m.GetUnifiedReadStateOrSnapshot() == warm.readState { + t.Fatal("GetUnifiedReadStateOrSnapshot served the view from before a metric tick") + } +} + +// The structure view answers for the estate it was built from. A structural +// change must reach its callers, not keep serving the old resource set. +func TestMockStructureReadStateFollowsAStructuralFixtureChange(t *testing.T) { + useMockEstate(t, 3, 5*time.Minute) + m := newMockEstateMonitor() + + before := m.GetUnifiedStructureReadState() + if before == nil || len(before.Nodes()) == 0 { + t.Fatal("the mock estate lists no nodes") + } + + grown := mock.GetConfig() + grown.NodeCount++ + mock.SetMockConfig(grown) + + after := m.GetUnifiedStructureReadState() + if after == before { + t.Fatal("the structure view outlived a structural fixture change") + } + if len(after.Nodes()) <= len(before.Nodes()) { + t.Fatalf("the structure view lists %d nodes after the estate grew from %d", len(after.Nodes()), len(before.Nodes())) + } +} + +// An operator link folds one resource into another; the structure view must +// stop listing the folded resource once the resource store applies the link. +func TestMockStructureReadStateFollowsAnOperatorLink(t *testing.T) { + useMockEstate(t, 4, 5*time.Minute) + store := unifiedresources.NewMemoryStore() + m := &Monitor{ + state: models.NewState(), + resourceStore: unifiedresources.NewMonitorAdapter(unifiedresources.NewRegistry(store)), + alertManager: alerts.NewManager(), + } + t.Cleanup(m.alertManager.Stop) + + var vmID, agentID string + for _, resource := range m.currentStructureUnifiedStateView().resources { + switch { + case vmID == "" && resource.Type == unifiedresources.ResourceTypeVM && resource.Agent == nil: + vmID = resource.ID + case agentID == "" && resource.Type == unifiedresources.ResourceTypeAgent && resource.Agent != nil && + len(resource.Sources) == 1 && resource.Sources[0] == unifiedresources.SourceAgent: + agentID = resource.ID + } + } + if vmID == "" || agentID == "" { + t.Fatalf("fixture graph needs an agentless VM and a standalone agent, got vm=%q agent=%q", vmID, agentID) + } + + if err := store.AddLink(unifiedresources.ResourceLink{ResourceA: vmID, ResourceB: agentID, PrimaryID: vmID}); err != nil { + t.Fatalf("AddLink: %v", err) + } + m.updateResourceStore(models.StateSnapshot{}, m.mockModeFence.begin()) + + for _, resource := range m.currentStructureUnifiedStateView().resources { + if resource.ID == agentID { + t.Fatalf("the structure view still lists the agent %q the operator folded into %q", agentID, vmID) + } + } +} + // TestMockModeDiscardsRealHostReports pins the push-side of the mock clean // room. Mock mode already suspends pull-based PVE/PBS/PMG collection, but a // real agent keeps POSTing regardless; ingesting those reports lands a real diff --git a/internal/monitoring/monitor_metrics.go b/internal/monitoring/monitor_metrics.go index e2eb1e9a16..4b0c811f95 100644 --- a/internal/monitoring/monitor_metrics.go +++ b/internal/monitoring/monitor_metrics.go @@ -3,6 +3,7 @@ package monitoring import ( "sort" "strings" + "sync" "time" "github.com/rcourtman/pulse-go-rewrite/internal/mock" @@ -226,7 +227,7 @@ func (m *Monitor) GetStorageMetricsForChart(storageID string, duration time.Dura inMemoryResult = m.metricsHistory.GetAllStorageMetrics(storageID, duration) } if mock.IsMockEnabled() { - return m.mockStorageMetricsForChartCached(storageID, duration, inMemoryResult) + return m.mockStorageMetricsForChartCached(storageID, duration, inMemoryResult, m.GetUnifiedReadStateOrSnapshot) } if m.metricsStore == nil { return inMemoryResult @@ -729,12 +730,14 @@ func (m *Monitor) GetStorageMetricsForChartBatch( result := make(map[string]map[string][]MetricPoint, len(storageIDs)) if mock.IsMockEnabled() { + // One read of the estate view for every pool that needs a total. + readState := sync.OnceValue(m.GetUnifiedReadStateOrSnapshot) for _, sid := range storageIDs { inMemory := map[string][]MetricPoint{} if m.metricsHistory != nil { inMemory = m.metricsHistory.GetAllStorageMetrics(sid, duration) } - result[sid] = m.mockStorageMetricsForChartCached(sid, duration, inMemory) + result[sid] = m.mockStorageMetricsForChartCached(sid, duration, inMemory, readState) } return result } @@ -1179,7 +1182,7 @@ func (m *Monitor) prewarmMockWorkloadChartCaches(duration time.Duration) { } requests := boundMockWorkloadChartPrewarmRequests( - mockWorkloadChartRequestsForReadState(m.GetUnifiedReadStateOrSnapshot()), + mockWorkloadChartRequestsForReadState(m.GetUnifiedStructureReadState()), ) if len(requests.vms) > 0 { _ = m.GetGuestMetricsForChartBatch("vm", requests.vms, duration) @@ -1309,6 +1312,7 @@ func (m *Monitor) mockStorageMetricsForChartCached( storageID string, duration time.Duration, inMemoryResult map[string][]MetricPoint, + readState func() unifiedresources.ReadState, ) map[string][]MetricPoint { cacheKey := mockChartMetricMapCacheKey{ kind: "storage", @@ -1325,7 +1329,7 @@ func (m *Monitor) mockStorageMetricsForChartCached( computed = downsampleMetricMapForMockChart(inMemoryResult, duration) } else { computed = downsampleMetricMapForMockChart( - m.mockStorageMetricsForChart(storageID, duration, inMemoryResult), + m.mockStorageMetricsForChart(storageID, duration, inMemoryResult, readState), duration, ) } diff --git a/internal/monitoring/monitor_metrics_slo_test.go b/internal/monitoring/monitor_metrics_slo_test.go index f59de6ea6e..756177dd3a 100644 --- a/internal/monitoring/monitor_metrics_slo_test.go +++ b/internal/monitoring/monitor_metrics_slo_test.go @@ -823,3 +823,33 @@ func TestMockChartCacheInvalidatesAfterMockHistoryRefresh(t *testing.T) { }, lastPoint) } } + +// A storage chart request totals each pool whose chart is not cached, and a +// linked pool takes its capacity from whichever source was seen last, which +// moves without a structure change, so the totals read the data-version view. +// A batch hands every pool one shared read (GetStorageMetricsForChartBatch): +// the view is rebuilt after a tick, and on a starved process a read per pool +// rebuilt it for each. A pool whose chart is cached reads nothing. +func TestMockStorageChartReadsTheEstateOnlyForAnUncachedChart(t *testing.T) { + useMockEstate(t, 4, 5*time.Minute) + m := newMockEstateMonitor() + pools := m.currentUnifiedStateView().readState.StoragePools() + if len(pools) == 0 { + t.Fatal("the mock estate lists no storage pool") + } + id := pools[0].SourceID() + + reads := 0 + readState := func() unifiedresources.ReadState { + reads++ + return m.GetUnifiedReadStateOrSnapshot() + } + first := m.mockStorageMetricsForChartCached(id, time.Hour, nil, readState) + if reads != 1 || len(first["total"]) == 0 { + t.Fatalf("an uncached chart read the estate %d times and returned %d total points, want one read and a total series", reads, len(first["total"])) + } + m.mockStorageMetricsForChartCached(id, time.Hour, nil, readState) + if reads != 1 { + t.Fatalf("a cached chart read the estate again: %d reads", reads) + } +} diff --git a/internal/monitoring/unified_structure_view.go b/internal/monitoring/unified_structure_view.go new file mode 100644 index 0000000000..00c7fe8d24 --- /dev/null +++ b/internal/monitoring/unified_structure_view.go @@ -0,0 +1,78 @@ +package monitoring + +import ( + "github.com/rcourtman/pulse-go-rewrite/internal/mock" + "github.com/rcourtman/pulse-go-rewrite/internal/models" + "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" +) + +// cachedMockStructureView returns the mock view the monitor has cached when it +// was built for the fixture's current structure revision and operator link +// list, however many metric ticks ago. The second result is false when there is +// no such view, and the first is then not for use. +func (m *Monitor) cachedMockStructureView() (monitorUnifiedStateView, bool) { + structure := mock.FixtureStructureRevision() + links := m.resourceStoreManualLinks() + + m.mockUnifiedViewMu.Lock() + defer m.mockUnifiedViewMu.Unlock() + reusable := m.mockUnifiedViewValid && + m.mockUnifiedViewStructure == structure && + sameManualLinks(m.mockUnifiedViewLinks, links) + return m.mockUnifiedView, reusable +} + +// currentStructureUnifiedStateView is currentUnifiedStateView for a caller that +// reads what the estate lists and how it connects: IDs, names, types, source +// mappings, parents, operator links and the metrics targets those yield. In +// mock mode that answers from the cached view for as long as the fixture +// structure and the link list are unchanged. The view is keyed on the data +// version otherwise, which advances on every metric tick, so a caller that +// needs only structure but asked for the data-version view built the whole +// estate once per tick: about half a second of CPU on an idle core, and on a +// starved process a build longer than the tick, so every call missed. A +// caller must not read what a tick moves (status, metric values, timestamps, +// sensors); the view it gets can be any number of ticks old. Outside mock mode +// this is currentUnifiedStateView. +func (m *Monitor) currentStructureUnifiedStateView() monitorUnifiedStateView { + if m == nil { + return monitorUnifiedStateView{} + } + if mock.IsMockEnabled() { + if view, ok := m.cachedMockStructureView(); ok { + return view + } + } + return m.currentUnifiedStateView() +} + +// GetUnifiedStructureReadState is GetUnifiedReadStateOrSnapshot for a caller +// that reads identity and topology only (see currentStructureUnifiedStateView): +// in mock mode it can be several metric ticks old. +func (m *Monitor) GetUnifiedStructureReadState() unifiedresources.ReadState { + if m == nil { + return nil + } + if !mock.IsMockEnabled() { + return m.GetUnifiedReadStateOrSnapshot() + } + return m.currentStructureUnifiedStateView().readState +} + +// currentModeStructureReadState is currentModeReadState for a caller that reads +// identity and topology only. +func (m *Monitor) currentModeStructureReadState() unifiedresources.ReadState { + if m == nil { + return nil + } + if mock.IsMockEnabled() { + return m.currentStructureUnifiedStateView().readState + } + return m.currentModeReadState() +} + +// structureNodes is NodesSnapshot for a caller that reads the nodes' identity +// and links only (ID, instance, linked agent). +func (m *Monitor) structureNodes() []models.Node { + return nodesSnapshotFromReadState(m.GetUnifiedStructureReadState()) +}