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/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 26d9d3b..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" @@ -242,6 +259,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 +294,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 +317,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 for the node-doctor agent. # Do NOT lower the CPU limit back toward 200m. At 200m the agent sat 30-71% CFS-throttled # across a 13-node fleet (10m rate, measured 2026-08-12: median 44.2%, max 71.3% on diff --git a/helm/node-doctor/values.yaml.template b/helm/node-doctor/values.yaml.template index 266a04e..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" @@ -242,6 +259,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 +294,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 +317,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 for the node-doctor agent. # Do NOT lower the CPU limit back toward 200m. At 200m the agent sat 30-71% CFS-throttled # across a 13-node fleet (10m rate, measured 2026-08-12: median 44.2%, max 71.3% on 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..575a34c --- /dev/null +++ b/test/integration/chart_probes_test.go @@ -0,0 +1,249 @@ +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) + } +} + +// 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. +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") + } +}