diff --git a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md index b8ed3f012b..600e45ca35 100644 --- a/docs/release-control/v6/internal/subsystems/agent-lifecycle.md +++ b/docs/release-control/v6/internal/subsystems/agent-lifecycle.md @@ -765,6 +765,15 @@ not fenced: a live report already past its mock-mode check when mock mode is switched on can still be evaluated after the clear, and its alerts can persist until mock mode is left. +Every tenant monitor registers its own fixture agents, and their node links, +in its own alert manager, so the toggle forgets the fixture agents of every +running monitor, not only those of the monitor it was called through. A link +left behind would keep owning its fixture node's usage alerts after mock mode +ends. `TestLeavingMockModeReleasesFixtureAgentNodeLinksOnEveryRunningMonitor` +in `internal/monitoring/monitor_host_agent_removal_lifecycle_test.go` switches +through the default monitor and fails if the other tenant's node stays +suppressed. + Physical-disk evidence collected by a host agent must survive projection back into monitoring's models. Absent evidence has to carry its declared sentinel rather than a zero value that reads as a real measurement: an absent @@ -8901,3 +8910,12 @@ agent from its last report instead of from the PVE poll, and mark it stale at their own heartbeat cutoff (`fleethealth.AgentStaleThreshold`, which is separate from the monitoring reporting lease). Agent registration, enrolment, install, update, removal and report identity are unchanged. + +### Demo write guard ignores websocket upgrade headers + +`internal/api/demo_middleware.go` changed only so a `POST`, `PUT`, `PATCH` or +`DELETE` carrying `Upgrade: websocket` is judged by the demo read-only guard +like any other write instead of always reaching its handler. Agent command +websocket handshakes to `/api/agent/ws` are HTTP/1.1 `GET` requests, the only +transport its gorilla upgrader serves, and still connect on a demo instance. +No agent registration, enrolment, install, update or removal path changed. diff --git a/docs/release-control/v6/internal/subsystems/api-contracts.md b/docs/release-control/v6/internal/subsystems/api-contracts.md index 6b204396df..4227abe9af 100644 --- a/docs/release-control/v6/internal/subsystems/api-contracts.md +++ b/docs/release-control/v6/internal/subsystems/api-contracts.md @@ -7651,6 +7651,17 @@ read-only demo account. That same hidden read-side boundary includes `GET` and `HEAD` reads for `/api/admin/users` and manual discovery at `/api/discover`; public demo mode may block writes generically, but it must not reveal that admin-user inventory or manual-discovery read routes exist. +That generic write block keys on the request method alone. The demo +middleware admits `GET`, `HEAD` and `OPTIONS` plus the login, logout, AI +execute and OIDC login/callback allowlist; HTTP/1.1 websocket handshakes for +`/ws` and `/api/agent/ws` are `GET` requests and connect through that read +branch, so an `Upgrade: websocket` or `Connection: Upgrade` header never +carries a `POST`, `PUT`, `PATCH` or `DELETE` past the guard. A demo deployment that signs +visitors in as the configured admin, as the public demo does, has only this +guard between that session and admin writes such as `/api/system/mock-mode`. +`TestContract_DemoModeUpgradeHeadersDoNotExemptWrites` in +`internal/api/contract_test.go` signs in as that admin through the full router +and pins both halves. That shared infrastructure install boundary now also preserves copied shell command payload continuity: any privilege-escalation wrapper applied at `frontend-modern/src/components/Settings/InfrastructureInstallerSection.tsx` diff --git a/docs/release-control/v6/internal/subsystems/frontend-primitives.md b/docs/release-control/v6/internal/subsystems/frontend-primitives.md index a5fa98dd52..7a78a86127 100644 --- a/docs/release-control/v6/internal/subsystems/frontend-primitives.md +++ b/docs/release-control/v6/internal/subsystems/frontend-primitives.md @@ -6666,6 +6666,11 @@ icon/content layout, and action-link chrome. The outdated-sensor notices, and the `platform-inline-notice-local-amber-shell` pattern guard blocks future `platformPage` files from reintroducing page-local amber notice shells. +A notice's render predicate reads what its page hydrates on every tab the +notice renders on. `collectOutdatedSensorSetupNodes` lists the PVE nodes that +carry the registry's `proxmox.sensorSetupOutdated` verdict and takes no disk +rows, because only the Proxmox Storage tab loads them; a predicate over them +left the notice silent on Overview. Alert incident-event filter containers, labels, and chips must now route through the shared presentation helpers in @@ -7886,6 +7891,25 @@ When the cell's value is a retained host-agent SMART temperature, it renders muted with a dotted underline and screen-reader "last known", as does a retained value in the guest card. Tooltip and Thermals rows say "(last known)", and the Thermals row carries the collection reason as its title. +The shared drawer History reads the same decision. For a physical disk, +`resourceDetailDrawerMetricsHistoryModel.ts` takes the reading from +`physicalDisk.temperature`, beside the collection state that qualifies it, +because websocket rows carry no top-level `temperature` for a disk. When that +state is set and is not `available` (an empty or absent state predates the +contract and stays current), `getResourceMetricsHistoryCurrentMetrics` offers +no current `smart_temp`. A positive retained reading then reaches +`getResourceMetricsHistoryDeferredMetrics` as a deferred metric: with no +history points the legend says "last known", never "current", and the reason +renders above the chart. +TrueNAS disk rows follow it too: the drawer's Temperature row reads +"(last known)" with the reason as title and no heat tone, and the one-line +summary leaves a retained reading out because it has no room for the reason. +The Proxmox outdated sensor setup notice reads the registry's per-node +`proxmox.sensorSetupOutdated` verdict, which counts a SATA or SAS disk as having +a temperature only when the reading was collected now +(`diskinventory.TemperatureCollected` in +`internal/unifiedresources/proxmox_sensor_setup.go`), so a retained one cannot +hide the notice for a disk Pulse cannot read now. The focused browser proofs are `frontend-modern/src/features/patrol/__tests__/patrolRunAcceptance.test.ts`, diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index 3c011447f5..ec49474ffd 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1899,11 +1899,33 @@ a live report or poll already past its mock-mode check when mock mode is switched on can still evaluate after that clear, and its alerts can persist until mock mode is left. -The fence and the clear are per monitor while the mode is process-wide. A -tenant monitor other than the one `SetMockMode` was called on neither drains -nor clears, so fixture alerts it raised outlive the switch as they did before -the fence; switching mock mode with several tenant monitors running is not -covered. +The mode is process-wide while each monitor keeps its own fence, alert manager, +fixture agents and state, and the server runs one monitor per organization, so +`SetMockMode` switches every running monitor, whichever tenant's monitor it is +called on (the mock-mode API calls the requesting tenant's, the demo-fixture +licence sync the default organization's). `Start` joins a process-wide set of +running monitors under `mockModeSwitchMu`, and leaves it when its loop returns. +A switch flips the mode once, then ends each monitor's epoch, waits for its +admitted calls, clears its alerts, forgets its fixture agents when leaving mock +mode and resets its state, and only then restarts the mode-dependent runtime of +each monitor in the set. `Start` sets its runtime context before it joins and +chooses the mock metrics sampler or discovery while it joins, under the same +lock, so no switch lands between reading the mode and starting the runtime for +it, and a switch that reaches a monitor not yet joined leaves the runtime to +its `Start`. A monitor that missed switches between `New` and `Start` first +leaves what it holds: `New` records the mode and the number of switches made so +far, and `Start` compares both, so even a round trip that brought the flag back +drops the fixture alerts a read path raised meanwhile, as well as the live +alerts `New` restored outside mock mode. A monitor that is not running is +switched only when the switch is made through it; a call for the mode the flag +already holds still moves any reached monitor whose alerts belong to the other +mode, and leaves the rest alone. + +Release builds reach several tenant monitors in mock mode only on an instance +running `DemoMode` with the `demo_fixtures` entitlement (release +`ValidateEnablement` refuses mock fixtures otherwise) and with more than one +organization's monitor running; the licence sync switches only for the +default organization. The proofs park a real evaluation at a fixed point, switch modes and release it. `TestLeavingMockModeRefusesTheRestOfAnInFlightPass` holds a mock pass at its @@ -1933,7 +1955,25 @@ previous-state carry returns fixture nodes or guests. All eight are in `TestStoreRefreshTakesItsMockModeScopeBeforeReadingState` in `internal/monitoring/canonical_guardrails_test.go` rejects a call that reads state in an argument ahead of `m.mockModeFence.begin()` and pins the -end-epoch-then-clear order in `SetMockMode`. +end-epoch-then-clear order in `endMockModeEpoch`, which `SetMockMode` runs for +every monitor it reaches. + +`TestLeavingMockModeClearsEveryRunningTenantMonitor` and +`TestEnteringMockModeClearsEveryRunningTenantMonitor` start the default and one +other tenant monitor through `MultiTenantMonitor`, wait until both have joined, +raise alerts in both, switch through one of them and check both. Against a +switch that reached only its own monitor, the default monitor kept its fixture +alerts (69 to 104 across runs) and all 69 fixture agents after leaving mock +mode through the other tenant, and that tenant kept its live alert after mock +mode was entered through the default monitor. `TestMonitorStartedAfterAMockModeSwitchLeavesTheModeItWasBuiltIn` +fails if `Start` joins without leaving the mode its monitor was built in, and +`TestSetMockModeAlignsAMonitorWhoseModeTheFlagAlreadyHolds` fails if a call for +the flag's current mode returns without moving such a monitor, and +`TestMockModeSwitchLeavesAnUnjoinedMonitorsRuntimeToItsStart` fails if a switch +starts the runtime of a monitor whose `Start` has not joined, and +`TestMonitorStartedAfterAMockModeRoundTripDropsWhatItRaisedMeanwhile` fails if +`Start` compares only the mode. All six are in +`internal/monitoring/monitor_mock_alerts_test.go`. The disk evaluation runs on the mock alert tick, not the poller's disk interval (five minutes by default, `PhysicalDiskPollingMinutes` per instance). @@ -3175,7 +3215,20 @@ Monitoring retains one current status per target and location, using server receipt time for remote freshness while keeping the agent-authored observation time as evidence metadata. A stale or disconnected agent path derives as indeterminate at read time; slow or fast agent clocks cannot manufacture or -conceal a disconnect. Aggregation is conservative: all reachable paths are +conceal a disconnect. A local path beside other locations that has not run +within the same window derives as indeterminate too ("no recent local check"). +The target takes its freshness from its newest location, so otherwise a +failing remote report kept an old local pass counted as reachable and the +check read available with current evidence. For the same reason an available +check's evidence lasts only as long as its newest reachable path, not a +fresher path that failed. When the external-probe entitlement lapses, a +single remote result stands in for the local location until the local poller +runs, with the earlier of its agent check time and the server's receipt time +as its check time, so a fast agent clock cannot keep it current. An aggregate +over several locations never stands in: a target without a retained local +observation reads as never checked, and not connected, until the local poller +runs. +Aggregation is conservative: all reachable paths are healthy, mixed reachable and failed/unknown paths are degraded, all current paths unreachable are unavailable, and no reachable path with incomplete or indeterminate coverage is unknown. Only aggregate unavailability advances the diff --git a/docs/release-control/v6/internal/subsystems/storage-recovery.md b/docs/release-control/v6/internal/subsystems/storage-recovery.md index 53b6bd3c34..1a55c2d23c 100644 --- a/docs/release-control/v6/internal/subsystems/storage-recovery.md +++ b/docs/release-control/v6/internal/subsystems/storage-recovery.md @@ -944,6 +944,15 @@ Patrol coverage, schedule, finding, and approval state belongs on Patrol-owned surfaces or explicit Patrol affordances; Proxmox overview must not treat it as backup coverage, restore readiness, PBS verification/protection proof, or a replacement for the Proxmox Backups tab and workload Backup column. +The outdated sensor setup notice above the Proxmox tabs is temperature +guidance, not storage or recovery evidence. It reads the registry's per-node +`proxmox.sensorSetupOutdated` verdict from the node rows the active tab already +loads (Overview, Storage, Replication and Backups load nodes; Ceph and Mail do +not, so it does not render there), so the Overview query must not add +`physical_disk` rows to feed it; the derived `ProxmoxData` flag opens no storage +or recovery path. +`ProxmoxPageSurface.contract.test.tsx` pins the notice on Overview with no disk +rows and an Overview query without `physical_disk`. The Proxmox inline estate totals are likewise an adjacent frontend-primitives and unified-resource projection, not backup or recovery evidence. `ProxmoxPageSurface` must flow the already-loaded workload inventory into the @@ -6886,3 +6895,14 @@ migration or recovery authority is added or moved. The performance report and reporting runtime snapshot handlers apply the same test before tabulating a disk temperature. That changes only which held value a report shows, not any storage or recovery path. + +### Demo write guard ignores websocket upgrade headers + +`internal/api/demo_middleware.go` no longer exempts every request carrying +`Upgrade: websocket` from the demo read-only guard. On a `DEMO_MODE` instance a +`POST`, `PUT`, `PATCH` or `DELETE` with websocket handshake headers is now +judged like any other write: outside the login, logout, AI execute and OIDC +allowlist it gets the generic demo `403`, or `404` on a hidden route, and that +includes config import at `/api/config/import`. HTTP/1.1 websocket handshakes +are `GET` requests and still connect. No storage, retention, backup, migration +or recovery path is added or moved. diff --git a/docs/release-control/v6/internal/subsystems/unified-resources.md b/docs/release-control/v6/internal/subsystems/unified-resources.md index 3aec852493..e812d7a3fe 100644 --- a/docs/release-control/v6/internal/subsystems/unified-resources.md +++ b/docs/release-control/v6/internal/subsystems/unified-resources.md @@ -1121,6 +1121,13 @@ for the Physical Disks verdict. The heat reason follows any native TrueNAS alert text, and a hot disk's status dot turns warning though its source state stays `online`, because phones show only the dot. `truenasPageModel.test.ts` and `TrueNASStorageTopologyTable.test.tsx` pin it. +The Temp column reads the same collection state through +`getPhysicalDiskTemperaturePresentation`. A disk temperature that is not +current, such as a host agent's last reading kept on a merged disk row after +the agent stopped reporting, renders as last known: muted, with its reason as +the title and screen-reader "last known" text. The Temp sort ranks it as no +reading, and the tree sort orders each sibling group, so a retained value +never leads its group hottest first. The vSphere Datastores and Networks tables follow the same exception-first Health column. `getVmwareDatastoreIssue` and `getVmwareNetworkIssue` return nothing for a green row; otherwise the reasons are, in order, an impaired @@ -2980,6 +2987,36 @@ or force every provider type back into the active route payload. Docker, Kubernetes, TrueNAS, and VMware surface contract tests pin their route query maps, and the shared hook test pins facet normalization. +### Proxmox node verdicts over disks arrive on the node + +The Proxmox page hydrates one query per workflow, and only its Storage tab +loads `physical_disk` rows, so a node verdict that depends on the node's disks +is derived in the registry and carried on the node, never rebuilt in the page +from disk rows. `ProxmoxData.SensorSetupOutdated` (`proxmox.sensorSetupOutdated` +on REST and websocket rows) is that verdict for the outdated sensor setup +notice. `refreshProxmoxSensorSetupLocked` +(`internal/unifiedresources/proxmox_sensor_setup.go`) runs after +`buildChildCounts` on every ingest path and gives every Proxmox node an +explicit true or false (other resources omit it), because browser facet merges +read an omitted field as a partial snapshot and would keep an earlier true. It +is true when the node's temperature payload is available and in the legacy +`sensors -j` format (`models.Temperature.LegacySensorsFormat`) and a disk whose +canonical parent is the node has no reading collected now +(`diskinventory.TemperatureCollected`) and a type whose temperature a PVE node +gets only through SMART: `sata`, `sas`, or the `hdd` and `ssd` form factors +Proxmox's own inventory reports. Proxmox also types a non-rotational USB device +`ssd`, so a USB device without SMART counts. A retained reading does not count +as current, including one kept while a host agent reports the disk in standby; +a reading a linked host agent collects now clears the disk. NVMe disks never +set it, because kernel hwmon reports them even on a legacy setup, and a host +agent the registry has not linked to the node contributes no disks. The +verdict reflects the registry's node and disk state at each ingest. +`TestProxmoxNodeSensorSetupOutdatedFromItsDisks` and +`TestProxmoxNodeSensorSetupOutdatedFollowsWhereTheReadingArrives` in +`internal/unifiedresources/registry_test.go` pin the rule, the linked-agent +case, the explicit false on the wire and the presented JSON; +`resourceStateAdapters.test.ts` pins that a false clears a merged true. + ### Canonical REST facets preserve realtime workload evidence The frontend REST projection retains the complete source-authored Proxmox @@ -3424,6 +3461,20 @@ It owns probe status, incidents, history, evidence, and the outgoing `checks` relationship. The matched resource carries every correlated check in the canonical `availabilityChecks` facet, keyed by saved target id, while `availability` remains an additive singular compatibility summary selected from that set. +The summary is the worst check: a confirmed outage first, then any other +observed failure, then an unchecked, ambiguous or unresolved check, then a +passing one, ties going to the first saved target id. A confirmed outage is an +enabled check whose observed failures reach its failure threshold, with an +unavailable aggregate when one is reported and an outcome that is not +indeterminate. That is the gate the availability poller applies before it +raises `availability_unreachable`, so a probe agent that stops reporting keeps +its old failure count but confirms nothing. `availabilityOutageConfirmed` in +`internal/unifiedresources/availability.go` holds that rule for both the +ranking and `EvaluateResourceHealth`, which reads the summary. A confirmed +outage on any attached check therefore makes the resource's health critical +even when a check that sorts earlier has failed below its threshold, with the +`availability_failed` reason unless a critical alert explains the verdict +first. Adding a second explicit or unambiguously correlated check must retain both source-owned endpoint rows, project both facets onto the same resource, and emit one `checks` relationship per target from the check to that resource. @@ -6177,9 +6228,20 @@ pass and manual links both apply that function: the best of equal ones. A quiet source drops out of that decision. - Once every source is quiet, an `offline` verdict survives and any other verdict reads `warning`. The best of those wins. -- A current facet sighting carries no verdict (the PBS host-agent association, - an availability check projected onto its target). It counts as `online` only - when no current source has a verdict, and reads `warning` once quiet. +- A current facet sighting carries no verdict (the PBS host-agent association). + It counts as `online` only when no current source has a verdict, and reads + `warning` once quiet. +- Availability checks projected onto a monitored resource rank like that facet, + but their shared sighting is not their verdict. `availabilityChecksProveOnline` + judges each check at the pass's `now` by its own evidence window + (`Evidence.ValidUntil`: two poll intervals for a single local check, the + probe report window for a remote or multi-location one). An enabled, + passing check with current evidence proves the resource answers and reads + `online` once one of the resource's own sources has gone quiet and none that + is current has a verdict. Every other check abstains, current or quiet: a + failing check proves only that one port or service does not answer, and a + pass whose evidence lapsed proves nothing now. A check's own row keeps the ordinary + rules. The stale pass used to read delivery as the verdict. A Proxmox node the cluster reported offline on a live poll came back `online` once its linked agent fell @@ -6203,24 +6265,52 @@ online stays `online` when its linked agent stops reporting. A live agent keeps a node `online` when the Proxmox poll reports it offline, which the Proxmox nodes table reads with `connectionHealth: error` as a stale provider. When a guest's or container's only source goes quiet, a running one stays `warning` -and a stopped one stays `offline`. +and a stopped one stays `offline`. In each quiet case a projected availability +check with current evidence lifts the resource to `online`, as described +below. The sighting still reads `stale`, so health keeps its `telemetry_stale` reason and monitored-system reasons are unchanged. In-memory clones keep the verdicts, including a facet's missing one. A serialized copy carries none, so `IngestResources` gives each sighting of a copy that lost them all the stored resource status; a merged row's separate source verdicts do not survive that -round trip. A manual link that joins two resources reported by one source keeps +round trip, and a copy whose status a passing check supplied gives that status +to every source. A manual link that joins two resources reported by one source keeps the fresher sighting. `registry_merge_policy_test.go` pins the node, poller-expired, guest, round-trip, copy, agent, Docker, Kubernetes, mixed-source and manual-link cases, and the aggregation table. -Availability checks still count by delivery. With every source that has a -verdict quiet, a current check keeps its target `online` even when the check -fails, as it did before. Several checks share one sighting whose freshness -comes from whichever check was projected last, so an availability verdict -needs each check's own freshness, including local checks that miss their -cadence, and has to survive manual links. That is left to a follow-up. +Availability checks used to count by delivery too. With every source that +has a verdict quiet, a current failing check kept a node `preserveOrExpireNodes` +expired `online`, and a quiet one lifted it to `warning`. Several checks share +one sighting whose freshness comes from whichever check was projected last, so +a failing check that just ran could also vouch for another check's old pass. +Each check is now judged by its own evidence. A failing probe no longer holds +an expired node `online`, and it never makes a guest read stopped to the +consumers that count `offline` as stopped (the workloads summary, the Proxmox +page counts). The failure stays on the check's own row, which raises an +outage incident once the check confirms one (past its failure threshold, from +every location). A check observed from several locations relies on the poller +ageing out a location that stopped running and lasting an available check's +evidence only as long as its newest reachable path (monitoring.md), so one +location's fresh failure cannot lend its freshness to another location's old +pass. + +The verdict is read from the checks on the resource when status is +aggregated. Projecting, re-running or retargeting a check also re-applies the +stale pass's rule to every target it touches whose status that pass owns, +because a check is not a delivery from the target's own sources and the +resources API replays checks after its stale pass. The pass owns a status only +through the target's own sightings: the checks' sighting going quiet neither +hands it the target nor, once removed, takes the target away, so a target +whose own sources have not gone quiet keeps the status they gave it. A check +retargeted away therefore takes its verdict with it at once. A +manual link keeps only the primary's own checks, so the merged resource is +judged by the checks it shows and never by the linked resource's fresher +sighting. `registry_test.go` pins the expired-node and quiet-guest cases, both +projection orders, re-judging on check changes, ownership through the +target's own sightings and the manual link; the aggregation table pins the +check rules. ### Canonical object drawer hierarchy @@ -6427,9 +6517,52 @@ conflict): the agent's row may be the slot's previous occupant, retained by a silent agent, and the join would hand the replacement that disk's serial, readings and canonical resource. A hardware identity match still joins on its own, and without a WWN on either side the path fallback stands. The seeded -source mapping reused after a restart is a separate path this does not cover. -`TestRegistrySASPathJoinRefusesContradictingWWN` covers the stale row and both -same-disk shapes; monitoring's SMART merge applies the matching guard first. +source mapping a rehydrated registry reuses applies the same refusal (next +section). `TestRegistrySASPathJoinRefusesContradictingWWN` covers the stale +row and both same-disk shapes; monitoring's SMART merge applies the matching +guard first. + +### Seeded slot mapping refuses a replaced disk + +`ingestRecord` honours a source key's existing mapping, such as one +`IngestResources` seeds from persisted unified resources, before any +identity matching. Proxmox keys a disk by its slot +(`ProxmoxPhysicalDiskSourceID`), so a disk swapped into the slot arrived +under the previous disk's key, and `mergeInto` kept that disk's canonical ID +and, where an agent report had been merged in, its serial, WWN, temperature +and failed health. A Proxmox disk observation now refuses the mapping when +the two carry hardware identities naming different disks +(`proxmoxDiskSlotHoldsAnotherDisk`: `HardwareIdentityConflict` after +`HardwareIdentityMatch`). Only fields reported on both sides count, so a +missing serial or WWN, including Proxmox's literal `unknown`, is no evidence. +Differing serials are no evidence when either side is SAS, because Proxmox +may report the SAS address there (#1595), or when the mapped disk was merged +with an agent report, whose serial `mergeInto` may have kept (collection +status does not reliably say which serial it holds), unless Proxmox read the +drive's own serial too, by the rule monitoring's +`hostAgentSMARTSerialComparable` applies: NVMe on both sides, or an agent +SATA disk under Proxmox vendor `ATA` that Proxmox does not type `usb`, not a +SCSI designator or a USB bridge's serial. A refused key falls through to the linked-disk join, +`findMatch` and `chooseNewID` like an unmapped one, except that none of them +may merge it into the refused disk: an ID that lands on it is replaced with +the key's source-specific ID. Agreement still wins, as it does in identity +matching, so drives that share a serial they do not own, such as a USB +bridge's, read as one disk: a second swap in the same enclosure joins the +first replacement, which reported the same bridge serial. That belongs to +identity matching, not this guard. The refused disk keeps its Proxmox +facet and is not updated, so until the registry is rebuilt both disks answer +to the slot's PVE alert reference, and canonical reference resolution treats +that reference as ambiguous rather than handing it to either. A later +rehydration may seed the slot key to the old disk again, and the refusal +repeats. No current caller ingests Proxmox disk records over a seeded +mapping: the monitor rebuilds a fresh registry each poll, and the resource +API, host-continuity and Patrol registries seeded from unified resources do +not replay Proxmox disk records. This closes the registry contract, not a +reported symptom. `TestRegistrySeededSlotMappingRefusesReplacedDisk` covers +SATA (with and without a new WWN), SAS, USB-enclosure and Proxmox-only +replacements, plus eight same-disk controls (including a SCSI designator +from an agent without collection status and a USB bridge serial under +Proxmox vendor `ATA`), each through two rehydrations. ### Drawer tab selection survives a transient snapshot change (#1723) diff --git a/frontend-modern/browser-verification/15418f6d41666705.json b/frontend-modern/browser-verification/15418f6d41666705.json new file mode 100644 index 0000000000..2ea8e947d3 --- /dev/null +++ b/frontend-modern/browser-verification/15418f6d41666705.json @@ -0,0 +1,46 @@ +{ + "version": 1, + "base_sha": "e02edd9989c88fbe6e5459afdf488c73538ddd24", + "verified_at": "2026-10-07T12:56:24Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/features/platformPage/sensorSetup.ts", + "frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx", + "frontend-modern/src/hooks/useUnifiedResources.ts", + "frontend-modern/src/types/resource.ts" + ], + "content_sha256": { + "frontend-modern/src/features/platformPage/sensorSetup.ts": "18e4da01eb9206bce83a2e7d2fbb9f97313685c5655d620092a9e0ca51804ef5", + "frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx": "db074960c69c59bb030814a04028a50ee5b347b7cf16b04b4c6edea708ffd782", + "frontend-modern/src/hooks/useUnifiedResources.ts": "77cc56a11213f951e051ceabd05bc2bf3161adedadc3586f2badee98bf1056c3", + "frontend-modern/src/types/resource.ts": "37e3ddb2e1334472a280e0ef93053292002567acabf84aace9fed1735a763b57" + }, + "routes": [ + "/proxmox", + "/proxmox/storage", + "/proxmox/replication", + "/proxmox/backups" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Private mock stack (8-node estate) built from this tree with a local-only generator overlay: pve2, pve3 and pve5 report the legacy sensors -j payload; pve2 has a Proxmox-inventory hdd disk with no temperature, pve3 a SATA disk whose reading is retained (host agent stopped reporting), pve5 a SATA disk with no temperature.", + "Overview, Storage, Replication and Backups each show the warning notice naming pve2, pve3 and pve5 at both viewports. The Overview query is type=agent,vm,system-container,oci-container and no physical_disk request is made on Overview, Replication or Backups; only Storage requests disk rows.", + "Before control on the same stack with origin/main's ProxmoxPageSurface.tsx and sensorSetup.ts swapped in: no notice on Overview, Replication or Backups, and Storage names only pve5 (hdd type and retained reading missed).", + "Websocket frames carry sensorSetupOutdated true on every tab. Fresh desktop load: no console errors and no 5xx responses. Notice text fits the 390px viewport without overflow; the node table stays below it in reading order.", + "Mock data only; not a real pre-rc.6 SSH node, installed upgrade or release acceptance." + ], + "interactions": [ + "Navigated each tab by URL at 1440x900 and 390x844 and waited 20s on Overview to outlast websocket row merges; the notice stayed.", + "Live clearing without reload: switching pve5's disk to a current 37C reading dropped pve5 from the notice (3 nodes to pve2, pve3) on the next websocket update. Control with the earlier omitempty backend kept pve5 for 90s.", + "Open Infrastructure settings link targets /settings/infrastructure (contract test)." + ] +} diff --git a/frontend-modern/browser-verification/294409f678589ef2.json b/frontend-modern/browser-verification/294409f678589ef2.json new file mode 100644 index 0000000000..726e7fa021 --- /dev/null +++ b/frontend-modern/browser-verification/294409f678589ef2.json @@ -0,0 +1,52 @@ +{ + "version": 1, + "base_sha": "41cea2ffab5ca2d39240d14138975bc4e902bb61", + "verified_at": "2026-10-07T11:28:58Z", + "result": "passed", + "changed_paths": [ + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts", + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts", + "frontend-modern/src/features/platformPage/sensorSetup.ts", + "frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx" + ], + "content_sha256": { + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts": "e6e8b46a156df4a1d1b391de19f041c0a1f74eb7e2ecdb78fdfa773a4c336262", + "frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts": "fa1c743eaec09f424c80aa5d3aee9082817e54353a853b542620eb45792f5887", + "frontend-modern/src/features/platformPage/sensorSetup.ts": "5b92f1fc07b513a11f0e8f2654b0717baa4b99d742eae76368844731a5f89eb9", + "frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx": "dd7c5189a950adba606a7ea4d0d40233d158e965883135b6fe82b772a6dcf5cd" + }, + "routes": [ + "/truenas/storage?kind=disks", + "/proxmox/storage", + "/proxmox" + ], + "viewports": [ + { + "width": 1440, + "height": 900 + }, + { + "width": 390, + "height": 844 + } + ], + "states": [ + "Vite dev server on this tree (origin/main 41cea2ffab plus the change) against a private PULSE_MOCK_MODE backend (8-node estate) built with a local-only go build -overlay (never committed). The overlay stamps TrueNAS disk sdb with physicalDisk.temperature 61 and collection.temperature unavailable 'host agent stopped reporting' (the shape a TrueNAS disk merged with a silent host agent row reaches), pins sdd to a current 58, gives pve3 SATA disk /dev/nvme2 the same retained state (51) and marks pve3's sensor payload legacySensorsFormat; /api/resources and the websocket rows carried those states", + "Before control in the same session (the four runtime files swapped to origin/main under the running Vite, then restored and compared byte for byte): sdb Temp cell plain 61.0°C sorting first in hottest-first, drawer Temperature row 61°C in the amber warning tone, TrueNAS summary 'SATA, Passed, 16.0 TB, 61°C', History with no smart_temp points showing '-', and no outdated sensor setup notice for pve3", + "After, TrueNAS storage disks view: sdb Temp cell '61.0°C' muted (rgb 71,85,105) with a dotted underline, title 'Last known reading, not current: host agent stopped reporting', data-temperature-reading=last-known and screen-reader ', last known'; current disks (sdd 58.0°C, sda 46.0°C) unchanged; no Attention reason for sdb", + "After, Temp sort: descending sdd 58, sdf 47, nvme0n1 46, sda 46, sde 42, nvme1n1 41, sdc 39, then sdb last; ascending sdc 39 ... sdd 58, then sdb last (a retained reading sorts as no reading)", + "After, sdb row drawer (Platform details > Show TrueNAS): Health Temperature row '61°C (last known)' in text-muted with the reason as its title; the collapsed TrueNAS summary reads 'SATA, Passed, 16.0 TB' with no temperature", + "After, sdb drawer History tab: with seeded points the legend shows the stored 53°C and the line 'Disk reading: Last known reading, not current: host agent stopped reporting.'; with the smart_temp series emptied by a Playwright route the legend reads 'Disk 61°C last known' (data-history-last-known=smart_temp, no 'current') with the same line; current disk sdd with no points reads 'Disk 58°C current'", + "After, /proxmox/storage: 'pve3 is using an older temperature monitoring setup that cannot read SATA/SAS disk temperatures. Re-run the node setup script to upgrade it.' notice shows at both widths (hidden on main); /proxmox Overview loads no physical disks so the notice cannot show there on main or after (pre-existing, chipped separately)", + "No page errors at 1440x900 or 390x844", + "Re-run after review edits (History model reads a disk's value only from physicalDisk, never a top-level fallback): every reading above was unchanged at both widths" + ], + "interactions": [ + "Opened /truenas/storage?kind=disks at 1440x900 and 390x844 and read every disk row's Temp cell (text, title, data attribute, computed colour and decoration)", + "Clicked the Temp column header twice at 1440x900 and read the disk row order for descending and ascending", + "Opened the sdb row drawer with 'Expand details for sdb', opened Platform details and clicked 'Show TrueNAS', read the summary and the Health Temperature row at both widths", + "Opened the drawer History tab with stored points, returned to Overview, then reopened History with the smart_temp series emptied, and read the Temperature legend and reason line at both widths; repeated for current disk sdd", + "Opened /proxmox and /proxmox/storage at both widths and read the outdated sensor setup notice", + "Ran the same Playwright pass against the origin/main runtime files as the before control, restored the change and confirmed the full git diff was byte-identical" + ] +} diff --git a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts index bae3d26c0e..9fe9a0ac3b 100644 --- a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts +++ b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerMetricsHistoryModel.branchcov0712.test.ts @@ -513,6 +513,66 @@ describe('getResourceMetricsHistoryCurrentMetrics branch coverage', () => { }); }); + it('offers a retained physical disk temperature only as last known, with its reason', () => { + // useUnifiedResources copies physicalDisk.temperature onto the resource. + // A disk in standby keeps its last reading under an unavailable state, so + // with no history points the chart must not label it current. + const disk = (state?: 'available' | 'unavailable', temperature = 47) => + baseResource({ + type: 'physical_disk', + metricsTarget: { resourceType: 'disk', resourceId: 'SERIAL-1' }, + temperature, + physicalDisk: { + devPath: '/dev/sdb', + temperature, + collection: state + ? { temperature: { state, source: 'smartctl', reason: 'disk is in standby' } } + : undefined, + }, + }); + + expect(getResourceMetricsHistoryCurrentMetrics(disk('unavailable'))).toMatchObject({ + smart_temp: undefined, + temperature: undefined, + }); + expect(getResourceMetricsHistoryDeferredMetrics(disk('unavailable')).smart_temp).toEqual({ + lastKnownValue: 47, + message: 'Last known reading, not current: disk is in standby.', + }); + // An unavailable state with no retained reading has nothing to offer. + expect(getResourceMetricsHistoryCurrentMetrics(disk('unavailable', 0)).smart_temp).toBe( + undefined, + ); + expect(getResourceMetricsHistoryDeferredMetrics(disk('unavailable', 0)).smart_temp).toBe( + undefined, + ); + + for (const current of [disk('available'), disk()]) { + expect(getResourceMetricsHistoryCurrentMetrics(current).smart_temp).toBe(47); + expect(getResourceMetricsHistoryDeferredMetrics(current).smart_temp).toBeUndefined(); + } + + // Websocket rows carry the reading only on physicalDisk, beside its state. + const websocketRow = (state: 'available' | 'unavailable') => { + const row = disk(state); + delete row.temperature; + return row; + }; + expect(getResourceMetricsHistoryCurrentMetrics(websocketRow('available')).smart_temp).toBe(47); + expect( + getResourceMetricsHistoryCurrentMetrics(websocketRow('unavailable')).smart_temp, + ).toBeUndefined(); + expect( + getResourceMetricsHistoryDeferredMetrics(websocketRow('unavailable')).smart_temp, + ).toMatchObject({ lastKnownValue: 47 }); + + // A top-level value never stands in for the disk's own missing reading, + // which the disk's collection state could not qualify. + const unpaired = disk('available'); + delete unpaired.physicalDisk!.temperature; + expect(getResourceMetricsHistoryCurrentMetrics(unpaired).smart_temp).toBeUndefined(); + }); + it('uses the source-aware host subset for API-only Proxmox node targets', () => { const resource = baseResource({ type: 'agent', diff --git a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts index 539aee7b89..1afd532170 100644 --- a/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts +++ b/frontend-modern/src/components/Infrastructure/__tests__/resourceDetailDrawerTrueNASModel.test.ts @@ -361,6 +361,43 @@ describe('resourceDetailDrawerTrueNASModel', () => { ]); }); + it('shows a retained disk temperature as last known, with its reason and no heat tone', () => { + // A TrueNAS disk merged with a host agent's row keeps the agent's last + // reading after the agent stops reporting. It is not current, so it must + // not read as a disk running hot now, nor sit in the summary as a reading. + const disk = (state: 'available' | 'unavailable') => + baseResource({ + type: 'physical_disk', + displayName: 'sdc', + platformScopes: ['truenas'], + physicalDisk: { + devPath: '/dev/sdc', + diskType: 'sata', + sizeBytes: 24 * 1024 ** 4, + health: 'PASSED', + temperature: 61, + collection: { + temperature: { state, source: 'agent', reason: 'host agent stopped reporting' }, + }, + }, + }); + const temperatureRow = (resource: Resource) => + buildTrueNASDetailSections(resource) + .find((section) => section.label === 'Health') + ?.rows.find((row) => row.label === 'Temperature'); + + expect(temperatureRow(disk('unavailable'))).toMatchObject({ + value: '61°C (last known)', + tone: 'muted', + title: 'Last known reading, not current: host agent stopped reporting', + }); + expect(buildTrueNASDetailsSummary(disk('unavailable'))).toBe('SATA, Passed, 24.0 TB'); + + expect(temperatureRow(disk('available'))).toMatchObject({ value: '61°C', tone: 'warning' }); + expect(temperatureRow(disk('available'))?.title).toBeUndefined(); + expect(buildTrueNASDetailsSummary(disk('available'))).toBe('SATA, Passed, 24.0 TB, 61°C'); + }); + it('shows the vdev layout as the storage kind while topology stays the pool discriminator', () => { // Regression guard for 599c8e634. `topology` is the cross-provider // discriminator ('pool' | 'dataset' | ...); the ZFS layout belongs in diff --git a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts index 2fc02a3e4c..8d4ee55a4b 100644 --- a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts +++ b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerMetricsHistoryModel.ts @@ -14,6 +14,10 @@ import { import type { HostGPUSensor } from '@/types/api'; import type { Resource } from '@/types/resource'; import { getDiskPercent, getMemoryPercent } from '@/types/resource'; +import { + getPhysicalDiskTemperaturePresentation, + isPhysicalDiskTemperatureCurrent, +} from '@/features/storageBackups/diskTemperaturePresentation'; import { asTrimmedString } from '@/utils/stringUtils'; import { getMemoryObservationPresentation } from '@/utils/memoryObservation'; import { getWorkloadGuestDiskStatusMessage } from '@/utils/workloadGuestPresentation'; @@ -174,6 +178,18 @@ const diskReadReason = (resource: Resource): string | undefined => ? resource.proxmox?.diskStatusReason || undefined : undefined; +// A physical disk's reading and its collection state both live on +// physicalDisk, so read them only together: websocket rows carry no top-level +// temperature for a disk, which only the REST mapper copies across. The +// reading may be a last known one kept while the current observation could +// not collect it, such as a disk in standby or a host agent past its +// reporting lease, and is then never current. +const resourceTemperature = (resource: Resource): number | undefined => + resource.physicalDisk ? resource.physicalDisk.temperature : resource.temperature; + +const isResourceTemperatureCurrent = (resource: Resource): boolean => + isPhysicalDiskTemperatureCurrent(resource.physicalDisk?.collection); + export const getResourceMetricsHistoryDeferredMetrics = ( resource: Resource, ): Record => { @@ -197,6 +213,19 @@ export const getResourceMetricsHistoryDeferredMetrics = ( message: getWorkloadGuestDiskStatusMessage(reason), }; } + if (!isResourceTemperatureCurrent(resource)) { + const temperature = resourceTemperature(resource); + const retained = getPhysicalDiskTemperaturePresentation({ + temperature: temperature ?? 0, + collection: resource.physicalDisk?.collection, + }); + if (retained?.title) { + metrics.smart_temp = { + lastKnownValue: temperature, + message: `${retained.title}.`, + }; + } + } return metrics; }; @@ -204,7 +233,9 @@ export const getResourceMetricsHistoryCurrentMetrics = ( resource: Resource, ): Record => { const diskPercent = resource.disk ? finiteMetric(getDiskPercent(resource)) : undefined; - const temperature = finiteMetric(resource.temperature); + const temperature = isResourceTemperatureCurrent(resource) + ? finiteMetric(resourceTemperature(resource)) + : undefined; const memory = memoryReading(resource); return { cpu: finiteMetric(resource.cpu?.current), diff --git a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts index f8a666aa5d..57d3fca8c9 100644 --- a/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts +++ b/frontend-modern/src/components/Infrastructure/resourceDetailDrawerTrueNASModel.ts @@ -20,6 +20,7 @@ import { type DetailSection, type DetailValueTone, } from '@/components/shared/detailSectionModel'; +import { getPhysicalDiskTemperaturePresentation } from '@/features/storageBackups/diskTemperaturePresentation'; import { hasImpairedResourceSource } from '@/utils/resourceSourceHealth'; export type ResourceDetailDrawerTrueNASRowTone = DetailValueTone; @@ -51,11 +52,6 @@ const formatPercent = (percent?: number): string | null => { return `${percent.toFixed(percent >= 10 ? 1 : 2)}%`; }; -const formatTemperature = (celsius?: number): string | null => { - if (typeof celsius !== 'number' || !Number.isFinite(celsius) || celsius <= 0) return null; - return `${celsius.toFixed(0)}°C`; -}; - const formatDurationSeconds = (seconds?: number): string | null => { const value = asPositiveNumber(seconds); if (!value) return null; @@ -396,6 +392,35 @@ const formatDiskHours = (hours?: number): string | null => { return `${formatDetailIntegerValue(value) ?? value.toFixed(0)}h`; }; +const diskTemperature = (disk: ResourcePhysicalDiskMeta) => + getPhysicalDiskTemperaturePresentation({ + temperature: disk.temperature ?? 0, + collection: disk.collection, + }); + +// A retained reading, such as a silent host agent's on a merged disk row, is +// not current: it takes no heat tone and its title says why. +const diskTemperatureRow = ( + disk: ResourcePhysicalDiskMeta, +): ResourceDetailDrawerTrueNASRow | null => { + const reading = diskTemperature(disk); + if (!reading) return null; + if (!reading.current) { + return row('Temperature', `${reading.label} (last known)`, { + title: reading.title, + tone: 'muted', + }); + } + return row('Temperature', reading.label, { + tone: (disk.temperature ?? 0) >= 55 ? 'warning' : 'default', + }); +}; + +const currentDiskTemperatureLabel = (disk: ResourcePhysicalDiskMeta): string | null => { + const reading = diskTemperature(disk); + return reading?.current ? reading.label : null; +}; + const buildTrueNASDiskSections = ( resource: Resource, disk: ResourcePhysicalDiskMeta, @@ -414,9 +439,7 @@ const buildTrueNASDiskSections = ( const healthRows = compactRows([ row('Health', normalizeDelimitedLabel(disk.health), { tone: diskStateTone(disk) }), - row('Temperature', formatTemperature(disk.temperature), { - tone: disk.temperature && disk.temperature >= 55 ? 'warning' : 'default', - }), + diskTemperatureRow(disk), row( 'Wearout', disk.wearout === undefined || disk.wearout < 0 ? null : formatPercent(disk.wearout), @@ -795,7 +818,8 @@ export const buildTrueNASDetailsSummary = (resource: Resource): string | null => diskTypeLabel(disk.diskType), normalizeDelimitedLabel(disk.health), formatDetailBytesValue(disk.sizeBytes), - formatTemperature(disk.temperature), + // The summary has no room to say a retained reading is not current. + currentDiskTemperatureLabel(disk), ].filter((value): value is string => Boolean(value)); return summary.length > 0 ? summary.join(', ') : null; } diff --git a/frontend-modern/src/features/platformPage/__tests__/sensorSetup.branchcov0712.test.ts b/frontend-modern/src/features/platformPage/__tests__/sensorSetup.branchcov0712.test.ts deleted file mode 100644 index ce1e3a6d06..0000000000 --- a/frontend-modern/src/features/platformPage/__tests__/sensorSetup.branchcov0712.test.ts +++ /dev/null @@ -1,208 +0,0 @@ -import { describe, expect, it } from 'vitest'; -import type { Resource } from '@/types/resource'; -import { collectOutdatedSensorSetupNodes } from '../sensorSetup'; - -// Branch-coverage tests for the two UNEXPORTED helpers inside sensorSetup.ts: -// - getNodeName (3-way `||` fallback chain + `.trim()`) -// - isSMARTOnlyDiskWithoutTemperature (no-meta guard, diskType normalize, -// SMART-set membership, `temperature ?? 0`) -// Both helpers are private, so every case drives them through the public -// `collectOutdatedSensorSetupNodes` and asserts the concrete observable shape. -// -// Conventions mirror the sibling `sensorSetup.test.ts` fixtures. Disks are -// linked to nodes either by `platformData.proxmox.nodeName` (the name-matching -// path, used to prove `getNodeName` resolution) or by `parentId` (which makes -// `matchesPhysicalDiskNode` short-circuit true, used to isolate the SMART-only -// predicate from name resolution). - -type LegacyNodeOver = { - id?: string; - name?: string; - proxmox?: Partial; -}; - -const legacyNode = (over: LegacyNodeOver = {}): Resource => - ({ - id: over.id ?? 'node-1', - name: over.name ?? '', - displayName: over.name ?? '', - type: 'node', - proxmox: { - temperatureDetails: { available: true, legacySensorsFormat: true }, - ...(over.proxmox ?? {}), - }, - }) as unknown as Resource; - -type DiskOver = { - id?: string; - parentId?: string; - nodeName?: string; - diskType?: string; - temperature?: number; - noMeta?: boolean; -}; - -const disk = (over: DiskOver = {}): Resource => - ({ - id: over.id ?? 'disk-1', - name: 'disk', - displayName: 'disk', - type: 'physical_disk', - parentId: over.parentId, - physicalDisk: over.noMeta - ? undefined - : { - diskType: over.diskType, - temperature: over.temperature, - }, - platformData: over.nodeName ? { proxmox: { nodeName: over.nodeName } } : {}, - }) as unknown as Resource; - -describe('getNodeName (via collectOutdatedSensorSetupNodes)', () => { - it('falls back to proxmox.nodeName when proxmox.node is absent', () => { - const node = legacyNode({ id: 'n-nodeName', proxmox: { nodeName: 'bravo' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - // Disk resolves to the node by the nodeName-derived identity, proving - // getNodeName returned 'bravo' (not '' and not node.name). - [disk({ id: 'd1', nodeName: 'bravo', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-nodeName', name: 'bravo' }]); - }); - - it('falls back to the resource name when both proxmox.node and nodeName are absent', () => { - const node = legacyNode({ id: 'n-name', name: 'charlie' }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd2', nodeName: 'charlie', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-name', name: 'charlie' }]); - }); - - it('trims surrounding whitespace from proxmox.node before using it as the node identity', () => { - const node = legacyNode({ id: 'n-trim', proxmox: { node: ' delta ' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - // matchesPhysicalDiskNode lowercases/trims both sides, so 'delta' must be - // the trimmed getNodeName output for the disk to match. - [disk({ id: 'd3', nodeName: 'delta', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-trim', name: 'delta' }]); - }); - - it('returns the empty string when every name source is blank, observable via a parentId-linked disk', () => { - // No proxmox.node, no proxmox.nodeName, name is whitespace -> getNodeName - // hits the final `|| ''` arm and trims to ''. The disk is matched by - // parentId so matchesPhysicalDiskNode short-circuits true regardless of the - // (empty) name, letting the flagged node surface with name: ''. - const node = legacyNode({ id: 'n-empty', name: ' ' }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd4', parentId: 'n-empty', diskType: 'sata', temperature: 0 })], - ); - expect(result).toStrictEqual([{ id: 'n-empty', name: '' }]); - }); -}); - -describe('isSMARTOnlyDiskWithoutTemperature (via collectOutdatedSensorSetupNodes)', () => { - it('returns false for a disk with no physicalDisk metadata, so the node is not flagged', () => { - // Disk is linked to the node by parentId (match guaranteed true); the only - // reason the node drops out is the `if (!meta) return false` guard. - const node = legacyNode({ id: 'n-nometa', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-nometa', parentId: 'n-nometa', noMeta: true })], - ); - expect(result).toStrictEqual([]); - }); - - it('returns false when diskType is missing, so a matched disk does not flag the node', () => { - // diskType undefined -> `(meta.diskType || '')` -> '' not in the SMART-only - // set. parentId link isolates the cause to the diskType check. - const node = legacyNode({ id: 'n-notype', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-notype', parentId: 'n-notype', temperature: 0 })], - ); - expect(result).toStrictEqual([]); - }); - - it('normalizes a whitespace-padded uppercase SAS diskType and flags the node', () => { - // Exercises `diskType.trim().toLowerCase()` -> 'sas' (in the SMART-only - // set) and temperature 0 -> `(0 ?? 0) <= 0` true. - const node = legacyNode({ id: 'n-sas', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [ - disk({ - id: 'd-sas', - parentId: 'n-sas', - diskType: ' SAS ', - temperature: 0, - }), - ], - ); - expect(result).toStrictEqual([{ id: 'n-sas', name: 'pve' }]); - }); - - it('uses 0 in place of an undefined temperature and flags the node', () => { - // temperature is omitted entirely -> `(meta.temperature ?? 0)` takes the - // nullish right-hand side (0) -> `<= 0` true. - const node = legacyNode({ id: 'n-undef-temp', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-undef-temp', parentId: 'n-undef-temp', diskType: 'sata' })], - ); - expect(result).toStrictEqual([{ id: 'n-undef-temp', name: 'pve' }]); - }); - - it('uses 0 in place of an explicit null temperature and flags the node', () => { - // Deliberately-malformed payload: temperature typed as number | undefined - // but carrying null. The `?? 0` coalesces it to 0 -> `<= 0` true. - const node = legacyNode({ id: 'n-null-temp', proxmox: { node: 'pve' } }); - const nullTempDisk = { - id: 'd-null-temp', - name: 'disk', - displayName: 'disk', - type: 'physical_disk', - parentId: 'n-null-temp', - physicalDisk: { diskType: 'sata', temperature: null }, - } as unknown as Resource; - const result = collectOutdatedSensorSetupNodes([node], [nullTempDisk]); - expect(result).toStrictEqual([{ id: 'n-null-temp', name: 'pve' }]); - }); - - it('flags the node for a SATA disk reporting a negative temperature', () => { - // Concrete negative value exercises the `<= 0` true arm (not the ?? arm). - const node = legacyNode({ id: 'n-neg-temp', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-neg-temp', parentId: 'n-neg-temp', diskType: 'sata', temperature: -5 })], - ); - expect(result).toStrictEqual([{ id: 'n-neg-temp', name: 'pve' }]); - }); - - it('does not flag the node when a SMART-only disk reports a positive temperature', () => { - // SAS at 41 degrees -> `(41 ?? 0) <= 0` is false. - const node = legacyNode({ id: 'n-warm', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [disk({ id: 'd-warm', parentId: 'n-warm', diskType: 'sas', temperature: 41 })], - ); - expect(result).toStrictEqual([]); - }); - - it('evaluates the no-meta guard on the first disk but still flags the node via a second matching disk', () => { - // Drives the `!meta` branch on d-bad, then `.some()` advances to d-good - // which is SMART-only without a temperature -> node flagged. - const node = legacyNode({ id: 'n-mixed', proxmox: { node: 'pve' } }); - const result = collectOutdatedSensorSetupNodes( - [node], - [ - disk({ id: 'd-bad', parentId: 'n-mixed', noMeta: true }), - disk({ id: 'd-good', parentId: 'n-mixed', diskType: 'sata', temperature: 0 }), - ], - ); - expect(result).toStrictEqual([{ id: 'n-mixed', name: 'pve' }]); - }); -}); diff --git a/frontend-modern/src/features/platformPage/sensorSetup.test.ts b/frontend-modern/src/features/platformPage/sensorSetup.test.ts index 9d42cbecc9..949c649186 100644 --- a/frontend-modern/src/features/platformPage/sensorSetup.test.ts +++ b/frontend-modern/src/features/platformPage/sensorSetup.test.ts @@ -3,124 +3,55 @@ import type { Resource } from '@/types/resource'; import { collectOutdatedSensorSetupNodes } from './sensorSetup'; const node = (over: Partial & { proxmox?: Resource['proxmox'] }): Resource => - ({ id: 'node-1', name: 'pve1', type: 'node', ...over }) as Resource; + ({ id: 'node-1', name: 'pve1', type: 'agent', ...over }) as Resource; -const disk = (over: { - id?: string; - parentId?: string; - nodeName?: string; - diskType?: string; - temperature?: number; -}): Resource => - ({ - id: over.id ?? 'disk-1', - name: 'disk', - type: 'physical_disk', - parentId: over.parentId, - physicalDisk: { - diskType: over.diskType ?? 'sata', - temperature: over.temperature ?? 0, - }, - platformData: over.nodeName ? { proxmox: { nodeName: over.nodeName } } : {}, - }) as unknown as Resource; - -const legacyNode = (over: Partial = {}): Resource => +const flaggedNode = (over: Partial = {}): Resource => node({ proxmox: { node: 'pve1', temperatureDetails: { available: true, legacySensorsFormat: true }, + sensorSetupOutdated: true, }, ...over, }); describe('collectOutdatedSensorSetupNodes', () => { - it('flags a legacy-format node with a SATA disk missing a temperature', () => { - const result = collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ nodeName: 'pve1', diskType: 'sata' })], - ); - expect(result).toEqual([{ id: 'node-1', name: 'pve1' }]); + it('lists a node carrying the registry verdict without any disk rows', () => { + expect(collectOutdatedSensorSetupNodes([flaggedNode()])).toEqual([ + { id: 'node-1', name: 'pve1' }, + ]); }); - it('matches disks to nodes by parentId as well as node name', () => { - const result = collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ parentId: 'node-1', diskType: 'sas' })], - ); - expect(result).toEqual([{ id: 'node-1', name: 'pve1' }]); - }); - - it('ignores nodes whose payload is the wrapper format', () => { - const wrapperNode = node({ - proxmox: { node: 'pve1', temperatureDetails: { available: true } }, - }); - expect(collectOutdatedSensorSetupNodes([wrapperNode], [disk({ nodeName: 'pve1' })])).toEqual( - [], - ); - }); - - it('ignores legacy nodes when temperature collection did not succeed', () => { - const unavailableNode = node({ + it('leaves a legacy-format node the registry did not flag', () => { + // The registry clears the verdict when every SATA/SAS disk under the node + // has a current temperature, for example from a linked host agent. + const reading = node({ proxmox: { node: 'pve1', - temperatureDetails: { available: false, legacySensorsFormat: true }, + temperatureDetails: { available: true, legacySensorsFormat: true }, }, }); - expect( - collectOutdatedSensorSetupNodes([unavailableNode], [disk({ nodeName: 'pve1' })]), - ).toEqual([]); + expect(collectOutdatedSensorSetupNodes([reading])).toEqual([]); }); - it('ignores legacy nodes whose SATA/SAS disks already have temperatures', () => { - expect( - collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ nodeName: 'pve1', diskType: 'sata', temperature: 34 })], - ), - ).toEqual([]); - }); - - it('ignores legacy nodes with only NVMe disks missing temperatures', () => { - expect( - collectOutdatedSensorSetupNodes( - [legacyNode()], - [disk({ nodeName: 'pve1', diskType: 'nvme' })], - ), - ).toEqual([]); - }); - - it('ignores disks that belong to a different node', () => { - expect(collectOutdatedSensorSetupNodes([legacyNode()], [disk({ nodeName: 'pve2' })])).toEqual( - [], - ); + it('names a node by its Proxmox node, then node name, then resource name, trimmed', () => { + const nodes = [ + node({ id: 'by-node', proxmox: { node: ' alpha ', sensorSetupOutdated: true } }), + node({ id: 'by-node-name', proxmox: { nodeName: 'bravo', sensorSetupOutdated: true } }), + node({ id: 'by-name', name: 'charlie', proxmox: { sensorSetupOutdated: true } }), + ]; + expect(collectOutdatedSensorSetupNodes(nodes)).toEqual([ + { id: 'by-node', name: 'alpha' }, + { id: 'by-node-name', name: 'bravo' }, + { id: 'by-name', name: 'charlie' }, + ]); }); it('sorts multiple affected nodes by name', () => { const nodes = [ - node({ - id: 'node-b', - name: 'pve-b', - proxmox: { - node: 'pve-b', - temperatureDetails: { available: true, legacySensorsFormat: true }, - }, - }), - node({ - id: 'node-a', - name: 'pve-a', - proxmox: { - node: 'pve-a', - temperatureDetails: { available: true, legacySensorsFormat: true }, - }, - }), + flaggedNode({ id: 'node-b', proxmox: { node: 'pve-b', sensorSetupOutdated: true } }), + flaggedNode({ id: 'node-a', proxmox: { node: 'pve-a', sensorSetupOutdated: true } }), ]; - const disks = [ - disk({ id: 'disk-a', nodeName: 'pve-a' }), - disk({ id: 'disk-b', nodeName: 'pve-b' }), - ]; - expect(collectOutdatedSensorSetupNodes(nodes, disks).map((n) => n.name)).toEqual([ - 'pve-a', - 'pve-b', - ]); + expect(collectOutdatedSensorSetupNodes(nodes).map((n) => n.name)).toEqual(['pve-a', 'pve-b']); }); }); diff --git a/frontend-modern/src/features/platformPage/sensorSetup.ts b/frontend-modern/src/features/platformPage/sensorSetup.ts index 04c1378370..2e27d70d5a 100644 --- a/frontend-modern/src/features/platformPage/sensorSetup.ts +++ b/frontend-modern/src/features/platformPage/sensorSetup.ts @@ -1,9 +1,4 @@ import type { Resource } from '@/types/resource'; -import { matchesPhysicalDiskNode } from '@/components/Storage/diskResourceUtils'; - -// Disk transports whose temperatures only arrive via SMART (the pulse-sensors -// wrapper). NVMe temps come from kernel hwmon and work even on legacy setups. -const SMART_ONLY_DISK_TYPES = new Set(['sata', 'sas']); export type OutdatedSensorSetupNode = { id: string; @@ -13,42 +8,16 @@ export type OutdatedSensorSetupNode = { const getNodeName = (node: Resource): string => (node.proxmox?.node || node.proxmox?.nodeName || node.name || '').trim(); -const isSMARTOnlyDiskWithoutTemperature = (disk: Resource): boolean => { - const meta = disk.physicalDisk; - if (!meta) return false; - const diskType = (meta.diskType || '').trim().toLowerCase(); - if (!SMART_ONLY_DISK_TYPES.has(diskType)) return false; - return (meta.temperature ?? 0) <= 0; -}; - -// Flags PVE nodes whose SSH temperature monitoring still runs the pre-rc.6 +// Lists PVE nodes whose SSH temperature monitoring still runs the pre-rc.6 // setup (authorized_keys locked to `sensors -j`). Such a payload parses fine // and delivers CPU/NVMe temps, but SMART (SATA/SAS) disk temperatures can -// never arrive, so they silently stay blank. Data-gated three ways: the last -// collection succeeded, the payload was legacy-format, and the node actually -// has SATA/SAS disks sitting without a temperature. Nodes whose disk temps -// arrive elsewhere (e.g. a linked host agent) fall out via the last check. -export function collectOutdatedSensorSetupNodes( - nodes: Resource[], - physicalDisks: Resource[], -): OutdatedSensorSetupNode[] { - if (!nodes.length || !physicalDisks.length) return []; - +// never arrive, so they silently stay blank. The registry derives the verdict +// on the node (`proxmox.sensorSetupOutdated`) from the node's last collection, +// its legacy payload and the disks under it, so every Proxmox tab that lists +// nodes can show the notice without loading the disk inventory. +export function collectOutdatedSensorSetupNodes(nodes: Resource[]): OutdatedSensorSetupNode[] { return nodes - .filter((node) => { - const details = node.proxmox?.temperatureDetails; - if (!details?.available || !details.legacySensorsFormat) return false; - const nodeName = getNodeName(node); - return physicalDisks.some( - (disk) => - isSMARTOnlyDiskWithoutTemperature(disk) && - matchesPhysicalDiskNode(disk, { - id: node.id, - name: nodeName, - instance: node.proxmox?.instance, - }), - ); - }) + .filter((node) => node.proxmox?.sensorSetupOutdated === true) .map((node) => ({ id: node.id, name: getNodeName(node) })) .sort((a, b) => a.name.localeCompare(b.name)); } diff --git a/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx b/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx index e34418e333..fa0a193e29 100644 --- a/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx +++ b/frontend-modern/src/features/proxmox/ProxmoxPageSurface.tsx @@ -286,11 +286,10 @@ export function ProxmoxPageSurface() { const serverVersionDisplay = createMemo(() => formatAgentVersionDisplay(agentUpdateTargetVersion()), ); + // The node carries the registry's verdict, so the notice reads the same on + // every tab that lists nodes; no tab hydrates disks for it. const outdatedSensorSetupNodes = createMemo(() => - collectOutdatedSensorSetupNodes( - model().pveNodes, - model().resources.filter((resource) => resource.type === 'physical_disk'), - ), + collectOutdatedSensorSetupNodes(model().pveNodes), ); // The hosts table at the top and the embedded WorkloadsSurface below share diff --git a/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx b/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx index 198c6b273c..63964ff67a 100644 --- a/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx +++ b/frontend-modern/src/features/proxmox/__tests__/ProxmoxPageSurface.contract.test.tsx @@ -375,6 +375,44 @@ describe('ProxmoxPageSurface contract', () => { expect(screen.queryByTestId('platform-outdated-agent-notice')).not.toBeInTheDocument(); }); + it('shows the outdated sensor setup notice on Overview without hydrating disks', () => { + const node = makeResource({ + id: 'agent:pve3', + name: 'pve3', + displayName: 'pve3', + type: 'agent', + proxmox: { + nodeName: 'pve3', + temperatureDetails: { available: true, legacySensorsFormat: true }, + sensorSetupOutdated: true, + }, + }); + // Like production, the Overview query returns nodes and guests only: the + // verdict has to arrive on the node, because no physical_disk rows do. + mockUseUnifiedResources.mockImplementation((options: { cacheKey?: string }) => ({ + resources: () => (options.cacheKey === 'proxmox-overview' ? [node] : []), + loading: () => false, + error: () => null, + refetch: vi.fn(), + })); + + renderSurface(); + + const overviewOptions = mockUseUnifiedResources.mock.calls + .map(([options]) => options as { cacheKey?: string; query?: string }) + .find((options) => options.cacheKey === 'proxmox-overview'); + expect(overviewOptions?.query).toBeDefined(); + expect(overviewOptions?.query).not.toContain('physical_disk'); + expect(screen.getByTestId('platform-section-tabs')).toHaveAttribute('data-active', 'overview'); + expect(screen.getByTestId('platform-outdated-sensor-setup-notice')).toHaveTextContent( + 'pve3 is using an older temperature monitoring setup that cannot read SATA/SAS disk temperatures.', + ); + expect(screen.getByRole('link', { name: 'Open Infrastructure settings' })).toHaveAttribute( + 'href', + '/settings/infrastructure', + ); + }); + it('folds estate topology into the existing nodes table header contract', () => { setResources([ makeResource({ diff --git a/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx b/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx index 27c9caf59a..baa629330f 100644 --- a/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx +++ b/frontend-modern/src/features/truenas/TrueNASStorageTopologyTable.tsx @@ -37,6 +37,10 @@ import { createPlatformResourceLabelResolver, getPlatformResourceDetailRowClass, } from '@/features/platformPage/PlatformResourceDetailTableRow'; +import { + PHYSICAL_DISK_TEMPERATURE_LAST_KNOWN_CLASS, + getPhysicalDiskTemperaturePresentation, +} from '@/features/storageBackups/diskTemperaturePresentation'; import { useAlertsActivation } from '@/stores/alertsActivation'; import type { Resource } from '@/types/resource'; import { @@ -200,6 +204,40 @@ const DiskEnduranceCell: Component<{ row: TrueNASStorageTopologyRow }> = (props) ); }; +// A disk row may carry a last known temperature it did not collect now, such +// as a host agent's reading after the agent stopped reporting on a merged disk. +const diskTemperatureReading = (row: TrueNASStorageTopologyRow) => { + const disk = row.kind === 'disk' ? row.resource.physicalDisk : undefined; + return disk + ? getPhysicalDiskTemperaturePresentation({ + temperature: disk.temperature ?? 0, + collection: disk.collection, + }) + : null; +}; + +// A retained reading reads as last known, never as the disk's temperature now. +const DiskTemperatureCell: Component<{ row: TrueNASStorageTopologyRow }> = (props) => { + const reading = () => diskTemperatureReading(props.row); + const value = () => + props.row.kind === 'disk' ? props.row.resource.physicalDisk?.temperature : undefined; + return ( + } + > + + + , last known + + + ); +}; + const ISSUE_TONES: Record = { attention: 'warning', offline: 'danger', @@ -253,9 +291,10 @@ const getTrueNASStorageSortValue = ( case 'endurance': return row.kind === 'disk' ? (diskEndurance(row)?.value ?? null) : null; case 'temp': { - if (row.kind !== 'disk') return null; - const temperature = row.resource.physicalDisk?.temperature; - return typeof temperature === 'number' && Number.isFinite(temperature) ? temperature : null; + // Hottest first ranks current readings only; a last known one sorts as + // no reading. + const reading = diskTemperatureReading(row); + return reading?.current ? (row.resource.physicalDisk?.temperature ?? null) : null; } case 'health': return HEALTH_SORT_RANK[ @@ -575,14 +614,7 @@ export const TrueNASStorageTopologyTable: Component<{ kindFilter() === 'disks' ? 'table-cell' : 'hidden lg:table-cell' } text-base-content`} > - + { ).toEqual(['sda', 'nvme0n1']); }); + it('shows a retained disk temperature as last known and sorts it as no reading', async () => { + // A TrueNAS disk merged with a host agent's row keeps the agent's last + // reading after the agent stops reporting, under an unavailable state. + const disk = (id: string, temperature: number, state: 'available' | 'unavailable') => + makeStorageResource({ + id, + type: 'physical_disk', + name: id, + storage: undefined, + physicalDisk: { + devPath: `/dev/${id}`, + serial: `serial-${id}`, + diskType: 'sata', + temperature, + collection: { + temperature: { state, source: 'agent', reason: 'host agent stopped reporting' }, + }, + }, + }); + const resources = [ + disk('sda', 64, 'unavailable'), + disk('sdb', 41, 'available'), + disk('sdc', 38, 'available'), + ]; + const { container } = render(() => ( + } + emptyTitle="No storage" + emptyDescription="No storage" + showToolbar={false} + /> + )); + const row = (id: string) => container.querySelector(`[data-truenas-storage-resource="${id}"]`); + const order = () => + [...container.querySelectorAll('[data-truenas-storage-resource]')].map((element) => + element.getAttribute('data-truenas-storage-resource'), + ); + + try { + const retained = row('sda')?.querySelector('[data-temperature-reading="last-known"]'); + expect(retained).toHaveTextContent('64.0°C, last known'); + expect(retained).toHaveAttribute( + 'title', + 'Last known reading, not current: host agent stopped reporting', + ); + expect(retained).toHaveClass('text-muted'); + // 64C is over the factory SATA trigger, but a retained reading is not heat. + expect(row('sda')?.querySelector('[data-truenas-storage-health]')).toBeNull(); + expect(row('sdb')?.querySelector('[data-temperature-reading="last-known"]')).toBeNull(); + expect(row('sdb')).toHaveTextContent('41.0°C'); + + // Hottest first ranks current readings only; the retained one stays last. + await fireEvent.click(screen.getByRole('columnheader', { name: /Temp/ })); + expect(order()).toEqual(['sdb', 'sdc', 'sda']); + await fireEvent.click(screen.getByRole('columnheader', { name: /Temp/ })); + expect(order()).toEqual(['sdc', 'sdb', 'sda']); + } finally { + window.localStorage.clear(); + } + }); + it('follows a disk temperature trigger the user raised in Alerts', async () => { const getConfig = vi.spyOn(AlertsAPI, 'getConfig').mockResolvedValue({ enabled: true, diff --git a/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts b/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts index 3f62f9fa63..3d1f06a4ba 100644 --- a/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts +++ b/frontend-modern/src/hooks/__tests__/useUnifiedResources.test.ts @@ -1943,7 +1943,7 @@ describe('useUnifiedResources', () => { dispose(); }); - it('passes proxmox temperatureDetails through for the outdated-sensor-setup gate', async () => { + it('passes the proxmox temperature payload and sensor setup verdict through', async () => { apiFetchMock.mockResolvedValueOnce({ ok: true, json: async () => ({ @@ -1956,6 +1956,7 @@ describe('useUnifiedResources', () => { nodeName: 'pve1', temperature: 58.4, temperatureDetails: { available: true, legacySensorsFormat: true }, + sensorSetupOutdated: true, }, }, ], @@ -1974,6 +1975,9 @@ describe('useUnifiedResources', () => { available: true, legacySensorsFormat: true, }); + // The outdated sensor setup notice reads the registry's verdict on the + // node, so it must survive the projection that rows on every tab go through. + expect(result!.resources()[0].proxmox?.sensorSetupOutdated).toBe(true); dispose(); }); diff --git a/frontend-modern/src/hooks/useUnifiedResources.ts b/frontend-modern/src/hooks/useUnifiedResources.ts index 489881c89c..6c6547e550 100644 --- a/frontend-modern/src/hooks/useUnifiedResources.ts +++ b/frontend-modern/src/hooks/useUnifiedResources.ts @@ -261,6 +261,7 @@ type APIResource = { available?: boolean; legacySensorsFormat?: boolean; }; + sensorSetupOutdated?: boolean; template?: boolean; containerType?: string; isOci?: boolean; diff --git a/frontend-modern/src/types/resource.ts b/frontend-modern/src/types/resource.ts index 0bc9b1826b..a80952ef36 100644 --- a/frontend-modern/src/types/resource.ts +++ b/frontend-modern/src/types/resource.ts @@ -773,6 +773,10 @@ export interface ResourceProxmoxMeta { available?: boolean; legacySensorsFormat?: boolean; }; + // Registry-derived on Proxmox nodes: the legacy setup above is in use and a + // SATA/SAS disk under the node has no current temperature. It arrives on the + // node so the outdated sensor setup notice never needs the disk inventory. + sensorSetupOutdated?: boolean; } export interface ResourceVirtualMachineMeta { diff --git a/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts b/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts index fa946df7c7..9673cfd24b 100644 --- a/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts +++ b/frontend-modern/src/utils/__tests__/resourceStateAdapters.test.ts @@ -1379,6 +1379,29 @@ describe('resourceStateAdapters unavailable memory contract', () => { }); describe('incremental canonical resource snapshots', () => { + it('clears a node sensor setup verdict when the next snapshot sends false', () => { + // Facets merge field by field and an omitted field is read as a partial + // snapshot, so the registry sends every Proxmox node an explicit verdict. + const node = (sensorSetupOutdated?: boolean) => + ({ + id: 'agent-pve3', + type: 'agent', + name: 'pve3', + status: 'online', + sources: ['proxmox'], + proxmox: { nodeName: 'pve3', sensorSetupOutdated }, + }) as unknown as Resource; + + const [cleared] = mergeCanonicalResourceSnapshot([node(false)], [node(true)]); + expect(cleared?.proxmox?.sensorSetupOutdated).toBe(false); + + const [omitted] = mergeCanonicalResourceSnapshot( + [{ ...node(), proxmox: { nodeName: 'pve3' } } as unknown as Resource], + [node(true)], + ); + expect(omitted?.proxmox?.sensorSetupOutdated).toBe(true); + }); + it('preserves untouched row identity while refreshing changed resources', () => { const unchanged = { id: 'vm-unchanged', diff --git a/internal/api/contract_test.go b/internal/api/contract_test.go index 1adcbc7d63..953ed0fba2 100644 --- a/internal/api/contract_test.go +++ b/internal/api/contract_test.go @@ -60,6 +60,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" "github.com/rcourtman/pulse-go-rewrite/internal/updates" "github.com/rcourtman/pulse-go-rewrite/internal/vmware" + pulsews "github.com/rcourtman/pulse-go-rewrite/internal/websocket" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" "github.com/rcourtman/pulse-go-rewrite/pkg/aicontracts" "github.com/rcourtman/pulse-go-rewrite/pkg/audit" @@ -9261,6 +9262,119 @@ func TestContract_DemoModeCommercialSurfacePolicy(t *testing.T) { }) } +// The public demo signs every visitor in as the configured admin, so admin +// routes accept a demo session and the demo guard is the only thing between it +// and a write. Websocket handshake headers must not carry a write past that +// guard, while a genuine handshake from the same session must still connect. +func TestContract_DemoModeUpgradeHeadersDoNotExemptWrites(t *testing.T) { + setMockModeForTest(t, true) + + dataDir := t.TempDir() + hashedPass, err := authpkg.HashPassword("demo") + if err != nil { + t.Fatalf("hash password: %v", err) + } + cfg := &config.Config{ + DataPath: dataDir, + ConfigPath: dataDir, + DemoMode: true, + AuthUser: "demo", + AuthPass: hashedPass, + } + + monitor, err := monitoring.New(cfg) + if err != nil { + t.Fatalf("new monitor: %v", err) + } + t.Cleanup(func() { monitor.Stop() }) + if err := monitor.SetMockMode(true); err != nil { + t.Fatalf("set monitor mock mode: %v", err) + } + + hub := pulsews.NewHub(nil) + go hub.Run() + t.Cleanup(hub.Stop) + router := NewRouter(cfg, monitor, nil, hub, nil, "1.0.0") + cleanupTestRouter(t, router) + server := newIPv4HTTPServer(t, router.Handler()) + t.Cleanup(server.Close) + + loginResp, err := http.Post(server.URL+"/api/login", "application/json", strings.NewReader(`{"username":"demo","password":"demo"}`)) + if err != nil { + t.Fatalf("demo login: %v", err) + } + loginResp.Body.Close() + if loginResp.StatusCode != http.StatusOK { + t.Fatalf("demo login status = %d, want %d", loginResp.StatusCode, http.StatusOK) + } + var cookiePairs []string + csrfToken := "" + for _, cookie := range loginResp.Cookies() { + cookiePairs = append(cookiePairs, cookie.Name+"="+cookie.Value) + if cookie.Name == CookieNameCSRF { + csrfToken = cookie.Value + } + } + if csrfToken == "" { + t.Fatal("expected CSRF cookie after demo login") + } + sessionCookies := strings.Join(cookiePairs, "; ") + + send := func(method, path, body string, headers map[string]string) (int, string) { + t.Helper() + req, err := http.NewRequest(method, server.URL+path, strings.NewReader(body)) + if err != nil { + t.Fatalf("build %s %s: %v", method, path, err) + } + req.Header.Set("Cookie", sessionCookies) + for key, value := range headers { + req.Header.Set(key, value) + } + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("%s %s: %v", method, path, err) + } + defer resp.Body.Close() + var payload bytes.Buffer + _, _ = payload.ReadFrom(resp.Body) + return resp.StatusCode, payload.String() + } + + if status, body := send(http.MethodGet, "/api/system/mock-mode", "", nil); status != http.StatusOK { + t.Fatalf("demo session read of admin route status = %d, want %d: %s", status, http.StatusOK, body) + } + + status, body := send(http.MethodPost, "/api/system/mock-mode", `{"enabled":false}`, map[string]string{ + "Content-Type": "application/json", + "X-CSRF-Token": csrfToken, + "Connection": "Upgrade", + "Upgrade": "websocket", + "Sec-WebSocket-Version": "13", + "Sec-WebSocket-Key": "dGhlIHNhbXBsZSBub25jZQ==", + }) + if status != http.StatusForbidden || !strings.Contains(body, "Demo mode enabled") { + t.Fatalf("demo write with websocket upgrade headers status = %d, want %d demo block: %s", status, http.StatusForbidden, body) + } + if !mock.IsMockEnabled() { + t.Fatal("demo write with websocket upgrade headers switched mock mode off") + } + + wsHeaders := wsHeadersForHTTP(t, server.URL) + wsHeaders.Set("Cookie", sessionCookies) + conn, resp, err := websocket.DefaultDialer.Dial(wsURLForHTTP(server.URL)+"/ws", wsHeaders) + if err != nil { + status := 0 + if resp != nil { + status = resp.StatusCode + } + t.Fatalf("demo session websocket handshake failed: %v (status %d)", err, status) + } + defer conn.Close() + if resp.StatusCode != http.StatusSwitchingProtocols { + t.Fatalf("demo session websocket handshake status = %d, want %d", resp.StatusCode, http.StatusSwitchingProtocols) + } +} + func TestContract_ReleaseDemoFixtureRuntimeGuardrailsRemainCanonical(t *testing.T) { requiredSnippets := map[string][]string{ "licensing_handlers.go": { diff --git a/internal/api/demo_middleware.go b/internal/api/demo_middleware.go index 7b55a8bf7c..3584529b27 100644 --- a/internal/api/demo_middleware.go +++ b/internal/api/demo_middleware.go @@ -33,18 +33,15 @@ func DemoModeMiddleware(cfg *config.Config, next http.Handler) http.Handler { } } - // Allow GET and HEAD requests (read-only) + // Allow GET and HEAD requests (read-only). WebSocket handshakes are GET + // requests (RFC 6455 section 4.1), so /ws and /api/agent/ws connect + // through this branch. An Upgrade header never exempts another method: + // a POST carrying "Upgrade: websocket" is still a write. if r.Method == http.MethodGet || r.Method == http.MethodHead || r.Method == http.MethodOptions { next.ServeHTTP(w, r) return } - // Allow WebSocket upgrades - if strings.ToLower(r.Header.Get("Upgrade")) == "websocket" { - next.ServeHTTP(w, r) - return - } - // Allow authentication endpoints (login is read-only - verifies credentials) authPaths := []string{ "/api/login", diff --git a/internal/api/demo_middleware_test.go b/internal/api/demo_middleware_test.go index 3ed38b032f..b4afd8aa97 100644 --- a/internal/api/demo_middleware_test.go +++ b/internal/api/demo_middleware_test.go @@ -4,8 +4,11 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strings" "testing" + "time" + "github.com/gorilla/websocket" "github.com/rcourtman/pulse-go-rewrite/internal/config" ) @@ -44,8 +47,8 @@ func TestDemoModeMiddleware(t *testing.T) { {"demo on websocket GET", true, http.MethodGet, "/api/ws", "websocket", true, http.StatusOK, true}, {"demo on websocket case insensitive", true, http.MethodGet, "/api/ws", "WebSocket", true, http.StatusOK, true}, {"demo on websocket uppercase", true, http.MethodGet, "/api/ws", "WEBSOCKET", true, http.StatusOK, true}, - // WebSocket upgrade with POST method (tests websocket branch after GET/HEAD/OPTIONS check) - {"demo on websocket POST", true, http.MethodPost, "/api/ws", "websocket", true, http.StatusOK, true}, + // An Upgrade header does not turn a write into a websocket handshake + {"demo on websocket POST", true, http.MethodPost, "/api/ws", "websocket", false, http.StatusForbidden, true}, // Demo mode enabled - auth endpoints allowed (POST) {"demo on login", true, http.MethodPost, "/api/login", "", true, http.StatusOK, true}, @@ -172,3 +175,79 @@ func TestDemoModeMiddleware_BlockedResponse(t *testing.T) { t.Error("message should not be empty") } } + +// A write that carries websocket handshake headers is still a write. The demo +// guard used to wave through any request with "Upgrade: websocket", so a demo +// session could switch mock mode off or change settings by adding two headers. +func TestDemoModeMiddlewareUpgradeHeadersDoNotExemptWrites(t *testing.T) { + handler := DemoModeMiddleware(&config.Config{DemoMode: true}, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + t.Errorf("write with websocket upgrade headers reached the handler: %s %s", r.Method, r.URL.Path) + })) + + methods := []string{http.MethodPost, http.MethodPut, http.MethodPatch, http.MethodDelete} + paths := []string{"/api/system/mock-mode", "/api/system/settings/update", "/api/orgs", "/ws", "/api/agent/ws"} + for _, method := range methods { + for _, path := range paths { + req := httptest.NewRequest(method, path, strings.NewReader(`{"enabled":false}`)) + req.Header.Set("Connection", "Upgrade") + req.Header.Set("Upgrade", "websocket") + req.Header.Set("Sec-WebSocket-Version", "13") + req.Header.Set("Sec-WebSocket-Key", "dGhlIHNhbXBsZSBub25jZQ==") + rec := httptest.NewRecorder() + handler.ServeHTTP(rec, req) + if rec.Code != http.StatusForbidden { + t.Errorf("%s %s with websocket upgrade headers: status = %d, want %d", method, path, rec.Code, http.StatusForbidden) + } + } + } +} + +func TestDemoModeMiddlewareAllowsWebSocketHandshake(t *testing.T) { + upgrader := websocket.Upgrader{CheckOrigin: func(*http.Request) bool { return true }} + echo := func(w http.ResponseWriter, r *http.Request) { + conn, err := upgrader.Upgrade(w, r, nil) + if err != nil { + return + } + defer conn.Close() + messageType, payload, err := conn.ReadMessage() + if err != nil { + return + } + _ = conn.WriteMessage(messageType, payload) + } + mux := http.NewServeMux() + mux.HandleFunc("/ws", echo) + mux.HandleFunc("/api/agent/ws", echo) + + server := newIPv4HTTPServer(t, DemoModeMiddleware(&config.Config{DemoMode: true}, mux)) + defer server.Close() + + for _, path := range []string{"/ws", "/api/agent/ws"} { + conn, resp, err := websocket.DefaultDialer.Dial(wsURLForHTTP(server.URL)+path, nil) + if err != nil { + status := 0 + if resp != nil { + status = resp.StatusCode + } + t.Fatalf("websocket handshake to %s failed in demo mode: %v (status %d)", path, err, status) + } + if resp.StatusCode != http.StatusSwitchingProtocols { + conn.Close() + t.Fatalf("websocket handshake to %s: status = %d, want %d", path, resp.StatusCode, http.StatusSwitchingProtocols) + } + if err := conn.SetReadDeadline(time.Now().Add(5 * time.Second)); err != nil { + conn.Close() + t.Fatalf("set read deadline for %s: %v", path, err) + } + if err := conn.WriteMessage(websocket.TextMessage, []byte("ping")); err != nil { + conn.Close() + t.Fatalf("write to %s: %v", path, err) + } + _, payload, err := conn.ReadMessage() + conn.Close() + if err != nil || string(payload) != "ping" { + t.Fatalf("echo from %s = %q, %v; want %q", path, payload, err, "ping") + } + } +} diff --git a/internal/monitoring/availability_poller.go b/internal/monitoring/availability_poller.go index 1684535a7a..5174dc796e 100644 --- a/internal/monitoring/availability_poller.go +++ b/internal/monitoring/availability_poller.go @@ -321,8 +321,7 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi } if legacyLocationID == expected[0] || expected[0] == config.AvailabilityObservationLocationLocal { if expected[0] == config.AvailabilityObservationLocationLocal { - legacy.ProbeAgentID = "" - legacy.ProbeReportReceivedAt = time.Time{} + legacy = adoptAvailabilityStatusAsLocal(legacy) } locations[expected[0]] = legacy } @@ -330,13 +329,22 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi } if len(expected) == 1 && expected[0] == config.AvailabilityObservationLocationLocal { if _, ok := locations[expected[0]]; !ok { - if legacy, exists := out[target.ID]; exists { - legacy.ProbeAgentID = "" - legacy.ProbeReportReceivedAt = time.Time{} - locations[expected[0]] = legacy - } else { + legacy, exists := out[target.ID] + switch { + case !exists: delete(out, target.ID) continue + case legacy.ExpectedLocations <= 1: + locations[expected[0]] = adoptAvailabilityStatusAsLocal(legacy) + default: + // An aggregate over several locations is not one observation + // the local location can stand in for: its freshness may be + // one path's and its availability another's. The target + // reads as never checked until the local poller runs, with + // an entry of its own so connection health cannot fall back + // to the cached state from before the lapse. + out[target.ID] = availabilityStatusFromTarget(target) + continue } } } @@ -348,6 +356,8 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi } if status.ProbeAgentID != "" { status = m.deriveAvailabilityProbeStaleness(target, status, now) + } else if len(expected) > 1 { + status = deriveLocalAvailabilityObservationStaleness(target, status, now) } locations[locationID] = status } @@ -356,6 +366,21 @@ func (m *Monitor) availabilityStatusSnapshotForTargets(targets []config.Availabi return out } +// adoptAvailabilityStatusAsLocal presents a retained observation as the local +// location's once the check falls back to local observation, as it does when +// the external-probe entitlement lapses. A local location's check time is its +// freshness, but a remote observation's check time is the agent's clock, so it +// is held to the server's receipt time: a fast agent clock must not keep the +// adopted result current after its receipt is cleared. +func adoptAvailabilityStatusAsLocal(status AvailabilityProbeStatus) AvailabilityProbeStatus { + if !status.ProbeReportReceivedAt.IsZero() && status.LastChecked.After(status.ProbeReportReceivedAt) { + status.LastChecked = status.ProbeReportReceivedAt + } + status.ProbeAgentID = "" + status.ProbeReportReceivedAt = time.Time{} + return status +} + func (m *Monitor) RefreshAvailabilityTargets() { if m == nil { return @@ -986,6 +1011,12 @@ func availabilityEvidenceEnvelope( } freshnessAt := status.FreshnessTime() + if reachableAt := availabilityReachablePathFreshness(status); !reachableAt.IsZero() { + // An available check's claim rests on its reachable paths alone, so + // its evidence lasts as long as the newest of them, not as long as a + // fresher path that failed. + freshnessAt = reachableAt + } if freshnessAt.IsZero() { freshnessAt = observedAt } @@ -1028,6 +1059,22 @@ func availabilityEvidenceEnvelope( return &envelope } +// availabilityReachablePathFreshness returns the newest server-authored +// freshness among the current paths that reached an available check observed +// from several locations, and zero otherwise. +func availabilityReachablePathFreshness(status AvailabilityProbeStatus) time.Time { + if !status.Available || len(status.Locations) < 2 { + return time.Time{} + } + var latest time.Time + for _, location := range status.Locations { + if location.Available && !location.Stale && location.FreshnessAt.After(latest) { + latest = location.FreshnessAt + } + } + return latest +} + func availabilityResourceTags(target config.AvailabilityTarget) []string { tags := []string{"agentless"} if target.TargetKind != "" { diff --git a/internal/monitoring/availability_probe_agent.go b/internal/monitoring/availability_probe_agent.go index a0175d6107..166739cfe7 100644 --- a/internal/monitoring/availability_probe_agent.go +++ b/internal/monitoring/availability_probe_agent.go @@ -23,6 +23,10 @@ const availabilityProbeStaleFloor = 5 * time.Minute // assigned agent stops reporting. const availabilityProbeStaleError = "no recent report from probe agent" +// availabilityLocalCheckStaleError explains a local observation that stopped +// running while the check's other locations kept reporting. +const availabilityLocalCheckStaleError = "no recent local check" + // ProbeAvailabilityResult is one availability observation reported by a remote // host agent that owns the target's execution. type ProbeAvailabilityResult struct { @@ -316,9 +320,34 @@ func (m *Monitor) availabilityProbeAssignmentReference(targetID, locationID stri return now } +// deriveLocalAvailabilityObservationStaleness ages out a local observation +// that stopped running when the check also has other locations. Alone, a +// local observation's check time is the check's freshness, so its evidence +// lapses with it. Beside other locations the check takes the newest +// location's freshness, so a local pass that stopped would otherwise keep the +// check available on the strength of a failing remote report. +func deriveLocalAvailabilityObservationStaleness( + target config.AvailabilityTarget, + status AvailabilityProbeStatus, + now time.Time, +) AvailabilityProbeStatus { + if status.LastChecked.IsZero() || !availabilityProbeReportIsStale(target, status.LastChecked, now) { + return status + } + status.Outcome = string(AvailabilityProbeIndeterminate) + status.Available = false + status.LastError = availabilityLocalCheckStaleError + status.LatencyMillis = 0 + return status +} + func availabilityProbeStatusIsStale(status AvailabilityProbeStatus) bool { - return status.Outcome == string(AvailabilityProbeIndeterminate) && - strings.EqualFold(strings.TrimSpace(status.LastError), availabilityProbeStaleError) + if status.Outcome != string(AvailabilityProbeIndeterminate) { + return false + } + lastError := strings.TrimSpace(status.LastError) + return strings.EqualFold(lastError, availabilityProbeStaleError) || + strings.EqualFold(lastError, availabilityLocalCheckStaleError) } func availabilityProbeReportIsStale(target config.AvailabilityTarget, lastChecked time.Time, now time.Time) bool { diff --git a/internal/monitoring/availability_probe_agent_test.go b/internal/monitoring/availability_probe_agent_test.go index c2ae3f1fde..67e4fb007b 100644 --- a/internal/monitoring/availability_probe_agent_test.go +++ b/internal/monitoring/availability_probe_agent_test.go @@ -165,6 +165,91 @@ func TestMultiLocationAvailabilityTreatsLapsedPathAsUnknownCoverage(t *testing.T } } +// The local twin of the lapsed-path case. A local observation that stopped +// running keeps its last outcome, and a check with several locations takes the +// newest location's freshness, so a failing edge report used to keep vouching +// for a local pass from an hour ago: the check read available with current +// evidence, which the registry takes as proof that its target answers. +func TestMultiLocationAvailabilityAgesOutALocalPathThatStopped(t *testing.T) { + target := config.NormalizeAvailabilityTarget(config.AvailabilityTarget{ + ID: "service", + Name: "Customer API", + Address: "api.service.local", + Protocol: config.AvailabilityProbeHTTPS, + Enabled: true, + PollIntervalSecs: 60, + FailureThreshold: 2, + ObservationLocationIDs: []string{config.AvailabilityObservationLocationLocal, config.AvailabilityAgentObservationLocationID("edge-1")}, + }) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + old := now.Add(-time.Hour) + monitor.applyAvailabilityObservation(target, "local-ok", old, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "", time.Time{}) + for attempt := 0; attempt < 2; attempt++ { + at := now.Add(time.Duration(attempt) * time.Second) + monitor.applyAvailabilityObservation(target, "edge-fail-"+string(rune('a'+attempt)), at, 30*time.Millisecond, AvailabilityProbeUnreachable, context.DeadlineExceeded, nil, "edge-1", at) + } + + readAt := now.Add(2 * time.Second) + status := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, readAt)[target.ID] + if status.Available || status.AggregateState != AvailabilityAggregateUnknown { + t.Fatalf("aggregate status = %+v, want unknown coverage rather than an available check", status) + } + if status.ReportingLocations != 1 || status.ExpectedLocations != 2 { + t.Fatalf("coverage = %d/%d, want the stopped local path excluded", status.ReportingLocations, status.ExpectedLocations) + } + local := status.Locations[0] + if local.LocationID != config.AvailabilityObservationLocationLocal || !local.Stale || + local.Outcome != string(AvailabilityProbeIndeterminate) || local.LastError != availabilityLocalCheckStaleError { + t.Fatalf("local location = %+v, want stale unknown evidence", local) + } + resource, _ := availabilityResourceFromTarget(target, status, "", readAt) + if resource.Availability.Available || resource.Status != unifiedresources.StatusWarning || len(resource.Incidents) != 0 { + t.Fatalf("resource = available %v status %q incidents %+v, want an unproven check and no outage", resource.Availability.Available, resource.Status, resource.Incidents) + } + + monitor.applyAvailabilityObservation(target, "local-ok-again", readAt, 9*time.Millisecond, AvailabilityProbeReachable, nil, nil, "", time.Time{}) + status = monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, readAt)[target.ID] + if !status.Available || status.AggregateState != AvailabilityAggregateDegraded || status.Locations[0].Stale { + t.Fatalf("aggregate status = %+v, want a current local pass to make the check available again", status) + } +} + +// An available check's evidence lasts as long as its newest reachable path. A +// registry that re-reads this generation later judges the check at its own +// time, so evidence anchored on a fresher failing path would keep proving the +// target answers after the only passing path had aged out. +func TestMultiLocationAvailableEvidenceLastsAsLongAsItsReachablePath(t *testing.T) { + target := config.NormalizeAvailabilityTarget(config.AvailabilityTarget{ + ID: "service", + Name: "Customer API", + Address: "api.service.local", + Protocol: config.AvailabilityProbeHTTPS, + Enabled: true, + PollIntervalSecs: 60, + FailureThreshold: 2, + ObservationLocationIDs: []string{config.AvailabilityObservationLocationLocal, config.AvailabilityAgentObservationLocationID("edge-1")}, + }) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + passedAt := now.Add(-4 * time.Minute) + monitor.applyAvailabilityObservation(target, "local-ok", passedAt, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "", time.Time{}) + monitor.applyAvailabilityObservation(target, "edge-fail", now, 30*time.Millisecond, AvailabilityProbeUnreachable, context.DeadlineExceeded, nil, "edge-1", now) + + status := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, now)[target.ID] + if !status.Available || status.AggregateState != AvailabilityAggregateDegraded { + t.Fatalf("aggregate status = %+v, want an available check with one failing path", status) + } + resource, _ := availabilityResourceFromTarget(target, status, "", now) + evidence := resource.Availability.Evidence + want := passedAt.Add(availabilityProbeStaleWindow(target)) + if evidence == nil || evidence.ValidUntil == nil || !evidence.ValidUntil.Equal(want) { + t.Fatalf("evidence = %+v, want it valid until %s, one window after the reachable path", evidence, want) + } +} + func licenseWithExternalProbe(enabled bool) func(string) bool { return func(feature string) bool { return enabled && feature == pkglicensing.FeatureExternalProbe @@ -237,6 +322,74 @@ func TestAvailabilityPollProviderSkipsProbeAssignedTargetsAndResumesOnLapse(t *t } } +// When the entitlement lapses, a remote check's last result stands in as the +// local location's until the local poller runs. A local location's check time +// is its freshness, so the agent's clock must not carry over: a result checked +// a day ahead would otherwise read current, and keep its target answering, for +// a day without a local execution. +func TestAvailabilityLapseAdoptsARemoteResultAtItsReceiptTime(t *testing.T) { + target := config.NormalizeAvailabilityTarget(probeAgentTarget("remote", "agent-1")) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + receivedAt := now.Add(-time.Minute) + agentClock := now.Add(24 * time.Hour) + monitor.applyAvailabilityObservation(target, "remote-ok", agentClock, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "agent-1", receivedAt) + + monitor.SetLicenseChecker(licenseWithExternalProbe(false)) + status := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, now)[target.ID] + if status.ProbeAgentID != "" || !status.LastChecked.Equal(receivedAt) { + t.Fatalf("adopted status = %+v, want a local result checked at the server receipt time", status) + } + resource, _ := availabilityResourceFromTarget(target, status, "", now) + evidence := resource.Availability.Evidence + want := receivedAt.Add(2 * time.Duration(target.EffectivePollIntervalSecs()) * time.Second) + if evidence == nil || evidence.ValidUntil == nil || !evidence.ValidUntil.Equal(want) { + t.Fatalf("evidence = %+v, want it valid until %s", evidence, want) + } +} + +// The stored result of a check with several locations is an aggregate, whose +// freshness may be one path's and whose availability another's. After a lapse +// it must not stand in for the local location: one path passing an hour ago +// and another failing now would read as a current local pass. +func TestAvailabilityLapseNeverAdoptsAMultiLocationAggregate(t *testing.T) { + target := config.NormalizeAvailabilityTarget(config.AvailabilityTarget{ + ID: "service", + Name: "Customer API", + Address: "api.service.local", + Protocol: config.AvailabilityProbeHTTPS, + Enabled: true, + PollIntervalSecs: 60, + FailureThreshold: 2, + ObservationLocationIDs: []string{ + config.AvailabilityAgentObservationLocationID("edge-1"), + config.AvailabilityAgentObservationLocationID("edge-2"), + }, + }) + monitor := newProbeAgentTestMonitor(t, target) + monitor.SetLicenseChecker(licenseWithExternalProbe(true)) + now := time.Now().UTC() + passedAt := now.Add(-time.Hour) + monitor.applyAvailabilityObservation(target, "edge-1-ok", passedAt, 8*time.Millisecond, AvailabilityProbeReachable, nil, nil, "edge-1", passedAt) + monitor.applyAvailabilityObservation(target, "edge-2-fail", now, 30*time.Millisecond, AvailabilityProbeUnreachable, context.DeadlineExceeded, nil, "edge-2", now) + + monitor.SetLicenseChecker(licenseWithExternalProbe(false)) + status, ok := monitor.availabilityStatusSnapshotForTargets([]config.AvailabilityTarget{target}, now)[target.ID] + if !ok || status.Available || !status.LastChecked.IsZero() || status.AggregateState != "" { + t.Fatalf("status after the lapse = %+v (present %v), want a never-checked target", status, ok) + } + if resource, _ := availabilityResourceFromTarget(target, status, "", now); resource.Status != unifiedresources.StatusUnknown { + t.Fatalf("check row status = %q, want unknown like any never-checked target", resource.Status) + } + // An absent entry would let connection health fall back to the last + // cached state, which still says connected. + connected, reported := availabilityPollProvider{}.ConnectionStatuses(monitor)[availabilityConnectionKey(target.ID)] + if !reported || connected { + t.Fatalf("connection status = %v (reported %v), want not connected", connected, reported) + } +} + func TestRefreshAvailabilityTargetsUnschedulesProbeAssignedTargets(t *testing.T) { monitor := newProbeAgentTestMonitor(t, probeAgentTarget("remote", "agent-1"), diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index fd27c2215b..8a13f8b631 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -543,8 +543,12 @@ func TestStoreRefreshTakesItsMockModeScopeBeforeReadingState(t *testing.T) { if err != nil { t.Fatalf("failed to read monitor.go: %v", err) } - if got := strings.Count(string(data), "m.mockModeFence.advance()\n\t\tm.alertManager.ClearActiveAlerts()"); got != 2 { - t.Fatalf("SetMockMode must end the mock-mode epoch immediately before clearing alerts in both directions, found %d", got) + source := string(data) + if got := strings.Count(source, "m.mockModeFence.advance()\n\tm.alertManager.ClearActiveAlerts()"); got != 1 { + t.Fatalf("endMockModeEpoch must end the mock-mode epoch immediately before clearing alerts, found %d", got) + } + if !strings.Contains(source, "for _, monitor := range monitors {\n\t\tmonitor.endMockModeEpoch(enable)\n\t}") { + t.Fatal("SetMockMode must end the epoch of every monitor the switch reaches") } } @@ -1664,16 +1668,11 @@ func TestMonitorSetMockModeAuthorizesBeforeResettingRuntimeState(t *testing.T) { if err != nil { t.Fatalf("failed to read monitor.go: %v", err) } - source := string(data) - requiredSnippets := []string{ - "if err := mock.SetEnabled(true); err != nil {", - "if err := mock.SetEnabled(false); err != nil {", - "return err", - } - for _, snippet := range requiredSnippets { - if !strings.Contains(source, snippet) { - t.Fatalf("monitor.go must contain %q", snippet) - } + source := strings.Join(strings.Fields(string(data)), " ") + authorize := strings.Index(source, "if err := mock.SetEnabled(enable); err != nil { return err }") + reset := strings.Index(source, "monitor.endMockModeEpoch(enable)") + if authorize < 0 || reset < 0 || authorize > reset { + t.Fatal("SetMockMode must return the mock.SetEnabled error before it ends any monitor's epoch or resets its state") } } diff --git a/internal/monitoring/issue1595_collection_trust_test.go b/internal/monitoring/issue1595_collection_trust_test.go index 7a52f71d8b..06e72b6a29 100644 --- a/internal/monitoring/issue1595_collection_trust_test.go +++ b/internal/monitoring/issue1595_collection_trust_test.go @@ -612,3 +612,164 @@ func TestRegistrySASPathJoinRefusesContradictingWWN(t *testing.T) { }) } } + +// A registry rehydrated from a persisted unified snapshot seeds each source +// key's mapping before the next poll arrives. Proxmox keys a disk by its slot, +// so a disk swapped into the slot arrives under the previous disk's key. The +// mapping is refused when the two carry hardware identities naming different +// disks, and nothing else may merge the replacement back into the old disk, +// so it gets its own canonical resource instead of the old disk's ID, serial, +// temperature and failed health. Identity missing on one side keeps the +// mapping, and so does a serial that may not be the drive's own (a SAS +// address, a SCSI designator, a USB bridge's serial) set against the agent's. +func TestRegistrySeededSlotMappingRefusesReplacedDisk(t *testing.T) { + const slotSourceID = "pve1-node1--dev-sdb" + // Each kind is the slot's previous disk as Proxmox and the agent reported + // it; the registry merged the two reports by WWN or by path, unless the + // case has Proxmox alone report it. + kinds := map[string]struct{ agentType, vendor, proxmoxType, proxmoxSerial string }{ + "sata": {agentType: "sata", vendor: "ATA", proxmoxType: "hdd", proxmoxSerial: "ZR5A0001"}, + "sas": {agentType: "sas", vendor: "SEAGATE", proxmoxType: "hdd", proxmoxSerial: "5000c500aaaa0003"}, + "usb": {agentType: "usb", vendor: "JMicron", proxmoxType: "usb", proxmoxSerial: "BRIDGE0001"}, + "scsi": {agentType: "sata", vendor: "SEAGATE", proxmoxType: "hdd", proxmoxSerial: "Z1Z0VPD0001"}, + "usb-ata": {agentType: "sata", vendor: "ATA", proxmoxType: "usb", proxmoxSerial: "BRIDGE0001"}, + } + for _, tc := range []struct { + name string + kind string + proxmoxFirst bool + proxmoxOnly bool + legacyAgent bool + serial string + wwn string + replacedDrive bool + }{ + {name: "replacement disk", kind: "sata", serial: "ZR5B0002", wwn: "0x5000c500bbbb0002", replacedDrive: true}, + {name: "replacement disk without a WWN", kind: "sata", serial: "ZR5B0002", wwn: "unknown", replacedDrive: true}, + {name: "same disk", kind: "sata", serial: "ZR5A0001", wwn: "0x5000c500aaaa0001"}, + {name: "same disk without a serial", kind: "sata", serial: "unknown", wwn: "0x5000c500aaaa0001"}, + {name: "SAS disk whose Proxmox serial is a SAS address", kind: "sas", serial: "5000c500aaaa0003", wwn: "0x5000c500aaaa0001"}, + {name: "SAS disk without a Proxmox WWN", kind: "sas", serial: "5000c500aaaa0003", wwn: "unknown"}, + {name: "USB disk whose Proxmox serial is the bridge's", kind: "usb", serial: "BRIDGE0001", wwn: "unknown"}, + {name: "USB bridge serial under Proxmox vendor ATA", kind: "usb-ata", serial: "BRIDGE0001", wwn: "unknown"}, + {name: "disk whose Proxmox serial is a SCSI designator", kind: "scsi", serial: "Z1Z0VPD0001", wwn: "unknown"}, + {name: "SAS replacement disk", kind: "sas", serial: "5000c500bbbb0003", wwn: "0x5000c500bbbb0002", replacedDrive: true}, + {name: "drive swapped in the same USB enclosure", kind: "usb", proxmoxFirst: true, + serial: "BRIDGE0001", wwn: "0x5000c500bbbb0002", replacedDrive: true}, + {name: "SCSI designator from an agent without collection status", kind: "scsi", legacyAgent: true, + serial: "Z1Z0VPD0001", wwn: "unknown"}, + {name: "replacement Proxmox alone reports, without a WWN", kind: "scsi", proxmoxOnly: true, + serial: "Z1Z0VPD0002", wwn: "unknown", replacedDrive: true}, + } { + t.Run(tc.name, func(t *testing.T) { + kind := kinds[tc.kind] + now := time.Now() + node := models.Node{ + ID: "pve1-node1", Name: "node1", Instance: "pve1", Status: "online", + LastSeen: now, LinkedAgentID: "agent-1", + } + host := models.Host{ + ID: "agent-1", Hostname: "node1", LinkedNodeID: "pve1-node1", Status: "online", LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sdb", Serial: "ZR5A0001", WWN: "5-c50-aaaa0001", Type: kind.agentType, + Health: "FAILED", Temperature: 55, + Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Available("smartctl"), + Temperature: diskinventory.Available("smartctl"), + }, + }}}, + } + // As monitor_pve builds the slot's row from disks/list. + slotDisk := func(serial, wwn, health string) models.PhysicalDisk { + disk := models.PhysicalDisk{ + ID: slotSourceID, Node: "node1", Instance: "pve1", DevPath: "/dev/sdb", Vendor: kind.vendor, + Serial: proxmoxReportedDiskIdentity(serial), WWN: proxmoxReportedDiskIdentity(wwn), + Type: kind.proxmoxType, Health: health, Wearout: -1, LastChecked: now, + Collection: &diskinventory.CollectionStatus{ + Serial: diskinventory.Missing("proxmox_disks", "disk serial was not reported"), + }, + } + if disk.Serial != "" { + disk.Collection.Serial = diskinventory.Available("proxmox_disks") + } + return disk + } + previousDisk := slotDisk(kind.proxmoxSerial, "0x5000c500aaaa0001", "FAILED") + previous := unifiedresources.NewRegistry(nil) + if tc.proxmoxFirst { + previous.IngestSnapshot(models.StateSnapshot{ + Nodes: []models.Node{node}, PhysicalDisks: []models.PhysicalDisk{previousDisk}, + }) + } + hosts := []models.Host{host} + if tc.proxmoxOnly { + hosts = nil + } + if tc.legacyAgent { + host.Sensors.SMART[0].Collection = nil + } + previous.IngestSnapshot(models.StateSnapshot{ + Nodes: []models.Node{node}, Hosts: hosts, PhysicalDisks: []models.PhysicalDisk{previousDisk}, + }) + persisted := previous.ListByType(unifiedresources.ResourceTypePhysicalDisk) + if len(persisted) != 1 || persisted[0].PhysicalDisk == nil || + !hasIssue1595Source(persisted[0].Sources, unifiedresources.SourceProxmox) || + hasIssue1595Source(persisted[0].Sources, unifiedresources.SourceAgent) == tc.proxmoxOnly { + t.Fatalf("want one disk merged from Proxmox and the agent (Proxmox alone: %v), got %+v", tc.proxmoxOnly, persisted) + } + oldID, oldDisk := persisted[0].ID, *persisted[0].PhysicalDisk + + // Restart: rehydrate from the persisted resources, then poll + // Proxmox before the agent has reported again. A second restart + // rehydrates from the result, where both disks may claim the slot. + current := slotDisk(tc.serial, tc.wwn, "PASSED") + seed := previous.List() + slotID := "" + for restart := 1; restart <= 2; restart++ { + rehydrated := unifiedresources.NewRegistry(nil) + rehydrated.IngestResources(seed) + rehydrated.IngestSnapshot(models.StateSnapshot{ + Nodes: []models.Node{node}, PhysicalDisks: []models.PhysicalDisk{current}, + }) + seed = rehydrated.List() + + var slot *unifiedresources.Resource + disks := rehydrated.ListByType(unifiedresources.ResourceTypePhysicalDisk) + for index := range disks { + for _, target := range rehydrated.SourceTargets(disks[index].ID) { + if target.Source == unifiedresources.SourceProxmox && target.SourceID == slotSourceID { + slot = &disks[index] + } + } + } + if slot == nil || slot.PhysicalDisk == nil { + t.Fatalf("restart %d: no disk holds the Proxmox slot key among %d disks", restart, len(disks)) + } + if slotID != "" && slot.ID != slotID { + t.Fatalf("restart %d: slot moved from %s to %s", restart, slotID, slot.ID) + } + slotID = slot.ID + if !tc.replacedDrive { + if slot.ID != oldID || len(disks) != 1 { + t.Fatalf("restart %d: slot disk = %s with %d disks, want the persisted disk %s alone", + restart, slot.ID, len(disks), oldID) + } + continue + } + if slot.ID == oldID || len(disks) != 2 { + t.Fatalf("restart %d: replacement took the persisted disk's canonical ID %s (%d disks)", restart, oldID, len(disks)) + } + if slot.PhysicalDisk.Serial != current.Serial || slot.PhysicalDisk.WWN != current.WWN || + slot.PhysicalDisk.Temperature != 0 || slot.PhysicalDisk.Health != "PASSED" { + t.Fatalf("restart %d: replacement took the persisted disk's identity or readings: %+v", restart, slot.PhysicalDisk) + } + for _, disk := range disks { + if disk.ID == oldID && (disk.PhysicalDisk == nil || + disk.PhysicalDisk.Serial != oldDisk.Serial || disk.PhysicalDisk.WWN != oldDisk.WWN) { + t.Fatalf("restart %d: persisted disk was rewritten: %+v", restart, disk.PhysicalDisk) + } + } + } + }) + } +} diff --git a/internal/monitoring/mock_mode_switch.go b/internal/monitoring/mock_mode_switch.go new file mode 100644 index 0000000000..4419f77b88 --- /dev/null +++ b/internal/monitoring/mock_mode_switch.go @@ -0,0 +1,75 @@ +package monitoring + +import ( + "sync" + "sync/atomic" + + "github.com/rcourtman/pulse-go-rewrite/internal/mock" +) + +// Mock mode is process-wide (mock.SetEnabled flips one package flag), while +// every Monitor keeps its own alert manager, fixture agents, state and +// mock-mode fence, and the server runs one Monitor per organization. A switch +// made through one monitor therefore has to reach every running monitor, or +// the others keep what they raised in the mode the process left: fixture +// alerts and fixture-agent node links in live mode, live alerts in mock mode, +// until the 24-hour stale sweep. +// +// mockModeSwitchMu serializes switches and guards the set of running monitors +// and each monitor's mockModeAligned and mockModeSwitchesSeen. A switch is +// several steps per monitor (end the epoch, clear, reset), so two interleaved +// switches could clear what the later one just admitted. Start joins the set +// under the same lock, so a monitor joins either before a switch, which then +// switches it with the others, or after it, and starts in the new mode. +// +// mockModeSwitchCount counts the flips SetMockMode made. A monitor that missed +// flips while it was not running can hold the alerts and state of a mode the +// process left even when a later flip brought the flag back to the mode it +// records (a read path evaluated fixture data before Start), so Start +// compares the count as well as the mode. +var ( + mockModeSwitchMu sync.Mutex + mockModeRunningMonitors = make(map[*Monitor]struct{}) + mockModeSwitchCount atomic.Uint64 +) + +// mockModeSwitchTargetsLocked returns the monitors a switch made through m +// reaches: m itself, which need not be running, and every running monitor. +func mockModeSwitchTargetsLocked(m *Monitor) []*Monitor { + targets := make([]*Monitor, 0, len(mockModeRunningMonitors)+1) + targets = append(targets, m) + for monitor := range mockModeRunningMonitors { + if monitor != m { + targets = append(targets, monitor) + } + } + return targets +} + +// joinMockModeSwitches adds m to the monitors every switch reaches, then runs +// start with the current mode while no switch can run, so the mode-dependent +// runtime start chooses (the mock metrics sampler or discovery) cannot be +// overtaken by a switch. It returns the function that removes m again. +// +// A switch can land between New and Start, while m is not yet running. New +// restores persisted alerts only outside mock mode, so m would then carry live +// alerts into mock mode, or fixture alerts that a read path raised before +// Start into live mode; m first leaves what it holds, as the switches it +// missed would have done. +func (m *Monitor) joinMockModeSwitches(start func(mockEnabled bool)) (leave func()) { + mockModeSwitchMu.Lock() + defer mockModeSwitchMu.Unlock() + + enabled := mock.IsMockEnabled() + if m.mockModeAligned != enabled || m.mockModeSwitchesSeen != mockModeSwitchCount.Load() { + m.endMockModeEpoch(enabled) + } + mockModeRunningMonitors[m] = struct{}{} + start(enabled) + + return func() { + mockModeSwitchMu.Lock() + defer mockModeSwitchMu.Unlock() + delete(mockModeRunningMonitors, m) + } +} diff --git a/internal/monitoring/monitor.go b/internal/monitoring/monitor.go index 25be288f63..36f1a3a536 100644 --- a/internal/monitoring/monitor.go +++ b/internal/monitoring/monitor.go @@ -13,6 +13,7 @@ import ( "os" "path/filepath" "reflect" + "slices" "sort" "strings" "sync" @@ -67,9 +68,9 @@ func keepRealPollingInMockMode() bool { // process that boots with mock mode already enabled never runs that path, so // alerts raised against real infrastructure are restored from disk and served // beside fixture data until something clears them. -func alertManagerRestoreOptions() []alerts.ManagerOption { +func alertManagerRestoreOptions(mockEnabled bool) []alerts.ManagerOption { options := []alerts.ManagerOption{alerts.WithDurableAlertStore()} - if mock.IsMockEnabled() { + if mockEnabled { options = append(options, alerts.WithoutPersistedAlertRestore()) } return options @@ -1332,6 +1333,8 @@ type Monitor struct { mockHostAgentsMu sync.Mutex mockHostAgents map[string]models.Host // Fixture agents evaluated by the last mock alert pass mockModeFence mockModeFence // Keeps mode-dependent alert evaluations inside the epoch they read in + mockModeAligned bool // Mock mode the alerts and state belong to; guarded by mockModeSwitchMu + mockModeSwitchesSeen uint64 // mockModeSwitchCount when they were last aligned; guarded by mockModeSwitchMu dockerChecker DockerChecker // Optional Docker checker for LXC containers dockerCheckerConfiguredAt time.Time // Last time the Docker checker was configured dockerCheckAllowedVMIDs map[int]struct{} // Optional VMID allowlist gating the LXC Docker socket probe; empty means all guests @@ -1820,6 +1823,12 @@ func New(cfg *config.Config) (*Monitor, error) { DataDir: cfg.DataPath, }) + // One read decides the alert restore and records the mode it belongs to, + // so Start can tell whether a mock-mode switch landed in between. The + // count is read first: a switch flips the flag before it counts, so one + // that overlaps these reads leaves a stale count, never a stale mode. + switchesSeen := mockModeSwitchCount.Load() + builtInMockMode := mock.IsMockEnabled() m := &Monitor{ config: cfg, state: models.NewState(), @@ -1853,7 +1862,9 @@ func New(cfg *config.Config) (*Monitor, error) { rateTracker: NewRateTracker(), metricsHistory: NewMetricsHistory(1000, 24*time.Hour), // Keep up to 1000 points (~8h @ 30s) metricsStore: metricsStore, // Persistent SQLite storage - alertManager: alerts.NewManagerWithDataDir(cfg.DataPath, alertManagerRestoreOptions()...), + alertManager: alerts.NewManagerWithDataDir(cfg.DataPath, alertManagerRestoreOptions(builtInMockMode)...), + mockModeAligned: builtInMockMode, + mockModeSwitchesSeen: switchesSeen, incidentStore: incidentStore, notificationMgr: notifications.NewNotificationManagerWithDeferredQueue(cfg.PublicURL, cfg.DataPath), deadMan: newDeadManRuntime(config.ResolveRuntimeDataDir(cfg.DataPath)), @@ -2135,33 +2146,13 @@ func (m *Monitor) Start(ctx context.Context, wsHub *websocket.Hub) { // PVE-proxied PBS backup points to prevent duplicate recovery entries. m.purgeStalePVEPBSBackupsBestEffort(ctx) - if mock.IsMockEnabled() { - m.startMockMetricsSampler(ctx) - } - - // Initialize and start discovery service if enabled - if mock.IsMockEnabled() { - log.Info().Msg("mock mode enabled - skipping discovery service") - m.discoveryService = nil - } else if m.config.DiscoveryEnabled { - discoverySubnet := m.config.DiscoverySubnet - if discoverySubnet == "" { - discoverySubnet = "auto" - } - cfgProvider := func() config.DiscoveryConfig { - return m.discoveryConfigSnapshot() - } - m.discoveryService = discovery.NewService(wsHub, 5*time.Minute, discoverySubnet, cfgProvider) - if m.discoveryService != nil { - m.discoveryService.Start(ctx) - log.Info().Msg("discovery service initialized and started") - } else { - log.Error().Msg("failed to initialize discovery service") - } - } else { - log.Info().Msg("discovery service disabled by configuration") - m.discoveryService = nil - } + // From here every mock-mode switch reaches this monitor. The sampler or + // discovery is chosen while no switch can run, so a switch cannot land + // between reading the mode and starting the runtime for it. + leaveMockModeSwitches := m.joinMockModeSwitches(func(mockEnabled bool) { + m.startModeRuntime(ctx, wsHub, mockEnabled) + }) + defer leaveMockModeSwitches() // Set up alert callbacks. Projection replay is deliberately absent here: // the canonical resource store is not attached yet, so a replay now could @@ -2300,6 +2291,38 @@ func (m *Monitor) Start(ctx context.Context, wsHub *websocket.Hub) { } } +// startModeRuntime starts the mock metrics sampler in mock mode, or the +// discovery service in live mode when it is enabled. +func (m *Monitor) startModeRuntime(ctx context.Context, wsHub *websocket.Hub, mockEnabled bool) { + if mockEnabled { + m.startMockMetricsSampler(ctx) + } + + // Initialize and start discovery service if enabled + if mockEnabled { + log.Info().Msg("mock mode enabled - skipping discovery service") + m.discoveryService = nil + } else if m.config.DiscoveryEnabled { + discoverySubnet := m.config.DiscoverySubnet + if discoverySubnet == "" { + discoverySubnet = "auto" + } + cfgProvider := func() config.DiscoveryConfig { + return m.discoveryConfigSnapshot() + } + m.discoveryService = discovery.NewService(wsHub, 5*time.Minute, discoverySubnet, cfgProvider) + if m.discoveryService != nil { + m.discoveryService.Start(ctx) + log.Info().Msg("discovery service initialized and started") + } else { + log.Error().Msg("failed to initialize discovery service") + } + } else { + log.Info().Msg("discovery service disabled by configuration") + m.discoveryService = nil + } +} + // wireExternalAlertCallbacks installs callbacks whose effects leave the alert // manager. It is safe to call again: these are single callback slots, so Start // replaces the constructor-time escalation closure with one carrying its hub. @@ -4626,65 +4649,93 @@ func (m *Monitor) broadcastEscalatedAlert(hub *websocket.Hub, alert *alerts.Aler hub.BroadcastAlertToTenant(m.GetOrgID(), alert) } -// mockModeSwitchMu serializes mock-mode switches. The mode is process-wide, -// and a switch is several steps (flip, end the epoch, clear, reset state), so -// two interleaved switches could clear what the later one just admitted. -var mockModeSwitchMu sync.Mutex - -// SetMockMode switches between mock data and real infrastructure data at runtime. -// It must not be called from inside an alert evaluation, whose completion it -// may wait for. +// SetMockMode switches between mock data and real infrastructure data at +// runtime. The mode is process-wide, so the switch reaches m and every running +// monitor (see mockModeSwitchMu), whichever tenant's monitor it was made +// through. It must not be called from inside an alert evaluation of any of +// them, whose completion it may wait for. func (m *Monitor) SetMockMode(enable bool) error { mockModeSwitchMu.Lock() defer mockModeSwitchMu.Unlock() - current := mock.IsMockEnabled() - if current == enable { - log.Info().Bool("mockMode", enable).Msg("mock mode already in desired state") - return nil + monitors := mockModeSwitchTargetsLocked(m) + flip := mock.IsMockEnabled() != enable + if !flip { + // The flag already holds the mode, but a monitor that was not running + // during an earlier switch can still hold the other mode's alerts + // and state; only those leave it. + monitors = slices.DeleteFunc(monitors, func(monitor *Monitor) bool { + return monitor.mockModeAligned == enable + }) + if len(monitors) == 0 { + log.Info().Bool("mockMode", enable).Msg("mock mode already in desired state") + return nil + } } - // Every evaluation of mode-dependent data that started before the flip - // must finish or be refused before the clear, or it reopens alerts for - // the side the monitor just left (see mockModeFence). + for _, monitor := range monitors { + monitor.stopMockMetricsSampler() + } + if flip { + if err := mock.SetEnabled(enable); err != nil { + return err + } + mockModeSwitchCount.Add(1) + } + for _, monitor := range monitors { + monitor.endMockModeEpoch(enable) + } + for _, monitor := range monitors { + _, running := mockModeRunningMonitors[monitor] + monitor.resumeAfterMockModeSwitch(enable, running) + } if enable { - m.stopMockMetricsSampler() - if err := mock.SetEnabled(true); err != nil { - return err - } - m.mockModeFence.advance() - m.alertManager.ClearActiveAlerts() - m.mu.Lock() - m.resetStateLocked() - m.metricsHistory.Reset() - m.mu.Unlock() - m.StopDiscoveryService() - m.mu.RLock() - ctx := m.runtimeCtx - m.mu.RUnlock() - if ctx != nil { - m.startMockMetricsSampler(ctx) - } - log.Info().Msg("switched monitor to mock mode") + log.Info().Int("monitors", len(monitors)).Msg("switched monitors to mock mode") } else { - m.stopMockMetricsSampler() - if err := mock.SetEnabled(false); err != nil { - return err - } - m.mockModeFence.advance() - m.alertManager.ClearActiveAlerts() - m.forgetMockHostAgents() - m.mu.Lock() - m.resetStateLocked() - m.metricsHistory.Reset() - m.mu.Unlock() - log.Info().Msg("switched monitor to real data mode") + log.Info().Int("monitors", len(monitors)).Msg("switched monitors to real data mode") } + return nil +} +// endMockModeEpoch leaves the mode the monitor's alerts and state belong to +// once the process flag already holds the new one. Every evaluation of +// mode-dependent data that started before the flip must finish or be refused +// before the clear, or it reopens alerts for the side the monitor just left +// (see mockModeFence). The caller holds mockModeSwitchMu. +func (m *Monitor) endMockModeEpoch(enable bool) { + m.mockModeFence.advance() + m.alertManager.ClearActiveAlerts() + if !enable { + m.forgetMockHostAgents() + } + m.mu.Lock() + m.resetStateLocked() + m.metricsHistory.Reset() + m.mu.Unlock() + if enable { + m.StopDiscoveryService() + } + m.mockModeAligned = enable + m.mockModeSwitchesSeen = mockModeSwitchCount.Load() +} + +// resumeAfterMockModeSwitch restarts the mode-dependent runtime of a monitor +// whose Start loop has joined the switches, once every monitor has ended its +// epoch. A monitor that has not joined yet starts that runtime itself when it +// does (Start sets its runtime context first), and one whose loop has stopped +// only broadcasts. +func (m *Monitor) resumeAfterMockModeSwitch(enable, running bool) { m.mu.RLock() ctx := m.runtimeCtx hub := m.wsHub m.mu.RUnlock() + if !running || (ctx != nil && ctx.Err() != nil) { + ctx = nil + } + + if enable && ctx != nil { + m.startMockMetricsSampler(ctx) + } if hub != nil { m.broadcastCurrentState(hub) @@ -4702,8 +4753,6 @@ func (m *Monitor) SetMockMode(enable bool) error { go m.StartDiscoveryService(ctx, hub, m.config.DiscoverySubnet) } } - - return nil } func (m *Monitor) resetStateLocked() { diff --git a/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go b/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go index dca2a68b1c..e9d81cf562 100644 --- a/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go +++ b/internal/monitoring/monitor_host_agent_removal_lifecycle_test.go @@ -11,6 +11,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/alerts" "github.com/rcourtman/pulse-go-rewrite/internal/alerts/eventlog" "github.com/rcourtman/pulse-go-rewrite/internal/config" + "github.com/rcourtman/pulse-go-rewrite/internal/mock" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" agentshost "github.com/rcourtman/pulse-go-rewrite/pkg/agents/host" @@ -857,3 +858,45 @@ func TestMockHostAgentLeavingFixtureUsesRemovalLifecycle(t *testing.T) { } t.Fatal("node pve9 raised no memory alert after its agent left the mock estate") } + +func TestLeavingMockModeReleasesFixtureAgentNodeLinksOnEveryRunningMonitor(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + mustSetMockEnabled(t, true) + defaultMonitor, tenantMonitor := startTenantMonitors(t) + + // org-b's start pass registers the fixture agents in org-b's own alert + // manager, each owning the usage alerts of the fixture node it runs on. + var linked models.Node + waitForCondition(t, 30*time.Second, func() bool { + tenantMonitor.mockHostAgentsMu.Lock() + defer tenantMonitor.mockHostAgentsMu.Unlock() + for _, host := range tenantMonitor.mockHostAgents { + if host.Status != "online" || host.LinkedNodeID == "" { + continue + } + for _, node := range mock.CurrentFixtureGraph().State.Nodes { + if node.ID == host.LinkedNodeID { + linked = node + return true + } + } + } + return false + }, "org-b registered no online fixture agent linked to a fixture node") + + // The demo-fixture licence sync switches through the default monitor. + mustSetMonitorMockMode(t, defaultMonitor, false) + + // org-b's fixture agents go through the removal lifecycle too, so the + // node an agent covered owns its metric alerts again. + linked.Status = "online" + linked.Memory = models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75} + tenantMonitor.alertManager.CheckNode(linked) + for _, alert := range tenantMonitor.alertManager.GetActiveAlerts() { + if alert.ResourceID == linked.ID && alert.Type == "memory" { + return + } + } + t.Fatalf("org-b's node %s raised no memory alert: its fixture agent's node link outlived a switch made through the default monitor", linked.ID) +} diff --git a/internal/monitoring/monitor_mock_alerts_test.go b/internal/monitoring/monitor_mock_alerts_test.go index 3c02565ece..ce0b725e54 100644 --- a/internal/monitoring/monitor_mock_alerts_test.go +++ b/internal/monitoring/monitor_mock_alerts_test.go @@ -15,6 +15,7 @@ import ( "github.com/rcourtman/pulse-go-rewrite/internal/alerts" "github.com/rcourtman/pulse-go-rewrite/internal/alerts/eventlog" + "github.com/rcourtman/pulse-go-rewrite/internal/config" "github.com/rcourtman/pulse-go-rewrite/internal/mock" "github.com/rcourtman/pulse-go-rewrite/internal/models" "github.com/rcourtman/pulse-go-rewrite/internal/unifiedresources" @@ -809,6 +810,254 @@ func TestLeavingMockModeDiscardsAnInFlightConnectionCheck(t *testing.T) { } } +// startTenantMonitors starts the default organization's monitor and one +// other tenant's through MultiTenantMonitor, as the server does, each with its +// own alert manager, and waits until both monitoring loops run. An hour +// between ticks leaves each loop the pass it runs on start, so the test drives +// every later pass itself. +func startTenantMonitors(t *testing.T) (defaultMonitor, tenantMonitor *Monitor) { + t.Helper() + mtp, _ := newTestTenantPersistence(t) + mtm := NewMultiTenantMonitor(&config.Config{DataPath: t.TempDir(), PVEPollingInterval: time.Hour}, mtp, nil) + t.Cleanup(mtm.Stop) + mtm.SetMonitorInitializer(func(monitor *Monitor) { + cfg := monitor.alertManager.GetConfig() + cfg.Enabled = true + cfg.TimeThresholds = map[string]int{} + cfg.MetricTimeThresholds = nil + cfg.BackupDefaults.Enabled = false + cfg.AgentDefaults.Memory = &alerts.HysteresisThreshold{Trigger: 1, Clear: 0.5} + cfg.NodeDefaults.Memory = &alerts.HysteresisThreshold{Trigger: 1, Clear: 0.5} + monitor.alertManager.UpdateConfig(cfg) + }) + if err := mtp.SaveOrganization(&models.Organization{ID: "org-b", DisplayName: "Org B"}); err != nil { + t.Fatalf("SaveOrganization(org-b): %v", err) + } + monitors := make([]*Monitor, 0, 2) + for _, orgID := range []string{"default", "org-b"} { + monitor, err := mtm.GetMonitor(orgID) + if err != nil { + t.Fatalf("GetMonitor(%s): %v", orgID, err) + } + // Start sets its runtime context before it joins the monitors a + // switch reaches, so wait for the join itself. + waitForCondition(t, 30*time.Second, func() bool { + mockModeSwitchMu.Lock() + defer mockModeSwitchMu.Unlock() + _, running := mockModeRunningMonitors[monitor] + return running + }, orgID+" monitoring loop did not join mock-mode switches") + monitors = append(monitors, monitor) + } + return monitors[0], monitors[1] +} + +func mockHostAgentCount(monitor *Monitor) int { + monitor.mockHostAgentsMu.Lock() + defer monitor.mockHostAgentsMu.Unlock() + return len(monitor.mockHostAgents) +} + +func TestLeavingMockModeClearsEveryRunningTenantMonitor(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + mustSetMockEnabled(t, true) + defaultMonitor, tenantMonitor := startTenantMonitors(t) + + // Each loop runs a mock pass on start and raises the fixture's alerts in + // its own alert manager. + monitors := map[string]*Monitor{"default": defaultMonitor, "org-b": tenantMonitor} + for name, monitor := range monitors { + waitForCondition(t, 30*time.Second, func() bool { + return len(monitor.alertManager.GetActiveAlerts()) > 0 && mockHostAgentCount(monitor) > 0 + }, name+" monitor raised no fixture alerts in mock mode") + } + + // The mock-mode API switches through the requesting tenant's monitor. + mustSetMonitorMockMode(t, tenantMonitor, false) + + for name, monitor := range monitors { + if active := monitor.alertManager.GetActiveAlerts(); len(active) != 0 { + t.Errorf("%s monitor kept %d fixture alerts after mock mode was left through org-b: %s", + name, len(active), describeAlerts(active)) + } + if count := mockHostAgentCount(monitor); count != 0 { + t.Errorf("%s monitor still tracks %d fixture agents in live mode", name, count) + } + } +} + +func TestEnteringMockModeClearsEveryRunningTenantMonitor(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + defaultMonitor, tenantMonitor := startTenantMonitors(t) + + monitors := map[string]*Monitor{"default": defaultMonitor, "org-b": tenantMonitor} + for name, monitor := range monitors { + monitor.alertManager.CheckNode(models.Node{ + ID: name + "-live-node", + Name: "live-node", + Instance: "live", + Status: "online", + Memory: models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75}, + }) + if len(monitor.alertManager.GetActiveAlerts()) == 0 { + t.Fatalf("live node raised no alert in %s", name) + } + } + + // The demo-fixture licence sync switches through the default monitor. + mustSetMonitorMockMode(t, defaultMonitor, true) + + for name, monitor := range monitors { + for _, alert := range monitor.alertManager.GetActiveAlerts() { + if alert.ResourceID == name+"-live-node" { + t.Errorf("%s kept its live %s alert after mock mode was entered through the default monitor", name, alert.Type) + } + } + } +} + +func TestMonitorStartedAfterAMockModeSwitchLeavesTheModeItWasBuiltIn(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + manager := newMockHostAlertTestManager(t) + monitor := &Monitor{ + state: models.NewState(), + alertManager: manager, + metricsHistory: NewMetricsHistory(10, time.Hour), + } + // Built in live mode: New restored this live alert from disk. + manager.CheckNode(models.Node{ + ID: "live-node", + Name: "live-node", + Instance: "live", + Status: "online", + Memory: models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75}, + }) + if len(manager.GetActiveAlerts()) == 0 { + t.Fatal("live node raised no alert") + } + + // Another monitor switched to mock mode before this one's Start joined. + mustSetMockEnabled(t, true) + started := false + leave := monitor.joinMockModeSwitches(func(mockEnabled bool) { started = mockEnabled }) + defer leave() + + if !started { + t.Fatal("Start was not handed mock mode") + } + if active := manager.GetActiveAlerts(); len(active) != 0 { + t.Fatalf("a monitor built in live mode started in mock mode with its live alerts: %s", describeAlerts(active)) + } + mockModeSwitchMu.Lock() + _, running := mockModeRunningMonitors[monitor] + mockModeSwitchMu.Unlock() + if !running { + t.Fatal("joined monitor is not reached by later switches") + } +} + +func TestMonitorStartedAfterAMockModeRoundTripDropsWhatItRaisedMeanwhile(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + newMonitor := func() *Monitor { + return &Monitor{ + state: models.NewState(), + alertManager: newMockHostAlertTestManager(t), + metricsHistory: NewMetricsHistory(10, time.Hour), + } + } + switcher, starting := newMonitor(), newMonitor() + // Built in live mode, as New records it, and not running yet. + mockModeSwitchMu.Lock() + starting.mockModeSwitchesSeen = mockModeSwitchCount.Load() + mockModeSwitchMu.Unlock() + + mustSetMonitorMockMode(t, switcher, true) + // A read path evaluates fixture data on the monitor before its Start runs. + fixture := mock.CurrentFixtureGraph().State.Nodes[0] + fixture.Status = "online" + fixture.Memory = models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75} + starting.alertManager.CheckNode(fixture) + if len(starting.alertManager.GetActiveAlerts()) == 0 { + t.Fatal("fixture node raised no alert") + } + mustSetMonitorMockMode(t, switcher, false) + + leave := starting.joinMockModeSwitches(func(bool) {}) + defer leave() + if active := starting.alertManager.GetActiveAlerts(); len(active) != 0 { + t.Fatalf("a monitor that missed a mock-mode round trip started in live mode with fixture alerts: %s", describeAlerts(active)) + } +} + +func TestSetMockModeAlignsAMonitorWhoseModeTheFlagAlreadyHolds(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + manager := newMockHostAlertTestManager(t) + monitor := &Monitor{ + state: models.NewState(), + alertManager: manager, + metricsHistory: NewMetricsHistory(10, time.Hour), + } + // Built in live mode, with a live alert, and not running yet. + manager.CheckNode(models.Node{ + ID: "live-node", + Name: "live-node", + Instance: "live", + Status: "online", + Memory: models.Memory{Total: 64 << 30, Used: 60 << 30, Free: 4 << 30, Usage: 93.75}, + }) + if len(manager.GetActiveAlerts()) == 0 { + t.Fatal("live node raised no alert") + } + + // Another monitor's switch already set the flag. + mustSetMockEnabled(t, true) + mustSetMonitorMockMode(t, monitor, true) + + if active := manager.GetActiveAlerts(); len(active) != 0 { + t.Fatalf("SetMockMode(true) left a monitor built in live mode with its live alerts: %s", describeAlerts(active)) + } +} + +func TestMockModeSwitchLeavesAnUnjoinedMonitorsRuntimeToItsStart(t *testing.T) { + setMockSamplerTestEnv(t, time.Hour, 5*time.Minute) + pinDefaultMockEstate(t) + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + // Start has set its runtime context but not joined the switches yet. + monitor := &Monitor{ + config: &config.Config{}, + state: models.NewState(), + alertManager: newMockHostAlertTestManager(t), + metricsHistory: NewMetricsHistory(10, time.Hour), + runtimeCtx: ctx, + } + t.Cleanup(monitor.stopMockMetricsSampler) + + mustSetMonitorMockMode(t, monitor, true) + monitor.mu.RLock() + started := monitor.mockMetricsCancel != nil + monitor.mu.RUnlock() + if started { + t.Fatal("a switch started the runtime of a monitor whose Start has not joined; that runtime belongs to its Start") + } + + leave := monitor.joinMockModeSwitches(func(mockEnabled bool) { + monitor.startModeRuntime(ctx, nil, mockEnabled) + }) + defer leave() + monitor.mu.RLock() + started = monitor.mockMetricsCancel != nil + monitor.mu.RUnlock() + if !started { + t.Fatal("joining in mock mode did not start the mock sampler") + } +} + // diskAlertKeys lists the active disk-health and disk-wearout alerts as // "type instance/node device" so assertions name the disk, not the alert ID. func diskAlertKeys(manager *alerts.Manager) map[string]bool { diff --git a/internal/unifiedresources/availability.go b/internal/unifiedresources/availability.go index 35620d04da..e11068303b 100644 --- a/internal/unifiedresources/availability.go +++ b/internal/unifiedresources/availability.go @@ -4,6 +4,9 @@ import ( "fmt" "sort" "strings" + "time" + + "github.com/rcourtman/pulse-go-rewrite/internal/operationaltrust" ) // AvailabilityChecksForResource returns the complete canonical availability @@ -28,6 +31,25 @@ func AvailabilityCheckByTargetID(resource Resource, targetID string) *Availabili return nil } +// availabilityChecksProveOnline reports whether any availability check +// projected onto a monitored resource proves, at now, that the resource +// answers. Only a passing check with current evidence does. Each check is +// judged by its own evidence window (two poll intervals for a local check, +// the report window for a remote probe), so one check's recent run cannot +// vouch for another check's old pass, and a local check that missed its +// cadence keeps Available but proves nothing. A failing check proves only +// that one port or service does not answer, so it never decides the +// resource's status at all: not online, offline or warning, current or quiet. +func availabilityChecksProveOnline(checks []AvailabilityData, now time.Time) bool { + for _, check := range checks { + if check.Enabled && check.Available && check.Evidence != nil && + check.Evidence.FreshnessAt(now) == operationaltrust.EvidenceFresh { + return true + } + } + return false +} + func normalizeResourceAvailability(resource *Resource) { if resource == nil { return @@ -99,6 +121,10 @@ func availabilityCheckKey(check AvailabilityData) string { return fmt.Sprintf("endpoint:%s:%s:%d:%s", protocol, address, check.Port, path) } +// primaryAvailabilityCheck selects the singular compatibility summary: the +// worst check, ties going to the first by key. Health and row presentation +// read only the summary, so a confirmed outage on any check must outrank a +// failure that has not yet reached its threshold. func primaryAvailabilityCheck(checks []AvailabilityData) *AvailabilityData { if len(checks) == 0 { return nil @@ -113,13 +139,39 @@ func primaryAvailabilityCheck(checks []AvailabilityData) *AvailabilityData { } func availabilityCheckPriority(check AvailabilityData) int { - if check.LastChecked != nil && !check.Available { + if availabilityOutageConfirmed(check) { return 0 } + if check.LastChecked != nil && !check.Available { + return 1 + } if check.LastChecked == nil || check.CorrelationState == AvailabilityCorrelationAmbiguous || check.CorrelationState == AvailabilityCorrelationUnresolved { - return 1 + return 2 } - return 2 + return 3 +} + +// availabilityOutageConfirmed reports whether an enabled check has failed at +// least its failure threshold in a row, by the same gate the availability +// poller uses before raising availability_unreachable: an observed failure +// whose aggregate, when present, is unavailable. A probe agent that stops +// reporting keeps its failure count but reads indeterminate with an unknown +// aggregate, and confirms nothing. +func availabilityOutageConfirmed(check AvailabilityData) bool { + if !check.Enabled || check.Available || check.LastChecked == nil { + return false + } + if state := strings.TrimSpace(check.AggregateState); state != "" && !strings.EqualFold(state, "unavailable") { + return false + } + if strings.EqualFold(strings.TrimSpace(check.ProbeOutcome), "indeterminate") { + return false + } + threshold := check.FailureThreshold + if threshold <= 0 { + threshold = 1 + } + return check.ConsecutiveFailures >= threshold } diff --git a/internal/unifiedresources/availability_link_test.go b/internal/unifiedresources/availability_link_test.go index 1daaa01432..fcb21d0289 100644 --- a/internal/unifiedresources/availability_link_test.go +++ b/internal/unifiedresources/availability_link_test.go @@ -403,6 +403,133 @@ func TestAvailabilityKeepsEveryConfiguredCheckWithMultipleServicesOnOneHost(t *t } } +func TestAvailabilitySummaryPrefersConfirmedOutageOverEarlierUnconfirmedFailure(t *testing.T) { + rr := NewRegistry(nil) + hostID := ingestAgentFixture(t, rr, "host-1", "machine-1") + now := time.Now().UTC() + check := func(targetID, address string, facet AvailabilityData) IngestRecord { + checkedAt := now + facet.LinkedResourceID = hostID + facet.Address = address + facet.Protocol = "https" + facet.Enabled = true + facet.LastChecked = &checkedAt + facet.FailureThreshold = 3 + return availabilityProbeRecord(targetID, address, &facet) + } + + // probe-a sorts first by target id and has failed once, below its + // threshold. probe-b is past its threshold and owns the outage incident. + unconfirmed := check("probe-a", "203.0.113.10", AvailabilityData{ + AggregateState: "unavailable", + ConsecutiveFailures: 1, + ApplicationFailureCode: "status_mismatch", + }) + unconfirmed.Resource.Status = StatusWarning + confirmed := check("probe-b", "203.0.113.11", AvailabilityData{ + AggregateState: "unavailable", + ConsecutiveFailures: 3, + }) + confirmed.Resource.Status = StatusOffline + confirmed.Resource.Incidents = []ResourceIncident{{ + Provider: string(SourceAvailability), + NativeID: "probe-b", + Code: "availability_unreachable", + }} + rr.IngestRecords(SourceAvailability, []IngestRecord{unconfirmed, confirmed}) + + host, ok := rr.Get(hostID) + if !ok || host == nil { + t.Fatalf("host %q missing after ingest", hostID) + } + if len(AvailabilityChecksForResource(*host)) != 2 { + t.Fatalf("availability checks = %+v, want both attached checks", host.AvailabilityChecks) + } + if host.Availability == nil || host.Availability.TargetID != "probe-b" { + t.Fatalf("availability summary = %+v, want the confirmed outage on probe-b", host.Availability) + } + health := EvaluateResourceHealth(*host, nil, now) + if health.Verdict != HealthCritical || len(health.Reasons) == 0 || + health.Reasons[0].Code != "availability_failed" || health.Reasons[0].Detail != "" { + t.Fatalf("host health = %+v, want availability_failed from probe-b", health) + } + + // Once probe-b recovers, the unconfirmed failure is the worst check left: + // it is the summary again, and it does not make the host an outage. + rr.IngestRecords(SourceAvailability, []IngestRecord{ + check("probe-b", "203.0.113.11", AvailabilityData{ + AggregateState: "healthy", + Available: true, + }), + }) + host, _ = rr.Get(hostID) + if host.Availability == nil || host.Availability.TargetID != "probe-a" { + t.Fatalf("availability summary = %+v, want the remaining failure on probe-a", host.Availability) + } + health = EvaluateResourceHealth(*host, nil, now) + for _, reason := range health.Reasons { + if reason.Code == "availability_failed" { + t.Fatalf("host health = %+v, want no outage below the failure threshold", health) + } + } +} + +func TestAvailabilitySummaryDoesNotConfirmOutageFromSilentProbeAgent(t *testing.T) { + rr := NewRegistry(nil) + hostID := ingestAgentFixture(t, rr, "host-1", "machine-1") + now := time.Now().UTC() + checkedAt := now + unconfirmed := availabilityProbeRecord("probe-a", "203.0.113.10", &AvailabilityData{ + LinkedResourceID: hostID, + Address: "203.0.113.10", + Protocol: "https", + Enabled: true, + AggregateState: "unavailable", + ProbeOutcome: "unreachable", + LastChecked: &checkedAt, + ConsecutiveFailures: 1, + FailureThreshold: 3, + }) + unconfirmed.Resource.Status = StatusWarning + + // The poller's read of a single-location probe whose agent stopped + // reporting: the old failure count survives, but the outcome is + // indeterminate, the aggregate unknown, and no incident is raised. + lastReport := now.Add(-time.Hour) + silent := availabilityProbeRecord("probe-b", "203.0.113.11", &AvailabilityData{ + LinkedResourceID: hostID, + Address: "203.0.113.11", + Protocol: "https", + ProbeAgentID: "agent-probe-1", + Enabled: true, + AggregateState: "unknown", + ProbeOutcome: "indeterminate", + ExpectedLocations: 1, + LastChecked: &lastReport, + ConsecutiveFailures: 5, + FailureThreshold: 3, + LastError: "no recent report from probe agent", + }) + silent.Resource.Status = StatusWarning + rr.IngestRecords(SourceAvailability, []IngestRecord{unconfirmed, silent}) + + host, ok := rr.Get(hostID) + if !ok || host == nil { + t.Fatalf("host %q missing after ingest", hostID) + } + if host.Availability == nil || host.Availability.TargetID != "probe-a" { + t.Fatalf("availability summary = %+v, want probe-a: a silent probe confirms no outage", host.Availability) + } + for _, resource := range []Resource{*host, availabilityEndpointByTarget(t, rr, "probe-b")} { + health := EvaluateResourceHealth(resource, nil, now) + for _, reason := range health.Reasons { + if reason.Code == "availability_failed" { + t.Fatalf("%s health = %+v, want no outage from a silent probe agent", resource.ID, health) + } + } + } +} + func TestAvailabilityEditReplacesEndpointAndMovesProjection(t *testing.T) { rr := NewRegistry(nil) hostA := ingestAgentFixture(t, rr, "host-a", "machine-a") diff --git a/internal/unifiedresources/clone.go b/internal/unifiedresources/clone.go index 104c712f9f..0beca03f35 100644 --- a/internal/unifiedresources/clone.go +++ b/internal/unifiedresources/clone.go @@ -198,6 +198,7 @@ func cloneProxmoxData(in *ProxmoxData) *ProxmoxData { out.DockerCheckedAt = cloneTimePtr(in.DockerCheckedAt) out.TemperatureMonitoringEnabled = cloneBoolPtr(in.TemperatureMonitoringEnabled) out.PendingUpdatesCheckedAt = cloneTimePtr(in.PendingUpdatesCheckedAt) + out.SensorSetupOutdated = cloneBoolPtr(in.SensorSetupOutdated) return &out } diff --git a/internal/unifiedresources/clone_test.go b/internal/unifiedresources/clone_test.go index e46dc22840..63b87b54a1 100644 --- a/internal/unifiedresources/clone_test.go +++ b/internal/unifiedresources/clone_test.go @@ -518,6 +518,25 @@ func TestCloneProxmoxData_TemperatureIsolation(t *testing.T) { } } +// List and presentation readers get detached clones of a node, so the +// registry-derived sensor setup verdict travels with the clone and stays put +// when the live node's verdict changes afterwards. +func TestCloneProxmoxDataKeepsDerivedSensorSetupVerdict(t *testing.T) { + outdated := true + original := &ProxmoxData{NodeName: "pve", SensorSetupOutdated: &outdated} + cloned := cloneProxmoxData(original) + if cloned.SensorSetupOutdated == nil || !*cloned.SensorSetupOutdated { + t.Fatal("clone dropped the derived sensor setup verdict") + } + if cloned.SensorSetupOutdated == original.SensorSetupOutdated { + t.Fatal("clone shares the live node's verdict pointer") + } + *original.SensorSetupOutdated = false + if !*cloned.SensorSetupOutdated { + t.Fatal("changing the live node's verdict changed a clone already handed out") + } +} + func TestCloneProxmoxData_NetworkInterfaceIsolation(t *testing.T) { original := &ProxmoxData{ NetworkInterfaces: []NetworkInterface{ diff --git a/internal/unifiedresources/health.go b/internal/unifiedresources/health.go index fabf9c7bd0..ef74dc69ea 100644 --- a/internal/unifiedresources/health.go +++ b/internal/unifiedresources/health.go @@ -187,15 +187,10 @@ func healthAlertSeverity(level string) int { } } +// availabilityFailureConfirmed reads the compatibility summary, which ranks a +// confirmed outage on any attached check first. func availabilityFailureConfirmed(resource Resource) bool { - if resource.Availability == nil || !resource.Availability.Enabled || resource.Availability.Available { - return false - } - threshold := resource.Availability.FailureThreshold - if threshold <= 0 { - threshold = 1 - } - return resource.Availability.ConsecutiveFailures >= threshold + return resource.Availability != nil && availabilityOutageConfirmed(*resource.Availability) } func availabilityFailureDetail(resource Resource) string { diff --git a/internal/unifiedresources/health_test.go b/internal/unifiedresources/health_test.go index c51de693e4..696733d0cb 100644 --- a/internal/unifiedresources/health_test.go +++ b/internal/unifiedresources/health_test.go @@ -31,7 +31,7 @@ func TestEvaluateResourceHealthPrecedenceAndVerdicts(t *testing.T) { }, { name: "confirmed availability failure is critical", - resource: Resource{ID: "check-1", Type: ResourceTypeNetworkEndpoint, Status: StatusOnline, Availability: &AvailabilityData{Enabled: true, Available: false, ConsecutiveFailures: 3, FailureThreshold: 3}}, + resource: Resource{ID: "check-1", Type: ResourceTypeNetworkEndpoint, Status: StatusOnline, Availability: &AvailabilityData{Enabled: true, Available: false, LastChecked: &now, ConsecutiveFailures: 3, FailureThreshold: 3}}, want: HealthCritical, code: "availability_failed", }, { diff --git a/internal/unifiedresources/proxmox_sensor_setup.go b/internal/unifiedresources/proxmox_sensor_setup.go new file mode 100644 index 0000000000..4ff3c9c1b9 --- /dev/null +++ b/internal/unifiedresources/proxmox_sensor_setup.go @@ -0,0 +1,79 @@ +package unifiedresources + +import ( + "strings" + + "github.com/rcourtman/pulse-go-rewrite/pkg/diskinventory" +) + +// smartTemperatureDiskTypes are the disk types whose temperature reaches a +// Proxmox node only through SMART: the transports a host agent or the +// pulse-sensors wrapper reports (sata, sas) and the form factors Proxmox's own +// disk inventory reports for those same disks (hdd, ssd). Proxmox also types +// a non-rotational USB device ssd. NVMe temperatures come from kernel hwmon +// and arrive even on a legacy setup. +var smartTemperatureDiskTypes = map[string]struct{}{ + "sata": {}, + "sas": {}, + "hdd": {}, + "ssd": {}, +} + +// refreshProxmoxSensorSetupLocked derives ProxmoxData.SensorSetupOutdated on +// every Proxmox node, as an explicit true or false. SSH temperature monitoring +// set up before v6.0.0-rc.6 locks the node's key to `sensors -j`: the payload +// still parses and carries CPU and NVMe temperatures, but SMART disk +// temperatures can never arrive, so they stay blank with no error anywhere. +// The flag is data-gated three ways: the node's last temperature collection +// succeeded, its payload was the legacy format, and a disk under it waits on a +// SMART temperature it does not have now. A last known temperature kept for a +// disk that could not be read now (standby, a host agent past its lease) is not +// a current one (diskinventory.TemperatureCollected), while a disk whose +// temperature arrives another way, such as a linked host agent, falls out. +// Disks count through their canonical parent, so an agent the registry has not +// linked to the node contributes none. Deriving the flag here lets every +// surface that lists nodes read it without loading the node's disk inventory. +// It must run after buildChildCounts, which resolves each disk's canonical +// parent node. +func (rr *ResourceRegistry) refreshProxmoxSensorSetupLocked() { + waiting := make(map[string]struct{}) + for _, resource := range rr.resources { + if resource == nil { + continue + } + if resource.Proxmox != nil { + // A fresh pointer every time: a facet copied by an earlier merge or + // clone must not see this generation's verdict. + resource.Proxmox.SensorSetupOutdated = nil + if CanonicalResourceType(resource.Type) == ResourceTypeAgent { + resource.Proxmox.SensorSetupOutdated = sensorSetupVerdict(false) + } + } + if resource.ParentID != nil && physicalDiskWaitsOnSMARTTemperature(resource) { + waiting[*resource.ParentID] = struct{}{} + } + } + for parentID := range waiting { + node := rr.resources[parentID] + if node == nil || node.Proxmox == nil || CanonicalResourceType(node.Type) != ResourceTypeAgent { + continue + } + details := node.Proxmox.TemperatureDetails + node.Proxmox.SensorSetupOutdated = sensorSetupVerdict(details != nil && details.Available && details.LegacySensorsFormat) + } +} + +func sensorSetupVerdict(outdated bool) *bool { + return &outdated +} + +func physicalDiskWaitsOnSMARTTemperature(resource *Resource) bool { + if CanonicalResourceType(resource.Type) != ResourceTypePhysicalDisk || resource.PhysicalDisk == nil { + return false + } + disk := resource.PhysicalDisk + if _, ok := smartTemperatureDiskTypes[strings.ToLower(strings.TrimSpace(disk.DiskType))]; !ok { + return false + } + return !diskinventory.TemperatureCollected(disk.Temperature, disk.Collection) +} diff --git a/internal/unifiedresources/registry.go b/internal/unifiedresources/registry.go index 7b452be5b7..a799e8182f 100644 --- a/internal/unifiedresources/registry.go +++ b/internal/unifiedresources/registry.go @@ -602,6 +602,7 @@ func (rr *ResourceRegistry) ingestSnapshot(snapshot models.StateSnapshot, thresh rr.refreshStoragePostureLocked() rr.refreshIncidentRollupsLocked() rr.buildChildCounts() + rr.refreshProxmoxSensorSetupLocked() rr.refreshCanonicalIdentitiesLocked() rr.markStaleLocked(time.Now().UTC(), thresholds) rr.invalidateViewsLocked() @@ -653,6 +654,7 @@ func (rr *ResourceRegistry) ingestRecords(source DataSource, records []IngestRec rr.refreshStoragePostureLocked() rr.refreshIncidentRollupsLocked() rr.buildChildCounts() + rr.refreshProxmoxSensorSetupLocked() rr.refreshCanonicalIdentitiesLocked() rr.invalidateViewsLocked() rr.mu.Unlock() @@ -849,6 +851,7 @@ func (rr *ResourceRegistry) ingestResources(resources []Resource, thresholds map rr.refreshStoragePostureLocked() rr.refreshIncidentRollupsLocked() rr.buildChildCounts() + rr.refreshProxmoxSensorSetupLocked() rr.refreshCanonicalIdentitiesLocked() rr.markStaleLocked(time.Now().UTC(), thresholds) rr.invalidateViewsLocked() @@ -1805,16 +1808,16 @@ func (rr *ResourceRegistry) markStaleLocked(now time.Time, thresholds map[DataSo changed = changed || status.Status != "stale" status.Status = "stale" resource.SourceStatus[source] = status - staleFound = true + // The sighting of checks projected onto a resource is not one + // of its own sources, so its going quiet alone hands the stale + // pass nothing to decide; see reapplyStaleStatusForChecks. + if source != SourceAvailability || isAvailabilityOwnedResource(*resource) { + staleFound = true + } } } if staleFound { - recomputed := aggregateStatus(resource) - if recomputed != StatusUnknown { - resource.Status = recomputed - } else if resource.Status == StatusOnline { - resource.Status = StatusWarning - } + applyStaleStatus(resource, now) } changed = changed || resource.Status != previousStatus } @@ -1823,6 +1826,34 @@ func (rr *ResourceRegistry) markStaleLocked(now time.Time, thresholds map[DataSo } } +// applyStaleStatus is the stale pass's status rule for a resource with a +// quiet sighting. +func applyStaleStatus(resource *Resource, now time.Time) { + recomputed := aggregateStatus(resource, now) + if recomputed != StatusUnknown { + resource.Status = recomputed + } else if resource.Status == StatusOnline { + resource.Status = StatusWarning + } +} + +// reapplyStaleStatusForChecks re-applies the stale pass's rule to a resource +// whose availability checks just changed, when that pass owns its status +// because one of the resource's own sources went quiet. Projecting or +// retargeting a check is not a delivery from those sources, and the resources +// API replays checks after its stale pass, so the verdict the old checks gave +// would otherwise stand. The checks' own sighting never confers ownership: +// a resource whose own sources have not gone quiet keeps the status they gave +// it, whatever its checks say. +func reapplyStaleStatusForChecks(resource *Resource, now time.Time) { + for source, sighting := range resource.SourceStatus { + if source != SourceAvailability && sighting.Status == "stale" { + applyStaleStatus(resource, now) + return + } + } +} + func (rr *ResourceRegistry) ingestProxmoxNode(node models.Node, linkedHost *models.Host) { resource, identity := resourceFromProxmoxNode(node, linkedHost) rr.ingest(SourceProxmox, node.ID, resource, identity) @@ -2888,22 +2919,30 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res // Rehydrated registries seed exact source mappings from the persisted // unified snapshot. Honor that durable mapping before attempting weaker // identity correlation, but fail closed if a colliding source key belongs - // to a different physical-disk parent. + // to a different physical-disk parent. A Proxmox disk key names a slot, so + // it is also refused when the slot now holds a different disk, and no + // later step may merge the observation into the refused disk either. + refusedDiskID := "" if mappedID := rr.bySource[source][sourceID]; mappedID != "" { if existing := rr.resources[mappedID]; existing != nil && existing.Type == resource.Type && (resource.Type != ResourceTypePhysicalDisk || physicalDiskMatchScopeCompatible(existing, &resource)) { - if onlyMissing { - return "" + if source == SourceProxmox && resource.Type == ResourceTypePhysicalDisk && + proxmoxDiskSlotHoldsAnotherDisk(existing, &resource) { + refusedDiskID = existing.ID + } else { + if onlyMissing { + return "" + } + rr.mergeInto(existing, resource, source, sourceID) + return existing.ID } - rr.mergeInto(existing, resource, source, sourceID) - return existing.ID } } // Linked resources must be mutually linked to avoid one-sided/ambiguous auto-merges. - if linked := rr.resolveLinkedResource(source, sourceID, resource); linked != "" { + if linked := rr.resolveLinkedResource(source, sourceID, resource); linked != "" && linked != refusedDiskID { existing := rr.resources[linked] if existing != nil { if onlyMissing { @@ -2918,7 +2957,7 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res candidateID := rr.sourceSpecificID(resource.Type, source, sourceID) if resource.Type == ResourceTypeAgent || resource.Type == ResourceTypePhysicalDisk { - if match, excluded := rr.findMatch(resource, candidateID); match != nil { + if match, excluded := rr.findMatch(resource, candidateID); match != nil && match.ResourceB != refusedDiskID { existing := rr.resources[match.ResourceB] if existing != nil { if onlyMissing { @@ -2947,6 +2986,9 @@ func (rr *ResourceRegistry) ingestRecord(source DataSource, sourceID string, res resource.ID = rr.physicalDiskIDForMachineLocked(resource.ID, candidateID, &resource, source, onlyMissing) } normalizeResourceRelationships(&resource) + if resource.ID == refusedDiskID { + resource.ID = candidateID + } if existing := rr.resources[resource.ID]; existing != nil { if onlyMissing { return "" @@ -3046,6 +3088,7 @@ func (rr *ResourceRegistry) projectAvailabilityCheckLocked( Status: sourceSightingStatus(checkResource.LastSeen), LastSeen: checkResource.LastSeen, } + reapplyStaleStatusForChecks(target, time.Now().UTC()) } func (rr *ResourceRegistry) removeAvailabilityProjectionLocked( @@ -3080,6 +3123,7 @@ func (rr *ResourceRegistry) removeAvailabilityProjectionLocked( resource.Sources = removeDataSource(resource.Sources, SourceAvailability) delete(resource.SourceStatus, SourceAvailability) } + reapplyStaleStatusForChecks(resource, time.Now().UTC()) } // Build a fresh slice rather than compacting in place: callers may hold @@ -3197,6 +3241,56 @@ func physicalDiskMatchScopeCompatible(existing, incoming *Resource) bool { return identitiesShareHostname(existing.Identity, incoming.Identity) } +// proxmoxDiskSlotHoldsAnotherDisk reports whether a Proxmox disk observed +// under a mapped slot key (ProxmoxPhysicalDiskSourceID) names a different disk +// than the resource the key maps to: the slot's disk was swapped, and merging +// would keep the previous disk's canonical ID and, where an agent report was +// merged in, its serial, WWN and readings. Only hardware identity reported on +// both sides counts, and differing serials only where +// physicalDiskMappedSerialsComparable says both are the drive's own. +func proxmoxDiskSlotHoldsAnotherDisk(existing, incoming *Resource) bool { + if existing.PhysicalDisk == nil || incoming.PhysicalDisk == nil { + return false + } + existingSerial, existingWWN := existing.PhysicalDisk.Serial, existing.PhysicalDisk.WWN + incomingSerial, incomingWWN := incoming.PhysicalDisk.Serial, incoming.PhysicalDisk.WWN + if diskinventory.HardwareIdentityMatch(existingSerial, existingWWN, incomingSerial, incomingWWN) { + return false + } + if !physicalDiskMappedSerialsComparable(existing, incoming) { + existingSerial, incomingSerial = "", "" + } + return diskinventory.HardwareIdentityConflict(existingSerial, existingWWN, incomingSerial, incomingWWN) +} + +// physicalDiskMappedSerialsComparable reports whether differing serials on a +// mapped disk and a Proxmox observation under its slot key name different +// disks. Proxmox may report a SAS disk's transport address as its serial +// (#1595). A disk Proxmox alone reports carries Proxmox's serial, compared as +// is. A disk merged with an agent report may carry the agent's serial instead +// (mergeInto restores it, and collection status does not reliably say which), +// so its serial is compared only where Proxmox read the drive's own serial +// too, as monitoring's hostAgentSMARTSerialComparable decides: an NVMe +// controller serial, or an ATA drive's IDENTIFY serial, not a SCSI designator +// or a USB bridge's serial. +func physicalDiskMappedSerialsComparable(existing, incoming *Resource) bool { + switch { + case physicalDiskTypeIs(existing, "sas") || physicalDiskTypeIs(incoming, "sas"): + return false + case !hasDataSource(existing.Sources, SourceAgent): + return true + case physicalDiskTypeIs(incoming, "nvme"): + return physicalDiskTypeIs(existing, "nvme") + case physicalDiskTypeIs(incoming, "usb"): + return false + } + return physicalDiskTypeIs(existing, "sata") && strings.EqualFold(strings.TrimSpace(incoming.PhysicalDisk.Vendor), "ATA") +} + +func physicalDiskTypeIs(disk *Resource, diskType string) bool { + return strings.EqualFold(strings.TrimSpace(disk.PhysicalDisk.DiskType), diskType) +} + func (rr *ResourceRegistry) resolveLinkedResource(source DataSource, sourceID string, resource Resource) string { if resource.Type == ResourceTypePhysicalDisk && (source == SourceAgent || source == SourceProxmox) { @@ -4784,7 +4878,7 @@ func (rr *ResourceRegistry) mergeResourceData(primary *Resource, other *Resource // Manual links combine already-normalized resources. Preserve each metric's // recorded source instead of flattening the linked resource to SourceAgent. primary.Metrics = mergeMetrics(primary, primary.Metrics, other.Metrics, "", time.Now().UTC(), primary.SourceStatus, thresholds) - primary.Status = aggregateStatus(primary) + primary.Status = aggregateStatus(primary, time.Now().UTC()) } func (rr *ResourceRegistry) updateSourceMappings(fromID, toID string) { @@ -6372,15 +6466,25 @@ func chooseStatus(existing ResourceStatus, incoming ResourceStatus, source DataS // order: the highest-priority verdict, the best of equal ones. A source that // went quiet drops out of that decision, so a node the cluster reports // offline stays offline when its linked agent falls silent. A current facet -// sighting without a verdict (the PBS association, an availability check) -// counts as online only when no current source has one. Once every source is -// quiet, an offline verdict survives (a node the poller expired, an agent past -// its lease) and any other reads as warning; the best of those wins. -func aggregateStatus(resource *Resource) ResourceStatus { +// sighting without a verdict (the PBS association) counts as online only when +// no current source has one. Availability checks projected onto a monitored +// resource rank the same way, but their sighting is not their verdict: one +// sighting stands for every check, so each check is judged at now by its own +// evidence (see availabilityChecksProveOnline), and a check that does not +// prove the resource answers abstains. Checks only decide once one of the +// resource's own sources has gone quiet, so the stale pass owns the status and +// reverts it when the evidence lapses. Once every source is quiet, an +// offline verdict survives (a node the poller expired, an agent past its +// lease) and any other reads as warning; the best of those wins. +func aggregateStatus(resource *Resource, now time.Time) ResourceStatus { var current, quiet ResourceStatus currentPriority := -1 deliveredWithoutVerdict := false + checkedTarget := !isAvailabilityOwnedResource(*resource) for source, sighting := range resource.SourceStatus { + if source == SourceAvailability && checkedTarget { + continue + } switch strings.ToLower(strings.TrimSpace(sighting.Status)) { case "online": switch sighting.reported { @@ -6408,6 +6512,8 @@ func aggregateStatus(resource *Resource) ResourceStatus { return current case deliveredWithoutVerdict: return StatusOnline + case checkedTarget && quiet != "" && availabilityChecksProveOnline(AvailabilityChecksForResource(*resource), now): + return StatusOnline case quiet != "": return quiet default: diff --git a/internal/unifiedresources/registry_merge_policy_test.go b/internal/unifiedresources/registry_merge_policy_test.go index 89fafc70b0..e4d7080211 100644 --- a/internal/unifiedresources/registry_merge_policy_test.go +++ b/internal/unifiedresources/registry_merge_policy_test.go @@ -712,18 +712,30 @@ func TestIngestResourcesKeepsAFacetSightingWithoutAVerdict(t *testing.T) { // aggregateStatus is the one rule the stale pass and manual links apply. // Current sources decide by their own verdicts in chooseStatus's priority -// order; quiet sources only decide when no source is current. +// order; quiet sources only decide when no source is current. Availability +// checks on a monitored resource are judged by their own evidence, not their +// shared sighting. func TestAggregateStatusReadsSourceVerdictsNotDelivery(t *testing.T) { + now := time.Now().UTC() + lapsed := now.Add(-10 * time.Minute) current := func(reported ResourceStatus) SourceStatus { return SourceStatus{Status: "online", reported: reported} } quiet := func(reported ResourceStatus) SourceStatus { return SourceStatus{Status: "stale", reported: reported} } + check := func(passing bool, checkedAt time.Time) []AvailabilityData { + return []AvailabilityData{{ + TargetID: "probe-1", Enabled: true, Available: passing, LastChecked: &checkedAt, + Evidence: availabilityProbeEvidence(t, "probe-1", checkedAt), + }} + } for _, tc := range []struct { - name string - sightings map[DataSource]SourceStatus - want ResourceStatus + name string + resourceType ResourceType + sightings map[DataSource]SourceStatus + checks []AvailabilityData + want ResourceStatus }{ { name: "live poll reports offline, agent quiet", @@ -767,9 +779,57 @@ func TestAggregateStatusReadsSourceVerdictsNotDelivery(t *testing.T) { }, { name: "a facet without a verdict never outranks a current verdict", - sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusOffline), SourceAvailability: current("")}, + sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusOffline), SourcePBS: current("")}, want: StatusOffline, }, + { + name: "a current failing check abstains", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOffline), SourceAvailability: current("")}, + checks: check(false, now), + want: StatusOffline, + }, + { + name: "a quiet failing check abstains rather than lifting an offline verdict to warning", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOffline), SourceAvailability: quiet("")}, + checks: check(false, lapsed), + want: StatusOffline, + }, + { + name: "a passing check whose evidence lapsed abstains under a current sighting", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOnline), SourceAvailability: current("")}, + checks: check(true, lapsed), + want: StatusWarning, + }, + { + name: "a passing check with current evidence proves the target answers under a quiet sighting", + sightings: map[DataSource]SourceStatus{SourceProxmox: quiet(StatusOffline), SourceAvailability: quiet("")}, + checks: check(true, now), + want: StatusOnline, + }, + { + name: "a passing check never lifts a resource whose own sources have not gone quiet", + sightings: map[DataSource]SourceStatus{SourceProxmox: {Status: "unknown"}, SourceAvailability: current("")}, + checks: check(true, now), + want: StatusUnknown, + }, + { + name: "a passing check never outranks a current verdict", + sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusOffline), SourceAvailability: current("")}, + checks: check(true, now), + want: StatusOffline, + }, + { + name: "a check's own row keeps its quiet verdict", + resourceType: ResourceTypeNetworkEndpoint, + sightings: map[DataSource]SourceStatus{SourceAvailability: quiet(StatusOffline)}, + want: StatusOffline, + }, + { + name: "a check's own row keeps its current verdict", + resourceType: ResourceTypeNetworkEndpoint, + sightings: map[DataSource]SourceStatus{SourceAvailability: current(StatusWarning)}, + want: StatusWarning, + }, { name: "current unknown verdict leaves the quiet sources to decide", sightings: map[DataSource]SourceStatus{SourceProxmox: current(StatusUnknown), SourceAgent: quiet(StatusOnline)}, @@ -782,7 +842,8 @@ func TestAggregateStatusReadsSourceVerdictsNotDelivery(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - if got := aggregateStatus(&Resource{SourceStatus: tc.sightings}); got != tc.want { + resource := &Resource{Type: tc.resourceType, SourceStatus: tc.sightings, AvailabilityChecks: tc.checks} + if got := aggregateStatus(resource, now); got != tc.want { t.Fatalf("aggregateStatus = %q, want %q", got, tc.want) } }) diff --git a/internal/unifiedresources/registry_test.go b/internal/unifiedresources/registry_test.go index 7c98c19bca..56a426e60c 100644 --- a/internal/unifiedresources/registry_test.go +++ b/internal/unifiedresources/registry_test.go @@ -5686,7 +5686,8 @@ func TestMarkStaleRecomputesFromRemainingFreshSources(t *testing.T) { Availability: &AvailabilityData{ TargetID: "probe-delly", LinkedResourceID: host.ID, Address: "192.0.2.5", Protocol: "tcp", Port: 8006, - Enabled: true, Available: true, + Enabled: true, Available: true, LastChecked: &recentNow, + Evidence: availabilityProbeEvidence(t, "probe-delly", recentNow), }, }, Identity: ResourceIdentity{IPAddresses: []string{"192.0.2.5"}}, @@ -5759,6 +5760,7 @@ func TestMarkStaleKeepsProxmoxRuntimeStateIndependentOfAvailabilityFacet(t *test Availability: &AvailabilityData{ TargetID: "probe-102", LinkedResourceID: checkedID, Address: "192.0.2.102", Protocol: "icmp", Enabled: true, Available: true, + LastChecked: &freshSeen, Evidence: availabilityProbeEvidence(t, "probe-102", freshSeen), }, }, }}) @@ -5781,6 +5783,305 @@ func TestMarkStaleKeepsProxmoxRuntimeStateIndependentOfAvailabilityFacet(t *test } } +// availabilityCheckRecord is a check the poller ran at checkedAt against the +// resource targetID names. Its evidence stays current for two minutes, the +// two poll intervals of a local check; Available stays as the last run left +// it, as a local check that misses its cadence does. +func availabilityCheckRecord(t *testing.T, checkID, targetID string, passing bool, checkedAt time.Time) IngestRecord { + t.Helper() + status, failures := StatusOnline, 0 + if !passing { + status, failures = StatusOffline, 3 + } + return IngestRecord{ + SourceID: checkID, + Resource: Resource{ + Type: ResourceTypeNetworkEndpoint, Name: checkID, Status: status, LastSeen: checkedAt, + Sources: []DataSource{SourceAvailability}, + Availability: &AvailabilityData{ + TargetID: checkID, LinkedResourceID: targetID, + Address: "192.0.2.80", Protocol: "http", Port: 443, + Enabled: true, Available: passing, LastChecked: &checkedAt, + ConsecutiveFailures: failures, FailureThreshold: 2, PollIntervalSeconds: 60, + Evidence: availabilityProbeEvidence(t, checkID, checkedAt), + }, + }, + } +} + +func ingestProxmoxGuestSeenAt(t *testing.T, rr *ResourceRegistry, sourceID string, seen time.Time) string { + t.Helper() + rr.IngestRecords(SourceProxmox, []IngestRecord{{ + SourceID: sourceID, + Resource: Resource{ + Type: ResourceTypeSystemContainer, Name: sourceID, Status: StatusOnline, LastSeen: seen, + Proxmox: &ProxmoxData{RuntimeStatus: "running", NodeName: "node-a"}, + }, + }}) + for _, resource := range rr.ListByType(ResourceTypeSystemContainer) { + if resource.Name == sourceID { + return resource.ID + } + } + t.Fatalf("guest %s not ingested", sourceID) + return "" +} + +// A failing availability check proves only that one port or service does not +// answer, so it never decides its target's status: a node the poller expired +// stays offline, a guest whose poll went quiet stays a warning instead of +// counting as stopped, and neither reads online. Only a passing check with +// current evidence proves the target answers. The stale pass used to count +// any current check as online, so a failing probe kept an expired node up. +func TestStalePassReadsAvailabilityCheckVerdicts(t *testing.T) { + now := time.Now().UTC() + quietSeen := now.Add(-5 * time.Minute) + lapsed := now.Add(-10 * time.Minute) + expiredNode := models.StateSnapshot{Nodes: []models.Node{{ + ID: "homelab-pve9", Name: "pve9", Instance: "homelab", ClusterName: "homelab", + Status: "offline", LastSeen: quietSeen, + }}} + quietGuest := models.StateSnapshot{VMs: []models.VM{{ + ID: "homelab:pve9:101", Name: "web", Node: "pve9", Instance: "homelab", + VMID: 101, Status: "running", Type: "qemu", LastSeen: quietSeen, + }}} + for _, tc := range []struct { + name string + snapshot models.StateSnapshot + targetType ResourceType + passing bool + checkedAt time.Time + want ResourceStatus + }{ + {"expired node, current failing check", expiredNode, ResourceTypeAgent, false, now, StatusOffline}, + {"expired node, lapsed failing check", expiredNode, ResourceTypeAgent, false, lapsed, StatusOffline}, + {"expired node, lapsed passing check", expiredNode, ResourceTypeAgent, true, lapsed, StatusOffline}, + {"expired node, current passing check", expiredNode, ResourceTypeAgent, true, now, StatusOnline}, + {"quiet guest, current failing check", quietGuest, ResourceTypeVM, false, now, StatusWarning}, + {"quiet guest, lapsed failing check", quietGuest, ResourceTypeVM, false, lapsed, StatusWarning}, + {"quiet guest, current passing check", quietGuest, ResourceTypeVM, true, now, StatusOnline}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := NewRegistry(nil) + rr.IngestSnapshot(tc.snapshot) + target := onlyResourceOfType(t, rr, tc.targetType) + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-1", target.ID, tc.passing, tc.checkedAt), + }) + rr.MarkStale(now, nil) + + got, ok := rr.Get(target.ID) + if !ok || len(AvailabilityChecksForResource(*got)) != 1 { + t.Fatalf("check not projected onto %s: %+v", target.ID, got.AvailabilityChecks) + } + if got.Status != tc.want { + t.Fatalf("status = %q, want %q", got.Status, tc.want) + } + }) + } +} + +// One availability sighting stands for every check on a target, and it +// carries whichever check was projected last. Each check is judged by its own +// evidence instead, so a failing check that just ran cannot lend its +// freshness to another check's old pass, in either projection order. +func TestAvailabilityChecksAreJudgedByTheirOwnEvidence(t *testing.T) { + now := time.Now().UTC() + lapsed := now.Add(-10 * time.Minute) + for _, tc := range []struct { + name string + passedAt, failedAt time.Time + want ResourceStatus + }{ + {name: "lapsed pass, current failure", passedAt: lapsed, failedAt: now, want: StatusWarning}, + {name: "current pass, lapsed failure", passedAt: now, failedAt: lapsed, want: StatusOnline}, + } { + for _, order := range [][]string{{"pass", "fail"}, {"fail", "pass"}} { + t.Run(tc.name+", "+strings.Join(order, " then "), func(t *testing.T) { + rr := NewRegistry(nil) + target := ingestProxmoxGuestSeenAt(t, rr, "lab:node-a:101", now.Add(-5*time.Minute)) + records := map[string]IngestRecord{ + "pass": availabilityCheckRecord(t, "probe-pass", target, true, tc.passedAt), + "fail": availabilityCheckRecord(t, "probe-fail", target, false, tc.failedAt), + } + for _, key := range order { + rr.IngestRecords(SourceAvailability, []IngestRecord{records[key]}) + } + rr.MarkStale(now, nil) + + got, ok := rr.Get(target) + if !ok { + t.Fatal("target missing") + } + if checks := AvailabilityChecksForResource(*got); len(checks) != 2 { + t.Fatalf("expected both checks on the target, got %+v", checks) + } + if got.Status != tc.want { + t.Fatalf("status = %q, want %q", got.Status, tc.want) + } + }) + } + } +} + +// A check's verdict moves with the check. Projecting, re-running or +// retargeting a check is not a delivery from the resource's own sources, so a +// target whose status the stale pass owns is judged again at once rather than +// keeping the old checks' verdict until the next pass; the resources API +// replays checks after its stale pass and never runs another. +func TestChangedAvailabilityChecksJudgeTheirTargetsAgain(t *testing.T) { + now := time.Now().UTC() + quietSeen := now.Add(-5 * time.Minute) + rr := NewRegistry(nil) + first := ingestProxmoxGuestSeenAt(t, rr, "lab:node-a:101", quietSeen) + second := ingestProxmoxGuestSeenAt(t, rr, "lab:node-a:102", quietSeen) + rr.MarkStale(now, nil) + + status := func(id string) ResourceStatus { + t.Helper() + resource, ok := rr.Get(id) + if !ok { + t.Fatalf("resource %s missing", id) + } + return resource.Status + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-moving", first, true, now), + availabilityCheckRecord(t, "probe-failing", first, false, now), + }) + if got := status(first); got != StatusOnline { + t.Fatalf("first status = %q once a passing check lands, want online", got) + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-moving", second, true, now), + }) + left, _ := rr.Get(first) + if checks := AvailabilityChecksForResource(*left); len(checks) != 1 || checks[0].TargetID != "probe-failing" { + t.Fatalf("first checks = %+v, want only probe-failing", checks) + } + if got := status(first); got != StatusWarning { + t.Fatalf("first status = %q after its passing check moved away, want warning", got) + } + if got := status(second); got != StatusOnline { + t.Fatalf("second status = %q after the passing check moved onto it, want online", got) + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-moving", second, false, now), + }) + if got := status(second); got != StatusWarning { + t.Fatalf("second status = %q once its only check fails, want warning", got) + } + + rr.MarkStale(now, nil) + if got := status(first); got != StatusWarning { + t.Fatalf("first status = %q after the next pass, want warning", got) + } + if got := status(second); got != StatusWarning { + t.Fatalf("second status = %q after the next pass, want warning", got) + } +} + +// A projected check's sighting is not one of its target's own sources. Its +// going quiet hands the stale pass nothing to decide, and replacing the check +// takes no ownership away, so a target whose own source has never delivered +// keeps the status that source gave it whatever its checks say. +func TestAvailabilitySightingNeverHandsTheStalePassItsTarget(t *testing.T) { + now := time.Now().UTC() + quickChecks := map[DataSource]time.Duration{SourceAvailability: 30 * time.Second} + for _, tc := range []struct { + name string + ingested ResourceStatus + passing bool + }{ + {name: "online guest, quiet failing check", ingested: StatusOnline, passing: false}, + {name: "offline placeholder, quiet passing check", ingested: StatusOffline, passing: true}, + } { + t.Run(tc.name, func(t *testing.T) { + rr := NewRegistry(nil) + rr.IngestRecords(SourceProxmox, []IngestRecord{{ + SourceID: "lab:node-a:101", + Resource: Resource{ + Type: ResourceTypeSystemContainer, Name: "lab:node-a:101", Status: tc.ingested, + Proxmox: &ProxmoxData{RuntimeStatus: "running", NodeName: "node-a"}, + }, + }}) + target := onlyResourceOfType(t, rr, ResourceTypeSystemContainer).ID + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-1", target, tc.passing, now.Add(-time.Minute)), + }) + rr.MarkStale(now, quickChecks) + got, _ := rr.Get(target) + if sighting := got.SourceStatus[SourceAvailability]; sighting.Status != "stale" { + t.Fatalf("availability sighting = %+v, want stale", sighting) + } + if got.Status != tc.ingested { + t.Fatalf("status = %q with a quiet check, want %q from the target's own source", got.Status, tc.ingested) + } + + rr.IngestRecords(SourceAvailability, []IngestRecord{ + availabilityCheckRecord(t, "probe-1", target, !tc.passing, now), + }) + rr.MarkStale(now, quickChecks) + if got, _ := rr.Get(target); got.Status != tc.ingested { + t.Fatalf("status = %q after the check flipped, want %q", got.Status, tc.ingested) + } + }) + } +} + +// A manual link keeps the primary's own availability checks and not the +// linked resource's, so the merged resource is judged by the checks it +// carries. The linked resource's fresher availability sighting survives the +// merge as delivery bookkeeping; it must not become a verdict for checks the +// merged resource does not show. +func TestManualLinkJudgesOnlyTheChecksTheMergedResourceCarries(t *testing.T) { + now := time.Now().UTC() + quietSeen := now.Add(-5 * time.Minute) + store := NewMemoryStore() + if err := store.AddLink(ResourceLink{ResourceA: "vm-web", ResourceB: "agent-web", PrimaryID: "vm-web"}); err != nil { + t.Fatalf("add link: %v", err) + } + rr := NewRegistry(store) + rr.IngestResources([]Resource{ + { + ID: "vm-web", Type: ResourceTypeVM, Name: "web", Status: StatusWarning, LastSeen: quietSeen, + Sources: []DataSource{SourceProxmox}, + SourceStatus: map[DataSource]SourceStatus{ + SourceProxmox: {Status: "stale", LastSeen: quietSeen, reported: StatusOnline}, + }, + }, + { + ID: "agent-web", Type: ResourceTypeAgent, Name: "web", Status: StatusWarning, LastSeen: now, + Sources: []DataSource{SourceAgent, SourceAvailability}, + SourceStatus: map[DataSource]SourceStatus{ + SourceAgent: {Status: "stale", LastSeen: quietSeen, reported: StatusOnline}, + SourceAvailability: {Status: "online", LastSeen: now}, + }, + AvailabilityChecks: []AvailabilityData{{ + TargetID: "probe-web", Enabled: true, Available: false, LastChecked: &now, + ConsecutiveFailures: 3, FailureThreshold: 2, + Evidence: availabilityProbeEvidence(t, "probe-web", now), + }}, + Agent: &AgentData{AgentID: "host-web", Hostname: "web"}, + }, + }) + + merged, ok := rr.Get("vm-web") + if !ok { + t.Fatal("linked resource missing") + } + if checks := AvailabilityChecksForResource(*merged); len(checks) != 0 { + t.Fatalf("merged resource exposes the linked resource's checks: %+v", checks) + } + if merged.Status != StatusWarning { + t.Fatalf("status = %q, want warning from its own quiet sources", merged.Status) + } +} + func TestResourceRegistryUsesConfiguredProxmoxStaleThresholds(t *testing.T) { seen := time.Now().UTC().Add(-90 * time.Second).Truncate(time.Millisecond) snapshot := models.StateSnapshot{ @@ -7144,3 +7445,248 @@ func TestPhysicalDiskRiskNeverJudgesTemperature(t *testing.T) { t.Fatalf("meta recompute risk = %+v, want none for a collected 72C", risk) } } + +// The Proxmox page's Overview lists nodes without their disk inventory, so the +// outdated sensor setup verdict has to arrive on the node itself. The registry +// derives it from the node's legacy-format temperature payload and the disks +// it parents, with the same reading rule every disk temperature consumer uses. +func TestProxmoxNodeSensorSetupOutdatedFromItsDisks(t *testing.T) { + now := time.Now() + legacy := &models.Temperature{Available: true, LegacySensorsFormat: true} + inventoryOnly := &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature"), + } + node := func(name string, temperature *models.Temperature) models.Node { + return models.Node{ + ID: "homelab-" + name, + Name: name, + Instance: "homelab", + Status: "online", + LastSeen: now, + Temperature: temperature, + } + } + disk := func(nodeName, devPath, diskType string, temperature int, collection *diskinventory.CollectionStatus) models.PhysicalDisk { + return models.PhysicalDisk{ + ID: ProxmoxPhysicalDiskSourceID("homelab", nodeName, devPath, "", ""), + Node: nodeName, + Instance: "homelab", + DevPath: devPath, + Model: "Disk " + devPath, + Serial: "SERIAL-" + nodeName + devPath, + Type: diskType, + Health: "PASSED", + Wearout: -1, + Temperature: temperature, + Collection: collection, + LastChecked: now, + } + } + + cases := []struct { + name string + nodes []models.Node + disks []models.PhysicalDisk + want bool + }{ + { + name: "SATA disk without a temperature", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 0, inventoryOnly)}, + want: true, + }, + { + // Proxmox's own disk inventory types these disks by form factor. + name: "Proxmox hdd and ssd inventory types", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "hdd", 0, inventoryOnly), disk("pve", "/dev/sdb", "ssd", 0, inventoryOnly)}, + want: true, + }, + { + name: "SAS disk whose reading is retained, not current", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sas", 41, &diskinventory.CollectionStatus{ + Temperature: diskinventory.Unavailable("host_agent", "host agent stopped reporting"), + })}, + want: true, + }, + { + name: "SATA disk whose reading the source marks missing", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 38, &diskinventory.CollectionStatus{ + Temperature: diskinventory.Missing("proxmox_node_smart", "temperature was not reported"), + })}, + want: true, + }, + { + name: "SATA disk with a current reading", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 36, &diskinventory.CollectionStatus{ + Temperature: diskinventory.Available("proxmox_node_smart"), + })}, + want: false, + }, + { + name: "reading from a source that predates collection state", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 36, nil)}, + want: false, + }, + { + // NVMe temperatures arrive through kernel hwmon even on a legacy setup. + name: "NVMe disk without a temperature", + nodes: []models.Node{node("pve", legacy)}, + disks: []models.PhysicalDisk{disk("pve", "/dev/nvme0n1", "nvme", 0, inventoryOnly)}, + want: false, + }, + { + name: "current setup payload", + nodes: []models.Node{node("pve", &models.Temperature{Available: true})}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 0, inventoryOnly)}, + want: false, + }, + { + name: "failed temperature collection", + nodes: []models.Node{node("pve", &models.Temperature{Available: false, LegacySensorsFormat: true})}, + disks: []models.PhysicalDisk{disk("pve", "/dev/sda", "sata", 0, inventoryOnly)}, + want: false, + }, + { + name: "waiting disk belongs to another node", + nodes: []models.Node{node("pve", legacy), node("pve2", &models.Temperature{Available: true})}, + disks: []models.PhysicalDisk{disk("pve2", "/dev/sda", "sata", 0, inventoryOnly)}, + want: false, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + registry := NewRegistry(nil) + registry.IngestSnapshot(models.StateSnapshot{Nodes: tc.nodes, PhysicalDisks: tc.disks}) + got := proxmoxNodeSensorSetupOutdated(t, registry.List(), "pve") + if got != tc.want { + t.Fatalf("pve sensorSetupOutdated = %v, want %v", got, tc.want) + } + }) + } +} + +// A linked host agent that reads the disk's SMART temperature itself answers +// what the legacy SSH setup cannot, so the node is not flagged; the verdict is +// derived on every ingest, so a later snapshot clears an earlier flag. +func TestProxmoxNodeSensorSetupOutdatedFollowsWhereTheReadingArrives(t *testing.T) { + now := time.Now() + nodes := []models.Node{{ + ID: "homelab-pve", + Name: "pve", + Instance: "homelab", + LinkedAgentID: "host-pve", + Status: "online", + LastSeen: now, + Temperature: &models.Temperature{Available: true, LegacySensorsFormat: true}, + }} + inventory := []models.PhysicalDisk{{ + ID: ProxmoxPhysicalDiskSourceID("homelab", "pve", "/dev/sda", "", ""), + Node: "pve", + Instance: "homelab", + DevPath: "/dev/sda", + Model: "WDC WD80EFAX", + Serial: "SERIAL-SDA", + Type: "hdd", + Health: "PASSED", + Wearout: -1, + Collection: &diskinventory.CollectionStatus{Temperature: diskinventory.Unsupported("proxmox_disks", "Proxmox disk inventory does not expose temperature")}, + LastChecked: now, + }} + agent := func(collection *diskinventory.CollectionStatus) []models.Host { + return []models.Host{{ + ID: "host-pve", + Hostname: "pve", + LinkedNodeID: "homelab-pve", + Status: "online", + LastSeen: now, + Sensors: models.HostSensorSummary{SMART: []models.HostDiskSMART{{ + Device: "/dev/sda", + Model: "WDC WD80EFAX", + Serial: "SERIAL-SDA", + Type: "sata", + Temperature: 34, + Health: "PASSED", + Collection: collection, + }}}, + }} + } + + registry := NewRegistry(nil) + registry.IngestSnapshot(models.StateSnapshot{Nodes: nodes, PhysicalDisks: inventory}) + if !proxmoxNodeSensorSetupOutdated(t, registry.List(), "pve") { + t.Fatal("legacy node with an unread hdd: sensorSetupOutdated = false, want true") + } + + registry.IngestSnapshot(models.StateSnapshot{ + Nodes: nodes, + PhysicalDisks: inventory, + Hosts: agent(&diskinventory.CollectionStatus{Temperature: diskinventory.Available("host_agent")}), + }) + if proxmoxNodeSensorSetupOutdated(t, registry.List(), "pve") { + t.Fatal("linked agent reads the disk now: sensorSetupOutdated = true, want the earlier flag cleared") + } + // Clients merge a facet field by field, so the cleared verdict must be on + // the wire as false, not omitted. + assertProxmoxNodeSensorSetupJSON(t, registry.ListForPresentation(), "pve", `"sensorSetupOutdated":false`) + + registry.IngestSnapshot(models.StateSnapshot{ + Nodes: nodes, + PhysicalDisks: inventory, + Hosts: agent(&diskinventory.CollectionStatus{Temperature: diskinventory.Unavailable("host_agent", "host agent stopped reporting")}), + }) + presented := registry.ListForPresentation() + if !proxmoxNodeSensorSetupOutdated(t, presented, "pve") { + t.Fatal("agent reading retained, not current: sensorSetupOutdated = false, want true") + } + assertProxmoxNodeSensorSetupJSON(t, presented, "pve", `"sensorSetupOutdated":true`) +} + +func assertProxmoxNodeSensorSetupJSON(t *testing.T, resources []Resource, nodeName, want string) { + t.Helper() + for _, resource := range resources { + if resource.Proxmox == nil || resource.Proxmox.NodeName != nodeName || CanonicalResourceType(resource.Type) != ResourceTypeAgent { + continue + } + payload, err := json.Marshal(resource.Proxmox) + if err != nil { + t.Fatalf("marshal node proxmox facet: %v", err) + } + if !strings.Contains(string(payload), want) { + t.Fatalf("node proxmox payload = %s, want %s for list readers", payload, want) + } + return + } + t.Fatalf("node %q not presented", nodeName) +} + +func proxmoxNodeSensorSetupOutdated(t *testing.T, resources []Resource, nodeName string) bool { + t.Helper() + var found *Resource + for i := range resources { + resource := &resources[i] + if CanonicalResourceType(resource.Type) != ResourceTypeAgent || resource.Proxmox == nil || resource.Proxmox.NodeName != nodeName { + continue + } + if found != nil { + t.Fatalf("node %q presented twice: %s and %s", nodeName, found.ID, resource.ID) + } + found = resource + } + if found == nil { + t.Fatalf("node %q not found among %d resources", nodeName, len(resources)) + } + for _, resource := range resources { + if CanonicalResourceType(resource.Type) != ResourceTypeAgent && resource.Proxmox != nil && resource.Proxmox.SensorSetupOutdated != nil { + t.Fatalf("%s %s carries the node verdict", resource.Type, resource.ID) + } + } + if found.Proxmox.SensorSetupOutdated == nil { + t.Fatalf("node %q has no explicit sensor setup verdict", nodeName) + } + return *found.Proxmox.SensorSetupOutdated +} diff --git a/internal/unifiedresources/types.go b/internal/unifiedresources/types.go index fe8acee7c7..d54c5e831b 100644 --- a/internal/unifiedresources/types.go +++ b/internal/unifiedresources/types.go @@ -414,6 +414,14 @@ type ProxmoxData struct { MemoryCache int64 `json:"memoryCache,omitempty"` Memory *models.Memory `json:"memory,omitempty"` Lock string `json:"lock,omitempty"` // Proxmox lock state (e.g. "backup", "migrate", "snapshot") + // SensorSetupOutdated is derived by the registry, never ingested: the + // node's SSH temperature monitoring still runs the pre-rc.6 setup and a + // disk under it waits on a SMART temperature that setup cannot deliver + // (refreshProxmoxSensorSetupLocked). Every Proxmox node carries an + // explicit true or false, so clients that merge facets field by field + // replace a cleared verdict instead of keeping an earlier true; other + // resources omit it. + SensorSetupOutdated *bool `json:"sensorSetupOutdated,omitempty"` // Internal link hint to a host agent resource. LinkedAgentID string `json:"-"` }