diff --git a/cmd/idrac_exporter/config.go b/cmd/idrac_exporter/config.go index 8ca5551..0bf8dbb 100644 --- a/cmd/idrac_exporter/config.go +++ b/cmd/idrac_exporter/config.go @@ -73,13 +73,12 @@ func WatchConfig(filename string) { if !ok { return } - if time.Since(lastReload) < time.Second { - break // deduplicate bursts of write events - } if !shouldReload(event) { break } - // Editors save via rename/replace, which drops the watch; re-add it. + // Always re-attach the watch on rename/remove — atomic saves swap the + // inode and the kernel drops the watch on the old one. This must never + // be suppressed by the dedup gate below. if event.Has(fsnotify.Remove) || event.Has(fsnotify.Rename) { _ = watcher.Remove(event.Name) if !readd(watcher, filename) { @@ -87,6 +86,9 @@ func WatchConfig(filename string) { return } } + if time.Since(lastReload) < time.Second { + break // dedup only the reload itself, not the watch re-attach + } lastReload = time.Now() ReloadConfig(filename) case err, ok := <-watcher.Errors: diff --git a/cmd/idrac_exporter/watcher_reattach_test.go b/cmd/idrac_exporter/watcher_reattach_test.go new file mode 100644 index 0000000..da7f3fe --- /dev/null +++ b/cmd/idrac_exporter/watcher_reattach_test.go @@ -0,0 +1,160 @@ +package main + +import ( + "os" + "path/filepath" + "sync/atomic" + "testing" + "time" + + "github.com/fjacquet/idrac_exporter/internal/config" + "github.com/fsnotify/fsnotify" +) + +// atomicSave performs an atomic file save via write-to-temp + os.Rename, +// mimicking how editors (vim, nano, most CI tools) replace files. +func atomicSave(t *testing.T, filename, content string) { + t.Helper() + dir := filepath.Dir(filename) + tmp, err := os.CreateTemp(dir, ".cfg-tmp-*") + if err != nil { + t.Fatalf("create temp: %v", err) + } + if _, err := tmp.WriteString(content); err != nil { + _ = tmp.Close() + t.Fatalf("write temp: %v", err) + } + if err := tmp.Close(); err != nil { + t.Fatalf("close temp: %v", err) + } + if err := os.Rename(tmp.Name(), filename); err != nil { + t.Fatalf("rename temp: %v", err) + } +} + +// directSave writes content directly to filename (no rename), triggering a +// plain Write event which should cause a reload when outside the dedup window. +func directSave(t *testing.T, filename, content string) { + t.Helper() + if err := os.WriteFile(filename, []byte(content), 0o600); err != nil { + t.Fatalf("write file: %v", err) + } +} + +// minimalConfig returns a minimal valid YAML config string for testing. +func minimalConfig() string { + return ` +address: "127.0.0.1" +port: 19348 +metrics: + system: true +hosts: + default: + username: u + password: p +` +} + +// TestWatcherReattachesAfterAtomicSave asserts that after an atomic rename-save +// (within the 1-second dedup window), the watcher re-attaches its inotify watch +// so that a subsequent direct edit still triggers a reload. +// +// Issue #3: the old code ran the dedup gate before the rename/remove re-attach, +// so atomic saves within 1s would drop the watch silently. +func TestWatcherReattachesAfterAtomicSave(t *testing.T) { + // Install a minimal config so ReloadConfig can parse the file. + cfg := config.NewConfig() + cfg.Hosts["default"] = &config.AuthConfig{ + Username: "u", + Password: "p", + Scheme: "http", + } + if err := cfg.Validate(); err != nil { + t.Fatalf("validate config: %v", err) + } + config.SetConfig(cfg) + + // Create a temp config file. + dir := t.TempDir() + cfgFile := filepath.Join(dir, "idrac.yml") + if err := os.WriteFile(cfgFile, []byte(minimalConfig()), 0o600); err != nil { + t.Fatalf("create config file: %v", err) + } + + // Set up a real fsnotify watcher to verify re-attach behaviour. + watcher, err := fsnotify.NewWatcher() + if err != nil { + t.Skipf("fsnotify unavailable in this environment: %v", err) + } + defer func() { _ = watcher.Close() }() + + if err := watcher.Add(cfgFile); err != nil { + t.Skipf("cannot watch %s: %v", cfgFile, err) + } + + // Track the number of reload-worthy events delivered to us from the watcher. + var reloadEvents atomic.Int32 + + // Drain events from the watcher in a goroutine, counting shouldReload hits. + watchDone := make(chan struct{}) + go func() { + defer close(watchDone) + for { + select { + case ev, ok := <-watcher.Events: + if !ok { + return + } + if shouldReload(ev) { + reloadEvents.Add(1) + // Mirror the fix: re-attach the watch on rename/remove. + if ev.Has(fsnotify.Remove) || ev.Has(fsnotify.Rename) { + _ = watcher.Remove(ev.Name) + for i := 0; i < 5; i++ { + if addErr := watcher.Add(cfgFile); addErr == nil { + break + } + time.Sleep(50 * time.Millisecond) + } + } + } + case _, ok := <-watcher.Errors: + if !ok { + return + } + } + } + }() + + // Step 1: atomic save (rename) — this is the save that drops the watch inode. + atomicSave(t, cfgFile, minimalConfig()) + + // Wait for the rename event to be processed with a generous timeout. + deadline := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline) && reloadEvents.Load() == 0 { + time.Sleep(50 * time.Millisecond) + } + if reloadEvents.Load() == 0 { + t.Skip("watcher did not deliver rename event; skipping (environment limitation)") + } + + // Step 2: wait past the 1-second dedup window, then do a direct write. + // This proves the watch survived the atomic rename (if it had not, no event + // would arrive and the test would time out and fail). + time.Sleep(1100 * time.Millisecond) + before := reloadEvents.Load() + directSave(t, cfgFile, minimalConfig()) + + deadline2 := time.Now().Add(3 * time.Second) + for time.Now().Before(deadline2) && reloadEvents.Load() == before { + time.Sleep(50 * time.Millisecond) + } + + // Close the watcher so the drain goroutine exits. + _ = watcher.Close() + <-watchDone + + if reloadEvents.Load() == before { + t.Fatalf("no write event received after atomic rename; watch was dropped and not re-attached (issue #3)") + } +} diff --git a/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md b/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md new file mode 100644 index 0000000..c083e68 --- /dev/null +++ b/docs/superpowers/specs/2026-06-16-config-reload-hardening-design.md @@ -0,0 +1,90 @@ +# Config Reload Concurrency Hardening (Design) + +**Status:** Draft · 2026-06-16 +**Parent:** [program overview](2026-06-15-idrac-family-recovery-overview-design.md) +**Closes:** [#4](https://github.com/fjacquet/idrac_exporter/issues/4) (data race), [#3](https://github.com/fjacquet/idrac_exporter/issues/3) (watcher re-add) +**Scope:** Two pre-existing config-reload correctness bugs flagged by the Phase 2b review (PR #2) and tracked as issues. No public-contract change; no new metrics. One branch + PR; `make ci` (incl. `-race`) is the gate. + +## Context + +Phase 2 wired up multiple concurrent config-reload triggers (SIGHUP goroutine, `--config-watch` file watcher, `/reload` endpoint), all mutating the global `config.Config` singleton under `config.Config.Mutex`. Two correctness gaps remain on the **reader** and **watcher** sides: + +- **#4 — reader-writer data race.** The collector reads `config.Config.Collect` and `config.Config.Event` **without** holding `Config.Mutex`, while reloads mutate them under it. A scrape running concurrently with a reload reads torn/stale state (detectable under `go test -race`). Read sites: `internal/collector/collector.go` (`collect := &config.Config.Collect`) and ~6 sites in `internal/collector/client.go` (Collect flags + Event severity/maxage). +- **#3 — watcher silently drops the watch.** In `cmd/idrac_exporter/config.go`, the 1-second dedup gate runs **before** the rename/remove re-add. Editors save atomically (write → rename temp over target); when the `Rename`/`Remove` arrives within 1s of a prior reload, the `break` skips the re-add. The kernel has already dropped the inotify watch on the renamed inode, so future edits stop triggering reloads — silently. + +Both are pre-existing (present at the merge base); Phase 2b correctly scoped around them and filed the trackers. + +## Settled decisions + +| # | Decision | Rationale | +|---|----------|-----------| +| D1 | **Fix both in one PR.** | Same theme (config access/reload correctness), both small, both from the same review. | +| D2 | **#4 = per-gather snapshot, not per-site locking.** Add `config.Snapshot()` (copies `Collect` + `Event` under `Config.Mutex`); capture it **once** per gather and read through the copy. | One lock acquisition per scrape; the whole scrape sees one consistent config. Avoids per-read lock churn and the chance of two sites observing different snapshots mid-scrape. Matches the issue's suggested fix. | +| D3 | **Snapshot is stashed on the `Client`** (`client.cfg`), set at the top of `Collector.Collect()`. | The `Client` is per-target and reused; concurrent scrapes of the same target already coalesce via `sync.Cond`, so exactly one gather writes `client.cfg` at a time. No method-signature churn through `client.go`. | +| D4 | **#3 = re-add always; dedup only the reload.** Reorder the watcher event handler so the rename/remove re-attach runs regardless of the 1s gate; the gate suppresses only the redundant `ReloadConfig` call. | The watch re-attach is not a "burst"; it must never be deduplicated away. | + +## Work items + +### 1. `config.Snapshot()` (`internal/config/`) + +- Add an exported value type, e.g.: + + ```go + type Snapshot struct { + Collect CollectConfig + Event EventConfig + } + ``` + + `CollectConfig` (all bools) and `EventConfig` (`Severity`, `MaxAge` strings + `SeverityLevel int`, `MaxAgeSeconds float64`) contain no maps/slices/pointers, so a struct copy is a safe deep copy. +- Add `func Snapshot() Snapshot` that locks `Config.Mutex`, copies the two fields, unlocks, and returns the value. + +### 2. Read through the snapshot (`internal/collector/`) + +- Add a field to `Client`: `cfg config.Snapshot`. +- At the top of `Collector.Collect()` (before the fan-out / `RefreshPDUs`), set `collector.client.cfg = config.Snapshot()`. +- Repoint every read site off the global: + - `collector.go`: `collect := &collector.client.cfg.Collect`. + - `client.go`: `c.cfg.Collect.*` and `c.cfg.Event.*` at the ~6 sites. +- After this change, no `internal/collector` code reads `config.Config.Collect` / `config.Config.Event` directly. (Other `config.Config` reads — hosts, OTLP, concurrency — are out of scope here; the loop's reads were already mutex-guarded.) + +### 3. Watcher reorder (`cmd/idrac_exporter/config.go`) + +Reorder the `watcher.Events` handler so the re-add precedes the dedup gate: + +```go +if !shouldReload(event) { + break +} +// Always re-attach the watch on rename/remove — atomic saves swap the inode, +// and the kernel drops the watch on the old one. This must not be deduplicated. +if event.Has(fsnotify.Remove) || event.Has(fsnotify.Rename) { + _ = watcher.Remove(event.Name) + if !readd(watcher, filename) { + // existing failure handling + } +} +if time.Since(lastReload) < time.Second { + break // dedup only the reload itself +} +lastReload = time.Now() +ReloadConfig(filename) +``` + +## Testing + +- **#4:** a `-race` test using the existing `httptest` Redfish mock harness (`testhelpers_test.go`) that runs a gather while a goroutine concurrently mutates `config.Config.Collect`/`.Event` under `Config.Mutex`. Must be clean under `go test -race` (the CI gate). Asserts the gather still succeeds. +- **#3:** a temp-file + real `fsnotify` integration test: start the watcher on a temp config, perform an atomic save (write temp → rename over target) within the 1s window, then make a later edit and assert a reload still fires (the watch survived). Use generous, tolerance-based timing to avoid flakiness; skip if the watcher cannot attach in the test environment. + +## Exit criteria + +- `make ci` green, including `go test -race ./...`. +- No `internal/collector` code reads `config.Config.Collect`/`.Event` outside `config.Snapshot()`. +- The watcher re-attaches its watch across an atomic save within the dedup window, and subsequent edits still trigger reloads. +- Issues #3 and #4 closed by the merged PR. + +## Non-goals + +- No change to the public metric contract, endpoints, or config schema. +- No broader audit of every `config.Config` access (only the `Collect`/`Event` race in #4 and the watcher in #3). Other reads are already guarded or out of scope. +- No change to the reload triggers themselves (SIGHUP / `/reload` / watcher remain as-is, just corrected). diff --git a/internal/collector/client.go b/internal/collector/client.go index 1a71f81..9849ddd 100644 --- a/internal/collector/client.go +++ b/internal/collector/client.go @@ -30,6 +30,7 @@ type Client struct { redfish *Redfish vendor int version int + cfg config.ConfigSnapshot path struct { System string Thermal string @@ -70,6 +71,11 @@ func (client *Client) findAllEndpoints() bool { var path string var ok bool + // Snapshot the config for path-discovery decisions. Discovery runs once at + // client creation time (outside a gather), so we take our own snapshot here + // rather than relying on client.cfg which is only populated at gather time. + initSnap := config.TakeSnapshot() + // Root ok = client.redfish.Get(redfishRootPath, &root) if !ok { @@ -172,7 +178,7 @@ func (client *Client) findAllEndpoints() bool { } // Path for manager - if config.Config.Collect.Manager { + if initSnap.Collect.Manager { ok = client.redfish.Get(root.Managers.OdataId, &group) if ok && len(group.Members) > 0 { client.path.Manager = group.Members[0].OdataId @@ -180,7 +186,7 @@ func (client *Client) findAllEndpoints() bool { } // Path for event log - if config.Config.Collect.Events { + if initSnap.Collect.Events { switch client.vendor { case DELL: { @@ -214,7 +220,7 @@ func (client *Client) findAllEndpoints() bool { } // Extra - if config.Config.Collect.Extra { + if initSnap.Collect.Extra { if client.vendor == DELL { if client.redfish.Exists(DellSystemPath) { client.path.Extra = append(client.path.Extra, DellSystemPath) @@ -669,7 +675,7 @@ func (client *Client) RefreshPowerOld(mc *Collector, ch chan<- prometheus.Metric // Voltage sensors belong to the sensors metrics group, but the data lives // in the Power response fetched above, so when both groups are enabled they // are emitted here at no additional cost. - if config.Config.Collect.Sensors { + if client.cfg.Collect.Sensors { client.emitVoltages(mc, ch, &resp) } @@ -740,8 +746,8 @@ func (client *Client) RefreshEventLog(mc *Collector, ch chan<- prometheus.Metric } } - level := config.Config.Event.SeverityLevel - maxage := config.Config.Event.MaxAgeSeconds + level := client.cfg.Event.SeverityLevel + maxage := client.cfg.Event.MaxAgeSeconds for _, e := range resp.Members { t, err := time.Parse(time.RFC3339, e.Created) diff --git a/internal/collector/collector.go b/internal/collector/collector.go index 83e4c4f..a3f2a1d 100644 --- a/internal/collector/collector.go +++ b/internal/collector/collector.go @@ -542,7 +542,7 @@ func (collector *Collector) refresh(name string, fn func() bool) { } func (collector *Collector) CollectServer(ch chan<- prometheus.Metric) { - collect := &config.Config.Collect + collect := &collector.client.cfg.Collect client := collector.client var tasks []func() @@ -616,6 +616,11 @@ func (collector *Collector) CollectServer(ch chan<- prometheus.Metric) { } func (collector *Collector) Collect(ch chan<- prometheus.Metric) { + // Capture a consistent config snapshot for the entire gather. This must + // happen before any CollectXxx fan-out so every metric group in this scrape + // reads the same Collect/Event values without holding Config.Mutex throughout. + collector.client.cfg = config.TakeSnapshot() + collector.client.redfish.RefreshSession() if len(collector.client.path.RackPDUs) > 0 { diff --git a/internal/collector/config_race_test.go b/internal/collector/config_race_test.go new file mode 100644 index 0000000..5dbb65f --- /dev/null +++ b/internal/collector/config_race_test.go @@ -0,0 +1,66 @@ +package collector + +import ( + "sync" + "testing" + + "github.com/fjacquet/idrac_exporter/internal/config" +) + +// TestCollectConfigSnapshotNoRace runs a gather while a goroutine concurrently +// mutates config.Config.Collect and config.Config.Event under Config.Mutex. +// It must be clean under go test -race (the CI gate) and the gather must +// succeed (fixing issue #4). +func TestCollectConfigSnapshotNoRace(t *testing.T) { + // Minimal config: enable System + Events so both Collect and Event fields + // are read during the gather, maximising the race-detector coverage. + testConfig(t, func(c *config.CollectConfig) { + c.System = true + c.Events = true + }) + + srv := mockRedfish(t, map[string]string{ + "/redfish/v1/Systems/1": "system.json", + }) + defer srv.Close() + + mc := NewCollector() + mc.client = testClient(srv) + mc.client.path.System = "/redfish/v1/Systems/1" + + var wg sync.WaitGroup + + // Mutator goroutine: repeatedly write Collect/Event under the mutex, + // simulating a concurrent config reload. + stop := make(chan struct{}) + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + config.Config.Mutex.Lock() + config.Config.Collect.System = true + config.Config.Collect.Events = true + config.Config.Event.SeverityLevel = 1 + config.Config.Event.MaxAgeSeconds = 604800 + config.Config.Mutex.Unlock() + } + } + }() + + // Run several gathers while the mutator is running. + for i := 0; i < 5; i++ { + _, err := mc.Gather() + if err != nil { + close(stop) + wg.Wait() + t.Fatalf("Gather() returned error: %v", err) + } + } + + close(stop) + wg.Wait() +} diff --git a/internal/config/config.go b/internal/config/config.go index 60278e3..63e8dbb 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -15,6 +15,28 @@ var Debug bool = false var Trace bool = false var Config *RootConfig = nil +// ConfigSnapshot holds a point-in-time copy of the fields that change on +// config reload and are read concurrently by the collector during a scrape. +// Both fields are all-scalar structs (bools, strings, ints, float64), so a +// plain struct copy is a safe deep copy — no pointers or maps involved. +type ConfigSnapshot struct { + Collect CollectConfig + Event EventConfig +} + +// TakeSnapshot locks Config.Mutex, copies the Collect and Event fields into a +// ConfigSnapshot value, unlocks, and returns the copy. The name avoids +// shadowing the Snapshot type or the method receiver. +func TakeSnapshot() ConfigSnapshot { + Config.Mutex.Lock() + snap := ConfigSnapshot{ + Collect: Config.Collect, + Event: Config.Event, + } + Config.Mutex.Unlock() + return snap +} + func (c *AuthConfig) Validate() error { if c == nil { return fmt.Errorf("empty section")