diff --git a/cmd/controller/main.go b/cmd/controller/main.go index b0741bb..1c75d02 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -1,12 +1,15 @@ package main import ( + "errors" "flag" + "fmt" "log" "os" "os/signal" "strconv" "strings" + "sync" "syscall" "github.com/sethpjohnson/only-fan-controller/internal/api" @@ -86,29 +89,77 @@ func applyEnvOverrides(cfg *config.Config) { } } -// FanControllerInterface allows both real and mock controllers -type FanControllerInterface interface { - Run() - Stop() - RestoreAutoMode() error - GetStatus() *controller.Status - AddHint(hint *controller.WorkloadHint) - RemoveHint(source string) - SetOverride(speed int, duration interface{}, reason string) - ClearOverride() +func main() { + os.Exit(run()) } -func main() { +// runControlLoop runs the fan control loop and guarantees that a panic restores +// BMC automatic fan control before the process gives up. Any panic is reported +// on errCh so main can shut down through the same path that restores auto mode. +// The only ways to exit without restoring auto mode are SIGKILL / power loss. +func runControlLoop(loop func(), restore func() error, errCh chan<- error) { + defer func() { + if r := recover(); r != nil { + log.Printf("PANIC in control loop: %v", r) + if err := restore(); err != nil { + log.Printf("CRITICAL: failed to restore auto fan mode after panic: %v", err) + } + errCh <- fmt.Errorf("control loop panic: %v", r) + } + }() + loop() +} + +// restoreOnce wraps a restore function so it runs at most once across all +// callers. On a control-loop panic the recover handler restores auto mode AND +// the shutdown path calls restore again; the guard makes the second call a +// no-op so the BMC is toggled exactly once. +func restoreOnce(restore func() error) func() error { + var once sync.Once + var firstErr error + return func() error { + once.Do(func() { firstErr = restore() }) + return firstErr + } +} + +// resolveConfig loads the config, distinguishing a genuinely absent file (safe +// to fall back to defaults + env overrides) from a present-but-invalid file +// (parse or safety-validation failure). For the latter we REFUSE to start rather +// than silently fall back to Default(), which would discard the operator's real +// iDRAC host/credentials and come up pointed at `-H "" -U root -P ""` — making +// every IPMI call fail, including RestoreAutoMode itself. A process that never +// starts never enables manual mode, so the BMC keeps automatic control: that is +// the fail-safe outcome. +func resolveConfig(path string) (*config.Config, error) { + cfg, err := config.Load(path) + if err == nil { + return cfg, nil + } + if errors.Is(err, os.ErrNotExist) { + log.Printf("Config file %s not found; using default configuration", path) + return config.Default(), nil + } + // File exists but is unreadable, unparseable, or fails safety validation. + return nil, err +} + +// run holds the real program body so that deferred cleanup (e.g. store.Close) +// always runs, even on an abnormal exit. main() only translates the returned +// status into a process exit code. +func run() int { configPath := flag.String("config", "/etc/only-fan-controller/config.yaml", "Path to configuration file") demoMode := flag.Bool("demo", false, "Run in demo mode with simulated temperatures (no actual fan control)") flag.Parse() // Load configuration - cfg, err := config.Load(*configPath) + cfg, err := resolveConfig(*configPath) if err != nil { - log.Printf("Warning: Could not load config from %s: %v", *configPath, err) - log.Println("Using default configuration") - cfg = config.Default() + log.Printf("FATAL: refusing to start with an invalid config %s: %v", *configPath, err) + log.Println("The config file exists but is invalid. NOT falling back to defaults, because that would") + log.Println("discard your iDRAC credentials and leave the controller unable to talk to the BMC.") + log.Println("The BMC keeps automatic fan control until you fix the config and restart.") + return 1 } // Override with environment variables @@ -128,19 +179,25 @@ func main() { // Initialize storage for history store, err := storage.New(cfg.Storage.Path) if err != nil { - log.Fatalf("Failed to initialize storage: %v", err) + log.Printf("Failed to initialize storage: %v", err) + return 1 } defer store.Close() + // errCh carries any fatal error (API server failure, control-loop panic) + // back to the shutdown path so cleanup + auto-mode restore always run. + errCh := make(chan error, 1) + var fanCtrl *controller.FanController + var restore func() error if *demoMode { - // Use mock controller + // Use mock controller. Its RestoreAutoMode is log-only, so it is safe to + // call on every exit path without touching hardware. mockCtrl := controller.NewMockFanController(cfg, store) fanCtrl = mockCtrl.FanController - - // Start mock control loop - go mockCtrl.Run() + restore = restoreOnce(mockCtrl.RestoreAutoMode) + go runControlLoop(mockCtrl.Run, restore, errCh) } else { // Initialize real monitors cpuMon := monitor.NewCPUMonitor(cfg) @@ -148,38 +205,45 @@ func main() { // Initialize real fan controller fanCtrl = controller.NewFanController(cfg, cpuMon, gpuMon, store) - - // Start the control loop - go fanCtrl.Run() + restore = restoreOnce(fanCtrl.RestoreAutoMode) + go runControlLoop(fanCtrl.Run, restore, errCh) } // Initialize API server apiServer := api.NewServer(cfg, fanCtrl, store) - // Start API server + // Start API server. On error, report through errCh instead of os.Exit so the + // shutdown path (which restores BMC auto mode) still runs. go func() { if err := apiServer.Run(); err != nil { - log.Fatalf("API server error: %v", err) + errCh <- fmt.Errorf("API server error: %w", err) } }() log.Printf("API server listening on %s:%d", cfg.API.Host, cfg.API.Port) log.Printf("Dashboard: http://localhost:%d/dashboard/", cfg.API.Port) - // Wait for shutdown signal + // Wait for a shutdown signal or a fatal error. sigChan := make(chan os.Signal, 1) signal.Notify(sigChan, syscall.SIGINT, syscall.SIGTERM) - <-sigChan - log.Println("Shutting down...") + exitCode := 0 + select { + case <-sigChan: + log.Println("Shutting down...") + case err := <-errCh: + log.Printf("Fatal: %v", err) + exitCode = 1 + } + fanCtrl.Stop() - // Restore Dell automatic fan control on exit (only in real mode) - if !*demoMode { - if err := fanCtrl.RestoreAutoMode(); err != nil { - log.Printf("Warning: Failed to restore auto fan mode: %v", err) - } + // Restore BMC automatic fan control on every exit path. This is the single + // choke point that keeps the machine cooled once we leave manual mode. + if err := restore(); err != nil { + log.Printf("Warning: Failed to restore auto fan mode: %v", err) } log.Println("Shutdown complete") + return exitCode } diff --git a/cmd/controller/main_test.go b/cmd/controller/main_test.go new file mode 100644 index 0000000..04eea7c --- /dev/null +++ b/cmd/controller/main_test.go @@ -0,0 +1,128 @@ +package main + +import ( + "errors" + "os" + "path/filepath" + "testing" + "time" +) + +func TestResolveConfigMissingFileFallsBackToDefaults(t *testing.T) { + missing := filepath.Join(t.TempDir(), "does-not-exist.yaml") + cfg, err := resolveConfig(missing) + if err != nil { + t.Fatalf("missing config file should fall back to defaults, got error: %v", err) + } + if cfg == nil { + t.Fatal("expected default config, got nil") + } +} + +func TestResolveConfigInvalidFileRefusesToStart(t *testing.T) { + // Valid YAML with REAL iDRAC credentials, but a safety-validation failure + // (critical_cpu_temp below the customized cpu_threshold). resolveConfig must + // NOT silently fall back to Default() (which would discard these credentials) + // — it must return an error so the process refuses to start. + path := filepath.Join(t.TempDir(), "config.yaml") + content := `idrac: + host: "192.168.1.50" + username: "operator" + password: "s3cret" +fan_control: + cpu_threshold: 90 + critical_cpu_temp: 85 +` + if err := os.WriteFile(path, []byte(content), 0o644); err != nil { + t.Fatalf("failed to write temp config: %v", err) + } + + cfg, err := resolveConfig(path) + if err == nil { + t.Fatalf("expected refusal on invalid config, got cfg=%+v", cfg) + } + if cfg != nil { + t.Fatalf("invalid config must not yield a usable (fallback) config, got %+v", cfg) + } +} + +func TestRunControlLoopRestoresAutoModeOnPanic(t *testing.T) { + restored := make(chan struct{}, 1) + errCh := make(chan error, 1) + + run := func() { panic("boom in control loop") } + restore := func() error { + restored <- struct{}{} + return nil + } + + runControlLoop(run, restore, errCh) + + select { + case <-restored: + default: + t.Fatal("RestoreAutoMode was not called when the control loop panicked") + } + + select { + case err := <-errCh: + if err == nil { + t.Fatal("expected a non-nil error on the error channel after panic") + } + case <-time.After(time.Second): + t.Fatal("no error reported on the error channel after panic") + } +} + +func TestRunControlLoopReportsRestoreFailureButStillExits(t *testing.T) { + errCh := make(chan error, 1) + run := func() { panic("boom") } + restore := func() error { return errors.New("ipmitool unreachable") } + + // Must not itself panic even if restore fails. + runControlLoop(run, restore, errCh) + + select { + case err := <-errCh: + if err == nil { + t.Fatal("expected an error after panic") + } + default: + t.Fatal("expected an error to be reported after panic") + } +} + +func TestRestoreOnceFiresUnderlyingExactlyOnce(t *testing.T) { + calls := 0 + restore := restoreOnce(func() error { + calls++ + return nil + }) + + // Simulate the panic path (recover restores) followed by the shutdown path + // (run() restores again). The underlying BMC toggle must happen once. + _ = restore() + _ = restore() + _ = restore() + + if calls != 1 { + t.Fatalf("underlying restore called %d times, want 1", calls) + } +} + +func TestRunControlLoopNormalReturnDoesNotReportError(t *testing.T) { + errCh := make(chan error, 1) + run := func() {} // returns normally + restore := func() error { + t.Fatal("restore must not be called on a normal return") + return nil + } + + runControlLoop(run, restore, errCh) + + select { + case err := <-errCh: + t.Fatalf("unexpected error on normal return: %v", err) + default: + } +} diff --git a/internal/config/config.go b/internal/config/config.go index e14f018..721dcf8 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -1,6 +1,7 @@ package config import ( + "fmt" "os" "gopkg.in/yaml.v3" @@ -25,12 +26,12 @@ type IDRACConfig struct { } type MonitoringConfig struct { - Interval int `yaml:"interval"` // Seconds between checks - HistoryRetention int `yaml:"history_retention"` // Seconds to keep history + Interval int `yaml:"interval"` // Seconds between checks + HistoryRetention int `yaml:"history_retention"` // Seconds to keep history } type GPUConfig struct { - Enabled bool `yaml:"enabled"` + Enabled bool `yaml:"enabled"` NvidiaSmiPath string `yaml:"nvidia_smi_path"` } @@ -42,17 +43,27 @@ type Zone struct { } type FanControlConfig struct { - MinSpeed int `yaml:"min_speed" json:"min_speed"` - MaxSpeed int `yaml:"max_speed" json:"max_speed"` - IdleSpeed int `yaml:"idle_speed" json:"idle_speed"` // Base fan speed when idle - CPUThreshold int `yaml:"cpu_threshold" json:"cpu_threshold"` // CPU temp that triggers fan increase - GPUThreshold int `yaml:"gpu_threshold" json:"gpu_threshold"` // GPU temp that triggers fan increase - StepSize int `yaml:"step_size" json:"step_size"` // Fan speed increment per threshold breach - CooldownDelay int `yaml:"cooldown_delay" json:"cooldown_delay"` // Seconds below threshold before ramping down + MinSpeed int `yaml:"min_speed" json:"min_speed"` + MaxSpeed int `yaml:"max_speed" json:"max_speed"` + IdleSpeed int `yaml:"idle_speed" json:"idle_speed"` // Base fan speed when idle + CPUThreshold int `yaml:"cpu_threshold" json:"cpu_threshold"` // CPU temp that triggers fan increase + GPUThreshold int `yaml:"gpu_threshold" json:"gpu_threshold"` // GPU temp that triggers fan increase + StepSize int `yaml:"step_size" json:"step_size"` // Fan speed increment per threshold breach + CooldownDelay int `yaml:"cooldown_delay" json:"cooldown_delay"` // Seconds below threshold before ramping down + // Critical temperatures trigger the emergency ramp (fans jump straight to + // MaxSpeed, bypassing step ramping and any manual override). These are the + // explicit, validated trigger thresholds — the emergency SPEED is always + // MaxSpeed and is never taken from operator-editable zone data. + CriticalCPUTemp int `yaml:"critical_cpu_temp" json:"critical_cpu_temp"` // CPU temp (>=) that forces the emergency ramp + CriticalGPUTemp int `yaml:"critical_gpu_temp" json:"critical_gpu_temp"` // GPU temp (>=) that forces the emergency ramp + // Fail-safe thresholds: after this many consecutive failures the controller + // hands cooling back to the BMC's automatic fan control. + SensorFailureLimit int `yaml:"sensor_failure_limit" json:"sensor_failure_limit"` // Consecutive sensor read failures before restoring auto mode + WriteFailureLimit int `yaml:"write_failure_limit" json:"write_failure_limit"` // Consecutive fan-write failures before restoring auto mode // Legacy fields (still supported) - RampUpStep int `yaml:"ramp_up_step" json:"ramp_up_step"` - RampDownStep int `yaml:"ramp_down_step" json:"ramp_down_step"` - ConstantIdle bool `yaml:"constant_idle" json:"constant_idle"` + RampUpStep int `yaml:"ramp_up_step" json:"ramp_up_step"` + RampDownStep int `yaml:"ramp_down_step" json:"ramp_down_step"` + ConstantIdle bool `yaml:"constant_idle" json:"constant_idle"` } type APIConfig struct { @@ -74,6 +85,32 @@ type LoggingConfig struct { File string `yaml:"file"` } +// Default normal-ramp thresholds, applied when the corresponding config field is +// left at its zero value. Kept here so config validation and the controller +// agree on the effective values. +const ( + defaultCPUThreshold = 65 + defaultGPUThreshold = 60 +) + +// EffectiveCPUThreshold returns the CPU ramp threshold actually in force, +// substituting the default when unset. +func (fc FanControlConfig) EffectiveCPUThreshold() int { + if fc.CPUThreshold > 0 { + return fc.CPUThreshold + } + return defaultCPUThreshold +} + +// EffectiveGPUThreshold returns the GPU ramp threshold actually in force, +// substituting the default when unset. +func (fc FanControlConfig) EffectiveGPUThreshold() int { + if fc.GPUThreshold > 0 { + return fc.GPUThreshold + } + return defaultGPUThreshold +} + // Load reads configuration from a YAML file func Load(path string) (*Config, error) { data, err := os.ReadFile(path) @@ -86,23 +123,69 @@ func Load(path string) (*Config, error) { return nil, err } + // Reject unsafe operator config so the caller can fall back to safe defaults. + // This is critical for the emergency-ramp path: a malformed thresholds/zones + // list must never be trusted to decide when to pin fans. + if err := cfg.Validate(); err != nil { + return nil, err + } + return cfg, nil } +// Validate checks that the safety-critical parts of the config are sane. It is +// intentionally strict about the fields that drive the emergency ramp and the +// fail-safe thresholds; an invalid config is rejected by Load so main falls back +// to the (always-valid) defaults rather than trusting bad operator input. +func (c *Config) Validate() error { + fc := c.FanControl + if fc.MinSpeed < 0 || fc.MaxSpeed > 100 || fc.MinSpeed > fc.MaxSpeed { + return fmt.Errorf("invalid fan speed bounds: min=%d max=%d (require 0<=min<=max<=100)", fc.MinSpeed, fc.MaxSpeed) + } + // Critical thresholds must be explicit and physically plausible. They are the + // sole trigger for the emergency ramp, so we do not accept 0 ("always + // critical") or absurd values. + if fc.CriticalCPUTemp < 40 || fc.CriticalCPUTemp > 120 { + return fmt.Errorf("invalid critical_cpu_temp: %d (require 40..120)", fc.CriticalCPUTemp) + } + if fc.CriticalGPUTemp < 40 || fc.CriticalGPUTemp > 120 { + return fmt.Errorf("invalid critical_gpu_temp: %d (require 40..120)", fc.CriticalGPUTemp) + } + // The critical trigger must sit above the normal ramp thresholds, otherwise + // the step-ramp band is unreachable. Compare against the EFFECTIVE thresholds + // (post-default) so an unset cpu_threshold: 0 does not silently skip the check + // — and so only genuinely conflicting customizations trip validation. + if cpuT := fc.EffectiveCPUThreshold(); fc.CriticalCPUTemp <= cpuT { + return fmt.Errorf("critical_cpu_temp (%d) must exceed effective cpu_threshold (%d)", fc.CriticalCPUTemp, cpuT) + } + if gpuT := fc.EffectiveGPUThreshold(); fc.CriticalGPUTemp <= gpuT { + return fmt.Errorf("critical_gpu_temp (%d) must exceed effective gpu_threshold (%d)", fc.CriticalGPUTemp, gpuT) + } + // Zones are informational (dashboard display), but if provided they must be + // monotonic non-decreasing so the display stays coherent. + for i := 1; i < len(c.Zones); i++ { + prev, cur := c.Zones[i-1], c.Zones[i] + if cur.CPUMax < prev.CPUMax || cur.GPUMax < prev.GPUMax || cur.FanSpeed < prev.FanSpeed { + return fmt.Errorf("zones must be monotonic non-decreasing; zone %q breaks ordering", cur.Name) + } + } + return nil +} + // Default returns a configuration with sensible defaults func Default() *Config { return &Config{ IDRAC: IDRACConfig{ - Host: "", // Must be set via config or IDRAC_HOST env var - Username: "root", // Dell iDRAC default - Password: "", // Must be set via config or IDRAC_PASSWORD env var + Host: "", // Must be set via config or IDRAC_HOST env var + Username: "root", // Dell iDRAC default + Password: "", // Must be set via config or IDRAC_PASSWORD env var }, Monitoring: MonitoringConfig{ Interval: 10, HistoryRetention: 3600, }, GPU: GPUConfig{ - Enabled: true, + Enabled: true, NvidiaSmiPath: "/usr/bin/nvidia-smi", }, Zones: []Zone{ @@ -113,16 +196,20 @@ func Default() *Config { {Name: "critical", CPUMax: 999, GPUMax: 999, FanSpeed: 100}, }, FanControl: FanControlConfig{ - MinSpeed: 5, - MaxSpeed: 100, - IdleSpeed: 20, // Base fan speed when idle (quiet) - CPUThreshold: 65, // Bump fans if CPU exceeds this - GPUThreshold: 60, // Bump fans if GPU exceeds this - StepSize: 10, // Increase fan by 10% per threshold breach - CooldownDelay: 60, // Wait 60s below threshold before ramping down - RampUpStep: 10, - RampDownStep: 5, - ConstantIdle: true, + MinSpeed: 5, + MaxSpeed: 100, + IdleSpeed: 20, // Base fan speed when idle (quiet) + CPUThreshold: 65, // Bump fans if CPU exceeds this + GPUThreshold: 60, // Bump fans if GPU exceeds this + StepSize: 10, // Increase fan by 10% per threshold breach + CooldownDelay: 60, // Wait 60s below threshold before ramping down + CriticalCPUTemp: 85, // Emergency ramp at/above this CPU temp + CriticalGPUTemp: 90, // Emergency ramp at/above this GPU temp + SensorFailureLimit: 3, // Restore BMC auto mode after 3 consecutive sensor read failures + WriteFailureLimit: 3, // Restore BMC auto mode after 3 consecutive fan-write failures + RampUpStep: 10, + RampDownStep: 5, + ConstantIdle: true, }, API: APIConfig{ Host: "0.0.0.0", @@ -142,14 +229,10 @@ func Default() *Config { } } -// GetZone returns the appropriate zone for given temperatures -func (c *Config) GetZone(cpuTemp, gpuTemp int) *Zone { - for i := range c.Zones { - zone := &c.Zones[i] - if cpuTemp <= zone.CPUMax && gpuTemp <= zone.GPUMax { - return zone - } - } - // Return last zone (critical) if nothing matches - return &c.Zones[len(c.Zones)-1] +// IsCritical reports whether the given temperatures have reached the explicit, +// validated critical thresholds. It deliberately does NOT consult the (operator- +// editable) Zones list: the trigger must not be steerable into pinning fans at a +// low speed. The emergency SPEED is decided separately (always MaxSpeed). +func (c *Config) IsCritical(cpuTemp, gpuTemp int) bool { + return cpuTemp >= c.FanControl.CriticalCPUTemp || gpuTemp >= c.FanControl.CriticalGPUTemp } diff --git a/internal/config/config_test.go b/internal/config/config_test.go new file mode 100644 index 0000000..b1ee7df --- /dev/null +++ b/internal/config/config_test.go @@ -0,0 +1,101 @@ +package config + +import "testing" + +func TestValidate(t *testing.T) { + tests := []struct { + name string + mutate func(c *Config) + wantErr bool + }{ + { + name: "default config is valid", + mutate: func(c *Config) {}, + wantErr: false, + }, + { + name: "critical CPU temp too low is rejected", + mutate: func(c *Config) { c.FanControl.CriticalCPUTemp = 30 }, + wantErr: true, + }, + { + name: "critical CPU temp absurdly high is rejected", + mutate: func(c *Config) { c.FanControl.CriticalCPUTemp = 200 }, + wantErr: true, + }, + { + name: "critical temp at/below normal threshold is rejected", + mutate: func(c *Config) { c.FanControl.CriticalCPUTemp = c.FanControl.CPUThreshold }, + wantErr: true, + }, + { + name: "unset cpu_threshold still checks critical against effective default", + mutate: func(c *Config) { + c.FanControl.CPUThreshold = 0 // unset -> effective default 65 + c.FanControl.CriticalCPUTemp = 60 // below effective default + }, + wantErr: true, + }, + { + name: "unset cpu_threshold with sane critical is valid", + mutate: func(c *Config) { + c.FanControl.CPUThreshold = 0 // effective default 65 + c.FanControl.CriticalCPUTemp = 70 // above effective default + }, + wantErr: false, + }, + { + name: "max speed above 100 is rejected", + mutate: func(c *Config) { c.FanControl.MaxSpeed = 120 }, + wantErr: true, + }, + { + name: "min speed above max is rejected", + mutate: func(c *Config) { c.FanControl.MinSpeed = 90; c.FanControl.MaxSpeed = 80 }, + wantErr: true, + }, + { + name: "non-monotonic zones are rejected", + mutate: func(c *Config) { + c.Zones = []Zone{ + {Name: "idle", CPUMax: 45, GPUMax: 40, FanSpeed: 10}, + {Name: "backwards", CPUMax: 30, GPUMax: 30, FanSpeed: 5}, + } + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := Default() + tt.mutate(c) + err := c.Validate() + if tt.wantErr && err == nil { + t.Fatal("expected validation error, got nil") + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected validation error: %v", err) + } + }) + } +} + +func TestIsCritical(t *testing.T) { + c := Default() // CriticalCPUTemp=85, CriticalGPUTemp=90 + tests := []struct { + cpu, gpu int + want bool + }{ + {cpu: 84, gpu: 50, want: false}, // just below critical CPU + {cpu: 85, gpu: 50, want: true}, // at critical CPU + {cpu: 40, gpu: 90, want: true}, // at critical GPU + {cpu: 40, gpu: 89, want: false}, // just below critical GPU + {cpu: 81, gpu: 50, want: false}, // hot-zone band stays reachable, not critical + } + for _, tt := range tests { + if got := c.IsCritical(tt.cpu, tt.gpu); got != tt.want { + t.Fatalf("IsCritical(%d, %d) = %v, want %v", tt.cpu, tt.gpu, got, tt.want) + } + } +} diff --git a/internal/controller/fan.go b/internal/controller/fan.go index 53ed088..85f653f 100644 --- a/internal/controller/fan.go +++ b/internal/controller/fan.go @@ -2,6 +2,7 @@ package controller import ( "bytes" + "context" "fmt" "log" "os/exec" @@ -13,28 +14,83 @@ import ( "github.com/sethpjohnson/only-fan-controller/internal/storage" ) +// cpuReader and gpuReader abstract the temperature sources so the control loop +// can be exercised with fakes in tests (the real implementations are the +// ipmitool/nvidia-smi backed monitors). +type cpuReader interface { + Read() (*monitor.CPUReading, error) +} + +type gpuReader interface { + Read() (*monitor.GPUReading, error) +} + +// runCommandFunc executes an external command with a context deadline. It is a +// field on FanController so tests can substitute a stub in place of real +// ipmitool invocations. +type runCommandFunc func(ctx context.Context, name string, args ...string) error + +// failsafeCause records why the controller handed cooling back to the BMC. +type failsafeCause int + +const ( + failsafeNone failsafeCause = iota + failsafeSensor // sensor loss — recoverable + failsafeWrite // fan-write failures — sticky until restart +) + +func (c failsafeCause) String() string { + switch c { + case failsafeSensor: + return "sensor-loss" + case failsafeWrite: + return "write-failure" + default: + return "none" + } +} + type FanController struct { - cfg *config.Config - cpuMon *monitor.CPUMonitor - gpuMon *monitor.GPUMonitor - store *storage.Store - + cfg *config.Config + cpuMon cpuReader + gpuMon gpuReader + store *storage.Store + + // runCommand runs external commands (ipmitool). Defaults to realRunCommand. + runCommand runCommandFunc + // State - mu sync.RWMutex - currentSpeed int - targetSpeed int - currentZone string - lastCPUReading *monitor.CPUReading - lastGPUReading *monitor.GPUReading - hints map[string]*WorkloadHint - override *Override - running bool - stopChan chan struct{} - + mu sync.RWMutex + currentSpeed int + targetSpeed int + currentZone string + lastCPUReading *monitor.CPUReading + lastGPUReading *monitor.GPUReading + hints map[string]*WorkloadHint + override *Override + running bool + stopChan chan struct{} + + // Fail-safe state. failsafeCause and lastWriteFailed are read by GetStatus + // (API goroutine) so they are guarded by mu. The consecutive-failure counters + // are only ever touched from the control loop goroutine. + // + // The cause is tracked (not a bare bool) so recovery is coherent per domain: + // - a SENSOR fail-safe is recoverable — a healthy sensor read reclaims + // manual control; + // - a WRITE fail-safe is STICKY — once the fan-write channel proves + // unreliable we leave the BMC in charge (no auto-recovery), because + // probing it every tick would flap the BMC between manual and auto. + failsafeCause failsafeCause + restoreConfirmed bool // true once RestoreAutoMode has actually succeeded for the current fail-safe + lastWriteFailed bool + sensorFailCount int + writeFailCount int + // History for trend analysis - cpuHistory []tempPoint - gpuHistory []tempPoint - + cpuHistory []tempPoint + gpuHistory []tempPoint + // Hysteresis tracking lastOverThreshold time.Time } @@ -45,13 +101,13 @@ type tempPoint struct { } type WorkloadHint struct { - Type string `json:"type"` - Action string `json:"action"` - Intensity string `json:"intensity"` - Source string `json:"source"` - MinFanSpeed int `json:"min_fan_speed"` - ExpiresAt time.Time `json:"expires_at"` - CreatedAt time.Time `json:"created_at"` + Type string `json:"type"` + Action string `json:"action"` + Intensity string `json:"intensity"` + Source string `json:"source"` + MinFanSpeed int `json:"min_fan_speed"` + ExpiresAt time.Time `json:"expires_at"` + CreatedAt time.Time `json:"created_at"` } type Override struct { @@ -62,29 +118,35 @@ type Override struct { } type Status struct { - Timestamp time.Time `json:"timestamp"` - CPU *monitor.CPUReading `json:"cpu"` - GPU *monitor.GPUReading `json:"gpu"` - CurrentSpeed int `json:"current_speed"` - TargetSpeed int `json:"target_speed"` - Zone string `json:"zone"` - Mode string `json:"mode"` - ActiveHints []*WorkloadHint `json:"active_hints"` - Override *Override `json:"override,omitempty"` - CPUTrend float64 `json:"cpu_trend"` - GPUTrend float64 `json:"gpu_trend"` - Zones []config.Zone `json:"zones"` - CPUThreshold int `json:"cpu_threshold"` - GPUThreshold int `json:"gpu_threshold"` - IdleSpeed int `json:"idle_speed"` -} - -func NewFanController(cfg *config.Config, cpuMon *monitor.CPUMonitor, gpuMon *monitor.GPUMonitor, store *storage.Store) *FanController { + Timestamp time.Time `json:"timestamp"` + CPU *monitor.CPUReading `json:"cpu"` + GPU *monitor.GPUReading `json:"gpu"` + CurrentSpeed int `json:"current_speed"` + TargetSpeed int `json:"target_speed"` + Zone string `json:"zone"` + Mode string `json:"mode"` + ActiveHints []*WorkloadHint `json:"active_hints"` + Override *Override `json:"override,omitempty"` + CPUTrend float64 `json:"cpu_trend"` + GPUTrend float64 `json:"gpu_trend"` + Zones []config.Zone `json:"zones"` + CPUThreshold int `json:"cpu_threshold"` + GPUThreshold int `json:"gpu_threshold"` + IdleSpeed int `json:"idle_speed"` + // Fail-safe visibility for operators / the dashboard. + FailsafeActive bool `json:"failsafe_active"` // true when cooling has been handed back to BMC auto mode + FailsafeReason string `json:"failsafe_reason"` // "none", "sensor-loss" or "write-failure" + RestorePending bool `json:"restore_pending"` // true when in fail-safe but RestoreAutoMode has not yet succeeded (BMC may still be in manual mode) + LastWriteFailed bool `json:"last_write_failed"` // true when the most recent fan-speed write failed +} + +func NewFanController(cfg *config.Config, cpuMon cpuReader, gpuMon gpuReader, store *storage.Store) *FanController { return &FanController{ cfg: cfg, cpuMon: cpuMon, gpuMon: gpuMon, store: store, + runCommand: realRunCommand, hints: make(map[string]*WorkloadHint), stopChan: make(chan struct{}), cpuHistory: make([]tempPoint, 0), @@ -92,6 +154,22 @@ func NewFanController(cfg *config.Config, cpuMon *monitor.CPUMonitor, gpuMon *mo } } +// realRunCommand runs an external command bounded by the context deadline. On a +// deadline it returns a descriptive error so the caller can distinguish a hung +// command (e.g. a stuck `ipmitool -I lanplus`) from a normal failure. +func realRunCommand(ctx context.Context, name string, args ...string) error { + cmd := exec.CommandContext(ctx, name, args...) + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + if ctx.Err() == context.DeadlineExceeded { + return fmt.Errorf("%s timed out: %w", name, err) + } + return fmt.Errorf("%s error: %v, stderr: %s", name, err, stderr.String()) + } + return nil +} + func (fc *FanController) Run() { fc.mu.Lock() fc.running = true @@ -126,48 +204,272 @@ func (fc *FanController) Stop() { } func (fc *FanController) controlLoop() { - // Read temperatures - cpuReading, err := fc.cpuMon.Read() - if err != nil { - log.Printf("CPU read error: %v", err) - cpuReading = &monitor.CPUReading{Temps: []int{}, Max: 0} + // If we are in fail-safe but the hand-back to BMC auto has not been confirmed + // (a previous RestoreAutoMode failed — e.g. the BMC was unreachable, which is + // exactly the condition that trips fail-safe), keep retrying every tick until + // it succeeds. This does not reintroduce flapping: restore-to-auto is + // idempotent and nothing re-enables manual mode while a restore is unconfirmed. + fc.ensureAutoRestored() + + // A WRITE fail-safe is sticky: once the fan-write channel has proven + // unreliable we leave the BMC in charge until the process restarts. Probing + // it every tick (reclaim manual -> write fails -> restore auto) would flap the + // BMC forever, so we deliberately do not auto-recover. We still refresh the + // readings for /api/status visibility. + if fc.currentFailsafeCause() == failsafeWrite { + if cpuReading, gpuReading, ok := fc.readSensors(); ok { + fc.recordReadings(cpuReading, gpuReading) + } + return + } + + // Read temperatures. A read error must NEVER be treated as 0°C ("cold"), + // which would ramp the fans down and cook the machine. Instead we hold the + // current speed and, after repeated failures, hand cooling back to the BMC. + cpuReading, gpuReading, ok := fc.readSensors() + if !ok { + fc.handleSensorFailure() + return + } + + // Sensors are healthy again: clear the failure count. + if fc.sensorFailCount > 0 { + log.Printf("Sensors recovered after %d consecutive failure(s)", fc.sensorFailCount) + fc.sensorFailCount = 0 + } + + // If we were in a (recoverable) sensor fail-safe, reclaim manual control + // before acting on the reading — but only from a CONFIRMED-restored state, so + // the manual<->auto handoff stays coherent. If the hand-back to BMC auto is + // still unconfirmed, ensureAutoRestored (above) keeps retrying and we wait. + // If reclaiming the write channel fails we stay in fail-safe and retry next + // tick; a failed enableManualMode does not change the BMC state, so it cannot + // flap. + if fc.currentFailsafeCause() == failsafeSensor { + if !fc.isRestoreConfirmed() { + log.Printf("Sensors recovered but BMC auto hand-back not yet confirmed; deferring manual reclaim") + return + } + if err := fc.enableManualMode(); err != nil { + log.Printf("Sensors recovered but reclaiming manual mode failed; staying in BMC auto: %v", err) + return + } + log.Printf("FAILSAFE CLEARED: sensors recovered, manual fan control reclaimed") + fc.clearFailsafe() } - gpuReading, err := fc.gpuMon.Read() - if err != nil { - log.Printf("GPU read error: %v", err) - gpuReading = &monitor.GPUReading{Devices: []monitor.GPUDevice{}, Max: 0} + fc.recordReadings(cpuReading, gpuReading) + + // Calculate target fan speed + target := fc.calculateTarget(cpuReading, gpuReading) + + // Apply fan speed. A failed write is a watchdog concern: after repeated + // failures we restore BMC auto mode rather than leave the fans wherever + // they were last set. + if err := fc.setFanSpeed(target); err != nil { + fc.handleWriteFailure(err) + } else { + fc.writeFailCount = 0 } + // Store reading + fc.store.RecordReading(cpuReading.Max, gpuReading.Max, target) + + fc.mu.RLock() + zone := fc.currentZone + fc.mu.RUnlock() + log.Printf("CPU: %d°C | GPU: %d°C | Zone: %s | Fan: %d%%", + cpuReading.Max, gpuReading.Max, zone, target) +} + +// readSensors reads both temperature sources. It returns ok=false if either read +// failed, logging each failure. It never fabricates a 0°C reading on failure. +func (fc *FanController) readSensors() (*monitor.CPUReading, *monitor.GPUReading, bool) { + cpuReading, cpuErr := fc.cpuMon.Read() + gpuReading, gpuErr := fc.gpuMon.Read() + if cpuErr != nil { + log.Printf("CPU sensor read failed: %v", cpuErr) + } + if gpuErr != nil { + log.Printf("GPU sensor read failed: %v", gpuErr) + } + if cpuErr != nil || gpuErr != nil { + return nil, nil, false + } + return cpuReading, gpuReading, true +} + +// recordReadings updates the last-known readings and history under the lock. +func (fc *FanController) recordReadings(cpuReading *monitor.CPUReading, gpuReading *monitor.GPUReading) { fc.mu.Lock() + defer fc.mu.Unlock() fc.lastCPUReading = cpuReading fc.lastGPUReading = gpuReading - - // Update history now := time.Now() fc.cpuHistory = append(fc.cpuHistory, tempPoint{temp: cpuReading.Max, timestamp: now}) fc.gpuHistory = append(fc.gpuHistory, tempPoint{temp: gpuReading.Max, timestamp: now}) - - // Trim old history fc.trimHistory() - - // Clean expired hints and overrides fc.cleanExpired() +} + +// handleSensorFailure records a consecutive sensor read failure. It holds the +// current fan speed (does not touch the fans) and, once the configured limit is +// reached, hands cooling back to the BMC's automatic control. +func (fc *FanController) handleSensorFailure() { + fc.sensorFailCount++ + limit := fc.sensorFailureLimit() + log.Printf("SENSOR FAILURE %d/%d - holding fan speed (not treating missing data as 0°C)", + fc.sensorFailCount, limit) + if fc.sensorFailCount >= limit { + fc.enterFailsafe(failsafeSensor) + } +} + +// handleWriteFailure records a consecutive fan-write failure and restores auto +// mode once the configured limit is reached. +func (fc *FanController) handleWriteFailure(err error) { + fc.writeFailCount++ + limit := fc.writeFailureLimit() + log.Printf("Fan write failure %d/%d: %v", fc.writeFailCount, limit, err) + if fc.writeFailCount >= limit { + fc.enterFailsafe(failsafeWrite) + } +} + +func (fc *FanController) currentFailsafeCause() failsafeCause { + fc.mu.RLock() + defer fc.mu.RUnlock() + return fc.failsafeCause +} + +func (fc *FanController) isRestoreConfirmed() bool { + fc.mu.RLock() + defer fc.mu.RUnlock() + return fc.restoreConfirmed +} + +// enterFailsafe hands cooling back to the BMC's automatic fan control, recording +// WHY. It only attempts RestoreAutoMode on the transition from active control +// into fail-safe; a write fail-safe never downgrades to sensor, so once sticky +// it stays sticky. If the restore attempt fails, restoreConfirmed stays false +// and ensureAutoRestored retries on subsequent ticks. +func (fc *FanController) enterFailsafe(cause failsafeCause) { + fc.mu.Lock() + prev := fc.failsafeCause + // Write is the stickiest cause; never let a later sensor failure overwrite it. + if prev == failsafeWrite { + fc.mu.Unlock() + return + } + fc.failsafeCause = cause fc.mu.Unlock() - // Calculate target fan speed - target := fc.calculateTarget(cpuReading, gpuReading) + if prev != failsafeNone { + // Already handed to BMC auto for some reason; ensureAutoRestored owns the + // (possibly still-pending) restore retry. + return + } + log.Printf("FAILSAFE ACTIVATED (%s): restoring BMC automatic fan control", cause) + fc.attemptRestore() +} - // Apply fan speed - if err := fc.setFanSpeed(target); err != nil { - log.Printf("Failed to set fan speed: %v", err) +// ensureAutoRestored retries the hand-back to BMC auto mode while we are in +// fail-safe but the restore has not yet been confirmed. Restore-to-auto is +// idempotent and nothing re-enables manual mode while unconfirmed, so this +// cannot flap. +func (fc *FanController) ensureAutoRestored() { + fc.mu.RLock() + pending := fc.failsafeCause != failsafeNone && !fc.restoreConfirmed + fc.mu.RUnlock() + if !pending { + return } + log.Printf("Fail-safe: BMC auto hand-back not confirmed, retrying RestoreAutoMode") + fc.attemptRestore() +} - // Store reading - fc.store.RecordReading(cpuReading.Max, gpuReading.Max, target) +// attemptRestore calls RestoreAutoMode and records whether it succeeded. +func (fc *FanController) attemptRestore() { + if err := fc.RestoreAutoMode(); err != nil { + log.Printf("CRITICAL: RestoreAutoMode failed; BMC hand-back NOT confirmed, will retry: %v", err) + return + } + fc.mu.Lock() + fc.restoreConfirmed = true + fc.mu.Unlock() + log.Printf("Fail-safe: BMC automatic fan control confirmed restored") +} + +// clearFailsafe returns to normal control and resets the fail-safe state so the +// next failure starts fresh. +func (fc *FanController) clearFailsafe() { + fc.mu.Lock() + fc.failsafeCause = failsafeNone + fc.restoreConfirmed = false + // sensorFailCount / writeFailCount are only ever touched from the single + // control-loop goroutine (here and in handleSensor/WriteFailure), so they + // need no lock; grouped here for a coherent reset of all fail-safe state. + fc.sensorFailCount = 0 + fc.writeFailCount = 0 + fc.mu.Unlock() +} + +func (fc *FanController) sensorFailureLimit() int { + if fc.cfg.FanControl.SensorFailureLimit > 0 { + return fc.cfg.FanControl.SensorFailureLimit + } + return 3 +} - log.Printf("CPU: %d°C | GPU: %d°C | Zone: %s | Fan: %d%%", - cpuReading.Max, gpuReading.Max, fc.currentZone, target) +func (fc *FanController) writeFailureLimit() int { + if fc.cfg.FanControl.WriteFailureLimit > 0 { + return fc.cfg.FanControl.WriteFailureLimit + } + return 3 +} + +// commandTimeout bounds every ipmitool invocation so a hung BMC connection can +// never freeze the control loop (or shutdown), while keeping a floor so that a +// short control interval does not make slow-but-healthy lanplus calls read as +// timeouts (which would feed the fail-safe counters). +func (fc *FanController) commandTimeout() time.Duration { + return commandTimeout(fc.cfg.Monitoring.Interval) +} + +// commandTimeout computes the external-command deadline: max(3s, min(10s, +// interval)). The 3s floor protects slow-but-healthy remote BMC calls. +func commandTimeout(intervalSeconds int) time.Duration { + const ( + minTimeout = 3 * time.Second + maxTimeout = 10 * time.Second + ) + interval := time.Duration(intervalSeconds) * time.Second + if interval > maxTimeout { + return maxTimeout + } + if interval < minTimeout { + return minTimeout + } + return interval +} + +// ipmitool runs an ipmitool raw command against the configured BMC (local or +// remote) with a deadline. +func (fc *FanController) ipmitool(rawArgs ...string) error { + ctx, cancel := context.WithTimeout(context.Background(), fc.commandTimeout()) + defer cancel() + + var args []string + if fc.cfg.IDRAC.Host != "local" { + args = append(args, + "-I", "lanplus", + "-H", fc.cfg.IDRAC.Host, + "-U", fc.cfg.IDRAC.Username, + "-P", fc.cfg.IDRAC.Password, + ) + } + args = append(args, rawArgs...) + return fc.runCommand(ctx, "ipmitool", args...) } func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuReading *monitor.GPUReading) int { @@ -177,8 +479,25 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead cpuMax := cpuReading.Max gpuMax := gpuReading.Max + // Safety first: a critical temperature bypasses step ramping AND any manual + // override, driving fans straight to MaxSpeed (effectively 100%). The + // emergency speed is a FIXED CEILING (MaxSpeed), never operator-editable zone + // data — that way a bad zones list can never turn the "emergency" into a low + // speed. Leaving fans pinned low during a thermal emergency is exactly what + // this controller must never do, so critical cooling wins over everything. + if fc.cfg.IsCritical(cpuMax, gpuMax) { + speed := fc.cfg.FanControl.MaxSpeed + if speed <= 0 || speed > 100 { + speed = 100 + } + fc.currentZone = "critical" + fc.targetSpeed = speed + return speed + } + // Check for manual override if fc.override != nil { + fc.targetSpeed = fc.override.Speed return fc.override.Speed } @@ -201,17 +520,11 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead } // Check if we're over thresholds - cpuThreshold := fc.cfg.FanControl.CPUThreshold - gpuThreshold := fc.cfg.FanControl.GPUThreshold - if cpuThreshold == 0 { - cpuThreshold = 65 // Default CPU threshold - } - if gpuThreshold == 0 { - gpuThreshold = 60 // Default GPU threshold - } + cpuThreshold := fc.cfg.FanControl.EffectiveCPUThreshold() + gpuThreshold := fc.cfg.FanControl.EffectiveGPUThreshold() overThreshold := cpuMax > cpuThreshold || gpuMax > gpuThreshold - + // Determine zone name for display if cpuMax > 80 || gpuMax > 85 { fc.currentZone = "hot" @@ -234,16 +547,16 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead if overThreshold { // Over threshold - increase fan speed and reset cooldown timer fc.lastOverThreshold = now - + // Calculate how much over threshold we are cpuOver := max(0, cpuMax-cpuThreshold) gpuOver := max(0, gpuMax-gpuThreshold) maxOver := max(cpuOver, gpuOver) - + // Each 5°C over threshold = one step increase stepsNeeded := (maxOver / 5) + 1 neededSpeed := baseSpeed + (stepsNeeded * stepSize) - + // Ramp up immediately if needed if neededSpeed > target { target = min(target+stepSize, neededSpeed) @@ -254,7 +567,7 @@ func (fc *FanController) calculateTarget(cpuReading *monitor.CPUReading, gpuRead if cooldownDuration == 0 { cooldownDuration = 60 * time.Second } - + if fc.lastOverThreshold.IsZero() { // Never been over threshold - go directly to idle speed target = baseSpeed @@ -306,7 +619,7 @@ func (fc *FanController) calculateTrend(history []tempPoint) float64 { first := recent[0] last := recent[len(recent)-1] duration := last.timestamp.Sub(first.timestamp).Minutes() - + if duration < 0.1 { return 0 } @@ -316,7 +629,7 @@ func (fc *FanController) calculateTrend(history []tempPoint) float64 { func (fc *FanController) trimHistory() { cutoff := time.Now().Add(-time.Duration(fc.cfg.Monitoring.HistoryRetention) * time.Second) - + var newCPU []tempPoint for _, p := range fc.cpuHistory { if p.timestamp.After(cutoff) { @@ -336,79 +649,45 @@ func (fc *FanController) trimHistory() { func (fc *FanController) cleanExpired() { now := time.Now() - + for key, hint := range fc.hints { if !hint.ExpiresAt.IsZero() && hint.ExpiresAt.Before(now) { delete(fc.hints, key) } } - + if fc.override != nil && !fc.override.ExpiresAt.IsZero() && fc.override.ExpiresAt.Before(now) { fc.override = nil } } func (fc *FanController) setFanSpeed(speed int) error { - fc.mu.Lock() - fc.currentSpeed = speed - fc.mu.Unlock() - // Convert percentage to hex (0-100 -> 0x00-0x64) hexSpeed := fmt.Sprintf("0x%02x", speed) - var cmd *exec.Cmd - if fc.cfg.IDRAC.Host == "local" { - cmd = exec.Command("ipmitool", "raw", "0x30", "0x30", "0x02", "0xff", hexSpeed) - } else { - cmd = exec.Command("ipmitool", - "-I", "lanplus", - "-H", fc.cfg.IDRAC.Host, - "-U", fc.cfg.IDRAC.Username, - "-P", fc.cfg.IDRAC.Password, - "raw", "0x30", "0x30", "0x02", "0xff", hexSpeed, - ) - } - - var stderr bytes.Buffer - cmd.Stderr = &stderr - - if err := cmd.Run(); err != nil { - return fmt.Errorf("ipmitool error: %v, stderr: %s", err, stderr.String()) + // currentSpeed must reflect what the fans are ACTUALLY set to, so it is only + // updated after a confirmed successful write. A failed write is surfaced via + // lastWriteFailed in /api/status. + if err := fc.ipmitool("raw", "0x30", "0x30", "0x02", "0xff", hexSpeed); err != nil { + fc.mu.Lock() + fc.lastWriteFailed = true + fc.mu.Unlock() + return err } + fc.mu.Lock() + fc.currentSpeed = speed + fc.lastWriteFailed = false + fc.mu.Unlock() return nil } func (fc *FanController) enableManualMode() error { - var cmd *exec.Cmd - if fc.cfg.IDRAC.Host == "local" { - cmd = exec.Command("ipmitool", "raw", "0x30", "0x30", "0x01", "0x00") - } else { - cmd = exec.Command("ipmitool", - "-I", "lanplus", - "-H", fc.cfg.IDRAC.Host, - "-U", fc.cfg.IDRAC.Username, - "-P", fc.cfg.IDRAC.Password, - "raw", "0x30", "0x30", "0x01", "0x00", - ) - } - return cmd.Run() + return fc.ipmitool("raw", "0x30", "0x30", "0x01", "0x00") } func (fc *FanController) RestoreAutoMode() error { - var cmd *exec.Cmd - if fc.cfg.IDRAC.Host == "local" { - cmd = exec.Command("ipmitool", "raw", "0x30", "0x30", "0x01", "0x01") - } else { - cmd = exec.Command("ipmitool", - "-I", "lanplus", - "-H", fc.cfg.IDRAC.Host, - "-U", fc.cfg.IDRAC.Username, - "-P", fc.cfg.IDRAC.Password, - "raw", "0x30", "0x30", "0x01", "0x01", - ) - } - return cmd.Run() + return fc.ipmitool("raw", "0x30", "0x30", "0x01", "0x01") } // AddHint registers a workload hint @@ -430,7 +709,7 @@ func (fc *FanController) AddHint(hint *WorkloadHint) { hint.CreatedAt = time.Now() fc.hints[hint.Source] = hint - + log.Printf("Hint registered: %s from %s (min fan: %d%%)", hint.Action, hint.Source, hint.MinFanSpeed) } @@ -446,17 +725,17 @@ func (fc *FanController) RemoveHint(source string) { func (fc *FanController) SetOverride(speed int, duration time.Duration, reason string) { fc.mu.Lock() defer fc.mu.Unlock() - + fc.override = &Override{ Speed: speed, Reason: reason, CreatedAt: time.Now(), } - + if duration > 0 { fc.override.ExpiresAt = time.Now().Add(duration) } - + log.Printf("Override set: %d%% (%s)", speed, reason) } @@ -486,31 +765,29 @@ func (fc *FanController) GetStatus() *Status { } // Build threshold info for dashboard - cpuThreshold := fc.cfg.FanControl.CPUThreshold - gpuThreshold := fc.cfg.FanControl.GPUThreshold - if cpuThreshold == 0 { - cpuThreshold = 65 - } - if gpuThreshold == 0 { - gpuThreshold = 60 - } + cpuThreshold := fc.cfg.FanControl.EffectiveCPUThreshold() + gpuThreshold := fc.cfg.FanControl.EffectiveGPUThreshold() return &Status{ - Timestamp: time.Now(), - CPU: fc.lastCPUReading, - GPU: fc.lastGPUReading, - CurrentSpeed: fc.currentSpeed, - TargetSpeed: fc.targetSpeed, - Zone: fc.currentZone, - Mode: mode, - ActiveHints: hints, - Override: fc.override, - CPUTrend: fc.calculateTrend(fc.cpuHistory), - GPUTrend: fc.calculateTrend(fc.gpuHistory), - Zones: fc.cfg.Zones, - CPUThreshold: cpuThreshold, - GPUThreshold: gpuThreshold, - IdleSpeed: fc.cfg.FanControl.IdleSpeed, + Timestamp: time.Now(), + CPU: fc.lastCPUReading, + GPU: fc.lastGPUReading, + CurrentSpeed: fc.currentSpeed, + TargetSpeed: fc.targetSpeed, + Zone: fc.currentZone, + Mode: mode, + ActiveHints: hints, + Override: fc.override, + CPUTrend: fc.calculateTrend(fc.cpuHistory), + GPUTrend: fc.calculateTrend(fc.gpuHistory), + Zones: fc.cfg.Zones, + CPUThreshold: cpuThreshold, + GPUThreshold: gpuThreshold, + IdleSpeed: fc.cfg.FanControl.IdleSpeed, + FailsafeActive: fc.failsafeCause != failsafeNone, + FailsafeReason: fc.failsafeCause.String(), + RestorePending: fc.failsafeCause != failsafeNone && !fc.restoreConfirmed, + LastWriteFailed: fc.lastWriteFailed, } } diff --git a/internal/controller/fan_test.go b/internal/controller/fan_test.go new file mode 100644 index 0000000..0d47ae3 --- /dev/null +++ b/internal/controller/fan_test.go @@ -0,0 +1,493 @@ +package controller + +import ( + "context" + "errors" + "path/filepath" + "testing" + "time" + + "github.com/sethpjohnson/only-fan-controller/internal/config" + "github.com/sethpjohnson/only-fan-controller/internal/monitor" + "github.com/sethpjohnson/only-fan-controller/internal/storage" +) + +// testConfig returns a fresh default config for deterministic tests. +func testConfig() *config.Config { + return config.Default() +} + +func cpuGpu(cpuMax, gpuMax int) (*monitor.CPUReading, *monitor.GPUReading) { + return &monitor.CPUReading{Temps: []int{cpuMax}, Max: cpuMax}, + &monitor.GPUReading{Max: gpuMax} +} + +func TestCalculateTarget(t *testing.T) { + tests := []struct { + name string + // setup mutates the controller before the call. + setup func(fc *FanController) + cpu int + gpu int + want int + }{ + { + name: "idle below all thresholds goes to idle speed", + setup: func(fc *FanController) { fc.currentSpeed = 0 }, + cpu: 40, gpu: 35, + want: 20, // IdleSpeed + }, + { + name: "threshold crossing ramps up by one step", + setup: func(fc *FanController) { + fc.currentSpeed = 20 + }, + cpu: 75, gpu: 35, + want: 30, // 20 + one StepSize + }, + { + name: "hysteresis holds speed during cooldown", + setup: func(fc *FanController) { + fc.currentSpeed = 40 + fc.lastOverThreshold = time.Now() + }, + cpu: 50, gpu: 35, + want: 40, // unchanged during cooldown + }, + { + name: "ramps down one step after cooldown elapses", + setup: func(fc *FanController) { + fc.currentSpeed = 40 + fc.lastOverThreshold = time.Now().Add(-2 * time.Minute) + }, + cpu: 50, gpu: 35, + want: 30, // 40 - one StepSize + }, + { + name: "manual override takes precedence over normal logic", + setup: func(fc *FanController) { + fc.currentSpeed = 20 + fc.override = &Override{Speed: 55} + }, + cpu: 50, gpu: 35, + want: 55, + }, + { + name: "critical temp overrides even a low manual override", + setup: func(fc *FanController) { + fc.currentSpeed = 10 + fc.override = &Override{Speed: 20} + }, + cpu: 95, gpu: 35, + want: 100, // critical zone speed + }, + { + name: "critical temp fast-ramps straight to 100 from idle", + setup: func(fc *FanController) { + fc.currentSpeed = 10 + }, + cpu: 95, gpu: 35, + want: 100, + }, + { + name: "critical GPU temp also fast-ramps", + setup: func(fc *FanController) { + fc.currentSpeed = 10 + }, + cpu: 40, gpu: 95, + want: 100, + }, + { + name: "target clamped up to configured min speed", + setup: func(fc *FanController) { + fc.currentSpeed = 0 + fc.cfg.FanControl.MinSpeed = 30 + }, + cpu: 40, gpu: 35, + want: 30, // idle speed 20 clamped up to min 30 + }, + { + name: "target clamped down to configured max speed", + setup: func(fc *FanController) { + fc.currentSpeed = 0 + fc.cfg.FanControl.IdleSpeed = 90 + fc.cfg.FanControl.MaxSpeed = 80 + }, + cpu: 40, gpu: 35, + want: 80, // idle speed 90 clamped down to max 80 + }, + { + name: "workload hint sets a fan floor", + setup: func(fc *FanController) { + fc.currentSpeed = 0 + fc.hints["test"] = &WorkloadHint{Source: "test", MinFanSpeed: 45} + }, + cpu: 40, gpu: 35, + want: 45, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + fc := NewFanController(testConfig(), nil, nil, nil) + if tt.setup != nil { + tt.setup(fc) + } + cpuR, gpuR := cpuGpu(tt.cpu, tt.gpu) + got := fc.calculateTarget(cpuR, gpuR) + if got != tt.want { + t.Fatalf("calculateTarget(cpu=%d, gpu=%d) = %d, want %d", tt.cpu, tt.gpu, got, tt.want) + } + }) + } +} + +// TestCriticalRampIgnoresOperatorZoneData proves the emergency ramp trusts the +// fixed MaxSpeed ceiling, never operator-editable zone fan speeds. Even a +// malicious/misconfigured Zones list whose top zone asks for a LOW speed must +// not weaken the emergency ramp, and critical still overrides a low manual +// override. +func TestCriticalRampIgnoresOperatorZoneData(t *testing.T) { + cfg := config.Default() + // Hostile zone list: "critical" zone requests a dangerously low 15%. + cfg.Zones = []config.Zone{ + {Name: "idle", CPUMax: 45, GPUMax: 40, FanSpeed: 10}, + {Name: "critical", CPUMax: 999, GPUMax: 999, FanSpeed: 15}, + } + cfg.FanControl.MaxSpeed = 100 + cfg.FanControl.CriticalCPUTemp = 85 + cfg.FanControl.CriticalGPUTemp = 90 + + fc := NewFanController(cfg, nil, nil, nil) + fc.override = &Override{Speed: 20} // low manual override + fc.currentSpeed = 10 + + cpuR, gpuR := cpuGpu(95, 35) // CPU critical + if got := fc.calculateTarget(cpuR, gpuR); got != 100 { + t.Fatalf("critical ramp used operator zone data / override instead of MaxSpeed: got %d, want 100", got) + } +} + +// --- fail-safe wiring tests --- + +// cmdRecorder captures the ipmitool invocations made through runCommand. +type cmdRecorder struct { + cmds [][]string + failOnFanSet bool + failRestore bool // simulate an unreachable BMC: RestoreAutoMode (0x01 0x01) fails +} + +func (r *cmdRecorder) run(_ context.Context, name string, args ...string) error { + call := append([]string{name}, args...) + r.cmds = append(r.cmds, call) + if r.failOnFanSet && containsArg(args, "0x02") { + return errors.New("simulated ipmitool fan-set failure") + } + if r.failRestore && endsWith(call, "0x01", "0x01") { + return errors.New("simulated unreachable BMC on RestoreAutoMode") + } + return nil +} + +func containsArg(args []string, want string) bool { + for _, a := range args { + if a == want { + return true + } + } + return false +} + +// restoreCount counts how many times BMC auto mode was restored (raw ... 0x01 0x01). +func (r *cmdRecorder) restoreCount() int { + n := 0 + for _, c := range r.cmds { + if endsWith(c, "0x01", "0x01") { + n++ + } + } + return n +} + +func (r *cmdRecorder) fanSetCount() int { + n := 0 + for _, c := range r.cmds { + if containsArg(c, "0x02") { + n++ + } + } + return n +} + +// manualModeCount counts how many times manual fan mode was (re-)enabled +// (raw ... 0x01 0x00). Together with restoreCount this exposes BMC flapping. +func (r *cmdRecorder) manualModeCount() int { + n := 0 + for _, c := range r.cmds { + if endsWith(c, "0x01", "0x00") { + n++ + } + } + return n +} + +func endsWith(s []string, a, b string) bool { + if len(s) < 2 { + return false + } + return s[len(s)-2] == a && s[len(s)-1] == b +} + +type failingCPU struct{} + +func (failingCPU) Read() (*monitor.CPUReading, error) { + return nil, errors.New("simulated CPU sensor loss") +} + +// flakyCPU fails its first failFor reads, then succeeds — used to test recovery. +type flakyCPU struct { + failFor int + calls int + max int +} + +func (f *flakyCPU) Read() (*monitor.CPUReading, error) { + f.calls++ + if f.calls <= f.failFor { + return nil, errors.New("simulated transient CPU sensor loss") + } + return &monitor.CPUReading{Temps: []int{f.max}, Max: f.max}, nil +} + +type staticCPU struct{ max int } + +func (s staticCPU) Read() (*monitor.CPUReading, error) { + return &monitor.CPUReading{Temps: []int{s.max}, Max: s.max}, nil +} + +type staticGPU struct{ max int } + +func (s staticGPU) Read() (*monitor.GPUReading, error) { + return &monitor.GPUReading{Max: s.max}, nil +} + +func newTestStore(t *testing.T) *storage.Store { + t.Helper() + store, err := storage.New(filepath.Join(t.TempDir(), "test.db")) + if err != nil { + t.Fatalf("failed to create test store: %v", err) + } + t.Cleanup(func() { store.Close() }) + return store +} + +func TestControlLoopHoldsSpeedOnSensorFailure(t *testing.T) { + rec := &cmdRecorder{} + fc := NewFanController(testConfig(), failingCPU{}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 40 + + fc.controlLoop() + + if fc.currentSpeed != 40 { + t.Fatalf("currentSpeed changed on sensor failure: got %d, want 40", fc.currentSpeed) + } + if writes := rec.fanSetCount(); writes != 0 { + t.Fatalf("fan speed was written on sensor failure (%d writes); must hold instead", writes) + } +} + +func TestSensorFailureThresholdRestoresAutoMode(t *testing.T) { + rec := &cmdRecorder{} + cfg := testConfig() + cfg.FanControl.SensorFailureLimit = 3 + fc := NewFanController(cfg, failingCPU{}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 40 + + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != 0 { + t.Fatalf("restored auto mode too early after 2 failures: %d restores", got) + } + fc.controlLoop() // third failure crosses the threshold + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected exactly one auto-mode restore after %d failures, got %d", cfg.FanControl.SensorFailureLimit, got) + } + + // Keep failing well past the threshold: must NOT re-restore or flap. + for i := 0; i < 6; i++ { + fc.controlLoop() + } + if got := rec.restoreCount(); got != 1 { + t.Fatalf("sensor fail-safe re-restored auto mode (oscillation): %d restores, want 1", got) + } + if got := rec.manualModeCount(); got != 0 { + t.Fatalf("sensor fail-safe flapped back to manual while sensors still failing: %d manual toggles", got) + } +} + +// TestWriteFailureStickyNoOscillation reproduces the reviewer's scenario: +// healthy sensors + persistently failing fan writes. The fail-safe must engage +// exactly once and then stay in BMC auto — no manual/auto flapping. +func TestWriteFailureStickyNoOscillation(t *testing.T) { + rec := &cmdRecorder{failOnFanSet: true} + cfg := testConfig() + cfg.FanControl.WriteFailureLimit = 3 + fc := NewFanController(cfg, staticCPU{max: 50}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != 0 { + t.Fatalf("restored auto mode too early after 2 write failures: %d restores", got) + } + fc.controlLoop() // third write failure crosses the threshold + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected exactly one auto-mode restore after %d write failures, got %d", cfg.FanControl.WriteFailureLimit, got) + } + + fanSetsAtTrip := rec.fanSetCount() + + // Run many more ticks with writes still failing. The reviewer's bug flapped + // once per tick; the sticky write fail-safe must converge. + for i := 0; i < 10; i++ { + fc.controlLoop() + } + if got := rec.restoreCount(); got != 1 { + t.Fatalf("write fail-safe oscillated: %d auto-mode restores after threshold, want 1", got) + } + if got := rec.manualModeCount(); got != 0 { + t.Fatalf("write fail-safe flapped back to manual: %d manual toggles, want 0", got) + } + if got := rec.fanSetCount(); got != fanSetsAtTrip { + t.Fatalf("write fail-safe kept attempting fan writes after going sticky: %d attempts, want %d", got, fanSetsAtTrip) + } + if fc.currentFailsafeCause() != failsafeWrite { + t.Fatalf("expected sticky write fail-safe, got cause %v", fc.currentFailsafeCause()) + } + // Readings must still be refreshed for /api/status visibility while sticky. + if fc.lastCPUReading == nil || fc.lastCPUReading.Max != 50 { + t.Fatalf("readings not refreshed during write fail-safe: %+v", fc.lastCPUReading) + } +} + +// TestSensorFailsafeRecoversAndReclaimsManual proves the recoverable domain: +// once sensors read successfully again, manual control is reclaimed exactly once. +func TestSensorFailsafeRecoversAndReclaimsManual(t *testing.T) { + rec := &cmdRecorder{} + cfg := testConfig() + cfg.FanControl.SensorFailureLimit = 3 + cpu := &flakyCPU{failFor: 3, max: 50} + fc := NewFanController(cfg, cpu, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 30 + + // 3 failing reads -> enter sensor fail-safe. + fc.controlLoop() + fc.controlLoop() + fc.controlLoop() + if fc.currentFailsafeCause() != failsafeSensor { + t.Fatalf("expected sensor fail-safe after 3 failures, got %v", fc.currentFailsafeCause()) + } + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected one auto restore, got %d", got) + } + + // 4th read succeeds -> reclaim manual and resume control. + fc.controlLoop() + if fc.currentFailsafeCause() != failsafeNone { + t.Fatalf("fail-safe not cleared after sensor recovery: %v", fc.currentFailsafeCause()) + } + if got := rec.manualModeCount(); got != 1 { + t.Fatalf("expected exactly one manual-mode reclaim on recovery, got %d", got) + } + if got := rec.fanSetCount(); got != 1 { + t.Fatalf("expected one fan write after resuming control, got %d", got) + } + // Counters reset for a fresh future failure window. + if fc.sensorFailCount != 0 { + t.Fatalf("sensorFailCount not reset after recovery: %d", fc.sensorFailCount) + } +} + +// TestFailsafeRetriesRestoreUntilConfirmed reproduces an unreachable BMC: the +// very condition that trips fail-safe also fails RestoreAutoMode. The controller +// must keep retrying every tick (not give up after one attempt) and report the +// restore as pending; once the BMC returns, restore succeeds once and retries +// stop. +func TestFailsafeRetriesRestoreUntilConfirmed(t *testing.T) { + rec := &cmdRecorder{failRestore: true} + cfg := testConfig() + cfg.FanControl.SensorFailureLimit = 3 + fc := NewFanController(cfg, failingCPU{}, staticGPU{max: 40}, newTestStore(t)) + fc.runCommand = rec.run + fc.currentSpeed = 40 + + // Cross the sensor threshold -> first (failed) restore attempt. + fc.controlLoop() + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != 1 { + t.Fatalf("expected 1 restore attempt at trip, got %d", got) + } + if fc.isRestoreConfirmed() { + t.Fatal("restore should not be confirmed while BMC is unreachable") + } + + // Keep failing for N more ticks: each tick must retry the restore. + const extra = 5 + for i := 0; i < extra; i++ { + fc.controlLoop() + } + if got := rec.restoreCount(); got != 1+extra { + t.Fatalf("expected %d restore attempts (retrying every tick), got %d", 1+extra, got) + } + if got := fc.GetStatus(); !got.RestorePending || !got.FailsafeActive { + t.Fatalf("status should show failsafe active + restore pending, got %+v", got) + } + + // BMC comes back: the next tick's retry succeeds, and retries then stop. + rec.failRestore = false + fc.controlLoop() + attemptsAfterRecovery := rec.restoreCount() + if !fc.isRestoreConfirmed() { + t.Fatal("restore should be confirmed once the BMC is reachable again") + } + fc.controlLoop() + fc.controlLoop() + if got := rec.restoreCount(); got != attemptsAfterRecovery { + t.Fatalf("restore retries did not stop after confirmation: %d -> %d", attemptsAfterRecovery, got) + } + if fc.GetStatus().RestorePending { + t.Fatal("restore_pending should be false after confirmation") + } +} + +func TestSetFanSpeedOnlyUpdatesCurrentSpeedOnSuccess(t *testing.T) { + rec := &cmdRecorder{failOnFanSet: true} + fc := NewFanController(testConfig(), nil, nil, nil) + fc.runCommand = rec.run + fc.currentSpeed = 25 + + if err := fc.setFanSpeed(80); err == nil { + t.Fatal("expected setFanSpeed to return the simulated failure") + } + if fc.currentSpeed != 25 { + t.Fatalf("currentSpeed updated despite failed write: got %d, want 25", fc.currentSpeed) + } + if !fc.lastWriteFailed { + t.Fatal("lastWriteFailed should be true after a failed write") + } + + rec.failOnFanSet = false + if err := fc.setFanSpeed(80); err != nil { + t.Fatalf("unexpected error on successful write: %v", err) + } + if fc.currentSpeed != 80 { + t.Fatalf("currentSpeed not updated after successful write: got %d, want 80", fc.currentSpeed) + } + if fc.lastWriteFailed { + t.Fatal("lastWriteFailed should be cleared after a successful write") + } +} diff --git a/internal/controller/mock.go b/internal/controller/mock.go index d349fc9..0327cc2 100644 --- a/internal/controller/mock.go +++ b/internal/controller/mock.go @@ -23,6 +23,7 @@ func NewMockFanController(cfg *config.Config, store *storage.Store) *MockFanCont cpuMon: nil, gpuMon: nil, store: store, + runCommand: realRunCommand, hints: make(map[string]*WorkloadHint), stopChan: make(chan struct{}), cpuHistory: make([]tempPoint, 0), @@ -84,13 +85,14 @@ func (mfc *MockFanController) mockControlLoop() { // Just update internal state (no actual IPMI commands) mfc.mu.Lock() mfc.currentSpeed = target + zone := mfc.currentZone mfc.mu.Unlock() // Store reading mfc.store.RecordReading(cpuReading.Max, gpuReading.Max, target) log.Printf("[MOCK] CPU: %d°C | GPU: %d°C | Zone: %s | Fan: %d%%", - cpuReading.Max, gpuReading.Max, mfc.currentZone, target) + cpuReading.Max, gpuReading.Max, zone, target) } func (mfc *MockFanController) generateMockCPU() *monitor.CPUReading { @@ -104,7 +106,7 @@ func (mfc *MockFanController) generateMockCPU() *monitor.CPUReading { } else { mockCPUBase -= 0.1 // Cooling } - + if mockCPUBase < 35 { mockCPUBase = 35 } diff --git a/internal/monitor/cpu.go b/internal/monitor/cpu.go index 1840f2b..cf57894 100644 --- a/internal/monitor/cpu.go +++ b/internal/monitor/cpu.go @@ -2,15 +2,37 @@ package monitor import ( "bytes" + "context" + "fmt" "log" "os/exec" "regexp" "strconv" "strings" + "time" "github.com/sethpjohnson/only-fan-controller/internal/config" ) +// commandTimeout bounds how long an external monitoring command may run. A hung +// command must never freeze the control loop, so it is capped at 10s; a 3s floor +// keeps a short monitoring interval from making slow-but-healthy remote calls +// read as timeouts. +func commandTimeout(cfg *config.Config) time.Duration { + const ( + minTimeout = 3 * time.Second + maxTimeout = 10 * time.Second + ) + interval := time.Duration(cfg.Monitoring.Interval) * time.Second + if interval > maxTimeout { + return maxTimeout + } + if interval < minTimeout { + return minTimeout + } + return interval +} + type CPUMonitor struct { cfg *config.Config } @@ -26,14 +48,17 @@ func NewCPUMonitor(cfg *config.Config) *CPUMonitor { // Read gets current CPU temperatures via IPMI func (m *CPUMonitor) Read() (*CPUReading, error) { + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout(m.cfg)) + defer cancel() + var cmd *exec.Cmd if m.cfg.IDRAC.Host == "local" { // Local IPMI access - cmd = exec.Command("ipmitool", "sdr", "type", "temperature") + cmd = exec.CommandContext(ctx, "ipmitool", "sdr", "type", "temperature") } else { // Remote IPMI access - cmd = exec.Command("ipmitool", + cmd = exec.CommandContext(ctx, "ipmitool", "-I", "lanplus", "-H", m.cfg.IDRAC.Host, "-U", m.cfg.IDRAC.Username, @@ -47,12 +72,22 @@ func (m *CPUMonitor) Read() (*CPUReading, error) { cmd.Stderr = &stderr if err := cmd.Run(); err != nil { + if ctx.Err() == context.DeadlineExceeded { + log.Printf("ipmitool CPU read timed out after %s", commandTimeout(m.cfg)) + return nil, fmt.Errorf("ipmitool CPU read timed out: %w", err) + } log.Printf("ipmitool error: %v, stderr: %s", err, stderr.String()) return nil, err } - temps := parseCPUTemps(stdout.String()) - + // Empty parse (e.g. after an iDRAC firmware update changed the output + // format) is an error, never a silent 0°C reading. + temps, err := parseCPUTemps(stdout.String()) + if err != nil { + log.Printf("CPU temperature parse failed: %v; raw output: %q", err, stdout.String()) + return nil, err + } + reading := &CPUReading{ Temps: temps, Max: maxInt(temps), @@ -63,13 +98,14 @@ func (m *CPUMonitor) Read() (*CPUReading, error) { // parseCPUTemps extracts CPU temperatures from ipmitool output // Example output from R730: -// Inlet Temp | 04h | ok | 7.1 | 20 degrees C -// Exhaust Temp | 01h | ok | 7.1 | 28 degrees C -// Temp | 0Eh | ok | 3.1 | 33 degrees C <- CPU 1 -// Temp | 0Fh | ok | 3.2 | 35 degrees C <- CPU 2 -func parseCPUTemps(output string) []int { +// +// Inlet Temp | 04h | ok | 7.1 | 20 degrees C +// Exhaust Temp | 01h | ok | 7.1 | 28 degrees C +// Temp | 0Eh | ok | 3.1 | 33 degrees C <- CPU 1 +// Temp | 0Fh | ok | 3.2 | 35 degrees C <- CPU 2 +func parseCPUTemps(output string) ([]int, error) { var temps []int - + lines := strings.Split(output, "\n") for _, line := range lines { // Skip inlet/exhaust temps, focus on CPU temps @@ -77,13 +113,13 @@ func parseCPUTemps(output string) []int { if strings.Contains(lower, "inlet") || strings.Contains(lower, "exhaust") { continue } - + // Only process lines that start with "Temp" (CPU temps on Dell servers) trimmed := strings.TrimSpace(line) if !strings.HasPrefix(trimmed, "Temp") { continue } - + // Extract temperature value: look for "NN degrees" re := regexp.MustCompile(`(\d+)\s*degrees`) matches := re.FindStringSubmatch(line) @@ -98,6 +134,11 @@ func parseCPUTemps(output string) []int { if len(temps) == 0 { re2 := regexp.MustCompile(`(\d+)\s*degrees`) for _, line := range lines { + // Still skip chassis inlet/exhaust sensors in the fallback path. + lower := strings.ToLower(line) + if strings.Contains(lower, "inlet") || strings.Contains(lower, "exhaust") { + continue + } matches := re2.FindStringSubmatch(line) if len(matches) >= 2 { if temp, err := strconv.Atoi(matches[1]); err == nil && temp > 0 && temp < 120 { @@ -107,7 +148,11 @@ func parseCPUTemps(output string) []int { } } - return temps + if len(temps) == 0 { + return nil, fmt.Errorf("no valid CPU temperatures found in ipmitool output") + } + + return temps, nil } func maxInt(vals []int) int { diff --git a/internal/monitor/cpu_test.go b/internal/monitor/cpu_test.go new file mode 100644 index 0000000..36c15eb --- /dev/null +++ b/internal/monitor/cpu_test.go @@ -0,0 +1,82 @@ +package monitor + +import "testing" + +func TestParseCPUTemps(t *testing.T) { + tests := []struct { + name string + output string + want []int + wantErr bool + }{ + { + name: "valid R730 output", + output: `Inlet Temp | 04h | ok | 7.1 | 20 degrees C +Exhaust Temp | 01h | ok | 7.1 | 28 degrees C +Temp | 0Eh | ok | 3.1 | 33 degrees C +Temp | 0Fh | ok | 3.2 | 35 degrees C`, + want: []int{33, 35}, + wantErr: false, + }, + { + name: "empty output is an error, never 0C", + output: "", + want: nil, + wantErr: true, + }, + { + name: "garbage output is an error", + output: "this is not ipmitool output at all\nrandom text", + want: nil, + wantErr: true, + }, + { + name: "only inlet and exhaust temps yields no CPU temps", + output: `Inlet Temp | 04h | ok | 7.1 | 20 degrees C +Exhaust Temp | 01h | ok | 7.1 | 28 degrees C`, + want: nil, + wantErr: true, + }, + { + name: "firmware update changed labels but temps still present via fallback", + output: `CPU1 Temp | 0Eh | ok | 3.1 | 41 degrees C +CPU2 Temp | 0Fh | ok | 3.2 | 44 degrees C`, + want: []int{41, 44}, + wantErr: false, + }, + { + name: "out-of-range readings are ignored and remaining empty is an error", + output: `Temp | 0Eh | ok | 3.1 | 0 degrees C +Temp | 0Fh | ok | 3.2 | 999 degrees C`, + want: nil, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := parseCPUTemps(tt.output) + if tt.wantErr && err == nil { + t.Fatalf("expected error, got nil (temps=%v)", got) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if !equalInts(got, tt.want) { + t.Fatalf("temps = %v, want %v", got, tt.want) + } + }) + } +} + +func equalInts(a, b []int) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} diff --git a/internal/monitor/gpu.go b/internal/monitor/gpu.go index 19f8f71..534dfb2 100644 --- a/internal/monitor/gpu.go +++ b/internal/monitor/gpu.go @@ -2,7 +2,9 @@ package monitor import ( "bytes" + "context" "encoding/csv" + "fmt" "log" "os/exec" "strconv" @@ -20,9 +22,9 @@ type GPUDevice struct { Name string `json:"name"` Temp int `json:"temp"` Utilization int `json:"utilization"` - MemoryUsed int `json:"memory_used"` // MB - MemoryTotal int `json:"memory_total"` // MB - PowerDraw int `json:"power_draw"` // Watts + MemoryUsed int `json:"memory_used"` // MB + MemoryTotal int `json:"memory_total"` // MB + PowerDraw int `json:"power_draw"` // Watts } type GPUReading struct { @@ -40,8 +42,11 @@ func (m *GPUMonitor) Read() (*GPUReading, error) { return &GPUReading{}, nil } + ctx, cancel := context.WithTimeout(context.Background(), commandTimeout(m.cfg)) + defer cancel() + // Query nvidia-smi for key metrics in CSV format - cmd := exec.Command(m.cfg.GPU.NvidiaSmiPath, + cmd := exec.CommandContext(ctx, m.cfg.GPU.NvidiaSmiPath, "--query-gpu=index,name,temperature.gpu,utilization.gpu,memory.used,memory.total,power.draw", "--format=csv,noheader,nounits", ) @@ -51,12 +56,24 @@ func (m *GPUMonitor) Read() (*GPUReading, error) { cmd.Stderr = &stderr if err := cmd.Run(); err != nil { + if ctx.Err() == context.DeadlineExceeded { + log.Printf("nvidia-smi timed out after %s", commandTimeout(m.cfg)) + return nil, fmt.Errorf("nvidia-smi timed out: %w", err) + } log.Printf("nvidia-smi error: %v, stderr: %s", err, stderr.String()) return nil, err } - devices := parseGPUOutput(stdout.String()) - + // Zero devices parsed while GPU monitoring is enabled is an error, not a + // silent 0°C reading — otherwise empty/garbage nvidia-smi output would look + // like a healthy "cold" GPU and the fail-safe would never engage. Mirrors the + // CPU treatment. + devices, err := parseGPUOutput(stdout.String()) + if err != nil { + log.Printf("GPU output parse failed: %v; raw output: %q", err, stdout.String()) + return nil, err + } + reading := &GPUReading{ Devices: devices, Max: maxGPUTemp(devices), @@ -65,15 +82,18 @@ func (m *GPUMonitor) Read() (*GPUReading, error) { return reading, nil } -// parseGPUOutput parses nvidia-smi CSV output -func parseGPUOutput(output string) []GPUDevice { +// parseGPUOutput parses nvidia-smi CSV output. It returns an error when no valid +// device rows are present. +func parseGPUOutput(output string) ([]GPUDevice, error) { var devices []GPUDevice reader := csv.NewReader(strings.NewReader(output)) + // Tolerate rows with unexpected field counts (skipped below) rather than + // failing the whole read; nvidia-smi occasionally emits ragged rows. + reader.FieldsPerRecord = -1 records, err := reader.ReadAll() if err != nil { - log.Printf("Failed to parse nvidia-smi output: %v", err) - return devices + return nil, fmt.Errorf("failed to parse nvidia-smi CSV output: %w", err) } for _, record := range records { @@ -99,7 +119,11 @@ func parseGPUOutput(output string) []GPUDevice { devices = append(devices, device) } - return devices + if len(devices) == 0 { + return nil, fmt.Errorf("no valid GPU device rows found in nvidia-smi output") + } + + return devices, nil } func parseInt(s string) int { @@ -108,12 +132,12 @@ func parseInt(s string) int { if s == "" || s == "[N/A]" || s == "N/A" { return 0 } - + // Remove any decimal part (e.g., "45.00" -> "45") if idx := strings.Index(s, "."); idx != -1 { s = s[:idx] } - + val, err := strconv.Atoi(s) if err != nil { return 0 diff --git a/internal/monitor/gpu_test.go b/internal/monitor/gpu_test.go new file mode 100644 index 0000000..0f38032 --- /dev/null +++ b/internal/monitor/gpu_test.go @@ -0,0 +1,69 @@ +package monitor + +import "testing" + +func TestParseGPUOutput(t *testing.T) { + tests := []struct { + name string + output string + wantTemps []int // temps of parsed devices, in order + wantErr bool + }{ + { + name: "valid two-GPU CSV", + output: "0, Tesla P40, 45, 80, 2048, 24576, 220\n1, Tesla P40, 47, 85, 4096, 24576, 230\n", + wantTemps: []int{45, 47}, + wantErr: false, + }, + { + name: "empty output is an error, never 0C", + output: "", + wantTemps: nil, + wantErr: true, + }, + { + name: "whitespace-only output is an error", + output: " \n\n", + wantTemps: nil, + wantErr: true, + }, + { + name: "garbage / non-CSV output is an error", + output: "nvidia-smi: command not found", + wantTemps: nil, + wantErr: true, + }, + { + name: "partial rows (too few fields) are skipped; all-skipped is an error", + output: "0, Tesla P40, 45\n1, Tesla P40\n", + wantTemps: nil, + wantErr: true, + }, + { + name: "mixed valid and partial rows keeps the valid one", + output: "0, Tesla P40, 45\n1, Tesla P40, 50, 80, 4096, 24576, 230\n", + wantTemps: []int{50}, + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + devices, err := parseGPUOutput(tt.output) + if tt.wantErr && err == nil { + t.Fatalf("expected error, got nil (devices=%+v)", devices) + } + if !tt.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(devices) != len(tt.wantTemps) { + t.Fatalf("device count = %d, want %d", len(devices), len(tt.wantTemps)) + } + for i, want := range tt.wantTemps { + if devices[i].Temp != want { + t.Fatalf("device[%d].Temp = %d, want %d", i, devices[i].Temp, want) + } + } + }) + } +}