diff --git a/internal/hostagent/agent_sensors_test.go b/internal/hostagent/agent_sensors_test.go index 8f3fa0fc1..a4b9919ce 100644 --- a/internal/hostagent/agent_sensors_test.go +++ b/internal/hostagent/agent_sensors_test.go @@ -57,6 +57,21 @@ func TestAgent_collectTemperatures_MapsKeys(t *testing.T) { } } +func TestAgent_collectTemperatures_ArmadaThermalFeedsMachineCPU(t *testing.T) { + mc := &mockCollector{ + goos: "linux", + sensorsLocalFn: func(context.Context) (string, error) { + return `{"armada_thermal-virtual-0":{"temp1":{"temp1_input":57}}}`, nil + }, + sensorsParseFn: sensors.Parse, + } + a := &Agent{logger: zerolog.Nop(), collector: mc} + got := a.collectTemperatures(context.Background()) + if got.TemperatureCelsius["cpu_package"] != 57 { + t.Fatalf("CPU package reading = %v, want 57", got.TemperatureCelsius) + } +} + func TestAgent_collectTemperatures_MergesNVIDIASMITemperatures(t *testing.T) { mc := &mockCollector{ goos: "linux", diff --git a/internal/sensors/collector.go b/internal/sensors/collector.go index 3471b85fb..b4f502dba 100644 --- a/internal/sensors/collector.go +++ b/internal/sensors/collector.go @@ -5,7 +5,9 @@ import ( "errors" "fmt" "io" + "os" "os/exec" + "path/filepath" "strconv" "strings" "time" @@ -20,77 +22,124 @@ const ( var ( errCommandOutputTooLarge = errors.New("command output exceeds size limit") - rpiThermalZonePath = "/sys/class/thermal/thermal_zone0/temp" + thermalZoneRoot = "/sys/class/thermal" + hwmonRoot = "/sys/class/hwmon" ) -var rpiThermalZoneTempPath = "/sys/class/thermal/thermal_zone0/temp" - -// CollectLocal reads sensor data from the local machine using lm-sensors. -// Returns the raw JSON output from `sensors -j` or an error if sensors is not available. +// CollectLocal reads lm-sensors JSON, or a recognised CPU thermal sysfs source +// when lm-sensors is unavailable or returns no data. A valid lm-sensors result +// is kept intact so drive, GPU and fan readings are not lost. func CollectLocal(ctx context.Context) (string, error) { ctx = normalizeCollectionContext(ctx) - // Check if sensors command exists sensorsPath, err := exec.LookPath("sensors") - if err != nil { - return "", fmt.Errorf("lm-sensors not installed: %w", err) - } + if err == nil { + cmdCtx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() - // Create context with timeout - cmdCtx, cancel := context.WithTimeout(ctx, 5*time.Second) - defer cancel() - - // Run sensors -j command with bounded output capture. - // sensors exits non-zero when optional subfeatures fail, so non-empty output is still accepted. - cmd := exec.CommandContext(cmdCtx, sensorsPath, "-j") - cmd.Stderr = io.Discard - output, err := runCommandOutputLimited(cmd, maxSensorsOutputSizeBytes) - if err != nil && errors.Is(err, errCommandOutputTooLarge) { - return "", fmt.Errorf("failed to execute sensors: %w", err) - } - - outputStr := strings.TrimSpace(string(output)) - if err != nil && outputStr == "" { - return "", fmt.Errorf("failed to execute sensors: %w", err) - } - - if outputStr == "" || outputStr == "{}" { - log.Debug(). - Str("component", "sensors_collector"). - Str("action", "collect_local_empty_output"). - Msg("lm-sensors returned empty output, attempting Raspberry Pi thermal fallback") - - // Try Raspberry Pi temperature method as fallback - cmd = exec.CommandContext(cmdCtx, "cat", rpiThermalZonePath) - rpiOutput, rpiErr := cmd.Output() - if rpiErr == nil { - rpiTemp := strings.TrimSpace(string(rpiOutput)) - if rpiTemp != "" { - parsed, parseErr := strconv.ParseFloat(rpiTemp, 64) - if parseErr != nil { - return "", fmt.Errorf("invalid thermal value %q: %w", rpiTemp, parseErr) - } - // Linux thermal_zone values are commonly millidegrees (e.g. 42000). - // Convert only when magnitude indicates millidegrees to keep degree inputs intact. - if parsed >= 1000 || parsed <= -1000 { - parsed = parsed / 1000.0 - } - rpiTemp = strconv.FormatFloat(parsed, 'f', 3, 64) - // Convert to pseudo-sensors format for compatibility - return fmt.Sprintf(`{"cpu_thermal-virtual-0":{"temp1":{"temp1_input":%s}}}`, rpiTemp), nil - } - } else { - log.Debug(). - Str("component", "sensors_collector"). - Str("action", "collect_local_rpi_fallback_failed"). - Str("thermal_path", "/sys/class/thermal/thermal_zone0/temp"). - Err(rpiErr). - Msg("Raspberry Pi thermal fallback failed") + // sensors can exit non-zero when optional subfeatures fail, so accept + // non-empty output even then. Never fall back after an output-limit hit. + cmd := exec.CommandContext(cmdCtx, sensorsPath, "-j") + cmd.Stderr = io.Discard + output, commandErr := runCommandOutputLimited(cmd, maxSensorsOutputSizeBytes) + if errors.Is(commandErr, errCommandOutputTooLarge) { + return "", fmt.Errorf("failed to execute sensors: %w", commandErr) } - return "", fmt.Errorf("sensors returned empty output") + outputStr := strings.TrimSpace(string(output)) + if outputStr != "" && outputStr != "{}" { + return outputStr, nil + } + if cmdCtx.Err() != nil { + return "", fmt.Errorf("failed to execute sensors: %w", cmdCtx.Err()) + } + log.Debug().Str("component", "sensors_collector"). + Str("action", "collect_local_empty_output"). + Msg("lm-sensors returned no data; trying recognised CPU thermal sysfs sources") } - return outputStr, nil + output, fallbackErr := collectSysfsCPUTemperature(ctx) + if fallbackErr == nil { + return output, nil + } + if err != nil { + return "", fmt.Errorf("lm-sensors unavailable and CPU thermal fallback failed: %w", fallbackErr) + } + return "", fmt.Errorf("sensors returned empty output and CPU thermal fallback failed: %w", fallbackErr) +} + +// collectSysfsCPUTemperature deliberately accepts only identified CPU/SoC +// sensors. thermal_zone0 is not necessarily a CPU on every Linux machine. +func collectSysfsCPUTemperature(ctx context.Context) (string, error) { + if err := ctx.Err(); err != nil { + return "", err + } + var lastErr error + for _, source := range []struct { + root, prefix, label, value string + }{ + {thermalZoneRoot, "thermal_zone", "type", "temp"}, + {hwmonRoot, "hwmon", "name", "temp1_input"}, + } { + entries, err := os.ReadDir(source.root) + if err != nil { + lastErr = err + continue + } + for _, entry := range entries { + if err := ctx.Err(); err != nil { + return "", err + } + if !strings.HasPrefix(entry.Name(), source.prefix) { + continue + } + dir := filepath.Join(source.root, entry.Name()) + name, err := readBoundedThermalFile(filepath.Join(dir, source.label)) + if err != nil || !isCPUSysfsThermalName(name) { + continue + } + raw, err := readBoundedThermalFile(filepath.Join(dir, source.value)) + if err != nil { + lastErr = err + continue + } + millidegrees, err := strconv.ParseInt(raw, 10, 64) + if err != nil || millidegrees < 1000 || millidegrees >= 150000 { + lastErr = fmt.Errorf("invalid CPU thermal millidegree value from %s", dir) + continue + } + celsius := float64(millidegrees) / 1000 + return fmt.Sprintf(`{"cpu_thermal-virtual-0":{"temp1":{"temp1_input":%.3f}}}`, celsius), nil + } + } + if lastErr != nil { + return "", fmt.Errorf("no valid CPU thermal sysfs reading: %w", lastErr) + } + return "", errors.New("no recognised CPU thermal sysfs source") +} + +func isCPUSysfsThermalName(name string) bool { + switch strings.ToLower(strings.TrimSpace(name)) { + case "armada_thermal", "cpu_thermal", "cpu-thermal", "soc_thermal", "soc-thermal", "x86_pkg_temp", "rpitemp": + return true + default: + return false + } +} + +func readBoundedThermalFile(path string) (string, error) { + f, err := os.Open(path) + if err != nil { + return "", err + } + defer f.Close() + contents, err := io.ReadAll(io.LimitReader(f, maxThermalFileReadBytes+1)) + if err != nil { + return "", err + } + if len(contents) > maxThermalFileReadBytes { + return "", fmt.Errorf("thermal sysfs value exceeds %d bytes", maxThermalFileReadBytes) + } + return strings.TrimSpace(string(contents)), nil } func runCommandOutputLimited(cmd *exec.Cmd, maxBytes int) ([]byte, error) { diff --git a/internal/sensors/collector_additional_test.go b/internal/sensors/collector_additional_test.go index d77ec07cb..b7a8dc4ed 100644 --- a/internal/sensors/collector_additional_test.go +++ b/internal/sensors/collector_additional_test.go @@ -3,6 +3,7 @@ package sensors import ( "context" "os" + "path/filepath" "strings" "testing" ) @@ -12,6 +13,7 @@ func TestCollectLocal_EmptyFallbackReturnsError(t *testing.T) { writeScript(t, dir, "sensors", "#!/bin/sh\necho '{}'\n") writeScript(t, dir, "cat", "#!/bin/sh\necho ''\n") t.Setenv("PATH", dir+":"+os.Getenv("PATH")) + useTestSysfsRoots(t) _, err := CollectLocal(context.Background()) if err == nil { @@ -22,6 +24,91 @@ func TestCollectLocal_EmptyFallbackReturnsError(t *testing.T) { } } +func TestCollectLocal_MissingSensorsUsesIdentifiedArmadaThermalZone(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + thermal, _ := useTestSysfsRoots(t) + writeSysfsSensor(t, thermal, "thermal_zone0", "type", "gpu-thermal", "70000") + writeSysfsSensor(t, thermal, "thermal_zone1", "type", "armada_thermal", "57000") + + out, err := CollectLocal(context.Background()) + if err != nil { + t.Fatal(err) + } + parsed, err := Parse(out) + if err != nil { + t.Fatal(err) + } + if !parsed.Available || parsed.CPUPackage != 57 || parsed.CPUMax != 57 { + t.Fatalf("armada sysfs reading not mapped to CPU: %+v", parsed) + } +} + +func TestCollectLocal_SysfsRejectsUnidentifiedAndBadReadings(t *testing.T) { + for _, tc := range []struct { + name, chip, value string + }{ + {"unidentified", "gpu-thermal", "57000"}, + {"empty", "armada_thermal", ""}, + {"degrees-not-millidegrees", "armada_thermal", "57"}, + {"negative", "armada_thermal", "-57000"}, + {"too-hot", "armada_thermal", "150000"}, + {"malformed", "armada_thermal", "57000C"}, + {"non-finite", "armada_thermal", "NaN"}, + {"oversized", "armada_thermal", strings.Repeat("5", maxThermalFileReadBytes+1)}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + thermal, _ := useTestSysfsRoots(t) + writeSysfsSensor(t, thermal, "thermal_zone0", "type", tc.chip, tc.value) + if out, err := CollectLocal(context.Background()); err == nil { + t.Fatalf("unexpected CPU temperature %s for %s", out, tc.name) + } + }) + } +} + +func TestCollectLocal_SysfsContinuesPastBadSource(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + thermal, hwmon := useTestSysfsRoots(t) + writeSysfsSensor(t, thermal, "thermal_zone0", "type", "armada_thermal", "invalid") + writeSysfsSensor(t, hwmon, "hwmon3", "name", "armada_thermal", "57500") + + out, err := CollectLocal(context.Background()) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, `57.500`) { + t.Fatalf("expected hwmon fallback, got %s", out) + } +} + +func TestCollectLocal_SensorsJSONPrecedesSysfs(t *testing.T) { + dir := t.TempDir() + writeScript(t, dir, "sensors", "#!/bin/sh\necho '{\"nvme-pci-0100\":{\"Composite\":{\"temp1_input\":39}}}'\n") + t.Setenv("PATH", dir+":"+os.Getenv("PATH")) + thermal, _ := useTestSysfsRoots(t) + writeSysfsSensor(t, thermal, "thermal_zone0", "type", "armada_thermal", "57000") + out, err := CollectLocal(context.Background()) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(out, "nvme-pci-0100") || strings.Contains(out, "cpu_thermal") { + t.Fatalf("lm-sensors result was replaced: %s", out) + } +} + +func TestCollectLocal_SysfsNameReadBounded(t *testing.T) { + t.Setenv("PATH", t.TempDir()) + thermal, _ := useTestSysfsRoots(t) + writeSysfsSensor(t, thermal, "thermal_zone0", "type", "armada_thermal", "57000") + if err := os.WriteFile(filepath.Join(thermal, "thermal_zone0", "type"), []byte(strings.Repeat("a", maxThermalFileReadBytes+1)), 0600); err != nil { + t.Fatal(err) + } + if out, err := CollectLocal(context.Background()); err == nil { + t.Fatalf("oversized type unexpectedly accepted: %s", out) + } +} + func TestCollectLocal_ContextCanceled(t *testing.T) { dir := t.TempDir() writeScript(t, dir, "sensors", "#!/bin/sh\necho '{\"chip\":{\"temp\":{\"temp1_input\":42}}}'\n") diff --git a/internal/sensors/collector_test.go b/internal/sensors/collector_test.go index 7dc6b157e..f332ff88a 100644 --- a/internal/sensors/collector_test.go +++ b/internal/sensors/collector_test.go @@ -16,9 +16,42 @@ func writeScript(t *testing.T, dir, name, content string) { } } +func useTestSysfsRoots(t *testing.T) (string, string) { + t.Helper() + root := t.TempDir() + thermal := filepath.Join(root, "thermal") + hwmon := filepath.Join(root, "hwmon") + for _, dir := range []string{thermal, hwmon} { + if err := os.Mkdir(dir, 0700); err != nil { + t.Fatal(err) + } + } + previousThermal, previousHwmon := thermalZoneRoot, hwmonRoot + thermalZoneRoot, hwmonRoot = thermal, hwmon + t.Cleanup(func() { thermalZoneRoot, hwmonRoot = previousThermal, previousHwmon }) + return thermal, hwmon +} + +func writeSysfsSensor(t *testing.T, root, dir, label, name, value string) { + t.Helper() + path := filepath.Join(root, dir) + if err := os.Mkdir(path, 0700); err != nil { + t.Fatal(err) + } + for file, content := range map[string]string{label: name, "temp": value} { + if label == "name" && file == "temp" { + file = "temp1_input" + } + if err := os.WriteFile(filepath.Join(path, file), []byte(content), 0600); err != nil { + t.Fatal(err) + } + } +} + func TestCollectLocalMissingSensors(t *testing.T) { dir := t.TempDir() t.Setenv("PATH", dir) + useTestSysfsRoots(t) if _, err := CollectLocal(context.Background()); err == nil { t.Fatal("expected error when sensors missing") @@ -57,15 +90,8 @@ func TestCollectLocalFallbackToPiTemp(t *testing.T) { dir := t.TempDir() writeScript(t, dir, "sensors", "#!/bin/sh\necho '{}'\n") t.Setenv("PATH", dir+":"+os.Getenv("PATH")) - thermalPath := filepath.Join(dir, "thermal_zone0_temp") - if err := os.WriteFile(thermalPath, []byte("42000\n"), 0600); err != nil { - t.Fatalf("write thermal file: %v", err) - } - originalThermalPath := rpiThermalZonePath - rpiThermalZonePath = thermalPath - t.Cleanup(func() { - rpiThermalZonePath = originalThermalPath - }) + thermal, _ := useTestSysfsRoots(t) + writeSysfsSensor(t, thermal, "thermal_zone0", "type", "cpu-thermal\n", "42000\n") out, err := CollectLocal(context.Background()) if err != nil { @@ -77,21 +103,12 @@ func TestCollectLocalFallbackToPiTemp(t *testing.T) { } } -func TestCollectLocalFallbackKeepsDegreeInput(t *testing.T) { +func TestCollectLocalFallbackToArmadaHwmon(t *testing.T) { dir := t.TempDir() writeScript(t, dir, "sensors", "#!/bin/sh\necho '{}'\n") - writeScript(t, dir, "cat", "#!/bin/sh\necho '42'\n") t.Setenv("PATH", dir+":"+os.Getenv("PATH")) - - thermalPath := filepath.Join(t.TempDir(), "thermal_zone0_temp") - if err := os.WriteFile(thermalPath, []byte("42000"), 0644); err != nil { - t.Fatalf("write thermal file: %v", err) - } - originalThermalPath := rpiThermalZoneTempPath - rpiThermalZoneTempPath = thermalPath - t.Cleanup(func() { - rpiThermalZoneTempPath = originalThermalPath - }) + _, hwmon := useTestSysfsRoots(t) + writeSysfsSensor(t, hwmon, "hwmon0", "name", "armada_thermal\n", "42000\n") out, err := CollectLocal(context.Background()) if err != nil { @@ -136,15 +153,8 @@ func TestCollectLocalFallbackRejectsInvalidThermalValue(t *testing.T) { dir := t.TempDir() writeScript(t, dir, "sensors", "#!/bin/sh\necho '{}'\n") t.Setenv("PATH", dir+":"+os.Getenv("PATH")) - thermalPath := filepath.Join(dir, "thermal_zone0_temp") - if err := os.WriteFile(thermalPath, []byte(`42"},"bad":{"temp1_input":1}`), 0600); err != nil { - t.Fatalf("write thermal file: %v", err) - } - originalThermalPath := rpiThermalZonePath - rpiThermalZonePath = thermalPath - t.Cleanup(func() { - rpiThermalZonePath = originalThermalPath - }) + thermal, _ := useTestSysfsRoots(t) + writeSysfsSensor(t, thermal, "thermal_zone0", "type", "cpu-thermal", `42"},"bad":{"temp1_input":1}`) if _, err := CollectLocal(context.Background()); err == nil { t.Fatal("expected error for invalid fallback thermal value") diff --git a/internal/sensors/parser.go b/internal/sensors/parser.go index 0acfa12db..d5f521cb5 100644 --- a/internal/sensors/parser.go +++ b/internal/sensors/parser.go @@ -126,7 +126,7 @@ func isCPUChip(chipLower string) bool { "it87", "nct6687", "nct6775", "nct6776", "nct6779", "nct6791", "nct6792", "nct6793", "nct6795", "nct6796", "nct6797", "nct6798", "w83627", "f71882", - "cpu_thermal", "rp1_adc", "rpitemp", + "cpu_thermal", "rp1_adc", "rpitemp", "armada_thermal", } for _, chip := range cpuChips { @@ -166,7 +166,7 @@ func parseCPUTemps(chipMap map[string]interface{}, data *TemperatureData) { // Capture generic temp1 for chips like cpu_thermal (RPi, ARM SoCs) // that don't have labeled sensors if sensorNameLower == "temp1" { - if tempVal := extractTempInput(sensorMap); !math.IsNaN(tempVal) && tempVal > 0 { + if tempVal := extractTempInput(sensorMap); !math.IsNaN(tempVal) && tempVal > 0 && tempVal < 150 { genericTemp = tempVal } } diff --git a/internal/sensors/parser_test.go b/internal/sensors/parser_test.go index 6803ae966..481ca485d 100644 --- a/internal/sensors/parser_test.go +++ b/internal/sensors/parser_test.go @@ -360,6 +360,7 @@ func TestIsCPUChip(t *testing.T) { {"cpu_thermal-virtual-0", true}, {"acpitz-acpi-0", true}, {"rp1_adc-isa-0000", true}, // Raspberry Pi RP1 ADC + {"armada_thermal-virtual-0", true}, {"nvme-pci-0100", false}, {"amdgpu-pci-0300", false}, {"unknown-chip", false}, @@ -375,6 +376,27 @@ func TestIsCPUChip(t *testing.T) { } } +func TestParseArmadaThermalCPUTemperatureAndOtherSensors(t *testing.T) { + input := `{"armada_thermal-virtual-0":{"temp1":{"temp1_input":57}},"nvme-pci-0100":{"Composite":{"temp1_input":39}}}` + data, err := Parse(input) + if err != nil { + t.Fatal(err) + } + if !data.Available || data.CPUPackage != 57 || data.CPUMax != 57 || data.NVMe["nvme0"] != 39 { + t.Fatalf("unexpected ARM and NVMe readings: %+v", data) + } + + for _, invalid := range []string{`null`, `"unavailable"`, `150`, `1000000`, `-3`} { + data, err := Parse(`{"armada_thermal-virtual-0":{"temp1":{"temp1_input":` + invalid + `}}}`) + if err != nil { + t.Fatal(err) + } + if data.CPUPackage != 0 { + t.Fatalf("invalid armada reading %s became CPU temperature %v", invalid, data.CPUPackage) + } + } +} + func TestExtractTempInput(t *testing.T) { tests := []struct { name string