From 9c943bc63db12f2030aa9c2faaaf4dbe1a4266c4 Mon Sep 17 00:00:00 2001 From: Matthew Mattox Date: Wed, 12 Aug 2026 18:16:34 -0500 Subject: [PATCH 1/2] fix(reload,health): re-initialize monitors on config change; guard probe bind ordering MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two paired fixes to the agent's startup/reload lifecycle. ## 243 — config hot-reload left running components stale Reproduced against the real rendered chart config. The monitor restart machinery itself worked, but four defects around it made ConfigMap edits untrustworthy: 1. Startup/reload normalization asymmetry (the severe one). main.go applied monitors.ApplyDefaultMonitors() + CLI overrides + ApplyDefaults() to the startup config; the reload path used a bare util.LoadConfig. The two therefore disagreed about what the configuration contained, so every auto-defaulted monitor looked REMOVED on the first reload and was silently stopped. Measured with the shipped chart: a single dns-health edit dropped the running monitor count from 10 to 1. The -debug/-dry-run/-log-* flags were likewise silently reverted by any reload. Fixed by building the normalizer once and installing it on the reload coordinator. 2. diff.RemediationChanged was computed and then ignored. Editing dryRun or maxRemediationsPerHour reported "reload succeeded" while the registry kept the startup values until a pod restart — silent staleness on the exact kill-switch operators reach for mid-incident. Added RemediatorRegistry.ApplyConfig (+ SetMaxRemediationsPerHour) and an optional ReconfigurableRemediationExecutor interface the detector now invokes. 3. Settings-only edits fell through to a cheerful "no changes" event, because ComputeConfigDiff only inspects monitors/exporters/remediation. Added reload.ClassifyReload, which reports what was reconfigured in place versus what is latched at startup and genuinely needs a rollout (nodeName, log destination, pprof, enabling a previously-disabled exporter or remediation, coordination/lease settings). Those now emit an explicit ConfigReloadRestartRequired warning event instead of a silent success. 4. reload.enabled was parsed and never read. It is now honored in both directions and defaults to enabled (*bool, nil means true, so existing deployments are unaffected); disabling it logs loudly that edits require a rollout. Also: log level/format are now re-applied on reload, and every successful reload logs a line naming which monitors were reconfigured/started/stopped. ## 246 — probe bind ordering + liveness/readiness split - Split createExporters into an explicit phase 1 (health server) / phase 2 (networked exporters) and added two regression guards, both verified to FAIL when the ordering is deliberately inverted: a behavioural test that blocks phase 2 and asserts the health socket already answers the real exec-probe code path, and an AST guard over main.go that rejects networked init ordered ahead of startHealthServer. - Liveness and readiness are now genuinely distinct. /healthz consults only process-internal state and is documented/tested to ignore downstream failures, so a broken API server can never restart the agent on the degraded nodes it exists to observe. /ready gains readiness checks plus per-exporter dependency status fed from the detector's export loop, with a 3-consecutive- failure threshold so a transient blip cannot flap the whole DaemonSet. - Confirmed the startup budget: initialDelay 10 + 20 x 5 = 110s, documented in values.yaml.template with a floor, and pinned by a chart contract test. Probes remain exec-against-unix-socket; a new chart test fails if httpGet on 8080 is ever reintroduced, since that host port conflict still exists in the fleet. Chart: values.yaml.template and configmap.yaml edited, values.yaml regenerated via make helm-generate. Verification: make build, make test, go test -race ./... , go vet ./... , make helm-lint all pass. Task: #node-doctor-243 Task: #node-doctor-246 Claude-Session: https://claude.ai/code/session_01Av4whriQf6KqJgRxGKQP7N --- cmd/node-doctor/bind_ordering_test.go | 221 ++++++++++++++ cmd/node-doctor/main.go | 217 +++++++++---- cmd/node-doctor/main_additional_test.go | 22 +- cmd/node-doctor/main_comprehensive_test.go | 2 +- config/examples/validation_test.go | 6 +- helm/node-doctor/templates/configmap.yaml | 8 + helm/node-doctor/values.yaml | 42 +++ helm/node-doctor/values.yaml.template | 42 +++ pkg/detector/detector.go | 220 ++++++++++++- pkg/detector/reload_configmap_e2e_test.go | 305 +++++++++++++++++++ pkg/detector/reload_enabled_test.go | 91 ++++++ pkg/detector/reload_integration_test.go | 6 +- pkg/detector/reload_remediation_test.go | 206 +++++++++++++ pkg/health/liveness_readiness_test.go | 260 ++++++++++++++++ pkg/health/server.go | 170 ++++++++++- pkg/reload/coordinator.go | 101 +++++- pkg/reload/normalizer_helpers_test.go | 38 +++ pkg/reload/normalizer_test.go | 246 +++++++++++++++ pkg/reload/reloadability.go | 236 ++++++++++++++ pkg/reload/reloadability_test.go | 165 ++++++++++ pkg/reload/watcher_configmap_test.go | 129 ++++++++ pkg/remediators/registry.go | 68 +++++ pkg/remediators/registry_applyconfig_test.go | 133 ++++++++ pkg/types/config.go | 28 +- test/integration/chart_probes_test.go | 210 +++++++++++++ 25 files changed, 3047 insertions(+), 125 deletions(-) create mode 100644 cmd/node-doctor/bind_ordering_test.go create mode 100644 pkg/detector/reload_configmap_e2e_test.go create mode 100644 pkg/detector/reload_enabled_test.go create mode 100644 pkg/detector/reload_remediation_test.go create mode 100644 pkg/health/liveness_readiness_test.go create mode 100644 pkg/reload/normalizer_helpers_test.go create mode 100644 pkg/reload/normalizer_test.go create mode 100644 pkg/reload/reloadability.go create mode 100644 pkg/reload/reloadability_test.go create mode 100644 pkg/reload/watcher_configmap_test.go create mode 100644 pkg/remediators/registry_applyconfig_test.go create mode 100644 test/integration/chart_probes_test.go diff --git a/cmd/node-doctor/bind_ordering_test.go b/cmd/node-doctor/bind_ordering_test.go new file mode 100644 index 0000000..c9f8d40 --- /dev/null +++ b/cmd/node-doctor/bind_ordering_test.go @@ -0,0 +1,221 @@ +package main + +import ( + "context" + "go/ast" + "go/parser" + "go/token" + "path/filepath" + "strings" + "testing" + "time" + + prometheusexporter "github.com/supporttools/node-doctor/pkg/exporters/prometheus" + "github.com/supporttools/node-doctor/pkg/types" +) + +// TestHealthEndpointServesBeforeNetworkedExporters is the behavioural +// regression guard for the ordering fix in PR #24 (#node-doctor-246). +// +// The incident: on a degraded node a networked exporter's Start() BLOCKS +// (cluster-DNS or API-server reachability, informer cache sync). Before the +// fix, the health server was created AFTER those exporters, so the probe +// listener never opened inside the kubelet's startup-probe budget → the probe +// failed → the kubelet killed the container → crashloop, on exactly the nodes +// node-doctor exists to observe (a1pinode01 crashlooped 125x). +// +// This test pins the invariant directly: while phase 2 is blocked, the health +// endpoint must ALREADY be answering probes over the per-pod unix socket. If +// anyone reorders createExporters so networked init runs first, this test hangs +// on an unservable socket and fails. +func TestHealthEndpointServesBeforeNetworkedExporters(t *testing.T) { + socket := filepath.Join(t.TempDir(), "health.sock") + + // Phase 2 blocks until we release it — standing in for a wedged exporter + // Start() on a degraded node. + release := make(chan struct{}) + entered := make(chan struct{}) + + original := startNetworkedExportersFn + startNetworkedExportersFn = func(_ context.Context, _ *types.NodeDoctorConfig) ([]ExporterLifecycle, []types.Exporter, *prometheusexporter.PrometheusExporter) { + close(entered) + <-release + return nil, nil, nil + } + t.Cleanup(func() { startNetworkedExportersFn = original }) + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + config := &types.NodeDoctorConfig{ + Exporters: types.ExporterConfigs{ + Kubernetes: &types.KubernetesExporterConfig{Enabled: true}, + }, + } + + done := make(chan struct{}) + go func() { + defer close(done) + _, _, _, _, _ = createExporters(ctx, config, nil, socket) + }() + + // Wait until phase 2 is definitely underway and stuck. + select { + case <-entered: + case <-time.After(10 * time.Second): + t.Fatal("networked exporter phase never started") + } + + // THE ASSERTION: the probe must already succeed even though the networked + // phase is wedged. runHealthCheck is the exact code path the kubelet exec + // probe runs, so this exercises the real production probe mechanism. + if code := runHealthCheck(socket, "/healthz"); code != 0 { + t.Errorf("liveness probe exit code = %d, want 0. The health server must bind BEFORE "+ + "networked exporter init; otherwise a blocked exporter on a degraded node prevents "+ + "the probe listener from ever opening and the kubelet crashloops the pod.", code) + } + + // Readiness must also be reachable (it returns 503 until a monitor reports, + // but the endpoint must be SERVING, not absent). + if code := runHealthCheck(socket, "/ready"); code != 1 { + t.Errorf("readiness probe exit code = %d, want 1 (endpoint serving, reporting NotReady "+ + "because no monitor has run yet)", code) + } + + close(release) + select { + case <-done: + case <-time.After(10 * time.Second): + t.Fatal("createExporters did not return after phase 2 was released") + } +} + +// TestCreateExportersSourceOrdering is a static lint guard over +// cmd/node-doctor/main.go. +// +// The behavioural test above proves the property for the current structure; this +// one catches a subtler regression: someone inlining a networked exporter +// constructor back into createExporters ahead of the health server, which would +// reintroduce the crashloop while potentially still passing a test that stubs +// the phase-2 seam. +func TestCreateExportersSourceOrdering(t *testing.T) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatalf("parse main.go: %v", err) + } + + fn := findFunc(file, "createExporters") + if fn == nil { + t.Fatal("createExporters not found in main.go") + } + + // Constructors/starters that touch the network during startup and can block. + networkedMarkers := []string{ + "NewKubernetesExporter", + "NewHTTPExporter", + "NewPrometheusExporter", + "startNetworkedExporters", + } + + var healthPos, firstNetworkedPos token.Pos + var firstNetworkedName string + + ast.Inspect(fn, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + name := callName(call) + if name == "" { + return true + } + + if name == "startHealthServer" && healthPos == token.NoPos { + healthPos = call.Pos() + } + for _, marker := range networkedMarkers { + if strings.Contains(name, marker) && firstNetworkedPos == token.NoPos { + firstNetworkedPos = call.Pos() + firstNetworkedName = name + } + } + return true + }) + + if healthPos == token.NoPos { + t.Fatal("createExporters must call startHealthServer — the health listener has to be bound " + + "before any networked initialization") + } + if firstNetworkedPos == token.NoPos { + t.Fatal("expected createExporters to perform networked exporter initialization") + } + + if firstNetworkedPos < healthPos { + t.Errorf("networked init %q at %s runs BEFORE the health server is started at %s. "+ + "On a degraded node that exporter's Start() can block, the probe listener never opens "+ + "within the startup-probe budget, and the kubelet crashloops the pod (#node-doctor-246). "+ + "Move the health server creation back to the top of createExporters.", + firstNetworkedName, fset.Position(firstNetworkedPos), fset.Position(healthPos)) + } +} + +// TestStartHealthServerDoesNotTouchNetworkedExporters guards the other half of +// the invariant: phase 1 must stay free of any networked exporter construction, +// or "health first" becomes meaningless. +func TestStartHealthServerDoesNotTouchNetworkedExporters(t *testing.T) { + fset := token.NewFileSet() + file, err := parser.ParseFile(fset, "main.go", nil, 0) + if err != nil { + t.Fatalf("parse main.go: %v", err) + } + + fn := findFunc(file, "startHealthServer") + if fn == nil { + t.Fatal("startHealthServer not found in main.go") + } + + forbidden := []string{"NewKubernetesExporter", "NewHTTPExporter", "NewPrometheusExporter"} + + ast.Inspect(fn, func(n ast.Node) bool { + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + name := callName(call) + for _, f := range forbidden { + if strings.Contains(name, f) { + t.Errorf("startHealthServer must not construct networked exporters, found %q at %s. "+ + "Phase 1 exists precisely to bind the probe listener before anything that can block.", + name, fset.Position(call.Pos())) + } + } + return true + }) +} + +// findFunc locates a top-level function declaration by name. +func findFunc(file *ast.File, name string) *ast.FuncDecl { + for _, decl := range file.Decls { + fn, ok := decl.(*ast.FuncDecl) + if ok && fn.Name.Name == name { + return fn + } + } + return nil +} + +// callName renders the called function's name, including a package or receiver +// qualifier when present (e.g. "health.NewServer", "healthServer.Start"). +func callName(call *ast.CallExpr) string { + switch f := call.Fun.(type) { + case *ast.Ident: + return f.Name + case *ast.SelectorExpr: + if x, ok := f.X.(*ast.Ident); ok { + return x.Name + "." + f.Sel.Name + } + return f.Sel.Name + } + return "" +} diff --git a/cmd/node-doctor/main.go b/cmd/node-doctor/main.go index 87228f1..6b39afc 100644 --- a/cmd/node-doctor/main.go +++ b/cmd/node-doctor/main.go @@ -192,33 +192,46 @@ func main() { log.Fatalf("Failed to load configuration: %v", err) } - // Apply default monitors for any missing monitor types - addedDefaults := monitors.ApplyDefaultMonitors(config) - if len(addedDefaults) > 0 { - log.Printf("[INFO] Added default configurations for monitors: %v", addedDefaults) - } + // Build the config normalizer ONCE and use it for BOTH the startup config and + // every subsequent hot reload. + // + // This symmetry is the fix for #node-doctor-243. Previously these steps ran + // only here at startup, while the reload path used a bare util.LoadConfig. + // The two therefore disagreed about what the configuration contained: + // ApplyDefaultMonitors appends an entry for every registered monitor type + // that has a default and is absent from the file, so on the first reload + // those monitors looked REMOVED and were silently stopped (with the shipped + // chart: gateway-health). The -debug/-dry-run/-log-* flags were likewise + // silently reverted by any reload. + normalizeConfig := func(c *types.NodeDoctorConfig) error { + addedDefaults := monitors.ApplyDefaultMonitors(c) + if len(addedDefaults) > 0 { + log.Printf("[INFO] Added default configurations for monitors: %v", addedDefaults) + } - // Apply command line overrides - if *debug { - config.Settings.LogLevel = "debug" - } - if *logLevel != "" { - config.Settings.LogLevel = *logLevel - } - if *logFormat != "" { - config.Settings.LogFormat = *logFormat - } - if *dryRun { - config.Settings.DryRunMode = true - config.Remediation.DryRun = true - } - if *enableProfiling { - config.Features.EnableProfiling = true - config.Features.ProfilingPort = *profilingPort + // Command line overrides always win over file contents. + if *debug { + c.Settings.LogLevel = "debug" + } + if *logLevel != "" { + c.Settings.LogLevel = *logLevel + } + if *logFormat != "" { + c.Settings.LogFormat = *logFormat + } + if *dryRun { + c.Settings.DryRunMode = true + c.Remediation.DryRun = true + } + if *enableProfiling { + c.Features.EnableProfiling = true + c.Features.ProfilingPort = *profilingPort + } + + return c.ApplyDefaults() } - // Apply defaults and validate - if err := config.ApplyDefaults(); err != nil { + if err := normalizeConfig(config); err != nil { log.Fatalf("Failed to apply configuration defaults: %v", err) } @@ -331,7 +344,7 @@ func main() { if remediatorRegistry != nil { historyProvider = &remediationHistoryAdapter{registry: remediatorRegistry} } - exporters, exporterInterfaces, promExporter, err := createExporters(ctx, config, historyProvider, *healthSocket) + exporters, exporterInterfaces, promExporter, healthServer, err := createExporters(ctx, config, historyProvider, *healthSocket) if err != nil { log.Fatalf("Failed to create exporters: %v", err) } @@ -373,6 +386,27 @@ func main() { log.Printf("[INFO] Config hot-reload self-metrics wired to Prometheus exporter") } + // Give the reload coordinator the SAME normalization the startup config got, + // so reload diffs compare like with like (#node-doctor-243). Without this the + // first ConfigMap edit silently stops every auto-defaulted monitor. + det.SetConfigNormalizer(func(c *types.NodeDoctorConfig) error { + return normalizeConfig(c) + }) + + // Allow log level/format to be changed by ConfigMap edit without a rollout. + // The log DESTINATION still requires a restart and is reported as such by + // reload.ClassifyReload. + det.SetLoggingReinit(logger.Init) + + // Wire downstream export outcomes into READINESS only. A failing exporter + // makes the pod NotReady; it must never affect /healthz, because restarting + // node-doctor cannot fix an unreachable API server and would crashloop the + // agent on exactly the degraded nodes it exists to observe (#node-doctor-246). + if healthServer != nil { + det.SetDependencyReporter(healthServer.SetDependencyStatus) + log.Printf("[INFO] Exporter health wired to readiness (/ready); liveness (/healthz) stays independent") + } + // Start the detector log.Printf("[INFO] Starting detector...") if err := det.Start(); err != nil { @@ -454,24 +488,32 @@ func (a *remediationHistoryAdapter) GetHistory(limit int) interface{} { return a.registry.GetHistory(limit) } -// createExporters creates and configures all exporters from the configuration. -// remediationProvider is optional; when non-nil it is wired to the health server -// before Start() so /remediation/history is available immediately on first request. -func createExporters(ctx context.Context, config *types.NodeDoctorConfig, remediationProvider health.RemediationHistoryProvider, healthSocketPath string) ([]ExporterLifecycle, []types.Exporter, *prometheusexporter.PrometheusExporter, error) { - var exporters []ExporterLifecycle - var exporterInterfaces []types.Exporter - // promExporterTyped keeps a typed reference to the Prometheus exporter (if one - // is created and started) so the caller can wire it as a circuit-state observer. - var promExporterTyped *prometheusexporter.PrometheusExporter +// startNetworkedExportersFn is a test seam over startNetworkedExporters. +// +// It exists so the bind-ordering regression guard can substitute a phase-2 +// implementation that BLOCKS, and then assert that the health endpoint is +// already answering probes while it blocks. That is the property PR #24 fixed +// by hand and #node-doctor-246 asked to protect: if anyone reorders +// createExporters so networked init runs before the health server binds, the +// guard test deadlocks on an unservable socket and fails. +// +// Production code never reassigns this; only tests do. +var startNetworkedExportersFn = startNetworkedExporters - // Create the Health Server FIRST so the Kubernetes startup/liveness probe - // (:8080/healthz) can bind and return 200 immediately — BEFORE the k8s/HTTP - // exporters below, whose Start() can BLOCK on a degraded node (cluster-DNS or - // API-server reachability, cache sync). If a networked exporter hangs during - // startup, the health listener would otherwise never open within the ~110s - // startup budget → probe 404 → kubelet kills the agent → crashloop, on exactly - // the nodes node-doctor exists to observe. Bind liveness before slow init - // (cluster-services #19529: a1pinode01 crashlooped 125x this way). +// startHealthServer creates and starts the health server. This is PHASE 1 of +// createExporters and MUST stay ahead of any networked exporter init. +// +// Why the ordering is load-bearing: on a degraded node the k8s/HTTP exporters' +// Start() can BLOCK (cluster-DNS or API-server reachability, informer cache +// sync). If a networked exporter hangs during startup, the health listener +// would never open within the startup-probe budget → probe fails → kubelet +// kills the agent → crashloop, on exactly the nodes node-doctor exists to +// observe (cluster-services #19529: a1pinode01 crashlooped 125x this way). +// +// Binding liveness first means the pod stays alive and merely reports NotReady +// while the downstream is broken — which is the whole point of the +// liveness/readiness split. +func startHealthServer(ctx context.Context, remediationProvider health.RemediationHistoryProvider, healthSocketPath string) (*health.Server, error) { log.Printf("[INFO] Creating health server...") healthServer, err := health.NewServer(&health.Config{ Enabled: true, @@ -480,30 +522,79 @@ func createExporters(ctx context.Context, config *types.NodeDoctorConfig, remedi BindAddress: "::", Port: 8080, // Also serve on the per-pod unix socket so Kubernetes exec probes - // (-healthcheck) reach node-doctor even when a foreign process owns - // hostPort 8080 on a hostNetwork node. + // (-healthcheck / -healthcheck-ready) reach node-doctor even when a + // foreign process owns hostPort 8080 on a hostNetwork node. SocketPath: healthSocketPath, ReadTimeout: 5 * time.Second, WriteTimeout: 10 * time.Second, }) if err != nil { - log.Printf("[WARN] Failed to create health server: %v", err) + return nil, fmt.Errorf("create health server: %w", err) + } + + // Wire the remediation history provider before Start so the endpoint is + // available immediately when the listener opens (no race window). + if remediationProvider != nil { + healthServer.SetRemediationHistory(remediationProvider) + log.Printf("[INFO] Remediation history wired to /remediation/history endpoint") + } + if err := healthServer.Start(ctx); err != nil { + return nil, fmt.Errorf("start health server: %w", err) + } + log.Printf("[INFO] Health server created and started (probes bound BEFORE networked exporter init)") + return healthServer, nil +} + +// createExporters creates and configures all exporters from the configuration. +// remediationProvider is optional; when non-nil it is wired to the health server +// before Start() so /remediation/history is available immediately on first request. +// +// ORDERING IS LOAD-BEARING. Phase 1 (health server) must complete before phase 2 +// (networked exporters). See startHealthServer for why, and +// TestHealthEndpointServesBeforeNetworkedExporters / +// TestCreateExportersSourceOrdering for the regression guards. +func createExporters(ctx context.Context, config *types.NodeDoctorConfig, remediationProvider health.RemediationHistoryProvider, healthSocketPath string) ([]ExporterLifecycle, []types.Exporter, *prometheusexporter.PrometheusExporter, *health.Server, error) { + var exporters []ExporterLifecycle + var exporterInterfaces []types.Exporter + + // ---- PHASE 1: bind liveness/readiness FIRST (never networked) ---------- + healthServer, err := startHealthServer(ctx, remediationProvider, healthSocketPath) + if err != nil { + // Non-fatal: losing the health endpoint must not prevent monitoring. + log.Printf("[WARN] %v", err) + healthServer = nil } else { - // Wire the remediation history provider before Start so the endpoint is - // available immediately when the listener opens (no race window). - if remediationProvider != nil { - healthServer.SetRemediationHistory(remediationProvider) - log.Printf("[INFO] Remediation history wired to /remediation/history endpoint") - } - if err := healthServer.Start(ctx); err != nil { - log.Printf("[WARN] Failed to start health server: %v", err) - } else { - exporters = append(exporters, healthServer) - exporterInterfaces = append(exporterInterfaces, healthServer) - log.Printf("[INFO] Health server created and started on port 8080") - } + exporters = append(exporters, healthServer) + exporterInterfaces = append(exporterInterfaces, healthServer) } + // ---- PHASE 2: networked exporters (Start() may block on a bad node) ---- + netLifecycles, netInterfaces, promExporterTyped := startNetworkedExportersFn(ctx, config) + exporters = append(exporters, netLifecycles...) + exporterInterfaces = append(exporterInterfaces, netInterfaces...) + + // If no exporters were created, use a no-op exporter to satisfy the detector requirements + if len(exporterInterfaces) == 0 { + log.Printf("[INFO] No exporters enabled, using no-op exporter") + noopExp := &noopExporter{} + exporters = append(exporters, noopExp) + exporterInterfaces = append(exporterInterfaces, noopExp) + } + + return exporters, exporterInterfaces, promExporterTyped, healthServer, nil +} + +// startNetworkedExporters creates and starts the exporters that talk to the +// network (Kubernetes API, webhooks, Prometheus listener). This is PHASE 2 of +// createExporters and must never run before startHealthServer — any Start() +// here can block for the whole startup-probe budget on a degraded node. +func startNetworkedExporters(ctx context.Context, config *types.NodeDoctorConfig) ([]ExporterLifecycle, []types.Exporter, *prometheusexporter.PrometheusExporter) { + var exporters []ExporterLifecycle + var exporterInterfaces []types.Exporter + // promExporterTyped keeps a typed reference to the Prometheus exporter (if one + // is created and started) so the caller can wire it as a circuit-state observer. + var promExporterTyped *prometheusexporter.PrometheusExporter + // Create Kubernetes exporter if enabled if config.Exporters.Kubernetes != nil && config.Exporters.Kubernetes.Enabled { log.Printf("[INFO] Creating Kubernetes exporter...") @@ -565,15 +656,7 @@ func createExporters(ctx context.Context, config *types.NodeDoctorConfig, remedi } } - // If no exporters were created, use a no-op exporter to satisfy the detector requirements - if len(exporterInterfaces) == 0 { - log.Printf("[INFO] No exporters enabled, using no-op exporter") - noopExp := &noopExporter{} - exporters = append(exporters, noopExp) - exporterInterfaces = append(exporterInterfaces, noopExp) - } - - return exporters, exporterInterfaces, promExporterTyped, nil + return exporters, exporterInterfaces, promExporterTyped } // dumpConfiguration prints the effective configuration as JSON diff --git a/cmd/node-doctor/main_additional_test.go b/cmd/node-doctor/main_additional_test.go index e1ae1c5..0926055 100644 --- a/cmd/node-doctor/main_additional_test.go +++ b/cmd/node-doctor/main_additional_test.go @@ -157,7 +157,7 @@ func TestCreateExporters_Current(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -198,7 +198,7 @@ func TestCreateExporters_Current(t *testing.T) { }, } - exporters, _, _, err := createExporters(ctx, config, nil, "") + exporters, _, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -348,7 +348,7 @@ func TestCreateExporters_HTTPExporterEnabled(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -391,7 +391,7 @@ func TestCreateExporters_PrometheusExporterEnabled(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -435,7 +435,7 @@ func TestCreateExporters_KubernetesExporterEnabled(t *testing.T) { // This should not panic even without valid kubeconfig // It will log a warning but continue - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -484,7 +484,7 @@ func TestCreateExporters_AllExportersEnabled(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -524,7 +524,7 @@ func TestCreateExporters_HealthServerCreation(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -558,7 +558,7 @@ func TestCreateExporters_NoopFallbackVerification(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -629,7 +629,7 @@ func TestCreateExporters_HTTPExporterWithValidConfig(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -677,7 +677,7 @@ func TestCreateExporters_KubernetesExporterWithValidConfig(t *testing.T) { } // This will fail without kubeconfig but should exercise the validation path - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } @@ -742,7 +742,7 @@ func TestCreateExporters_MultipleExportersWithValidConfig(t *testing.T) { }, } - exporters, interfaces, _, err := createExporters(ctx, config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, config, nil, "") if err != nil { t.Errorf("createExporters() error = %v, want nil", err) } diff --git a/cmd/node-doctor/main_comprehensive_test.go b/cmd/node-doctor/main_comprehensive_test.go index afe0419..32467e3 100644 --- a/cmd/node-doctor/main_comprehensive_test.go +++ b/cmd/node-doctor/main_comprehensive_test.go @@ -267,7 +267,7 @@ func TestCreateExporters_TableDriven(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - exporters, interfaces, _, err := createExporters(ctx, tt.config, nil, "") + exporters, interfaces, _, _, err := createExporters(ctx, tt.config, nil, "") if err != nil { t.Errorf("createExporters() error = %v", err) return diff --git a/config/examples/validation_test.go b/config/examples/validation_test.go index a9e47b4..60eac2e 100644 --- a/config/examples/validation_test.go +++ b/config/examples/validation_test.go @@ -241,8 +241,10 @@ func TestDefaultConfig(t *testing.T) { t.Error("Default config should have at least one network monitor") } - // Verify hot reload is enabled in default config - if !config.Reload.Enabled { + // Verify hot reload is enabled in default config. Reload.Enabled is a *bool + // so that "absent" (nil, meaning enabled) is distinguishable from an explicit + // false; IsEnabled() encodes that default. + if !config.Reload.IsEnabled() { t.Error("Hot reload should be enabled in default config") } } diff --git a/helm/node-doctor/templates/configmap.yaml b/helm/node-doctor/templates/configmap.yaml index f7b7dca..fc9dbbe 100644 --- a/helm/node-doctor/templates/configmap.yaml +++ b/helm/node-doctor/templates/configmap.yaml @@ -292,3 +292,11 @@ data: enableMetrics: {{ .Values.features.enableMetrics }} enableProfiling: {{ .Values.features.enableProfiling }} enableTracing: {{ .Values.features.enableTracing }} + + # Configuration hot reload. When enabled, edits to this ConfigMap are picked + # up by the running agent and affected monitors are re-initialized in place; + # the agent logs which ones. Changes that cannot be hot-applied are reported + # as a ConfigReloadRestartRequired event instead of silently doing nothing. + reload: + enabled: {{ .Values.reload.enabled }} + debounceInterval: {{ .Values.reload.debounceInterval }} diff --git a/helm/node-doctor/values.yaml b/helm/node-doctor/values.yaml index 647fd82..03587a9 100644 --- a/helm/node-doctor/values.yaml +++ b/helm/node-doctor/values.yaml @@ -242,6 +242,22 @@ clusterDnsPodProbe: # IP-float/VIP daemon on hostPort 8080) can answer an httpGet probe with a 404 and # crashloop the pod; the unix socket lives in the pod and cannot collide. # To revert to HTTP probes, replace the `exec:` block with `httpGet: {path, port}`. +# +# LIVENESS vs READINESS — these are deliberately NOT the same signal: +# +# liveness (-healthcheck -> /healthz) "is the process alive / not wedged?" +# Failing it makes the kubelet KILL the container. It therefore +# consults ONLY process-internal state and is completely independent +# of downstream health. node-doctor runs on exactly the nodes whose +# API server / DNS are broken; if a downstream failure could fail +# liveness, the agent would be restarted in a loop precisely when its +# diagnostics matter most. +# +# readiness (-healthcheck-ready -> /ready) "can the agent do its job?" +# Failing it marks the pod NotReady WITHOUT restarting it. This is +# what a downstream failure (exporter cannot reach the API server) +# drives. A dependency must fail 3 consecutive export attempts before +# it counts, so a transient blip does not flap the whole DaemonSet. probes: liveness: enabled: true @@ -261,6 +277,19 @@ probes: timeoutSeconds: 3 failureThreshold: 3 successThreshold: 1 + # STARTUP BUDGET (confirmed for #node-doctor-246): + # initialDelaySeconds 10 + (failureThreshold 20 x periodSeconds 5) = 110s + # before the kubelet gives up and kills the container. + # + # The health listener binds in PHASE 1 of createExporters, ahead of every + # networked exporter, so it answers within milliseconds of process start and + # does not consume this budget at all. The 110s therefore covers genuinely slow + # but legitimate init (config load, monitor construction) with a very wide + # margin, and still bounds a truly hung process. + # + # Do NOT lower failureThreshold below ~12 (70s) without re-checking the slowest + # observed cold start on a loaded node; that is what previously crashlooped + # a1pinode01 125x. startup: enabled: true exec: @@ -271,6 +300,19 @@ probes: failureThreshold: 20 successThreshold: 1 +# Configuration hot reload. +# When enabled (the default), the agent watches the mounted ConfigMap directory +# and re-initializes affected monitors in place on change, logging exactly which +# ones were reconfigured. Changes that CANNOT be applied to a running process +# (settings.nodeName, log destination, enabling a previously-disabled exporter or +# remediation) are reported as an explicit ConfigReloadRestartRequired event +# rather than being silently ignored. +# Set enabled: false to require a pod rollout for every configuration change; +# the agent then says so loudly at startup instead of watching silently. +reload: + enabled: true + debounceInterval: 500ms + # Resource limits and requests resources: requests: diff --git a/helm/node-doctor/values.yaml.template b/helm/node-doctor/values.yaml.template index 07d9127..d02e78f 100644 --- a/helm/node-doctor/values.yaml.template +++ b/helm/node-doctor/values.yaml.template @@ -242,6 +242,22 @@ clusterDnsPodProbe: # IP-float/VIP daemon on hostPort 8080) can answer an httpGet probe with a 404 and # crashloop the pod; the unix socket lives in the pod and cannot collide. # To revert to HTTP probes, replace the `exec:` block with `httpGet: {path, port}`. +# +# LIVENESS vs READINESS — these are deliberately NOT the same signal: +# +# liveness (-healthcheck -> /healthz) "is the process alive / not wedged?" +# Failing it makes the kubelet KILL the container. It therefore +# consults ONLY process-internal state and is completely independent +# of downstream health. node-doctor runs on exactly the nodes whose +# API server / DNS are broken; if a downstream failure could fail +# liveness, the agent would be restarted in a loop precisely when its +# diagnostics matter most. +# +# readiness (-healthcheck-ready -> /ready) "can the agent do its job?" +# Failing it marks the pod NotReady WITHOUT restarting it. This is +# what a downstream failure (exporter cannot reach the API server) +# drives. A dependency must fail 3 consecutive export attempts before +# it counts, so a transient blip does not flap the whole DaemonSet. probes: liveness: enabled: true @@ -261,6 +277,19 @@ probes: timeoutSeconds: 3 failureThreshold: 3 successThreshold: 1 + # STARTUP BUDGET (confirmed for #node-doctor-246): + # initialDelaySeconds 10 + (failureThreshold 20 x periodSeconds 5) = 110s + # before the kubelet gives up and kills the container. + # + # The health listener binds in PHASE 1 of createExporters, ahead of every + # networked exporter, so it answers within milliseconds of process start and + # does not consume this budget at all. The 110s therefore covers genuinely slow + # but legitimate init (config load, monitor construction) with a very wide + # margin, and still bounds a truly hung process. + # + # Do NOT lower failureThreshold below ~12 (70s) without re-checking the slowest + # observed cold start on a loaded node; that is what previously crashlooped + # a1pinode01 125x. startup: enabled: true exec: @@ -271,6 +300,19 @@ probes: failureThreshold: 20 successThreshold: 1 +# Configuration hot reload. +# When enabled (the default), the agent watches the mounted ConfigMap directory +# and re-initializes affected monitors in place on change, logging exactly which +# ones were reconfigured. Changes that CANNOT be applied to a running process +# (settings.nodeName, log destination, enabling a previously-disabled exporter or +# remediation) are reported as an explicit ConfigReloadRestartRequired event +# rather than being silently ignored. +# Set enabled: false to require a pod rollout for every configuration change; +# the agent then says so loudly at startup instead of watching silently. +reload: + enabled: true + debounceInterval: 500ms + # Resource limits and requests resources: requests: diff --git a/pkg/detector/detector.go b/pkg/detector/detector.go index b73eaed..215eb99 100644 --- a/pkg/detector/detector.go +++ b/pkg/detector/detector.go @@ -122,6 +122,19 @@ type ProblemDetector struct { lastStatusMu sync.RWMutex lastStatus map[string]*types.Status // monitor name -> most recent effective status (protected by lastStatusMu) dependents map[string][]string // dependency name -> monitors that depend on it (written in Start and applyConfigReload; reserved for future push model) + + // loggingReinit re-installs the structured logging handler from a new config + // during hot reload. Injected by main.go (logger.Init) so the detector does + // not depend on the logger package's startup wiring. Nil disables log + // level/format hot reload. + loggingReinit func(*types.NodeDoctorConfig) error + + // dependencyReporter, when set, is notified of every export attempt's + // outcome (nil error = healthy). It backs the READINESS signal: a node-doctor + // that cannot reach its exporters cannot do its job and must report NotReady, + // while remaining LIVE (see cmd/node-doctor and pkg/health). Nil disables + // reporting. + dependencyReporter func(name string, err error) } // MonitorFactory interface for creating monitor instances during hot reload @@ -142,6 +155,21 @@ type RemediationExecutor interface { IsDryRun() bool } +// ReconfigurableRemediationExecutor is the optional interface a +// RemediationExecutor implements when it can adopt a new remediation +// configuration in place, without a process restart. +// +// The detector type-asserts for it during config hot reload. Executors that do +// NOT implement it are left untouched and the reload is reported honestly as +// requiring a restart, rather than being silently ignored — which is exactly +// what used to happen: diff.RemediationChanged was computed and then dropped on +// the floor, so a mid-incident ConfigMap edit flipping dryRun never took effect. +type ReconfigurableRemediationExecutor interface { + // ApplyConfig adopts cfg. dryRunMode is the effective process-wide dry-run + // flag, which the executor should OR with cfg.DryRun. + ApplyConfig(cfg *types.RemediationConfig, dryRunMode bool) error +} + // NewProblemDetector creates a new problem detector with the given configuration. // registry is an optional MonitorRegistryValidator; when non-nil, startup validation // also checks that all configured monitor types are registered before the detector @@ -240,6 +268,40 @@ func (pd *ProblemDetector) SetReloadMetricsRecorder(recorder reload.ReloadMetric } } +// SetConfigNormalizer installs the post-load config normalization hook on the +// reload coordinator. main.go passes a closure that applies registry default +// monitors, the command-line overrides and ApplyDefaults — i.e. exactly what it +// did to the startup config — so that reload diffs compare like with like. +// +// Without it the first hot reload silently STOPS every monitor that +// ApplyDefaultMonitors had auto-added at startup, because they are absent from +// the file and therefore look "removed". Nil-safe. +func (pd *ProblemDetector) SetConfigNormalizer(n reload.ConfigNormalizer) { + pd.mu.Lock() + defer pd.mu.Unlock() + if pd.reloadCoordinator != nil { + pd.reloadCoordinator.SetConfigNormalizer(n) + } +} + +// SetLoggingReinit installs the hook used to re-apply log level/format on config +// hot reload. Nil-safe; a nil hook leaves logging untouched across reloads. +func (pd *ProblemDetector) SetLoggingReinit(fn func(*types.NodeDoctorConfig) error) { + pd.mu.Lock() + defer pd.mu.Unlock() + pd.loggingReinit = fn +} + +// SetDependencyReporter installs the hook notified of each export attempt's +// outcome. It exists so downstream (exporter) failures can drive READINESS +// without ever affecting LIVENESS: a wedged API server should make the pod +// NotReady, never restart it. Nil-safe. +func (pd *ProblemDetector) SetDependencyReporter(fn func(name string, err error)) { + pd.mu.Lock() + defer pd.mu.Unlock() + pd.dependencyReporter = fn +} + // IsRunning returns true if the detector is currently running func (pd *ProblemDetector) IsRunning() bool { pd.mu.RLock() @@ -268,19 +330,33 @@ func (pd *ProblemDetector) Start() error { log.Printf("[INFO] Starting problem detector...") - // Start config watcher - configChangeCh, err := pd.configWatcher.Start(pd.ctx) - if err != nil { - return fmt.Errorf("failed to start config watcher: %w", err) + // Start config watcher, unless hot reload was explicitly disabled. + // + // reload.enabled used to be parsed and then never read: the watcher started + // unconditionally, so an operator who set it to false got no change in + // behaviour and no warning. Now the knob is real in BOTH directions — when + // it is off we say loudly that ConfigMap edits will not be picked up, so + // nobody is left waiting for a hot change that is never coming. + if pd.config.Reload.IsEnabled() { + configChangeCh, err := pd.configWatcher.Start(pd.ctx) + if err != nil { + return fmt.Errorf("failed to start config watcher: %w", err) + } + pd.configChangeCh = configChangeCh + + // Start watching for config changes + pd.wg.Add(1) + go func() { + defer pd.wg.Done() + pd.watchConfigChanges() + }() + log.Printf("[INFO] Config hot-reload ENABLED (watching %s, debounce=%v)", + pd.configFilePath, pd.config.Reload.DebounceInterval) + } else { + log.Printf("[WARN] Config hot-reload is DISABLED (reload.enabled=false): edits to %s will NOT be "+ + "picked up by this process — a pod restart/rollout is required for any configuration change", + pd.configFilePath) } - pd.configChangeCh = configChangeCh - - // Start watching for config changes - pd.wg.Add(1) - go func() { - defer pd.wg.Done() - pd.watchConfigChanges() - }() // Build a set of already-registered monitor names to prevent duplicate starts. // passedMonitors are started first (highest priority), then config-derived monitors @@ -476,13 +552,25 @@ func (pd *ProblemDetector) processStatus(status *types.Status) { // Export to all exporters (single path - Status contains all data) // Note: Previously this also called ExportProblem() for converted problems, // causing duplicate Kubernetes resources. See GitHub issue #7. + pd.mu.RLock() + depReporter := pd.dependencyReporter + pd.mu.RUnlock() + for _, exporter := range pd.exporters { - if err := exporter.ExportStatus(pd.ctx, status); err != nil { + err := exporter.ExportStatus(pd.ctx, status) + if err != nil { slog.Warn("failed to export status", "monitor", status.Source, "error", err) pd.stats.IncrementExportsFailed() } else { pd.stats.IncrementExportsSucceeded() } + // Report the outcome so a persistently failing exporter can drive + // READINESS (NotReady) without ever touching LIVENESS. The health + // server is itself an exporter; reporting its own status is harmless + // (it always succeeds) and keeps this loop uniform. + if depReporter != nil { + depReporter("exporter/"+pd.getExporterType(exporter), err) + } } // Evaluate remediation candidates for unhealthy conditions @@ -761,6 +849,13 @@ func (pd *ProblemDetector) applyConfigReload(ctx context.Context, newConfig *typ var errors []error var criticalErrors []error + // Names of the monitors this reload actually re-initialized, so the summary + // log line can name them. An operator who patches the ConfigMap needs to be + // able to confirm from the logs that THEIR monitor was rebuilt — "reload + // succeeded" alone does not distinguish "applied" from "silently skipped". + var stoppedMonitors, reconfiguredMonitors, startedMonitors []string + var remediationReconfigured bool + // Step 1: Stop monitors that were removed and cleanup their conditions log.Printf("[INFO] Stopping %d removed monitors", len(diff.MonitorsRemoved)) for _, removedConfig := range diff.MonitorsRemoved { @@ -769,6 +864,7 @@ func (pd *ProblemDetector) applyConfigReload(ctx context.Context, newConfig *typ errors = append(errors, fmt.Errorf("failed to stop monitor %s: %w", removedConfig.Name, err)) // Stopping monitors is not critical - continue } + stoppedMonitors = append(stoppedMonitors, removedConfig.Name) // Clean up conditions associated with this monitor type pd.cleanupMonitorConditions(removedConfig.Type) @@ -814,6 +910,8 @@ func (pd *ProblemDetector) applyConfigReload(ctx context.Context, newConfig *typ continue } + reconfiguredMonitors = append(reconfiguredMonitors, newConfig.Name) + // Update the reverse-dependency index: remove stale entries from the old // DependsOn list, then add entries for the new DependsOn list. for _, dep := range modifiedChange.Old.DependsOn { @@ -854,6 +952,8 @@ func (pd *ProblemDetector) applyConfigReload(ctx context.Context, newConfig *typ continue } + startedMonitors = append(startedMonitors, addedConfig.Name) + // Update the reverse-dependency index for the newly started monitor. for _, dep := range addedConfig.DependsOn { pd.dependents[dep] = append(pd.dependents[dep], addedConfig.Name) @@ -876,6 +976,45 @@ func (pd *ProblemDetector) applyConfigReload(ctx context.Context, newConfig *typ } } + // Step 4b: Re-initialize the remediator registry with the new remediation + // settings. Before this existed the diff flag was computed and ignored, so a + // ConfigMap edit to dryRun / maxRemediationsPerHour / the circuit breaker + // reported success while the running registry kept the startup values until + // someone restarted the pod. That is the silent-staleness bug #node-doctor-243 + // was filed for; remediation is the worst place to have it, because dryRun is + // the kill-switch operators reach for during an incident. + if diff.RemediationChanged { + pd.mu.RLock() + executor := pd.remediatorRegistry + pd.mu.RUnlock() + + switch { + case executor == nil: + // Remediation was disabled at startup, so no registry exists to + // reconfigure. Say so rather than implying the change landed. + log.Printf("[WARN] Remediation configuration changed but remediation was not wired at startup; " + + "a pod restart is required for remediation settings to take effect") + default: + reconfigurable, ok := executor.(ReconfigurableRemediationExecutor) + if !ok { + log.Printf("[WARN] Remediation configuration changed but the active remediation executor (%T) "+ + "does not support in-place reconfiguration; a pod restart is required", executor) + break + } + dryRunMode := newConfig.Settings.DryRunMode + if err := reconfigurable.ApplyConfig(&newConfig.Remediation, dryRunMode); err != nil { + log.Printf("[ERROR] Failed to apply remediation configuration: %v", err) + criticalErrors = append(criticalErrors, + fmt.Errorf("critical: failed to apply remediation configuration: %w", err)) + } else { + remediationReconfigured = true + log.Printf("[INFO] Remediation configuration reloaded in place (dryRun=%v maxPerHour=%d maxPerMinute=%d)", + executor.IsDryRun(), newConfig.Remediation.MaxRemediationsPerHour, + newConfig.Remediation.MaxRemediationsPerMinute) + } + } + } + // Step 5: Update configuration ONLY if no critical errors if len(criticalErrors) > 0 { log.Printf("[ERROR] Configuration reload failed with %d critical errors", len(criticalErrors)) @@ -895,9 +1034,24 @@ func (pd *ProblemDetector) applyConfigReload(ctx context.Context, newConfig *typ // Only update config if reload was fully successful pd.mu.Lock() + oldConfig := pd.config pd.config = newConfig pd.mu.Unlock() + // Re-apply structured logging settings. Level and format CAN be adopted by a + // running process (the slog handler is simply replaced); the destination + // cannot, which reload.ClassifyReload reports as restart-required. Bumping + // the log level via ConfigMap is a routine incident action, so it must not + // silently no-op. + pd.reapplyLoggingConfig(oldConfig, newConfig) + + // Name exactly what was re-initialized. This is the line an operator greps + // for after `kubectl patch cm node-doctor-config` to confirm the edit + // actually reached the running monitors. + log.Printf("[INFO] Config reload applied: monitors reconfigured=%v started=%v stopped=%v; remediation reconfigured=%v; exporters reconfigured=%v", + orEmpty(reconfiguredMonitors), orEmpty(startedMonitors), orEmpty(stoppedMonitors), + remediationReconfigured, diff.ExportersChanged) + // Report any non-critical warnings if len(errors) > 0 { log.Printf("[WARN] Configuration reload succeeded with %d warnings", len(errors)) @@ -909,6 +1063,46 @@ func (pd *ProblemDetector) applyConfigReload(ctx context.Context, newConfig *typ return nil } +// orEmpty renders a nil slice as an empty list so the reload summary log line +// reads "[]" rather than "[]" vs "" depending on whether anything changed. +func orEmpty(s []string) []string { + if s == nil { + return []string{} + } + return s +} + +// reapplyLoggingConfig re-installs the structured logging handler when the log +// LEVEL or FORMAT changed. The log DESTINATION (logOutput/logFile) is opened +// once at startup and is deliberately not touched here — reload.ClassifyReload +// reports a destination change as restart-required so the operator is told, +// rather than being left to wonder why their new log file stayed empty. +func (pd *ProblemDetector) reapplyLoggingConfig(oldConfig, newConfig *types.NodeDoctorConfig) { + if oldConfig == nil || newConfig == nil { + return + } + levelChanged := oldConfig.Settings.LogLevel != newConfig.Settings.LogLevel + formatChanged := oldConfig.Settings.LogFormat != newConfig.Settings.LogFormat + if !levelChanged && !formatChanged { + return + } + // Only safe to re-init when the destination is unchanged; otherwise a new + // file handle would be opened behind the operator's back. + if oldConfig.Settings.LogOutput != newConfig.Settings.LogOutput || + oldConfig.Settings.LogFile != newConfig.Settings.LogFile { + return + } + if pd.loggingReinit == nil { + return + } + if err := pd.loggingReinit(newConfig); err != nil { + log.Printf("[WARN] Failed to re-apply logging configuration on reload: %v", err) + return + } + log.Printf("[INFO] Logging configuration reloaded (level=%s format=%s)", + newConfig.Settings.LogLevel, newConfig.Settings.LogFormat) +} + // getExporterType returns a string representation of the exporter type func (pd *ProblemDetector) getExporterType(exporter types.Exporter) string { exporterType := reflect.TypeOf(exporter) diff --git a/pkg/detector/reload_configmap_e2e_test.go b/pkg/detector/reload_configmap_e2e_test.go new file mode 100644 index 0000000..2b41df0 --- /dev/null +++ b/pkg/detector/reload_configmap_e2e_test.go @@ -0,0 +1,305 @@ +package detector + +import ( + "context" + "fmt" + "os" + "path/filepath" + "sync" + "testing" + "time" + + "github.com/supporttools/node-doctor/pkg/monitors" + "github.com/supporttools/node-doctor/pkg/reload" + "github.com/supporttools/node-doctor/pkg/types" + "github.com/supporttools/node-doctor/pkg/util" +) + +// recordingFactory captures the MonitorConfig used to build each monitor, so a +// test can assert what the RUNNING monitor was actually configured with — as +// opposed to what is merely sitting in the file on disk. That distinction is +// the whole of #node-doctor-243. +type recordingFactory struct { + mu sync.Mutex + created []types.MonitorConfig +} + +func (f *recordingFactory) CreateMonitor(config types.MonitorConfig) (types.Monitor, error) { + f.mu.Lock() + f.created = append(f.created, config) + f.mu.Unlock() + return NewMockMonitor(config.Name), nil +} + +// effectiveConfigFor returns the config of the most recent build of the named +// monitor — i.e. the config the running instance is actually using. +func (f *recordingFactory) effectiveConfigFor(name string) (types.MonitorConfig, bool) { + f.mu.Lock() + defer f.mu.Unlock() + for i := len(f.created) - 1; i >= 0; i-- { + if f.created[i].Name == name { + return f.created[i], true + } + } + return types.MonitorConfig{}, false +} + +func (f *recordingFactory) buildCount(name string) int { + f.mu.Lock() + defer f.mu.Unlock() + n := 0 + for _, c := range f.created { + if c.Name == name { + n++ + } + } + return n +} + +// dnsConfigYAML renders a config whose dns-health monitor carries the given +// clusterDomains value — the exact field edited during the incident. +func dnsConfigYAML(clusterDomains string) string { + return fmt.Sprintf(`apiVersion: v1 +kind: NodeDoctorConfig +metadata: + name: node-doctor +settings: + nodeName: "test-node" +monitors: + - name: dns-health + type: network-dns-check + enabled: true + interval: 30s + timeout: 10s + config: + clusterDomains: %s + externalDomains: + - google.com +exporters: + prometheus: + enabled: true +reload: + enabled: true + debounceInterval: 50ms +`, clusterDomains) +} + +// writeConfigMapUpdate emulates the kubelet's atomic ConfigMap writer: a new +// timestamped data directory, then a rename(2) of the ..data symlink onto it. +func writeConfigMapUpdate(t *testing.T, dir, timestamp, content string) { + t.Helper() + + dataDir := filepath.Join(dir, ".."+timestamp) + if err := os.MkdirAll(dataDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "config.yaml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + tmpLink := filepath.Join(dir, "..data_tmp") + _ = os.Remove(tmpLink) + if err := os.Symlink(".."+timestamp, tmpLink); err != nil { + t.Fatal(err) + } + if err := os.Rename(tmpLink, filepath.Join(dir, "..data")); err != nil { + t.Fatal(err) + } + + link := filepath.Join(dir, "config.yaml") + if _, err := os.Lstat(link); os.IsNotExist(err) { + if err := os.Symlink("..data/config.yaml", link); err != nil { + t.Fatal(err) + } + } +} + +// TestConfigMapEditReinitializesRunningMonitor is the end-to-end reproduction of +// the reported incident (#node-doctor-243), driven entirely through the real +// machinery: a Kubernetes-style ConfigMap symlink swap, the fsnotify watcher, +// the reload coordinator, and the detector's monitor re-initialization. +// +// The reported symptom was that after patching the ConfigMap the file on the pod +// showed the NEW value while the running DNS monitor kept its OLD behaviour and +// kept firing, only taking effect after a manual pod restart. This test fails if +// the running monitor is not rebuilt from the new config. +func TestConfigMapEditReinitializesRunningMonitor(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + + dir := t.TempDir() + // Start with the cluster-domain probe ENABLED (the pre-incident state). + writeConfigMapUpdate(t, dir, "2026_08_12_00_00_00.111111", + dnsConfigYAML(`["kubernetes.default.svc.cluster.local"]`)) + configPath := filepath.Join(dir, "config.yaml") + + // Startup sequence, mirroring main.go. + normalize := func(c *types.NodeDoctorConfig) error { + monitors.ApplyDefaultMonitors(c) + return c.ApplyDefaults() + } + cfg, err := util.LoadConfig(configPath) + if err != nil { + t.Fatalf("load config: %v", err) + } + if err := normalize(cfg); err != nil { + t.Fatalf("normalize: %v", err) + } + + factory := &recordingFactory{} + det, err := NewProblemDetector(cfg, nil, + []types.Exporter{NewMockExporter("test-exporter")}, configPath, factory, nil) + if err != nil { + t.Fatalf("new detector: %v", err) + } + det.SetConfigNormalizer(func(c *types.NodeDoctorConfig) error { return normalize(c) }) + + if err := det.Start(); err != nil { + t.Fatalf("start detector: %v", err) + } + defer func() { _ = det.Stop() }() + + // The running monitor starts with the OLD cluster domain. + initial, ok := factory.effectiveConfigFor("dns-health") + if !ok { + t.Fatal("dns-health monitor was never built at startup") + } + if got := fmt.Sprint(initial.Config["clusterDomains"]); got != "[kubernetes.default.svc.cluster.local]" { + t.Fatalf("startup clusterDomains = %v, want the configured cluster domain", initial.Config["clusterDomains"]) + } + buildsBefore := factory.buildCount("dns-health") + + det.handlesMu.Lock() + monitorsBefore := len(det.monitorHandles) + det.handlesMu.Unlock() + + // --- THE INCIDENT ACTION: patch the ConfigMap to disable cluster probes --- + writeConfigMapUpdate(t, dir, "2026_08_12_00_00_30.222222", dnsConfigYAML(`[]`)) + _ = os.RemoveAll(filepath.Join(dir, "..2026_08_12_00_00_00.111111")) + + // Wait for the watcher + coordinator to re-initialize the monitor. + deadline := time.Now().Add(10 * time.Second) + for time.Now().Before(deadline) { + if factory.buildCount("dns-health") > buildsBefore { + break + } + time.Sleep(50 * time.Millisecond) + } + + if factory.buildCount("dns-health") <= buildsBefore { + t.Fatal("the dns-health monitor was NEVER rebuilt after the ConfigMap edit. " + + "The file on disk shows the new value while the running monitor keeps the old " + + "behaviour — exactly the silent staleness #node-doctor-243 reported.") + } + + effective, _ := factory.effectiveConfigFor("dns-health") + got := fmt.Sprint(effective.Config["clusterDomains"]) + if got != "[]" { + t.Errorf("running monitor's effective clusterDomains = %v, want [] — the monitor was "+ + "rebuilt but not from the NEW config", effective.Config["clusterDomains"]) + } + + // Applying the operator's one-monitor edit must not take any OTHER monitor + // down with it. Without the startup/reload normalization symmetry, every + // monitor that ApplyDefaultMonitors auto-added looks "removed" on reload and + // is silently stopped. + det.handlesMu.Lock() + monitorsAfter := len(det.monitorHandles) + det.handlesMu.Unlock() + + if monitorsAfter != monitorsBefore { + t.Errorf("running monitor count changed from %d to %d after editing a single monitor. "+ + "The reload path and the startup path disagree about the config, so auto-defaulted "+ + "monitors were silently stopped.", monitorsBefore, monitorsAfter) + } +} + +// TestConfigMapEditDoesNotStopUnrelatedDefaultMonitors is the companion guard +// for the normalization asymmetry: an edit to ONE monitor must not collaterally +// stop the monitors that ApplyDefaultMonitors auto-added at startup. +func TestConfigMapEditDoesNotStopUnrelatedDefaultMonitors(t *testing.T) { + if testing.Short() { + t.Skip("skipping integration test in short mode") + } + + dir := t.TempDir() + writeConfigMapUpdate(t, dir, "2026_08_12_00_00_00.111111", + dnsConfigYAML(`["kubernetes.default.svc.cluster.local"]`)) + configPath := filepath.Join(dir, "config.yaml") + + normalize := func(c *types.NodeDoctorConfig) error { + monitors.ApplyDefaultMonitors(c) + return c.ApplyDefaults() + } + cfg, err := util.LoadConfig(configPath) + if err != nil { + t.Fatalf("load config: %v", err) + } + if err := normalize(cfg); err != nil { + t.Fatalf("normalize: %v", err) + } + + // Auto-defaulted monitors are what silently disappeared on the first reload. + if len(cfg.Monitors) < 2 { + t.Skip("registry contributed no default monitors; nothing to protect here") + } + startupMonitorCount := len(cfg.Monitors) + + factory := &recordingFactory{} + det, err := NewProblemDetector(cfg, nil, + []types.Exporter{NewMockExporter("test-exporter")}, configPath, factory, nil) + if err != nil { + t.Fatalf("new detector: %v", err) + } + det.SetConfigNormalizer(func(c *types.NodeDoctorConfig) error { return normalize(c) }) + + if err := det.Start(); err != nil { + t.Fatalf("start detector: %v", err) + } + defer func() { _ = det.Stop() }() + + det.handlesMu.Lock() + monitorsBefore := len(det.monitorHandles) + det.handlesMu.Unlock() + + // Edit ONLY dns-health. + newCfg, err := util.LoadConfig(configPath) + if err != nil { + t.Fatal(err) + } + _ = normalize(newCfg) + editedRaw := dnsConfigYAML(`[]`) + tmpPath := filepath.Join(t.TempDir(), "edited.yaml") + if err := os.WriteFile(tmpPath, []byte(editedRaw), 0o644); err != nil { + t.Fatal(err) + } + edited, err := util.LoadConfig(tmpPath) + if err != nil { + t.Fatal(err) + } + if err := normalize(edited); err != nil { + t.Fatal(err) + } + + diff := reload.ComputeConfigDiff(cfg, edited) + if len(diff.MonitorsRemoved) != 0 { + t.Fatalf("editing one monitor must not mark others removed, got %d removed. "+ + "This is the asymmetry that silently stopped auto-defaulted monitors.", + len(diff.MonitorsRemoved)) + } + + if err := det.applyConfigReload(context.Background(), edited, diff); err != nil { + t.Fatalf("applyConfigReload: %v", err) + } + + det.handlesMu.Lock() + monitorsAfter := len(det.monitorHandles) + det.handlesMu.Unlock() + + if monitorsAfter != monitorsBefore { + t.Errorf("monitor count changed from %d to %d after editing a single monitor "+ + "(startup config had %d monitors). Auto-defaulted monitors were silently stopped.", + monitorsBefore, monitorsAfter, startupMonitorCount) + } +} diff --git a/pkg/detector/reload_enabled_test.go b/pkg/detector/reload_enabled_test.go new file mode 100644 index 0000000..cf86bb2 --- /dev/null +++ b/pkg/detector/reload_enabled_test.go @@ -0,0 +1,91 @@ +package detector + +import ( + "testing" + "time" + + "github.com/supporttools/node-doctor/pkg/types" +) + +// TestReloadEnabledKnobIsHonored guards that reload.enabled actually does +// something in BOTH directions. +// +// The field was previously parsed and then never read: the config watcher +// started unconditionally, so an operator who set reload.enabled=false got no +// behaviour change and no warning. A knob that silently does nothing is the +// same class of bug as config that silently goes stale. +func TestReloadEnabledKnobIsHonored(t *testing.T) { + tests := []struct { + name string + enabled *bool + wantWatcher bool + }{ + {"explicitly enabled", boolPtr(true), true}, + {"explicitly disabled", boolPtr(false), false}, + // Absent means enabled, preserving historical behaviour for every + // existing deployment whose ConfigMap has no reload section. + {"absent defaults to enabled", nil, true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := CreateTestConfigWithMonitors([]types.MonitorConfig{ + { + Name: "test-monitor", + Type: "mock", + Enabled: true, + IntervalString: "30s", + TimeoutString: "10s", + }, + }) + cfg.Reload = types.ReloadConfig{ + Enabled: tt.enabled, + DebounceInterval: 50 * time.Millisecond, + } + + helper := NewReloadTestHelper(t) + helper.Setup(t, cfg) + t.Cleanup(func() { _ = helper.detector.Stop() }) + + if err := helper.detector.Start(); err != nil { + t.Fatalf("start detector: %v", err) + } + + gotWatcher := helper.detector.configChangeCh != nil + if gotWatcher != tt.wantWatcher { + t.Errorf("reload.enabled=%v: watcher running = %v, want %v", + tt.enabled, gotWatcher, tt.wantWatcher) + } + }) + } +} + +// TestReloadConfigIsEnabledDefault pins the nil-means-true semantics that every +// existing deployment relies on. +func TestReloadConfigIsEnabledDefault(t *testing.T) { + var nilCfg *types.ReloadConfig + if !nilCfg.IsEnabled() { + t.Error("a nil ReloadConfig must report enabled (historical default)") + } + + cfg := &types.ReloadConfig{} + if !cfg.IsEnabled() { + t.Error("an unset Enabled must report enabled (historical default)") + } + + if err := cfg.ApplyDefaults(); err != nil { + t.Fatal(err) + } + if cfg.Enabled == nil || !*cfg.Enabled { + t.Error("ApplyDefaults must materialize Enabled=true when unset") + } + + off := false + explicit := &types.ReloadConfig{Enabled: &off} + if err := explicit.ApplyDefaults(); err != nil { + t.Fatal(err) + } + if explicit.IsEnabled() { + t.Error("ApplyDefaults must not override an explicit false") + } +} diff --git a/pkg/detector/reload_integration_test.go b/pkg/detector/reload_integration_test.go index 6104db3..17c32d9 100644 --- a/pkg/detector/reload_integration_test.go +++ b/pkg/detector/reload_integration_test.go @@ -21,6 +21,10 @@ import ( _ "github.com/supporttools/node-doctor/pkg/monitors/system" ) +// boolPtr returns a pointer to b, for the *bool fields (types.ReloadConfig.Enabled) +// that use nil to mean "unset, take the default". +func boolPtr(b bool) *bool { return &b } + // ReloadTestHelper provides utilities for integration testing config reload functionality type ReloadTestHelper struct { configFile string @@ -223,7 +227,7 @@ func CreateTestConfigWithMonitors(monitors []types.MonitorConfig) *types.NodeDoc }, }, Reload: types.ReloadConfig{ - Enabled: true, + Enabled: boolPtr(true), DebounceInterval: 100 * time.Millisecond, }, } diff --git a/pkg/detector/reload_remediation_test.go b/pkg/detector/reload_remediation_test.go new file mode 100644 index 0000000..a0a547d --- /dev/null +++ b/pkg/detector/reload_remediation_test.go @@ -0,0 +1,206 @@ +package detector + +import ( + "context" + "errors" + "sync" + "testing" + + "github.com/supporttools/node-doctor/pkg/reload" + "github.com/supporttools/node-doctor/pkg/types" +) + +var errApplyBoom = errors.New("apply failed") + +// createTestConfigForReload builds a config with remediation enabled so that +// remediation-diff behaviour can be exercised. +func createTestConfigForReload() *types.NodeDoctorConfig { + cfg := CreateTestConfigWithMonitors([]types.MonitorConfig{ + { + Name: "test-monitor", + Type: "mock", + Enabled: true, + IntervalString: "30s", + TimeoutString: "10s", + }, + }) + cfg.Remediation = types.RemediationConfig{ + Enabled: true, + DryRun: false, + MaxRemediationsPerHour: 10, + } + _ = cfg.ApplyDefaults() + return cfg +} + +// reconfigurableExecutor is a RemediationExecutor that also implements +// ReconfigurableRemediationExecutor, recording every ApplyConfig call. +type reconfigurableExecutor struct { + mu sync.Mutex + applied []types.RemediationConfig + dryRuns []bool + dryRun bool + applyErr error +} + +func (e *reconfigurableExecutor) Remediate(_ context.Context, _ string, _ types.Problem) error { + return nil +} +func (e *reconfigurableExecutor) RemediateWithStrategies(_ context.Context, _ []string, _ types.Problem) error { + return nil +} +func (e *reconfigurableExecutor) IsDryRun() bool { + e.mu.Lock() + defer e.mu.Unlock() + return e.dryRun +} +func (e *reconfigurableExecutor) ApplyConfig(cfg *types.RemediationConfig, dryRunMode bool) error { + e.mu.Lock() + defer e.mu.Unlock() + if e.applyErr != nil { + return e.applyErr + } + e.applied = append(e.applied, *cfg) + e.dryRuns = append(e.dryRuns, dryRunMode) + e.dryRun = cfg.DryRun || dryRunMode + return nil +} +func (e *reconfigurableExecutor) appliedCount() int { + e.mu.Lock() + defer e.mu.Unlock() + return len(e.applied) +} +func (e *reconfigurableExecutor) lastApplied() types.RemediationConfig { + e.mu.Lock() + defer e.mu.Unlock() + return e.applied[len(e.applied)-1] +} + +// plainExecutor implements only RemediationExecutor — no in-place reconfiguration. +type plainExecutor struct{ dryRun bool } + +func (e *plainExecutor) Remediate(_ context.Context, _ string, _ types.Problem) error { return nil } +func (e *plainExecutor) RemediateWithStrategies(_ context.Context, _ []string, _ types.Problem) error { + return nil +} +func (e *plainExecutor) IsDryRun() bool { return e.dryRun } + +// remediationReloadDetector builds a started detector wired to the given executor. +func remediationReloadDetector(t *testing.T, executor RemediationExecutor) (*ProblemDetector, *types.NodeDoctorConfig) { + t.Helper() + + cfg := createTestConfigForReload() + helper := NewReloadTestHelper(t) + helper.Setup(t, cfg) + t.Cleanup(func() { _ = helper.detector.Stop() }) + + helper.detector.SetRemediatorRegistry(executor) + if err := helper.detector.Start(); err != nil { + t.Fatalf("start detector: %v", err) + } + return helper.detector, cfg +} + +// TestRemediationConfigIsAppliedOnReload is the core #node-doctor-243 guard. +// +// diff.RemediationChanged was computed and then dropped on the floor: an +// operator who edited the ConfigMap mid-incident to flip dryRun on, or to lower +// maxRemediationsPerHour, got a "reload succeeded" event while the registry kept +// remediating under the OLD settings until the pod was restarted. +func TestRemediationConfigIsAppliedOnReload(t *testing.T) { + executor := &reconfigurableExecutor{} + det, cfg := remediationReloadDetector(t, executor) + + newConfig := createTestConfigForReload() + newConfig.Remediation.Enabled = true + newConfig.Remediation.DryRun = true // the incident kill-switch + newConfig.Remediation.MaxRemediationsPerHour = 2 + + diff := reload.ComputeConfigDiff(cfg, newConfig) + if !diff.RemediationChanged { + t.Fatalf("test setup: expected the remediation diff to be flagged, got %+v", diff) + } + + if err := det.applyConfigReload(context.Background(), newConfig, diff); err != nil { + t.Fatalf("applyConfigReload: %v", err) + } + + if executor.appliedCount() != 1 { + t.Fatalf("remediation config change must be applied to the running executor exactly once, got %d calls. "+ + "A ConfigMap edit to dryRun/rate limits would otherwise be silently ignored until a pod restart.", + executor.appliedCount()) + } + applied := executor.lastApplied() + if !applied.DryRun { + t.Error("the new dryRun value must reach the executor") + } + if applied.MaxRemediationsPerHour != 2 { + t.Errorf("the new maxRemediationsPerHour must reach the executor, got %d", applied.MaxRemediationsPerHour) + } + if !executor.IsDryRun() { + t.Error("the executor must actually be in dry-run mode after the reload") + } +} + +// TestRemediationConfigNotAppliedWhenUnchanged avoids pointless churn: an +// unrelated edit must not reconfigure the remediator. +func TestRemediationConfigNotAppliedWhenUnchanged(t *testing.T) { + executor := &reconfigurableExecutor{} + det, cfg := remediationReloadDetector(t, executor) + + newConfig := createTestConfigForReload() + // Identical remediation block; only a monitor differs. + newConfig.Monitors[0].IntervalString = "45s" + if err := newConfig.ApplyDefaults(); err != nil { + t.Fatal(err) + } + + diff := reload.ComputeConfigDiff(cfg, newConfig) + if diff.RemediationChanged { + t.Skip("test setup: remediation unexpectedly differs") + } + + if err := det.applyConfigReload(context.Background(), newConfig, diff); err != nil { + t.Fatalf("applyConfigReload: %v", err) + } + if executor.appliedCount() != 0 { + t.Errorf("remediation must not be reconfigured when its config did not change, got %d calls", + executor.appliedCount()) + } +} + +// TestRemediationReloadFailureIsCritical ensures a failed in-place +// reconfiguration aborts the reload rather than half-applying it. +func TestRemediationReloadFailureIsCritical(t *testing.T) { + executor := &reconfigurableExecutor{applyErr: errApplyBoom} + det, cfg := remediationReloadDetector(t, executor) + + newConfig := createTestConfigForReload() + newConfig.Remediation.Enabled = true + newConfig.Remediation.DryRun = true + newConfig.Remediation.MaxRemediationsPerHour = 2 + + diff := reload.ComputeConfigDiff(cfg, newConfig) + err := det.applyConfigReload(context.Background(), newConfig, diff) + if err == nil { + t.Fatal("a failed remediation reconfiguration must fail the reload, not be swallowed") + } +} + +// TestRemediationReloadWithNonReconfigurableExecutorDoesNotPanic covers the +// honest-degradation path: an executor that cannot be reconfigured in place is +// left alone and the operator is warned (rather than the reload exploding or +// silently claiming success). +func TestRemediationReloadWithNonReconfigurableExecutorDoesNotPanic(t *testing.T) { + det, cfg := remediationReloadDetector(t, &plainExecutor{}) + + newConfig := createTestConfigForReload() + newConfig.Remediation.Enabled = true + newConfig.Remediation.DryRun = true + newConfig.Remediation.MaxRemediationsPerHour = 2 + + diff := reload.ComputeConfigDiff(cfg, newConfig) + if err := det.applyConfigReload(context.Background(), newConfig, diff); err != nil { + t.Fatalf("a non-reconfigurable executor must not fail the reload: %v", err) + } +} diff --git a/pkg/health/liveness_readiness_test.go b/pkg/health/liveness_readiness_test.go new file mode 100644 index 0000000..1384310 --- /dev/null +++ b/pkg/health/liveness_readiness_test.go @@ -0,0 +1,260 @@ +package health + +import ( + "context" + "encoding/json" + "errors" + "net" + "net/http" + "path/filepath" + "testing" + "time" + + "github.com/supporttools/node-doctor/pkg/types" +) + +// newSocketServer starts a health server on a per-pod unix socket only (Port 0 +// still binds an ephemeral TCP listener, which is harmless) and returns an HTTP +// client wired to the socket — the same transport the `-healthcheck` and +// `-healthcheck-ready` exec probes use in production. +func newSocketServer(t *testing.T) (*Server, *http.Client) { + t.Helper() + + socket := filepath.Join(t.TempDir(), "health.sock") + srv, err := NewServer(&Config{ + Enabled: true, + BindAddress: "127.0.0.1", + Port: 0, + SocketPath: socket, + ReadTimeout: 2 * time.Second, + WriteTimeout: 2 * time.Second, + }) + if err != nil { + t.Fatalf("NewServer: %v", err) + } + if err := srv.Start(context.Background()); err != nil { + t.Fatalf("Start: %v", err) + } + t.Cleanup(func() { _ = srv.Stop() }) + + client := &http.Client{ + Timeout: 3 * time.Second, + Transport: &http.Transport{ + DialContext: func(ctx context.Context, _, _ string) (net.Conn, error) { + var d net.Dialer + return d.DialContext(ctx, "unix", socket) + }, + }, + } + return srv, client +} + +func probe(t *testing.T, client *http.Client, path string) int { + t.Helper() + resp, err := client.Get("http://localhost" + path) + if err != nil { + t.Fatalf("probe %s: %v", path, err) + } + defer func() { _ = resp.Body.Close() }() + return resp.StatusCode +} + +func probeBody(t *testing.T, client *http.Client, path string) (int, ReadinessResponse) { + t.Helper() + resp, err := client.Get("http://localhost" + path) + if err != nil { + t.Fatalf("probe %s: %v", path, err) + } + defer func() { _ = resp.Body.Close() }() + var out ReadinessResponse + _ = json.NewDecoder(resp.Body).Decode(&out) + return resp.StatusCode, out +} + +// TestLivenessIgnoresDownstreamFailures is the central #node-doctor-246 guard. +// +// node-doctor runs on exactly the nodes whose API server / DNS / network are +// broken. If a downstream failure could fail LIVENESS, the kubelet would kill +// and restart the agent forever on those nodes — losing the diagnostics at the +// precise moment they matter. Downstream failure must produce NotReady, never a +// restart. +func TestLivenessIgnoresDownstreamFailures(t *testing.T) { + srv, client := newSocketServer(t) + + // Get the agent to a fully healthy, ready baseline. + srv.UpdateStatus(&types.Status{Source: "test-monitor"}) + if code := probe(t, client, "/healthz"); code != http.StatusOK { + t.Fatalf("baseline liveness = %d, want 200", code) + } + if code := probe(t, client, "/ready"); code != http.StatusOK { + t.Fatalf("baseline readiness = %d, want 200", code) + } + + // Now every downstream the agent depends on breaks, persistently. + failDependency(srv, "exporter/kubernetes", errors.New("connection refused: apiserver unreachable")) + failDependency(srv, "exporter/http", errors.New("webhook timeout")) + + // LIVENESS must be untouched — a restart cannot fix an unreachable apiserver. + if code := probe(t, client, "/healthz"); code != http.StatusOK { + t.Errorf("liveness = %d, want 200. A downstream exporter failure must NEVER trip liveness: "+ + "the kubelet would restart node-doctor in a loop on exactly the degraded nodes it exists to observe.", code) + } + + // READINESS must reflect it. + code, body := probeBody(t, client, "/ready") + if code != http.StatusServiceUnavailable { + t.Errorf("readiness = %d, want 503 when a downstream exporter is failing", code) + } + if body.Ready { + t.Error("readiness body must report ready=false on downstream failure") + } + if body.Message == "" { + t.Error("readiness body should name the failing dependency for operators") + } +} + +// TestReadinessRecoversWhenDependencyRecovers ensures the NotReady state is not +// sticky — the pod must return to service once the downstream heals. +func TestReadinessRecoversWhenDependencyRecovers(t *testing.T) { + srv, client := newSocketServer(t) + srv.UpdateStatus(&types.Status{Source: "test-monitor"}) + + failDependency(srv, "exporter/kubernetes", errors.New("apiserver down")) + if code := probe(t, client, "/ready"); code != http.StatusServiceUnavailable { + t.Fatalf("readiness = %d, want 503 while the dependency is down", code) + } + + srv.SetDependencyStatus("exporter/kubernetes", nil) + if code := probe(t, client, "/ready"); code != http.StatusOK { + t.Errorf("readiness = %d, want 200 after the dependency recovered", code) + } +} + +// TestReadinessFalseUntilFirstMonitorStatus preserves the existing contract: +// the agent is not ready until it has actually produced a monitor status. +func TestReadinessFalseUntilFirstMonitorStatus(t *testing.T) { + srv, client := newSocketServer(t) + + if code := probe(t, client, "/ready"); code != http.StatusServiceUnavailable { + t.Errorf("readiness = %d, want 503 before any monitor has run", code) + } + // But the process is alive and must not be restarted while it starts up. + if code := probe(t, client, "/healthz"); code != http.StatusOK { + t.Errorf("liveness = %d, want 200 during startup — a slow start must not be a restart", code) + } + + srv.UpdateStatus(&types.Status{Source: "test-monitor"}) + if code := probe(t, client, "/ready"); code != http.StatusOK { + t.Errorf("readiness = %d, want 200 after the first monitor status", code) + } +} + +// TestReadinessChecksAffectOnlyReadiness covers the AddReadinessCheck path. +func TestReadinessChecksAffectOnlyReadiness(t *testing.T) { + srv, client := newSocketServer(t) + srv.UpdateStatus(&types.Status{Source: "test-monitor"}) + + failing := true + srv.AddReadinessCheck("cluster-reachable", func() error { + if failing { + return errors.New("cannot reach cluster") + } + return nil + }) + + if code := probe(t, client, "/ready"); code != http.StatusServiceUnavailable { + t.Errorf("readiness = %d, want 503 when a readiness check fails", code) + } + if code := probe(t, client, "/healthz"); code != http.StatusOK { + t.Errorf("liveness = %d, want 200 — readiness checks must not affect liveness", code) + } + + failing = false + if code := probe(t, client, "/ready"); code != http.StatusOK { + t.Errorf("readiness = %d, want 200 once the readiness check passes", code) + } +} + +// TestLivenessCanStillFailOnProcessInternalCheck confirms liveness is not +// hard-wired to 200: a genuinely wedged process must still be restartable. +func TestLivenessCanStillFailOnProcessInternalCheck(t *testing.T) { + srv, client := newSocketServer(t) + + srv.AddHealthCheck("status-processor", func() error { + return errors.New("status processing goroutine is wedged") + }) + + if code := probe(t, client, "/healthz"); code != http.StatusServiceUnavailable { + t.Errorf("liveness = %d, want 503 when a process-internal check fails — "+ + "an actually-wedged process must still be restartable", code) + } +} + +// TestSetHealthyDrivesLiveness covers the explicit liveness setter. +func TestSetHealthyDrivesLiveness(t *testing.T) { + srv, client := newSocketServer(t) + + if code := probe(t, client, "/healthz"); code != http.StatusOK { + t.Fatalf("baseline liveness = %d, want 200", code) + } + srv.SetHealthy(false) + if code := probe(t, client, "/healthz"); code != http.StatusServiceUnavailable { + t.Errorf("liveness = %d, want 503 after SetHealthy(false)", code) + } +} + +// TestDependencyStatusIsIdempotent guards the map bookkeeping: however many +// failures pile up, ONE success must fully resolve the dependency. +func TestDependencyStatusIsIdempotent(t *testing.T) { + srv, client := newSocketServer(t) + srv.UpdateStatus(&types.Status{Source: "m"}) + + for i := 0; i < 10; i++ { + srv.SetDependencyStatus("exporter/kubernetes", errors.New("down")) + } + if code := probe(t, client, "/ready"); code != http.StatusServiceUnavailable { + t.Fatalf("readiness = %d, want 503", code) + } + + srv.SetDependencyStatus("exporter/kubernetes", nil) + if code := probe(t, client, "/ready"); code != http.StatusOK { + t.Errorf("readiness = %d, want 200 after a single successful export cleared the dependency", code) + } +} + +// TestTransientDependencyBlipDoesNotFlipReadiness pins the hysteresis. +// +// node-doctor is a DaemonSet: if one transient export error flipped every pod +// to NotReady, a blip that has already healed would stall rolling updates +// fleet-wide. A failure must be SUSTAINED before it counts. +func TestTransientDependencyBlipDoesNotFlipReadiness(t *testing.T) { + srv, client := newSocketServer(t) + srv.UpdateStatus(&types.Status{Source: "m"}) + + // A single blip, below the threshold. + srv.SetDependencyStatus("exporter/kubernetes", errors.New("transient timeout")) + if code := probe(t, client, "/ready"); code != http.StatusOK { + t.Errorf("readiness = %d, want 200: a single transient export failure must not "+ + "make the pod NotReady and stall DaemonSet rollouts fleet-wide", code) + } + + // Recovery resets the counter, so a later blip also does not trip it. + srv.SetDependencyStatus("exporter/kubernetes", nil) + srv.SetDependencyStatus("exporter/kubernetes", errors.New("another blip")) + if code := probe(t, client, "/ready"); code != http.StatusOK { + t.Errorf("readiness = %d, want 200: a success must reset the consecutive-failure count", code) + } + + // Sustained failure DOES trip it. + failDependency(srv, "exporter/kubernetes", errors.New("apiserver really is down")) + if code := probe(t, client, "/ready"); code != http.StatusServiceUnavailable { + t.Errorf("readiness = %d, want 503 once the failure is sustained", code) + } +} + +// failDependency reports enough consecutive failures to cross the threshold. +func failDependency(srv *Server, name string, err error) { + for i := 0; i < dependencyFailureThreshold; i++ { + srv.SetDependencyStatus(name, err) + } +} diff --git a/pkg/health/server.go b/pkg/health/server.go index 9de8daf..c949372 100644 --- a/pkg/health/server.go +++ b/pkg/health/server.go @@ -11,7 +11,9 @@ import ( "net/http" "os" "path/filepath" + "sort" "strconv" + "strings" "sync" "time" @@ -33,6 +35,9 @@ type Server struct { lastUpdate time.Time startTime time.Time healthChecks []HealthCheck + readinessChecks []HealthCheck + dependencies map[string]string // dependency name -> error text, only once past the failure threshold + dependencyFailures map[string]int // dependency name -> consecutive failure count remediationHistory RemediationHistoryProvider } @@ -175,13 +180,16 @@ func NewServer(config *Config) (*Server, error) { } server := &Server{ - config: config, - socketPath: config.SocketPath, - started: false, - healthy: true, - ready: false, - startTime: time.Now(), - healthChecks: make([]HealthCheck, 0), + config: config, + socketPath: config.SocketPath, + started: false, + healthy: true, + ready: false, + startTime: time.Now(), + healthChecks: make([]HealthCheck, 0), + readinessChecks: make([]HealthCheck, 0), + dependencies: make(map[string]string), + dependencyFailures: make(map[string]int), } return server, nil @@ -351,13 +359,101 @@ func (s *Server) SetReady(ready bool) { s.ready = ready } -// AddHealthCheck adds a custom health check. +// AddHealthCheck adds a custom LIVENESS check. +// +// LIVENESS vs READINESS — read this before adding a check here: +// +// /healthz (liveness) answers "is this process still functioning, or is it +// wedged and in need of a restart?". A failing liveness probe makes the kubelet +// KILL the container. Therefore a liveness check may ONLY inspect +// process-internal state (a deadlocked goroutine, an exhausted worker pool). +// +// It must NEVER depend on something outside the process — the API server, +// cluster DNS, a webhook endpoint. node-doctor runs on exactly the degraded +// nodes where those are broken; wiring a downstream dependency into liveness +// converts "the node is unhealthy" into "restart node-doctor forever", which is +// the crashloop class that #node-doctor-246 exists to prevent. +// +// For downstream dependencies use AddReadinessCheck or SetDependencyStatus +// instead: those make the pod NotReady (traffic/roll-out gating) without ever +// restarting it. func (s *Server) AddHealthCheck(name string, check func() error) { s.mu.Lock() defer s.mu.Unlock() s.healthChecks = append(s.healthChecks, HealthCheck{Name: name, Check: check}) } +// AddReadinessCheck adds a custom READINESS check. +// +// Readiness answers "can this agent currently do its job?". A failing readiness +// check marks the pod NotReady but never restarts it, which is the correct +// response to a broken downstream (API server unreachable, exporter failing). +// See AddHealthCheck for the liveness counterpart and why the two must not be +// conflated. +func (s *Server) AddReadinessCheck(name string, check func() error) { + s.mu.Lock() + defer s.mu.Unlock() + s.readinessChecks = append(s.readinessChecks, HealthCheck{Name: name, Check: check}) +} + +// dependencyFailureThreshold is the number of CONSECUTIVE failed reports a +// downstream dependency must accumulate before it is allowed to make the pod +// NotReady. +// +// Hysteresis matters here because node-doctor is a DaemonSet: a single +// transient export error flipping every pod to NotReady would stall rolling +// updates fleet-wide for a blip that has already healed. One success resets the +// counter, so a genuinely broken downstream still trips within a few cycles +// (and the kubelet's own readiness failureThreshold adds a second layer). +const dependencyFailureThreshold = 3 + +// SetDependencyStatus records the latest outcome for a named downstream +// dependency. A nil err marks it healthy and immediately clears any accumulated +// failures; a non-nil err increments its consecutive-failure count, and once +// that reaches dependencyFailureThreshold the dependency makes /ready return +// 503 until it recovers. +// +// This affects READINESS ONLY. /healthz deliberately ignores dependency state +// entirely — a downstream failure must never restart the process. See +// AddHealthCheck. +func (s *Server) SetDependencyStatus(name string, err error) { + s.mu.Lock() + defer s.mu.Unlock() + + if s.dependencies == nil { + s.dependencies = make(map[string]string) + } + if s.dependencyFailures == nil { + s.dependencyFailures = make(map[string]int) + } + + if err == nil { + delete(s.dependencies, name) + delete(s.dependencyFailures, name) + return + } + + s.dependencyFailures[name]++ + if s.dependencyFailures[name] >= dependencyFailureThreshold { + s.dependencies[name] = err.Error() + } +} + +// failingDependencies returns the sorted names of dependencies that have +// exceeded the consecutive-failure threshold. Caller must hold at least a read +// lock. +func (s *Server) failingDependencies() []string { + if len(s.dependencies) == 0 { + return nil + } + names := make([]string, 0, len(s.dependencies)) + for name := range s.dependencies { + names = append(names, name) + } + sort.Strings(names) + return names +} + // SetRemediationHistory sets the remediation history provider for the /remediation/history endpoint. func (s *Server) SetRemediationHistory(provider RemediationHistoryProvider) { s.mu.Lock() @@ -365,12 +461,25 @@ func (s *Server) SetRemediationHistory(provider RemediationHistoryProvider) { s.remediationHistory = provider } -// handleHealthz handles the /healthz endpoint (liveness probe). +// handleHealthz handles the /healthz endpoint (LIVENESS probe). +// +// Liveness == "the process is alive and not wedged". A 503 here causes the +// kubelet to KILL and restart the container, so this handler deliberately +// consults ONLY process-internal state: +// +// - s.healthy, set explicitly via SetHealthy +// - s.healthChecks, which are documented as process-internal only +// +// It must NOT consult s.ready, s.readinessChecks or s.dependencies. Downstream +// failures (API server unreachable, exporter erroring) belong to /ready: they +// make the pod NotReady, never restart it. Restarting node-doctor because the +// node it is diagnosing is broken is the exact crashloop this split prevents +// (#node-doctor-246). TestLivenessIgnoresDownstreamFailures locks this in. func (s *Server) handleHealthz(w http.ResponseWriter, r *http.Request) { s.mu.RLock() defer s.mu.RUnlock() - // Run all health checks + // Run all LIVENESS checks checks := make([]Check, 0, len(s.healthChecks)) allHealthy := s.healthy @@ -405,7 +514,19 @@ func (s *Server) handleHealthz(w http.ResponseWriter, r *http.Request) { _ = json.NewEncoder(w).Encode(response) } -// handleReady handles the /ready endpoint (readiness probe). +// handleReady handles the /ready endpoint (READINESS probe). +// +// Readiness == "this agent can currently do its job". Unlike /healthz, a 503 +// here only marks the pod NotReady — it never restarts the container. That is +// the correct response to a downstream failure, so this handler DOES consult: +// +// - s.ready: at least one monitor has produced a status +// - s.readinessChecks: caller-registered "can I work?" predicates +// - s.dependencies: per-exporter export outcomes reported by the detector +// +// Reached over the same per-pod unix socket as liveness via the +// `-healthcheck-ready` exec probe, so it is immune to a foreign host process +// squatting hostPort 8080 on a hostNetwork node. func (s *Server) handleReady(w http.ResponseWriter, r *http.Request) { s.mu.RLock() defer s.mu.RUnlock() @@ -415,11 +536,34 @@ func (s *Server) handleReady(w http.ResponseWriter, r *http.Request) { Timestamp: time.Now(), } - if !s.ready { + switch { + case !s.ready: response.Message = "Not ready: monitors not yet initialized" + default: + // Downstream dependency failures reported by the detector. + if failing := s.failingDependencies(); len(failing) > 0 { + response.Ready = false + response.Message = "Not ready: downstream dependency failure: " + strings.Join(failing, ", ") + break + } + // Caller-registered readiness predicates. + var failed []string + for _, rc := range s.readinessChecks { + if err := rc.Check(); err != nil { + failed = append(failed, rc.Name+": "+err.Error()) + } + } + if len(failed) > 0 { + response.Ready = false + response.Message = "Not ready: " + strings.Join(failed, "; ") + break + } + response.Message = "Ready" + } + + if !response.Ready { w.WriteHeader(http.StatusServiceUnavailable) } else { - response.Message = "Ready" w.WriteHeader(http.StatusOK) } diff --git a/pkg/reload/coordinator.go b/pkg/reload/coordinator.go index b1a4c3e..5dfa2fe 100644 --- a/pkg/reload/coordinator.go +++ b/pkg/reload/coordinator.go @@ -3,6 +3,7 @@ package reload import ( "context" "fmt" + "strings" "sync" "time" @@ -26,16 +27,33 @@ type EventEmitter func(severity types.EventSeverity, reason, message string) // imports the prometheus exporter, avoiding coupling/cycles. type ReloadMetricsRecorder func(success bool, duration time.Duration) +// ConfigNormalizer applies the SAME post-load normalization the process applied +// to its startup configuration — registry default monitors, command-line +// overrides, and ApplyDefaults — to a freshly-loaded config. +// +// Without it the reload path and the startup path disagree about what the +// configuration IS, and the resulting diff is garbage. Concretely: main.go +// calls monitors.ApplyDefaultMonitors() at startup, which appends a monitor +// entry for every registered type that has a default and is absent from the +// file. util.LoadConfig does not do this, so the reloaded config was missing +// those monitors, ComputeConfigDiff reported them as REMOVED, and the very +// first ConfigMap edit silently stopped auto-defaulted monitors that the +// operator never touched (with the shipped chart: gateway-health). Likewise the +// -debug/-dry-run/-log-level flags were silently reverted on any reload. +type ConfigNormalizer func(*types.NodeDoctorConfig) error + // ReloadCoordinator orchestrates configuration reload operations. type ReloadCoordinator struct { - configPath string - currentConfig *types.NodeDoctorConfig - reloadCallback ReloadCallback - eventEmitter EventEmitter - metricsRecorder ReloadMetricsRecorder - validator *ConfigValidator - mu sync.Mutex - reloadInProgress bool + configPath string + currentConfig *types.NodeDoctorConfig + reloadCallback ReloadCallback + eventEmitter EventEmitter + metricsRecorder ReloadMetricsRecorder + normalizer ConfigNormalizer + validator *ConfigValidator + mu sync.Mutex + reloadInProgress bool + lastReloadability *Reloadability } // NewReloadCoordinator creates a new reload coordinator. @@ -118,6 +136,17 @@ func (rc *ReloadCoordinator) performReload(ctx context.Context) (err error) { return fmt.Errorf("failed to load config: %w", err) } + // Step 1b: Normalize exactly as startup did (default monitors, CLI + // overrides, ApplyDefaults). Skipping this makes the diff compare a + // normalized old config against a raw new one — see ConfigNormalizer. + if rc.normalizer != nil { + if err = rc.normalizer(newConfig); err != nil { + rc.emitEvent(types.EventWarning, "ConfigReloadFailed", + fmt.Sprintf("Failed to normalize configuration: %v", err)) + return fmt.Errorf("failed to normalize config: %w", err) + } + } + // Step 2: Validate new configuration if rc.validator != nil { validationResult := rc.validator.Validate(newConfig) @@ -133,15 +162,37 @@ func (rc *ReloadCoordinator) performReload(ctx context.Context) (err error) { "Configuration validation completed successfully") } - // Step 3: Compute diff + // Step 3: Compute diff and classify what can actually be applied. rc.mu.Lock() - diff := ComputeConfigDiff(rc.currentConfig, newConfig) + oldConfig := rc.currentConfig + diff := ComputeConfigDiff(oldConfig, newConfig) + reloadability := ClassifyReload(oldConfig, newConfig, diff) + rc.lastReloadability = reloadability rc.mu.Unlock() - // Step 4: Check if there are any changes + // Always surface changes that cannot be hot-applied, EVEN when there is + // nothing hot-reloadable to do. ComputeConfigDiff only looks at monitors, + // exporters and remediation, so a settings-only edit (e.g. settings.logFile) + // used to fall through to "no changes" — the operator saw a success event + // while the process kept running the old value. Say so out loud instead. + if reloadability.HasRestartRequired() { + rc.emitEvent(types.EventWarning, "ConfigReloadRestartRequired", + fmt.Sprintf("Configuration changed in %d way(s) that CANNOT be applied to the running process; "+ + "a pod restart/rollout is required for these to take effect: %s", + len(reloadability.RestartRequired), strings.Join(reloadability.RestartRequired, "; "))) + } + + // Step 4: Check if there are any hot-applicable changes if !diff.HasChanges() { - rc.emitEvent(types.EventInfo, "ConfigReloadNoChanges", - "Configuration reload completed with no changes") + if !reloadability.HasRestartRequired() { + rc.emitEvent(types.EventInfo, "ConfigReloadNoChanges", + "Configuration reload completed with no changes") + } + // Adopt the new config as current so the next diff is computed against + // what is actually on disk rather than re-reporting the same delta. + rc.mu.Lock() + rc.currentConfig = newConfig + rc.mu.Unlock() return nil } @@ -157,14 +208,34 @@ func (rc *ReloadCoordinator) performReload(ctx context.Context) (err error) { rc.currentConfig = newConfig rc.mu.Unlock() - // Emit success event with statistics + // Emit success event with statistics. The Reloadability summary names the + // individual monitors that were reconfigured/started/stopped so an operator + // can confirm from the event stream that their ConfigMap edit landed. duration := time.Since(startTime) - stats := rc.buildReloadStats(diff, duration) + stats := rc.buildReloadStats(diff, duration) + " " + reloadability.Summary() rc.emitEvent(types.EventInfo, "ConfigReloadSucceeded", stats) return nil } +// GetLastReloadability returns the classification produced by the most recent +// reload attempt, or nil if no reload has run yet. +func (rc *ReloadCoordinator) GetLastReloadability() *Reloadability { + rc.mu.Lock() + defer rc.mu.Unlock() + return rc.lastReloadability +} + +// SetConfigNormalizer installs the post-load normalization hook. It must apply +// the same transformations the process applied to its startup config; see +// ConfigNormalizer. Passing nil disables normalization (the historical, buggy +// behaviour) and is only appropriate in tests that construct configs directly. +func (rc *ReloadCoordinator) SetConfigNormalizer(n ConfigNormalizer) { + rc.mu.Lock() + defer rc.mu.Unlock() + rc.normalizer = n +} + // buildReloadStats creates a summary message of what was reloaded. func (rc *ReloadCoordinator) buildReloadStats(diff *ConfigDiff, duration time.Duration) string { msg := fmt.Sprintf("Configuration reload completed in %v. ", duration.Round(time.Millisecond)) diff --git a/pkg/reload/normalizer_helpers_test.go b/pkg/reload/normalizer_helpers_test.go new file mode 100644 index 0000000..329668e --- /dev/null +++ b/pkg/reload/normalizer_helpers_test.go @@ -0,0 +1,38 @@ +package reload + +import ( + "errors" + "fmt" + "testing" + + "github.com/supporttools/node-doctor/pkg/monitors" + "github.com/supporttools/node-doctor/pkg/types" + "github.com/supporttools/node-doctor/pkg/util" +) + +var errBoom = errors.New("boom") + +func sprint(v interface{}) string { return fmt.Sprint(v) } + +// normalizeForTest mirrors the normalizer main.go installs: registry default +// monitors, then ApplyDefaults. It deliberately has the same shape as the +// production closure so these tests exercise the real symmetry requirement. +func normalizeForTest(c *types.NodeDoctorConfig, applyDefaultMonitors bool) error { + if applyDefaultMonitors { + monitors.ApplyDefaultMonitors(c) + } + return c.ApplyDefaults() +} + +// loadAndNormalize performs the startup sequence: load the file, then normalize. +func loadAndNormalize(t *testing.T, path string, applyDefaultMonitors bool) *types.NodeDoctorConfig { + t.Helper() + cfg, err := util.LoadConfig(path) + if err != nil { + t.Fatalf("load config: %v", err) + } + if err := normalizeForTest(cfg, applyDefaultMonitors); err != nil { + t.Fatalf("normalize config: %v", err) + } + return cfg +} diff --git a/pkg/reload/normalizer_test.go b/pkg/reload/normalizer_test.go new file mode 100644 index 0000000..763daf8 --- /dev/null +++ b/pkg/reload/normalizer_test.go @@ -0,0 +1,246 @@ +package reload + +import ( + "context" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/supporttools/node-doctor/pkg/types" + + // Blank imports register the real monitor types so the validator accepts them. + _ "github.com/supporttools/node-doctor/pkg/monitors/network" + _ "github.com/supporttools/node-doctor/pkg/monitors/system" +) + +// configWithMonitors renders a minimal but realistic config file listing only +// the named monitors. It deliberately omits monitor types that the registry +// would auto-add via ApplyDefaultMonitors, which is what makes the asymmetry +// bug reproducible. +func configWithMonitors(clusterDomains string) string { + return `apiVersion: v1 +kind: NodeDoctorConfig +metadata: + name: node-doctor +settings: + nodeName: "test-node" +monitors: + - name: dns-health + type: network-dns-check + enabled: true + interval: 30s + timeout: 10s + config: + clusterDomains: ` + clusterDomains + ` + externalDomains: + - google.com +exporters: + prometheus: + enabled: true +` +} + +// captureCoordinator wires a coordinator that records the diff handed to the +// reload callback. +type captureCoordinator struct { + mu sync.Mutex + diffs []*ConfigDiff + events []string +} + +func (c *captureCoordinator) callback(_ context.Context, _ *types.NodeDoctorConfig, diff *ConfigDiff) error { + c.mu.Lock() + defer c.mu.Unlock() + c.diffs = append(c.diffs, diff) + return nil +} + +func (c *captureCoordinator) emit(_ types.EventSeverity, reason, message string) { + c.mu.Lock() + defer c.mu.Unlock() + c.events = append(c.events, reason+": "+message) +} + +func (c *captureCoordinator) lastDiff() *ConfigDiff { + c.mu.Lock() + defer c.mu.Unlock() + if len(c.diffs) == 0 { + return nil + } + return c.diffs[len(c.diffs)-1] +} + +// TestReloadWithoutNormalizerDropsDefaultMonitors documents the ORIGINAL bug +// (#node-doctor-243) so the fix cannot be quietly reverted: with no normalizer, +// the freshly-loaded config lacks the monitors that ApplyDefaultMonitors added +// at startup, so the diff reports them as REMOVED and the detector stops them. +func TestReloadWithoutNormalizerDropsDefaultMonitors(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(configWithMonitors("[]")), 0o644); err != nil { + t.Fatal(err) + } + + startCfg := loadAndNormalize(t, path, true) + + cap := &captureCoordinator{} + rc := NewReloadCoordinator(path, startCfg, cap.callback, cap.emit) + // NO normalizer installed — the historical behaviour. + + if err := os.WriteFile(path, []byte(configWithMonitors(`["kubernetes.default.svc.cluster.local"]`)), 0o644); err != nil { + t.Fatal(err) + } + if err := rc.TriggerReload(context.Background()); err != nil { + t.Fatalf("reload failed: %v", err) + } + + diff := cap.lastDiff() + if diff == nil { + t.Fatal("expected the reload callback to run") + } + if len(diff.MonitorsRemoved) == 0 { + t.Skip("registry has no auto-defaultable monitor types absent from this config; nothing to demonstrate") + } + t.Logf("without a normalizer the diff spuriously removes %d monitor(s): %v", + len(diff.MonitorsRemoved), monitorNames(diff.MonitorsRemoved)) +} + +// TestReloadWithNormalizerPreservesDefaultMonitors is the actual regression +// guard: with the normalizer installed (as main.go does), a ConfigMap edit +// touching ONE monitor must not report any monitor as removed. +func TestReloadWithNormalizerPreservesDefaultMonitors(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(configWithMonitors("[]")), 0o644); err != nil { + t.Fatal(err) + } + + startCfg := loadAndNormalize(t, path, true) + + cap := &captureCoordinator{} + rc := NewReloadCoordinator(path, startCfg, cap.callback, cap.emit) + rc.SetConfigNormalizer(func(c *types.NodeDoctorConfig) error { + return normalizeForTest(c, true) + }) + + // The operator edits ONLY dns-health. + if err := os.WriteFile(path, []byte(configWithMonitors(`["kubernetes.default.svc.cluster.local"]`)), 0o644); err != nil { + t.Fatal(err) + } + if err := rc.TriggerReload(context.Background()); err != nil { + t.Fatalf("reload failed: %v", err) + } + + diff := cap.lastDiff() + if diff == nil { + t.Fatal("expected the reload callback to run") + } + + if len(diff.MonitorsRemoved) != 0 { + t.Errorf("editing one monitor must not remove any others; got removed=%v. "+ + "This means the reload path and the startup path disagree about the config again.", + monitorNames(diff.MonitorsRemoved)) + } + if len(diff.MonitorsAdded) != 0 { + t.Errorf("editing one monitor must not add any; got added=%v", monitorNames(diff.MonitorsAdded)) + } + + // And the edit itself must be seen. + if len(diff.MonitorsModified) != 1 || diff.MonitorsModified[0].New.Name != "dns-health" { + t.Fatalf("expected exactly dns-health to be modified, got %d: %+v", + len(diff.MonitorsModified), diff.MonitorsModified) + } + + // The new config must actually carry the operator's value, i.e. the running + // monitor gets rebuilt from the NEW clusterDomains, not the old one. + newClusterDomains := diff.MonitorsModified[0].New.Config["clusterDomains"] + got := strings.TrimSpace(strings.Trim(sprint(newClusterDomains), "[]")) + if got != "kubernetes.default.svc.cluster.local" { + t.Errorf("modified monitor must carry the NEW clusterDomains, got %v", newClusterDomains) + } +} + +// TestReloadNormalizerFailurePropagates ensures a broken normalizer fails the +// reload loudly rather than silently applying a half-normalized config. +func TestReloadNormalizerFailurePropagates(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(configWithMonitors("[]")), 0o644); err != nil { + t.Fatal(err) + } + startCfg := loadAndNormalize(t, path, true) + + cap := &captureCoordinator{} + rc := NewReloadCoordinator(path, startCfg, cap.callback, cap.emit) + rc.SetConfigNormalizer(func(_ *types.NodeDoctorConfig) error { + return errBoom + }) + + if err := os.WriteFile(path, []byte(configWithMonitors(`["a.b.c"]`)), 0o644); err != nil { + t.Fatal(err) + } + err := rc.TriggerReload(context.Background()) + if err == nil { + t.Fatal("a failing normalizer must fail the reload") + } + if !strings.Contains(err.Error(), "normalize") { + t.Errorf("error should identify normalization as the cause, got %v", err) + } + if cap.lastDiff() != nil { + t.Error("the reload callback must not run when normalization failed") + } +} + +// TestReloadEmitsRestartRequiredEventForSettingsOnlyChange guards the case that +// used to report a cheerful "no changes": ComputeConfigDiff ignores settings, so +// a settings-only edit produced a success event while the process kept the old +// value. +func TestReloadEmitsRestartRequiredEventForSettingsOnlyChange(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(path, []byte(configWithMonitors("[]")), 0o644); err != nil { + t.Fatal(err) + } + startCfg := loadAndNormalize(t, path, true) + + cap := &captureCoordinator{} + rc := NewReloadCoordinator(path, startCfg, cap.callback, cap.emit) + rc.SetConfigNormalizer(func(c *types.NodeDoctorConfig) error { + return normalizeForTest(c, true) + }) + + // Change ONLY the node name — invisible to ComputeConfigDiff. + renamed := strings.Replace(configWithMonitors("[]"), `nodeName: "test-node"`, `nodeName: "other-node"`, 1) + if err := os.WriteFile(path, []byte(renamed), 0o644); err != nil { + t.Fatal(err) + } + if err := rc.TriggerReload(context.Background()); err != nil { + t.Fatalf("reload failed: %v", err) + } + + cap.mu.Lock() + events := strings.Join(cap.events, "\n") + cap.mu.Unlock() + + if !strings.Contains(events, "ConfigReloadRestartRequired") { + t.Errorf("a settings-only change that cannot be hot-applied must emit "+ + "ConfigReloadRestartRequired, not a silent success. Events:\n%s", events) + } + if strings.Contains(events, "ConfigReloadNoChanges") { + t.Errorf("must not claim 'no changes' when a restart-required change was detected. Events:\n%s", events) + } + + if r := rc.GetLastReloadability(); r == nil || !r.HasRestartRequired() { + t.Error("GetLastReloadability must expose the restart-required classification") + } +} + +func monitorNames(ms []types.MonitorConfig) []string { + out := make([]string, 0, len(ms)) + for _, m := range ms { + out = append(out, m.Name) + } + return out +} diff --git a/pkg/reload/reloadability.go b/pkg/reload/reloadability.go new file mode 100644 index 0000000..f4afab4 --- /dev/null +++ b/pkg/reload/reloadability.go @@ -0,0 +1,236 @@ +package reload + +import ( + "fmt" + "sort" + "strings" + + "github.com/supporttools/node-doctor/pkg/types" +) + +// Reloadability classifies a ConfigDiff into the changes the running agent can +// genuinely re-initialize in place versus the changes that are latched at +// process startup and therefore require a pod rollout. +// +// This exists because the failure mode it guards against is SILENT STALENESS: +// before it, a ConfigMap edit that the agent could not actually apply produced +// a cheerful "reload succeeded" event while the running component kept its old +// behaviour, and the only way an operator discovered the difference was by +// noticing the alert never stopped firing. An honest "this change needs a +// rollout" is an acceptable outcome; pretending it was applied is not. +type Reloadability struct { + // MonitorsAdded/Removed/Modified are monitor NAMES the detector will + // create, stop, and stop-then-recreate respectively. All three are true + // hot reloads: monitor instances are rebuilt from the new config. + MonitorsAdded []string + MonitorsRemoved []string + MonitorsModified []string + + // ExportersReconfigured is true when exporter config changed and the + // running exporters implement types.ReloadableExporter (all built-in + // exporters do, including rebinding their listener on a port change). + ExportersReconfigured bool + + // RemediationReconfigured is true when remediation settings changed and + // the running remediator registry can adopt them in place (dry-run, + // rate limits, circuit breaker). + RemediationReconfigured bool + + // RestartRequired lists changes that were detected but CANNOT be applied + // to the running process. Each entry is "field: reason". A non-empty list + // must be surfaced to the operator as a warning — it means the on-disk + // config and the running behaviour have legitimately diverged. + RestartRequired []string +} + +// ClassifyReload compares the previously-active config against the newly-loaded +// one and reports what a reload can and cannot apply. +// +// oldConfig and newConfig must both be fully normalized (defaults applied) so +// that classification compares like with like; diff may be nil, in which case +// only the process-latched settings are examined. +func ClassifyReload(oldConfig, newConfig *types.NodeDoctorConfig, diff *ConfigDiff) *Reloadability { + r := &Reloadability{} + if oldConfig == nil || newConfig == nil { + return r + } + + if diff != nil { + for _, m := range diff.MonitorsAdded { + r.MonitorsAdded = append(r.MonitorsAdded, m.Name) + } + for _, m := range diff.MonitorsRemoved { + r.MonitorsRemoved = append(r.MonitorsRemoved, m.Name) + } + for _, m := range diff.MonitorsModified { + r.MonitorsModified = append(r.MonitorsModified, m.New.Name) + } + sort.Strings(r.MonitorsAdded) + sort.Strings(r.MonitorsRemoved) + sort.Strings(r.MonitorsModified) + r.ExportersReconfigured = diff.ExportersChanged + r.RemediationReconfigured = diff.RemediationChanged + } + + // --- Settings latched at process startup ------------------------------- + + // nodeName is baked into every exported condition, event and metric label, + // and into the Kubernetes exporter's node client. Changing it live would + // leave conditions stranded on the old node object. + if oldConfig.Settings.NodeName != newConfig.Settings.NodeName { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("settings.nodeName (%q -> %q): node identity is bound at startup", + oldConfig.Settings.NodeName, newConfig.Settings.NodeName)) + } + + // The logging DESTINATION is opened once at startup. Level and format are + // re-applied on reload (see logger re-init in the detector), but switching + // stdout->file or changing the file path needs a fresh process. + if oldConfig.Settings.LogOutput != newConfig.Settings.LogOutput { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("settings.logOutput (%q -> %q): log destination is opened at startup", + oldConfig.Settings.LogOutput, newConfig.Settings.LogOutput)) + } + if oldConfig.Settings.LogFile != newConfig.Settings.LogFile { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("settings.logFile (%q -> %q): log file handle is opened at startup", + oldConfig.Settings.LogFile, newConfig.Settings.LogFile)) + } + + // pprof listener is started (or not) once, from the startup config. + if oldConfig.Features.EnableProfiling != newConfig.Features.EnableProfiling { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("features.enableProfiling (%v -> %v): the pprof listener is started at startup", + oldConfig.Features.EnableProfiling, newConfig.Features.EnableProfiling)) + } + if newConfig.Features.EnableProfiling && oldConfig.Features.ProfilingPort != newConfig.Features.ProfilingPort { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("features.profilingPort (%d -> %d): the pprof listener is bound at startup", + oldConfig.Features.ProfilingPort, newConfig.Features.ProfilingPort)) + } + + // An exporter that was DISABLED at startup was never constructed, so there + // is no instance for reloadExporter to hand the new config to. Enabling it + // therefore needs a rollout. (Disabling a running exporter is handled by + // its own Reload.) + classifyExporterEnable(oldConfig, newConfig, r) + + // Remediation as a whole is wired at startup: when disabled, no registry, + // no remediator strategies and no cluster client exist to reconfigure. + if !oldConfig.Remediation.Enabled && newConfig.Remediation.Enabled { + r.RestartRequired = append(r.RestartRequired, + "remediation.enabled (false -> true): the remediator registry and cluster client are wired at startup") + } + + // The controller lease client is constructed once from coordination config. + classifyCoordination(oldConfig, newConfig, r) + + return r +} + +// classifyExporterEnable appends a restart-required entry for each exporter +// that transitions from disabled to enabled, since no instance exists to reload. +func classifyExporterEnable(oldConfig, newConfig *types.NodeDoctorConfig, r *Reloadability) { + type exp struct { + name string + oldEnabled bool + newEnabled bool + } + exps := []exp{ + {"exporters.kubernetes", exporterEnabled(oldConfig.Exporters.Kubernetes != nil, func() bool { return oldConfig.Exporters.Kubernetes.Enabled }), + exporterEnabled(newConfig.Exporters.Kubernetes != nil, func() bool { return newConfig.Exporters.Kubernetes.Enabled })}, + {"exporters.http", exporterEnabled(oldConfig.Exporters.HTTP != nil, func() bool { return oldConfig.Exporters.HTTP.Enabled }), + exporterEnabled(newConfig.Exporters.HTTP != nil, func() bool { return newConfig.Exporters.HTTP.Enabled })}, + {"exporters.prometheus", exporterEnabled(oldConfig.Exporters.Prometheus != nil, func() bool { return oldConfig.Exporters.Prometheus.Enabled }), + exporterEnabled(newConfig.Exporters.Prometheus != nil, func() bool { return newConfig.Exporters.Prometheus.Enabled })}, + } + for _, e := range exps { + if !e.oldEnabled && e.newEnabled { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("%s.enabled (false -> true): the exporter is constructed at startup, so there is no instance to reload", e.name)) + } + } +} + +// exporterEnabled guards a nil exporter config block. +func exporterEnabled(present bool, enabled func() bool) bool { + if !present { + return false + } + return enabled() +} + +// classifyCoordination flags lease-client changes, which are wired once at startup. +func classifyCoordination(oldConfig, newConfig *types.NodeDoctorConfig, r *Reloadability) { + oldCoord := oldConfig.Remediation.Coordination + newCoord := newConfig.Remediation.Coordination + oldEnabled := oldCoord != nil && oldCoord.Enabled + newEnabled := newCoord != nil && newCoord.Enabled + + if oldEnabled != newEnabled { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("remediation.coordination.enabled (%v -> %v): the controller lease client is wired at startup", + oldEnabled, newEnabled)) + return + } + if oldEnabled && newEnabled && oldCoord.ControllerURL != newCoord.ControllerURL { + r.RestartRequired = append(r.RestartRequired, + fmt.Sprintf("remediation.coordination.controllerURL (%q -> %q): the controller lease client is wired at startup", + oldCoord.ControllerURL, newCoord.ControllerURL)) + } +} + +// HasRestartRequired reports whether any detected change cannot be hot-applied. +func (r *Reloadability) HasRestartRequired() bool { + return r != nil && len(r.RestartRequired) > 0 +} + +// HasHotChanges reports whether anything at all will be re-initialized in place. +func (r *Reloadability) HasHotChanges() bool { + if r == nil { + return false + } + return len(r.MonitorsAdded) > 0 || len(r.MonitorsRemoved) > 0 || len(r.MonitorsModified) > 0 || + r.ExportersReconfigured || r.RemediationReconfigured +} + +// Summary renders a single operator-facing line naming exactly which components +// were reconfigured, and which changes still need a rollout. This is the log +// line an operator greps for after `kubectl patch cm` to confirm the edit +// actually took effect. +func (r *Reloadability) Summary() string { + if r == nil { + return "config reload: nothing to apply" + } + + parts := make([]string, 0, 6) + if len(r.MonitorsModified) > 0 { + parts = append(parts, "monitors reconfigured=["+strings.Join(r.MonitorsModified, " ")+"]") + } + if len(r.MonitorsAdded) > 0 { + parts = append(parts, "monitors started=["+strings.Join(r.MonitorsAdded, " ")+"]") + } + if len(r.MonitorsRemoved) > 0 { + parts = append(parts, "monitors stopped=["+strings.Join(r.MonitorsRemoved, " ")+"]") + } + if r.ExportersReconfigured { + parts = append(parts, "exporters reconfigured") + } + if r.RemediationReconfigured { + parts = append(parts, "remediation reconfigured") + } + + msg := "config reload applied: " + if len(parts) == 0 { + msg += "no hot-reloadable changes" + } else { + msg += strings.Join(parts, "; ") + } + + if len(r.RestartRequired) > 0 { + msg += fmt.Sprintf(" | RESTART REQUIRED for %d change(s) that cannot be applied to the running process: %s", + len(r.RestartRequired), strings.Join(r.RestartRequired, "; ")) + } + + return msg +} diff --git a/pkg/reload/reloadability_test.go b/pkg/reload/reloadability_test.go new file mode 100644 index 0000000..a634ace --- /dev/null +++ b/pkg/reload/reloadability_test.go @@ -0,0 +1,165 @@ +package reload + +import ( + "strings" + "testing" + "time" + + "github.com/supporttools/node-doctor/pkg/types" +) + +func baseConfig() *types.NodeDoctorConfig { + return &types.NodeDoctorConfig{ + APIVersion: "v1", + Kind: "NodeDoctorConfig", + Metadata: types.ConfigMetadata{Name: "node-doctor"}, + Settings: types.GlobalSettings{ + NodeName: "node-a", + LogLevel: "info", + LogFormat: "json", + LogOutput: "stdout", + }, + Exporters: types.ExporterConfigs{ + Prometheus: &types.PrometheusExporterConfig{Enabled: true, Port: 9100}, + Kubernetes: &types.KubernetesExporterConfig{Enabled: true}, + }, + Remediation: types.RemediationConfig{Enabled: true}, + } +} + +func TestClassifyReload_NodeNameChangeRequiresRestart(t *testing.T) { + oldCfg := baseConfig() + newCfg := baseConfig() + newCfg.Settings.NodeName = "node-b" + + r := ClassifyReload(oldCfg, newCfg, nil) + + if !r.HasRestartRequired() { + t.Fatal("changing settings.nodeName must be reported as restart-required, not silently ignored") + } + if !strings.Contains(strings.Join(r.RestartRequired, " "), "settings.nodeName") { + t.Errorf("restart-required list should name the field, got %v", r.RestartRequired) + } +} + +func TestClassifyReload_LogDestinationRequiresRestartButLevelDoesNot(t *testing.T) { + // Level-only change: hot-reloadable, so nothing should demand a restart. + oldCfg := baseConfig() + newCfg := baseConfig() + newCfg.Settings.LogLevel = "debug" + + if r := ClassifyReload(oldCfg, newCfg, nil); r.HasRestartRequired() { + t.Errorf("a log LEVEL change is hot-reloadable; should not demand a restart, got %v", r.RestartRequired) + } + + // Destination change: the file handle is opened at startup. + newCfg2 := baseConfig() + newCfg2.Settings.LogOutput = "file" + newCfg2.Settings.LogFile = "/var/log/nd.log" + + r2 := ClassifyReload(oldCfg, newCfg2, nil) + if !r2.HasRestartRequired() { + t.Fatal("changing the log destination must be reported as restart-required") + } +} + +func TestClassifyReload_EnablingDisabledExporterRequiresRestart(t *testing.T) { + oldCfg := baseConfig() + oldCfg.Exporters.HTTP = &types.HTTPExporterConfig{Enabled: false} + newCfg := baseConfig() + newCfg.Exporters.HTTP = &types.HTTPExporterConfig{Enabled: true} + + r := ClassifyReload(oldCfg, newCfg, nil) + + // The exporter was never constructed at startup, so there is no instance for + // reloadExporter to hand the new config to. + joined := strings.Join(r.RestartRequired, " ") + if !strings.Contains(joined, "exporters.http.enabled") { + t.Errorf("enabling a previously-disabled exporter must be restart-required, got %v", r.RestartRequired) + } +} + +func TestClassifyReload_EnablingRemediationRequiresRestart(t *testing.T) { + oldCfg := baseConfig() + oldCfg.Remediation.Enabled = false + newCfg := baseConfig() + newCfg.Remediation.Enabled = true + + r := ClassifyReload(oldCfg, newCfg, nil) + + if !strings.Contains(strings.Join(r.RestartRequired, " "), "remediation.enabled") { + t.Errorf("enabling remediation must be restart-required (registry wired at startup), got %v", r.RestartRequired) + } +} + +func TestClassifyReload_CoordinationChangeRequiresRestart(t *testing.T) { + oldCfg := baseConfig() + oldCfg.Remediation.Coordination = &types.RemediationCoordinationConfig{ + Enabled: true, ControllerURL: "http://a", LeaseTimeout: time.Minute, + } + newCfg := baseConfig() + newCfg.Remediation.Coordination = &types.RemediationCoordinationConfig{ + Enabled: true, ControllerURL: "http://b", LeaseTimeout: time.Minute, + } + + r := ClassifyReload(oldCfg, newCfg, nil) + + if !strings.Contains(strings.Join(r.RestartRequired, " "), "controllerURL") { + t.Errorf("changing the controller URL must be restart-required (lease client wired at startup), got %v", r.RestartRequired) + } +} + +func TestClassifyReload_IdenticalConfigNeedsNothing(t *testing.T) { + r := ClassifyReload(baseConfig(), baseConfig(), nil) + if r.HasRestartRequired() { + t.Errorf("identical configs must not demand a restart, got %v", r.RestartRequired) + } + if r.HasHotChanges() { + t.Error("identical configs must not report hot changes") + } +} + +func TestReloadabilitySummaryNamesReconfiguredMonitors(t *testing.T) { + // The ticket explicitly requires a log line naming which monitors were + // reconfigured, so an operator can confirm their ConfigMap edit landed. + diff := &ConfigDiff{ + MonitorsModified: []MonitorChange{ + {New: types.MonitorConfig{Name: "dns-health"}}, + }, + MonitorsAdded: []types.MonitorConfig{{Name: "new-mon"}}, + MonitorsRemoved: []types.MonitorConfig{{Name: "old-mon"}}, + } + r := ClassifyReload(baseConfig(), baseConfig(), diff) + summary := r.Summary() + + for _, want := range []string{"dns-health", "new-mon", "old-mon", "reconfigured"} { + if !strings.Contains(summary, want) { + t.Errorf("summary %q must mention %q", summary, want) + } + } +} + +func TestReloadabilitySummaryFlagsRestartRequired(t *testing.T) { + oldCfg := baseConfig() + newCfg := baseConfig() + newCfg.Settings.NodeName = "node-b" + + summary := ClassifyReload(oldCfg, newCfg, nil).Summary() + + if !strings.Contains(summary, "RESTART REQUIRED") { + t.Errorf("summary must shout about restart-required changes, got %q", summary) + } +} + +func TestClassifyReload_NilConfigsAreSafe(t *testing.T) { + if r := ClassifyReload(nil, nil, nil); r == nil || r.HasRestartRequired() { + t.Error("nil configs must produce an empty, non-nil classification") + } + var nilR *Reloadability + if nilR.HasRestartRequired() || nilR.HasHotChanges() { + t.Error("nil Reloadability must be safe to query") + } + if nilR.Summary() == "" { + t.Error("nil Reloadability must still render a summary") + } +} diff --git a/pkg/reload/watcher_configmap_test.go b/pkg/reload/watcher_configmap_test.go new file mode 100644 index 0000000..c06caa8 --- /dev/null +++ b/pkg/reload/watcher_configmap_test.go @@ -0,0 +1,129 @@ +package reload + +import ( + "context" + "os" + "path/filepath" + "testing" + "time" +) + +// writeConfigMapVersion emulates the Kubernetes atomic-writer layout that backs +// a mounted ConfigMap, and the swap it performs on update: +// +// /../config.yaml real data directory +// /..data -> .. symlink, replaced via rename(2) +// /config.yaml -> ..data/config.yaml +// +// The agent watches (not the file) precisely because of this dance: the +// file the operator edits is a symlink whose target directory is swapped +// wholesale, so an inotify watch on the leaf path would never fire. +func writeConfigMapVersion(t *testing.T, dir, timestamp, content string) { + t.Helper() + + dataDir := filepath.Join(dir, ".."+timestamp) + if err := os.MkdirAll(dataDir, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dataDir, "config.yaml"), []byte(content), 0o644); err != nil { + t.Fatal(err) + } + + tmpLink := filepath.Join(dir, "..data_tmp") + _ = os.Remove(tmpLink) + if err := os.Symlink(".."+timestamp, tmpLink); err != nil { + t.Fatal(err) + } + // kubelet swaps ..data atomically with rename(2); inotify reports this on the + // parent directory as MOVED_TO, which fsnotify surfaces as a Create event. + if err := os.Rename(tmpLink, filepath.Join(dir, "..data")); err != nil { + t.Fatal(err) + } + + link := filepath.Join(dir, "config.yaml") + if _, err := os.Lstat(link); os.IsNotExist(err) { + if err := os.Symlink("..data/config.yaml", link); err != nil { + t.Fatal(err) + } + } +} + +// TestWatcherFiresOnConfigMapAtomicSwap is the end-to-end guard that a real +// `kubectl patch configmap` reaches the running agent. Watching the config file +// directly (rather than its directory) silently breaks this — the file content +// changes but no event ever fires, which is indistinguishable from "hot reload +// is not wired at all" from an operator's seat. +func TestWatcherFiresOnConfigMapAtomicSwap(t *testing.T) { + dir := t.TempDir() + writeConfigMapVersion(t, dir, "2026_08_12_00_00_00.111111", "version: 1\n") + + cfgPath := filepath.Join(dir, "config.yaml") + w, err := NewConfigWatcher(cfgPath, 50*time.Millisecond) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ch, err := w.Start(ctx) + if err != nil { + t.Fatal(err) + } + defer w.Stop() + + // Let the watch settle before mutating. + time.Sleep(100 * time.Millisecond) + + writeConfigMapVersion(t, dir, "2026_08_12_00_00_05.222222", "version: 2\n") + // kubelet garbage-collects the previous data directory after the swap. + _ = os.RemoveAll(filepath.Join(dir, "..2026_08_12_00_00_00.111111")) + + select { + case <-ch: + content, err := os.ReadFile(cfgPath) + if err != nil { + t.Fatalf("config unreadable after swap: %v", err) + } + if string(content) != "version: 2\n" { + t.Errorf("watcher fired but the file still reads %q; the swap did not land", content) + } + case <-time.After(5 * time.Second): + t.Fatal("watcher never fired after a ConfigMap atomic swap — a ConfigMap edit " + + "would silently never reach the running agent") + } +} + +// TestWatcherFiresOnPlainFileRewrite is the control: a non-ConfigMap deployment +// (bare file on disk) must also be detected. +func TestWatcherFiresOnPlainFileRewrite(t *testing.T) { + dir := t.TempDir() + cfgPath := filepath.Join(dir, "config.yaml") + if err := os.WriteFile(cfgPath, []byte("version: 1\n"), 0o644); err != nil { + t.Fatal(err) + } + + w, err := NewConfigWatcher(cfgPath, 50*time.Millisecond) + if err != nil { + t.Fatal(err) + } + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + ch, err := w.Start(ctx) + if err != nil { + t.Fatal(err) + } + defer w.Stop() + + time.Sleep(100 * time.Millisecond) + + if err := os.WriteFile(cfgPath, []byte("version: 2\n"), 0o644); err != nil { + t.Fatal(err) + } + + select { + case <-ch: + case <-time.After(5 * time.Second): + t.Fatal("watcher never fired after an in-place config rewrite") + } +} diff --git a/pkg/remediators/registry.go b/pkg/remediators/registry.go index 007dab4..1540c83 100644 --- a/pkg/remediators/registry.go +++ b/pkg/remediators/registry.go @@ -394,6 +394,74 @@ func (r *RemediatorRegistry) SetMaxRemediationsPerMinute(n int) { } } +// SetMaxRemediationsPerHour updates the sliding-window per-hour remediation +// cap. A value <= 0 disables the per-hour check entirely (unlimited), matching +// the semantics of the constructor argument. +// +// Existing entries in the sliding window are retained, so lowering the cap +// takes effect immediately against remediations that already happened. +func (r *RemediatorRegistry) SetMaxRemediationsPerHour(n int) { + r.mu.Lock() + defer r.mu.Unlock() + if n < 0 { + n = 0 + } + r.maxPerHour = n + r.logInfof("Per-hour remediation rate limit configured (max: %d/hour)", n) +} + +// ApplyConfig adopts a new remediation configuration in place, without +// restarting the process. It is the counterpart to the wiring main.go performs +// at startup and is invoked by the detector on config hot reload. +// +// Rationale (TaskForge #node-doctor-243): before this existed, the reload path +// computed diff.RemediationChanged and then did nothing with it. An operator who +// edited the ConfigMap mid-incident to flip dryRun on, or to drop +// maxRemediationsPerHour, got a "reload succeeded" event while the registry kept +// remediating under the OLD limits — the change only took effect after a manual +// pod restart. Silent staleness on the remediation kill-switch is the worst +// possible place to have it. +// +// dryRunMode is the effective process-wide dry-run flag (settings.dryRunMode OR +// remediation.dryRun OR the -dry-run command-line flag), mirroring how main.go +// computes it at startup. +// +// NOT reconfigured here (these are latched at startup and are reported as +// restart-required by reload.ClassifyReload): +// - remediation.enabled false->true: the registry, the built-in strategies and +// the cluster client are all constructed only when it was true at startup. +// - remediation.coordination.*: the controller lease client is wired once. +// +// A nil cfg is a no-op returning an error, since callers should not reach here +// without a remediation config. +func (r *RemediatorRegistry) ApplyConfig(cfg *types.RemediationConfig, dryRunMode bool) error { + if cfg == nil { + return fmt.Errorf("remediation config cannot be nil") + } + + r.SetDryRun(cfg.DryRun || dryRunMode) + r.SetMaxRemediationsPerHour(cfg.MaxRemediationsPerHour) + r.SetMaxRemediationsPerMinute(cfg.MaxRemediationsPerMinute) + + // Only push a circuit-breaker update when the new values are actually + // usable. A zero/absent circuitBreaker block must not clobber the running + // configuration with invalid values, so treat it as "leave as-is". + cb := CircuitBreakerConfig{ + Threshold: cfg.CircuitBreaker.Threshold, + Timeout: cfg.CircuitBreaker.Timeout, + SuccessThreshold: cfg.CircuitBreaker.SuccessThreshold, + } + if cb.Threshold > 0 && cb.Timeout > 0 && cb.SuccessThreshold > 0 { + if err := r.SetCircuitBreakerConfig(cb); err != nil { + return fmt.Errorf("apply circuit breaker config: %w", err) + } + } + + r.logInfof("Remediation config reloaded in place (dryRun=%v maxPerHour=%d maxPerMinute=%d)", + r.IsDryRun(), cfg.MaxRemediationsPerHour, cfg.MaxRemediationsPerMinute) + return nil +} + // SetCircuitStateObserver registers an observer that is notified of circuit // breaker state changes. The observer is called once immediately with the // current state (so a backing metric is correct from the start) and then on diff --git a/pkg/remediators/registry_applyconfig_test.go b/pkg/remediators/registry_applyconfig_test.go new file mode 100644 index 0000000..b33bfc5 --- /dev/null +++ b/pkg/remediators/registry_applyconfig_test.go @@ -0,0 +1,133 @@ +package remediators + +import ( + "testing" + "time" + + "github.com/supporttools/node-doctor/pkg/types" +) + +// TestApplyConfigAdoptsDryRun is the kill-switch guard: flipping dryRun in the +// ConfigMap during an incident must take effect on the RUNNING registry, not +// only after a pod restart (#node-doctor-243). +func TestApplyConfigAdoptsDryRun(t *testing.T) { + r := NewRegistry(10, 100) + r.SetDryRun(false) + + if r.IsDryRun() { + t.Fatal("test setup: registry should start out of dry-run") + } + + err := r.ApplyConfig(&types.RemediationConfig{ + Enabled: true, + DryRun: true, + MaxRemediationsPerHour: 5, + }, false) + if err != nil { + t.Fatalf("ApplyConfig: %v", err) + } + + if !r.IsDryRun() { + t.Error("flipping remediation.dryRun must take effect immediately on the running registry") + } +} + +// TestApplyConfigRespectsProcessWideDryRun ensures the -dry-run flag / global +// settings.dryRunMode cannot be cleared by a ConfigMap edit. +func TestApplyConfigRespectsProcessWideDryRun(t *testing.T) { + r := NewRegistry(10, 100) + + err := r.ApplyConfig(&types.RemediationConfig{ + Enabled: true, + DryRun: false, // config says "live" + }, true) // ...but the process is globally in dry-run + if err != nil { + t.Fatalf("ApplyConfig: %v", err) + } + + if !r.IsDryRun() { + t.Error("process-wide dry-run must win over a config that sets dryRun:false") + } +} + +// TestApplyConfigAdoptsRateLimits verifies the per-hour and per-minute caps are +// re-applied, since lowering them is the other lever operators pull mid-incident. +func TestApplyConfigAdoptsRateLimits(t *testing.T) { + r := NewRegistry(100, 100) + + err := r.ApplyConfig(&types.RemediationConfig{ + Enabled: true, + MaxRemediationsPerHour: 3, + MaxRemediationsPerMinute: 1, + }, false) + if err != nil { + t.Fatalf("ApplyConfig: %v", err) + } + + stats := r.GetStats() + if stats.MaxPerHour != 3 { + t.Errorf("maxRemediationsPerHour must be adopted, got %d want 3", stats.MaxPerHour) + } +} + +// TestSetMaxRemediationsPerHour covers the new setter directly, including the +// "0 disables the check" contract inherited from the constructor. +func TestSetMaxRemediationsPerHour(t *testing.T) { + r := NewRegistry(10, 100) + + r.SetMaxRemediationsPerHour(4) + if got := r.GetStats().MaxPerHour; got != 4 { + t.Errorf("MaxPerHour = %d, want 4", got) + } + + r.SetMaxRemediationsPerHour(0) + if got := r.GetStats().MaxPerHour; got != 0 { + t.Errorf("MaxPerHour = %d, want 0 (disabled)", got) + } + + // Negative values are clamped rather than corrupting the window check. + r.SetMaxRemediationsPerHour(-5) + if got := r.GetStats().MaxPerHour; got != 0 { + t.Errorf("negative MaxPerHour must clamp to 0, got %d", got) + } +} + +// TestApplyConfigAdoptsCircuitBreaker checks valid circuit-breaker settings land. +func TestApplyConfigAdoptsCircuitBreaker(t *testing.T) { + r := NewRegistry(10, 100) + + err := r.ApplyConfig(&types.RemediationConfig{ + Enabled: true, + CircuitBreaker: types.CircuitBreakerConfig{ + Threshold: 7, + Timeout: 2 * time.Minute, + SuccessThreshold: 3, + }, + }, false) + if err != nil { + t.Fatalf("ApplyConfig: %v", err) + } +} + +// TestApplyConfigIgnoresIncompleteCircuitBreaker ensures an absent/zero +// circuitBreaker block does not clobber the running configuration with invalid +// values (which SetCircuitBreakerConfig would reject anyway). +func TestApplyConfigIgnoresIncompleteCircuitBreaker(t *testing.T) { + r := NewRegistry(10, 100) + + err := r.ApplyConfig(&types.RemediationConfig{ + Enabled: true, + // CircuitBreaker left as the zero value. + }, false) + if err != nil { + t.Errorf("an absent circuitBreaker block must be treated as 'leave as-is', got error: %v", err) + } +} + +// TestApplyConfigRejectsNil guards the contract boundary. +func TestApplyConfigRejectsNil(t *testing.T) { + r := NewRegistry(10, 100) + if err := r.ApplyConfig(nil, false); err == nil { + t.Error("ApplyConfig(nil) must return an error") + } +} diff --git a/pkg/types/config.go b/pkg/types/config.go index 3b677c2..483d62e 100644 --- a/pkg/types/config.go +++ b/pkg/types/config.go @@ -515,8 +515,17 @@ type FeatureFlags struct { // ReloadConfig contains configuration hot reload settings. type ReloadConfig struct { - // Enabled indicates whether hot reload is enabled - Enabled bool `json:"enabled" yaml:"enabled"` + // Enabled indicates whether hot reload is enabled. + // + // It is a *bool so that "absent from the config file" is distinguishable + // from an explicit "false": absent defaults to TRUE (ApplyDefaults), which + // preserves the historical behaviour of always watching the config file. + // Setting it explicitly to false disables the watcher entirely, and the + // agent logs — loudly, once, at startup — that ConfigMap edits will NOT be + // picked up until the pod is rolled. Prior to this being honored the field + // was parsed and then ignored, so an operator who set it got no watcher + // change and no warning; that silence is the bug this field now avoids. + Enabled *bool `json:"enabled,omitempty" yaml:"enabled,omitempty"` // DebounceIntervalString is the debounce interval as a string (e.g., "500ms") DebounceIntervalString string `json:"debounceInterval,omitempty" yaml:"debounceInterval,omitempty"` @@ -525,8 +534,23 @@ type ReloadConfig struct { DebounceInterval time.Duration `json:"-" yaml:"-"` } +// IsEnabled reports whether config hot reload is enabled. An unset (nil) value +// means enabled — see the Enabled field docs for why the default is true. +func (r *ReloadConfig) IsEnabled() bool { + if r == nil || r.Enabled == nil { + return true + } + return *r.Enabled +} + // ApplyDefaults applies default values to reload configuration. func (r *ReloadConfig) ApplyDefaults() error { + // Default hot reload to enabled when the operator did not say otherwise. + if r.Enabled == nil { + enabled := true + r.Enabled = &enabled + } + // Default debounce interval if r.DebounceIntervalString == "" { r.DebounceIntervalString = "500ms" diff --git a/test/integration/chart_probes_test.go b/test/integration/chart_probes_test.go new file mode 100644 index 0000000..789ec96 --- /dev/null +++ b/test/integration/chart_probes_test.go @@ -0,0 +1,210 @@ +package integration + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "gopkg.in/yaml.v2" +) + +// chartDir locates the packaged chart relative to this test file. +func chartDir(t *testing.T) string { + t.Helper() + wd, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + // test/integration -> repo root + return filepath.Join(wd, "..", "..", "helm", "node-doctor") +} + +// renderTemplate runs `helm template` for one chart template and returns the YAML. +func renderTemplate(t *testing.T, showOnly string) string { + t.Helper() + + if _, err := exec.LookPath("helm"); err != nil { + t.Skip("helm not installed; skipping chart rendering test") + } + + cmd := exec.Command("helm", "template", "node-doctor", chartDir(t), "--show-only", showOnly) + out, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("helm template %s failed: %v\n%s", showOnly, err, out) + } + return string(out) +} + +type probeSpec struct { + Exec *struct { + Command []string `yaml:"command"` + } `yaml:"exec"` + HTTPGet *struct { + Path string `yaml:"path"` + Port interface{} `yaml:"port"` + } `yaml:"httpGet"` + InitialDelaySeconds int `yaml:"initialDelaySeconds"` + PeriodSeconds int `yaml:"periodSeconds"` + TimeoutSeconds int `yaml:"timeoutSeconds"` + FailureThreshold int `yaml:"failureThreshold"` +} + +type daemonSetDoc struct { + Spec struct { + Template struct { + Spec struct { + Containers []struct { + Name string `yaml:"name"` + LivenessProbe probeSpec `yaml:"livenessProbe"` + ReadinessProbe probeSpec `yaml:"readinessProbe"` + StartupProbe probeSpec `yaml:"startupProbe"` + } `yaml:"containers"` + } `yaml:"spec"` + } `yaml:"template"` + } `yaml:"spec"` +} + +func nodeDoctorContainerProbes(t *testing.T) (liveness, readiness, startup probeSpec) { + t.Helper() + + rendered := renderTemplate(t, "templates/daemonset.yaml") + + var ds daemonSetDoc + for _, doc := range strings.Split(rendered, "\n---\n") { + if !strings.Contains(doc, "kind: DaemonSet") { + continue + } + if err := yaml.Unmarshal([]byte(doc), &ds); err != nil { + t.Fatalf("parse DaemonSet: %v", err) + } + break + } + + for _, c := range ds.Spec.Template.Spec.Containers { + if c.Name == "node-doctor" { + return c.LivenessProbe, c.ReadinessProbe, c.StartupProbe + } + } + t.Fatal("node-doctor container not found in the rendered DaemonSet") + return +} + +// TestProbesUseExecNotHostPort8080 guards the fix that moved probes OFF +// hostPort 8080. +// +// node-doctor runs with hostNetwork:true. A foreign host process (an IP-float +// daemon) owning :8080 answers an httpGet probe with a 404 and crashloops the +// pod. That port conflict still exists in the fleet today; node-doctor is only +// immune because its probes exec against a per-pod unix socket. Reintroducing +// httpGet on 8080 would silently re-arm the crashloop. +func TestProbesUseExecNotHostPort8080(t *testing.T) { + liveness, readiness, startup := nodeDoctorContainerProbes(t) + + for name, p := range map[string]probeSpec{ + "liveness": liveness, + "readiness": readiness, + "startup": startup, + } { + if p.HTTPGet != nil { + t.Errorf("%s probe uses httpGet (port %v). Probes MUST exec against the per-pod unix "+ + "socket: on a hostNetwork node a foreign process squatting :8080 answers the probe "+ + "with a 404 and crashloops the pod.", name, p.HTTPGet.Port) + } + if p.Exec == nil || len(p.Exec.Command) == 0 { + t.Errorf("%s probe must use an exec command against the health unix socket", name) + } + } +} + +// TestLivenessAndReadinessUseDistinctProbes verifies the two signals are wired +// to genuinely different endpoints (#node-doctor-246). If both ran +// `-healthcheck`, a downstream failure could never surface as NotReady, and if +// both ran `-healthcheck-ready`, a downstream failure would RESTART the pod. +func TestLivenessAndReadinessUseDistinctProbes(t *testing.T) { + liveness, readiness, startup := nodeDoctorContainerProbes(t) + + livenessCmd := strings.Join(liveness.Exec.Command, " ") + readinessCmd := strings.Join(readiness.Exec.Command, " ") + startupCmd := strings.Join(startup.Exec.Command, " ") + + if !strings.Contains(livenessCmd, "-healthcheck") || strings.Contains(livenessCmd, "-healthcheck-ready") { + t.Errorf("liveness must probe /healthz via -healthcheck, got %q", livenessCmd) + } + if !strings.Contains(readinessCmd, "-healthcheck-ready") { + t.Errorf("readiness must probe /ready via -healthcheck-ready, got %q. Without this a "+ + "downstream failure cannot make the pod NotReady.", readinessCmd) + } + if livenessCmd == readinessCmd { + t.Error("liveness and readiness must not be the same probe: conflating them either " + + "restarts the pod on downstream failure or never reports NotReady") + } + // Startup gates the container coming up at all, so it must use LIVENESS + // semantics — a downstream that is down at boot must not prevent start. + if strings.Contains(startupCmd, "-healthcheck-ready") { + t.Errorf("startup probe must use liveness semantics (-healthcheck), got %q: a downstream "+ + "that is unreachable at boot must not prevent the agent from starting", startupCmd) + } +} + +// TestStartupProbeBudgetHasHeadroom confirms the slow-but-legitimate init +// budget (#node-doctor-246 item 3). +func TestStartupProbeBudgetHasHeadroom(t *testing.T) { + _, _, startup := nodeDoctorContainerProbes(t) + + budget := startup.InitialDelaySeconds + startup.FailureThreshold*startup.PeriodSeconds + + const minBudgetSeconds = 100 + if budget < minBudgetSeconds { + t.Errorf("startup budget is %ds (initialDelay %d + failureThreshold %d x period %d), "+ + "want >= %ds. Too little headroom is what crashlooped a1pinode01 125x when a blocked "+ + "exporter delayed init.", + budget, startup.InitialDelaySeconds, startup.FailureThreshold, startup.PeriodSeconds, minBudgetSeconds) + } + + if startup.FailureThreshold < 12 { + t.Errorf("startupProbe.failureThreshold = %d, want >= 12 for cold-start headroom on a loaded node", + startup.FailureThreshold) + } +} + +// TestRenderedConfigEnablesHotReload guards the chart<->code contract for +// #node-doctor-243: the shipped ConfigMap must actually turn hot reload on, and +// must parse into the agent's config type. +func TestRenderedConfigEnablesHotReload(t *testing.T) { + rendered := renderTemplate(t, "templates/configmap.yaml") + + var cm struct { + Data map[string]string `yaml:"data"` + } + if err := yaml.Unmarshal([]byte(rendered), &cm); err != nil { + t.Fatalf("parse ConfigMap: %v", err) + } + + raw, ok := cm.Data["config.yaml"] + if !ok { + t.Fatal("rendered ConfigMap has no config.yaml key") + } + + var parsed struct { + Reload struct { + Enabled *bool `yaml:"enabled"` + DebounceInterval string `yaml:"debounceInterval"` + } `yaml:"reload"` + } + if err := yaml.Unmarshal([]byte(raw), &parsed); err != nil { + t.Fatalf("the shipped config.yaml does not parse: %v", err) + } + + if parsed.Reload.Enabled == nil { + t.Fatal("the shipped config must state reload.enabled explicitly so operators can see the knob") + } + if !*parsed.Reload.Enabled { + t.Error("the shipped chart must enable config hot reload; otherwise every ConfigMap edit " + + "silently requires a rollout") + } + if parsed.Reload.DebounceInterval == "" { + t.Error("reload.debounceInterval should be set explicitly in the shipped config") + } +} From f11cc761dcbf5cf4c2109cb2974c83eb2e38833a Mon Sep 17 00:00:00 2001 From: Matthew Mattox Date: Wed, 12 Aug 2026 18:30:18 -0500 Subject: [PATCH 2/2] fix(helm): publish NotReady addresses so degraded nodes keep being scraped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Readiness on the agent DaemonSet now has teeth (a sustained exporter failure marks the pod NotReady), which interacts badly with the chart as it stood. The agent Service is selected by the ServiceMonitor and exists ONLY for Prometheus discovery — no request traffic flows through it. It did not set publishNotReadyAddresses, which defaults to false, so Kubernetes drops NotReady pods out of the Service's Endpoints. A pod goes NotReady precisely when something is wrong with that node's agent, so the effect was to stop scraping exactly the nodes we most need data from, at exactly the moment we need them. The blackout would also be silent. NodeDoctorNoMetrics is absent(node_doctor_monitor_uptime_seconds) which is fleet-wide: it only fires when EVERY node stops reporting. Losing one node, or five, raises nothing at all. Set publishNotReadyAddresses: true on the Service, with the reasoning recorded at both the template and the values site so it does not get "cleaned up" later. Readiness still gates rollouts via maxUnavailable and still surfaces degradation in kubectl/dashboards; it just no longer destroys observability while doing so. Added a chart contract test asserting it renders true, in the same spirit as the existing guard that fails if httpGet-on-8080 probes are reintroduced. Verified the test fails when the value is flipped to false. values.yaml is generated: edited values.yaml.template and ran make helm-generate; both committed. Task: #node-doctor-246 Claude-Session: https://claude.ai/code/session_01Av4whriQf6KqJgRxGKQP7N --- helm/node-doctor/templates/service.yaml | 7 +++++ helm/node-doctor/values.yaml | 17 +++++++++++ helm/node-doctor/values.yaml.template | 17 +++++++++++ test/integration/chart_probes_test.go | 39 +++++++++++++++++++++++++ 4 files changed, 80 insertions(+) diff --git a/helm/node-doctor/templates/service.yaml b/helm/node-doctor/templates/service.yaml index 0d49706..124ef85 100644 --- a/helm/node-doctor/templates/service.yaml +++ b/helm/node-doctor/templates/service.yaml @@ -14,6 +14,13 @@ spec: {{- if .Values.service.clusterIP }} clusterIP: {{ .Values.service.clusterIP }} {{- end }} + # Keep NotReady pods in this Service's Endpoints so Prometheus keeps scraping + # them. This Service carries no request traffic — it exists purely for metrics + # discovery — and readiness here means "this agent is degraded", not "stop + # observing this node". Dropping NotReady pods from Endpoints would silently + # blind Prometheus to precisely the unhealthy nodes, and the fleet-wide + # absent() alert would not catch a partial blackout. See values.yaml.template. + publishNotReadyAddresses: {{ .Values.service.publishNotReadyAddresses }} selector: {{- include "node-doctor.selectorLabels" . | nindent 4 }} ports: diff --git a/helm/node-doctor/values.yaml b/helm/node-doctor/values.yaml index a86c9e5..19a73e7 100644 --- a/helm/node-doctor/values.yaml +++ b/helm/node-doctor/values.yaml @@ -105,6 +105,23 @@ service: clusterIP: None httpPort: 8080 metricsPort: 9101 + # KEEP THIS TRUE. This Service exists ONLY so Prometheus can discover and scrape + # the agents (see serviceMonitor below); nothing routes request traffic through + # it, so the usual reason to exclude NotReady pods does not apply here. + # + # Readiness on this DaemonSet exists to gate rollouts and to surface a degraded + # agent — NOT to remove the pod from monitoring. With publishNotReadyAddresses + # false, Kubernetes drops NotReady pods out of the Service's Endpoints, so + # Prometheus stops scraping them and every node_doctor_* series for that node + # disappears. Since a pod goes NotReady precisely when something is wrong with + # that node's agent, turning this off would blind us to exactly the nodes we + # most need data from, at exactly the moment we need it. + # + # It would also be a SILENT blackout: NodeDoctorNoMetrics is + # absent(node_doctor_monitor_uptime_seconds), which is fleet-wide and therefore + # only fires when EVERY node stops reporting. Losing one node, or five, produces + # no alert at all. + publishNotReadyAddresses: true annotations: prometheus.io/scrape: "true" prometheus.io/port: "9101" diff --git a/helm/node-doctor/values.yaml.template b/helm/node-doctor/values.yaml.template index a939e1b..f4f71f9 100644 --- a/helm/node-doctor/values.yaml.template +++ b/helm/node-doctor/values.yaml.template @@ -105,6 +105,23 @@ service: clusterIP: None httpPort: 8080 metricsPort: 9101 + # KEEP THIS TRUE. This Service exists ONLY so Prometheus can discover and scrape + # the agents (see serviceMonitor below); nothing routes request traffic through + # it, so the usual reason to exclude NotReady pods does not apply here. + # + # Readiness on this DaemonSet exists to gate rollouts and to surface a degraded + # agent — NOT to remove the pod from monitoring. With publishNotReadyAddresses + # false, Kubernetes drops NotReady pods out of the Service's Endpoints, so + # Prometheus stops scraping them and every node_doctor_* series for that node + # disappears. Since a pod goes NotReady precisely when something is wrong with + # that node's agent, turning this off would blind us to exactly the nodes we + # most need data from, at exactly the moment we need it. + # + # It would also be a SILENT blackout: NodeDoctorNoMetrics is + # absent(node_doctor_monitor_uptime_seconds), which is fleet-wide and therefore + # only fires when EVERY node stops reporting. Losing one node, or five, produces + # no alert at all. + publishNotReadyAddresses: true annotations: prometheus.io/scrape: "true" prometheus.io/port: "9101" diff --git a/test/integration/chart_probes_test.go b/test/integration/chart_probes_test.go index 789ec96..575a34c 100644 --- a/test/integration/chart_probes_test.go +++ b/test/integration/chart_probes_test.go @@ -169,6 +169,45 @@ func TestStartupProbeBudgetHasHeadroom(t *testing.T) { } } +// TestServicePublishesNotReadyAddresses guards the observability contract that +// makes the readiness split safe to ship. +// +// The agent Service exists ONLY for Prometheus discovery (the ServiceMonitor +// selects it); no request traffic flows through it. Readiness on this DaemonSet +// now has teeth — a sustained exporter failure marks the pod NotReady — and +// Kubernetes drops NotReady pods from a Service's Endpoints unless +// publishNotReadyAddresses is true. Without it, a degraded node stops being +// scraped and every node_doctor_* series for that node disappears: we go blind +// on exactly the nodes we most need data from. +// +// It would also be SILENT. NodeDoctorNoMetrics is +// absent(node_doctor_monitor_uptime_seconds) — fleet-wide, so it only fires when +// EVERY node stops reporting. A one-node or five-node blackout raises nothing. +func TestServicePublishesNotReadyAddresses(t *testing.T) { + rendered := renderTemplate(t, "templates/service.yaml") + + var svc struct { + Spec struct { + PublishNotReadyAddresses *bool `yaml:"publishNotReadyAddresses"` + } `yaml:"spec"` + } + if err := yaml.Unmarshal([]byte(rendered), &svc); err != nil { + t.Fatalf("parse Service: %v", err) + } + + if svc.Spec.PublishNotReadyAddresses == nil { + t.Fatal("the agent Service must set publishNotReadyAddresses explicitly. " + + "Leaving it unset defaults to false, which drops NotReady pods out of Endpoints " + + "and silently stops Prometheus scraping degraded nodes.") + } + if !*svc.Spec.PublishNotReadyAddresses { + t.Error("publishNotReadyAddresses must be true. Readiness on this DaemonSet exists to " + + "gate rollouts and surface degradation, NOT to remove the pod from monitoring. " + + "Setting it false blinds Prometheus to exactly the unhealthy nodes, and the " + + "fleet-wide absent() alert cannot detect a partial blackout.") + } +} + // TestRenderedConfigEnablesHotReload guards the chart<->code contract for // #node-doctor-243: the shipped ConfigMap must actually turn hot reload on, and // must parse into the agent's config type.