From c1d8cd8f11c943b9df1654c38be48f8546eea9eb Mon Sep 17 00:00:00 2001 From: rcourtman Date: Fri, 3 Jul 2026 10:43:28 +0100 Subject: [PATCH] Prefer agent temperatures over SSH fallback Refs #1473 Refs #1509 --- docs/FAQ.md | 2 +- docs/TEMPERATURE_MONITORING.md | 12 +++-- .../v6/internal/subsystems/monitoring.md | 17 +++---- docs/security/TEMPERATURE_MONITORING.md | 4 +- .../monitoring/canonical_guardrails_test.go | 9 +++- internal/monitoring/host_agent_temps.go | 32 +++++++++++- internal/monitoring/host_agent_temps_test.go | 49 ++++++++++++++++--- .../monitor_polling_node_helpers.go | 6 +-- 8 files changed, 101 insertions(+), 30 deletions(-) diff --git a/docs/FAQ.md b/docs/FAQ.md index 60f9396c2..579b6e99f 100644 --- a/docs/FAQ.md +++ b/docs/FAQ.md @@ -65,7 +65,7 @@ Recommended: install the unified agent on your Proxmox hosts with Proxmox integr 1. Install `lm-sensors` on the host (`apt install lm-sensors && sensors-detect`) 2. Install `pulse-agent` with `--enable-proxmox` -If you do not run the agent, Pulse can collect temperatures over SSH. See [Temperature Monitoring](TEMPERATURE_MONITORING.md). +If you do not run the agent, Pulse can collect temperatures over SSH. When the agent is reporting usable temperatures, Pulse uses the agent path and does not also require SSH for that host. See [Temperature Monitoring](TEMPERATURE_MONITORING.md). --- diff --git a/docs/TEMPERATURE_MONITORING.md b/docs/TEMPERATURE_MONITORING.md index 68b45e191..92693a4b4 100644 --- a/docs/TEMPERATURE_MONITORING.md +++ b/docs/TEMPERATURE_MONITORING.md @@ -19,16 +19,17 @@ curl -fsSL http://:7655/install.sh | \ Notes: - Install `lm-sensors` on each host (`apt install lm-sensors && sensors-detect --auto`). - Temperatures appear automatically once the agent reports. +- When a Proxmox host has recent usable agent temperature data, Pulse treats the agent as the source of truth and does not also try SSH temperature collection for that host. ## SSH-Based Collection (Fallback) -Pulse can also collect temperatures by SSHing into each host and running `sensors -j`, with a fallback to `/sys/class/thermal/thermal_zone0/temp` when available (for example, on Raspberry Pi). +Pulse can also collect temperatures by SSHing into each host that does not have usable agent temperature data. The SSH path runs the Pulse sensor wrapper when present, falls back to `sensors -j`, and can fall back again to `/sys/class/thermal/thermal_zone0/temp` when available (for example, on Raspberry Pi). ### Requirements - SSH connectivity from the Pulse server to each host - `lm-sensors` installed and `sensors -j` returning JSON on the host -- A restricted SSH key entry that only allows `sensors -j` +- A restricted SSH key entry that only allows the Pulse sensor wrapper ### Setup @@ -39,10 +40,10 @@ Pulse can also collect temperatures by SSHing into each host and running `sensor - Add a restricted SSH key entry for temperature collection - Install `lm-sensors` (optional) -The SSH entry added to `authorized_keys` is restricted to `sensors -j`, for example: +The SSH entry added to `authorized_keys` is restricted to the Pulse sensor wrapper, for example: ```text -command="sensors -j",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty # pulse-sensors +command="/usr/local/sbin/pulse-sensors",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty # pulse-sensors ``` If you use a non-standard SSH port, set `SSH_PORT` (system-wide) or configure it in **Settings -> System**. @@ -73,7 +74,8 @@ ssh -i /path/to/key root@node "cat /sys/class/thermal/thermal_zone0/temp" - If `sensors -j` returns empty output, run `sensors-detect --auto` and retry. - If temperatures show as unavailable, confirm the host actually exposes sensor data. -- Ensure the SSH key entry is present and restricted to `sensors -j`. +- If the unified agent is already reporting temperatures for a Proxmox host, SSH collection is not required for that host. +- Ensure the SSH key entry is present and restricted to `/usr/local/sbin/pulse-sensors`. ## Legacy Cleanup (If Upgrading) diff --git a/docs/release-control/v6/internal/subsystems/monitoring.md b/docs/release-control/v6/internal/subsystems/monitoring.md index e0aa7f664..88f1c48fa 100644 --- a/docs/release-control/v6/internal/subsystems/monitoring.md +++ b/docs/release-control/v6/internal/subsystems/monitoring.md @@ -1067,15 +1067,14 @@ into the canonical physical-disk model without overwriting provider truth. The Proxmox polling runtime in `internal/monitoring/monitor_pve.go` must evaluate disk alerts only after that merged disk view exists, so controller-backed disks do not lose health and endurance coverage between collection and alerting. -That same host-agent temperature boundary must not suppress SSH SMART disk -collection just because the agent already reported CPU package or NVMe -temperatures. `internal/monitoring/monitor_polling_node_helpers.go` may skip -SSH only once the host-agent temperature payload already has usable SMART disk -temperatures rather than identity-only or zero-temperature SMART rows. Those -identity-only host-agent rows must still allow wrapper or proxy SMART -augmentation, and proxy SMART temperatures may replace them so nodes keep their -disk-temperature and SMART augmentation when the host agent is present but lacks -usable SMART temperature support. +That same host-agent temperature boundary must prefer a recent linked host-agent +payload over legacy SSH collection once the agent provides any usable CPU, NVMe, +GPU, or SMART temperature reading. `internal/monitoring/monitor_polling_node_helpers.go` +may invoke SSH only when no linked, recent, available host-agent temperature +exists or the agent payload has no usable positive reading. Identity-only or +zero-temperature SMART rows do not count as usable by themselves, but the +runtime must not keep probing legacy SSH solely to augment an otherwise healthy +agent temperature payload with SMART data. Legacy SSH temperature collection must also use the Pulse sensor-wrapper contract before falling back to raw lm-sensors output. `internal/monitoring/temperature.go` must request `/usr/local/sbin/pulse-sensors` when it exists, parse the wrapper diff --git a/docs/security/TEMPERATURE_MONITORING.md b/docs/security/TEMPERATURE_MONITORING.md index f2a5fdcf3..aa96b43f7 100644 --- a/docs/security/TEMPERATURE_MONITORING.md +++ b/docs/security/TEMPERATURE_MONITORING.md @@ -15,12 +15,12 @@ See [docs/TEMPERATURE_MONITORING.md](../TEMPERATURE_MONITORING.md) for setup. ## SSH-Based Collection -SSH-based temperature monitoring uses a restricted key entry that only allows `sensors -j` to run. This limits the blast radius if a key leaks. +SSH-based temperature monitoring uses a restricted key entry that only allows the Pulse sensor wrapper to run. This limits the blast radius if a key leaks. Recommended restrictions: ```text -command="sensors -j",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty # pulse-sensors +command="/usr/local/sbin/pulse-sensors",no-port-forwarding,no-X11-forwarding,no-agent-forwarding,no-pty # pulse-sensors ``` Additional notes: diff --git a/internal/monitoring/canonical_guardrails_test.go b/internal/monitoring/canonical_guardrails_test.go index f9f820883..a70b27227 100644 --- a/internal/monitoring/canonical_guardrails_test.go +++ b/internal/monitoring/canonical_guardrails_test.go @@ -1107,11 +1107,16 @@ func TestStoragePollingKeepsSharedStorageClusterStatusCanonical(t *testing.T) { } } -func TestMonitoringTemperatureFallbackUsesSMARTAwareSSHSkipRule(t *testing.T) { +func TestMonitoringTemperatureFallbackPrefersRecentAgentTelemetry(t *testing.T) { requiredSnippets := map[string][]string{ "host_agent_temps.go": { "func shouldSkipTemperatureSSHCollection(hostAgentTemp *models.Temperature) bool {", - "return hasUsableSMARTTemperature(hostAgentTemp)", + "return hasUsableTemperatureReading(hostAgentTemp)", + "func hasUsableTemperatureReading(temp *models.Temperature) bool {", + "temp.CPUPackage > 0 || temp.CPUMax > 0", + "core.Temp > 0", + "device.Temp > 0", + "gpu.Edge > 0 || gpu.Junction > 0 || gpu.Mem > 0", "func hasUsableSMARTTemperature(temp *models.Temperature) bool {", "disk.Temperature > 0 && !disk.StandbySkipped", }, diff --git a/internal/monitoring/host_agent_temps.go b/internal/monitoring/host_agent_temps.go index 4604d9c82..fd35a6787 100644 --- a/internal/monitoring/host_agent_temps.go +++ b/internal/monitoring/host_agent_temps.go @@ -31,7 +31,37 @@ func shouldSkipTemperatureSSHCollection(hostAgentTemp *models.Temperature) bool return false } - return hasUsableSMARTTemperature(hostAgentTemp) + return hasUsableTemperatureReading(hostAgentTemp) +} + +func hasUsableTemperatureReading(temp *models.Temperature) bool { + if temp == nil { + return false + } + + if temp.CPUPackage > 0 || temp.CPUMax > 0 { + return true + } + + for _, core := range temp.Cores { + if core.Temp > 0 { + return true + } + } + + for _, device := range temp.NVMe { + if device.Temp > 0 { + return true + } + } + + for _, gpu := range temp.GPU { + if gpu.Edge > 0 || gpu.Junction > 0 || gpu.Mem > 0 { + return true + } + } + + return hasUsableSMARTTemperature(temp) } func hasUsableSMARTTemperature(temp *models.Temperature) bool { diff --git a/internal/monitoring/host_agent_temps_test.go b/internal/monitoring/host_agent_temps_test.go index 9c8f309cd..b03dd6149 100644 --- a/internal/monitoring/host_agent_temps_test.go +++ b/internal/monitoring/host_agent_temps_test.go @@ -159,7 +159,7 @@ func TestShouldSkipTemperatureSSHCollection(t *testing.T) { } }) - t.Run("cpu only host agent temp does not skip", func(t *testing.T) { + t.Run("host agent temp without recent timestamp does not skip", func(t *testing.T) { host := &models.Temperature{ Available: true, HasCPU: true, @@ -170,15 +170,51 @@ func TestShouldSkipTemperatureSSHCollection(t *testing.T) { } }) - t.Run("recent cpu only host agent temp does not skip", func(t *testing.T) { + t.Run("recent cpu package host agent temp skips", func(t *testing.T) { host := &models.Temperature{ Available: true, HasCPU: true, CPUPackage: 55, LastUpdate: time.Now(), } - if shouldSkipTemperatureSSHCollection(host) { - t.Fatal("expected recent CPU-only host agent temp to allow SSH SMART augmentation") + if !shouldSkipTemperatureSSHCollection(host) { + t.Fatal("expected recent CPU host agent temp to skip legacy SSH collection") + } + }) + + t.Run("recent cpu core host agent temp skips", func(t *testing.T) { + host := &models.Temperature{ + Available: true, + HasCPU: true, + Cores: []models.CoreTemp{{Core: 0, Temp: 51}}, + LastUpdate: time.Now(), + } + if !shouldSkipTemperatureSSHCollection(host) { + t.Fatal("expected recent CPU core host agent temp to skip legacy SSH collection") + } + }) + + t.Run("recent nvme host agent temp skips", func(t *testing.T) { + host := &models.Temperature{ + Available: true, + HasNVMe: true, + NVMe: []models.NVMeTemp{{Device: "nvme0", Temp: 42}}, + LastUpdate: time.Now(), + } + if !shouldSkipTemperatureSSHCollection(host) { + t.Fatal("expected recent NVMe host agent temp to skip legacy SSH collection") + } + }) + + t.Run("recent gpu host agent temp skips", func(t *testing.T) { + host := &models.Temperature{ + Available: true, + HasGPU: true, + GPU: []models.GPUTemp{{Device: "gpu0", Edge: 62}}, + LastUpdate: time.Now(), + } + if !shouldSkipTemperatureSSHCollection(host) { + t.Fatal("expected recent GPU host agent temp to skip legacy SSH collection") } }) @@ -219,16 +255,15 @@ func TestShouldSkipTemperatureSSHCollection(t *testing.T) { } }) - t.Run("host agent smart inventory without temperatures does not skip", func(t *testing.T) { + t.Run("host agent smart inventory without any temperature does not skip", func(t *testing.T) { host := &models.Temperature{ Available: true, - HasCPU: true, HasSMART: true, SMART: []models.DiskTemp{{Device: "/dev/sda", Temperature: 0}}, LastUpdate: time.Now(), } if shouldSkipTemperatureSSHCollection(host) { - t.Fatal("expected identity-only SMART rows to allow SSH SMART augmentation") + t.Fatal("expected identity-only SMART rows without a temperature to allow legacy SSH fallback") } }) } diff --git a/internal/monitoring/monitor_polling_node_helpers.go b/internal/monitoring/monitor_polling_node_helpers.go index ac94f715e..2b01497ee 100644 --- a/internal/monitoring/monitor_polling_node_helpers.go +++ b/internal/monitoring/monitor_polling_node_helpers.go @@ -159,9 +159,9 @@ func (m *Monitor) collectNodeTemperatureData( sshHost = node.Node } - // Skip SSH only when the host agent already has SMART data too. - // If the host agent only has CPU/NVMe readings, SSH can still - // augment the node with SMART disk temperatures. + // A recent host agent payload is authoritative for this node. + // Legacy SSH remains only as a fallback when no usable local + // temperature reading has arrived from the linked agent. skipSSHCollection := shouldSkipTemperatureSSHCollection(hostAgentTemp) if !skipSSHCollection {