fix(reload,health): re-initialize monitors on config change; guard probe bind ordering

[mattmattox]

Paired because both touch cmd/node-doctor/main.go and the agent's startup/reload lifecycle.

243 — config hot-reload did not re-initialize running components

I reproduced this against the real rendered chart config rather than a synthetic one. Two findings up front, because they change the shape of the fix:

• The parts people usually suspect were fine: the fsnotify watcher does fire on a Kubernetes ConfigMap atomic symlink swap (it watches the directory, and ..data arrives as a Create), and a modified monitor was being stopped and rebuilt. Both are now pinned by tests so they stay fine.
• The actual damage was in four places around that machinery.

1. Startup/reload normalization asymmetry — the severe one

main.go applied monitors.ApplyDefaultMonitors() + CLI overrides + ApplyDefaults() to the startup config. The reload path used a bare util.LoadConfig. So the two disagreed about what the configuration contained: every monitor ApplyDefaultMonitors had auto-added is absent from the file, so on the first reload it looked REMOVED and was silently stopped.

Measured with the shipped chart, editing only dns-health:

running monitor count changed from 10 to 1

The -debug / -dry-run / -log-* flags were also silently reverted by any reload. Fixed by constructing the normalizer once and installing it on the reload coordinator (SetConfigNormalizer), so both paths normalize identically.

2. diff.RemediationChanged was computed and then dropped on the floor

Editing dryRun or maxRemediationsPerHour produced a "reload succeeded" event while the registry kept its startup values until a pod restart. That is silent staleness on the exact kill-switch an operator reaches for mid-incident. Added RemediatorRegistry.ApplyConfig (+ SetMaxRemediationsPerHour) and an optional ReconfigurableRemediationExecutor interface the detector now invokes.

3. Settings-only edits reported "no changes"

ComputeConfigDiff only inspects monitors/exporters/remediation, so e.g. a settings.logFile edit fell through to a cheerful success event.

New reload.ClassifyReload separates what is genuinely re-initialized in place from what is latched at process startup:

Hot-reloadable	Requires restart (reported explicitly)
monitors (add/remove/modify)	settings.nodeName
exporter config incl. port rebind	settings.logOutput / logFile
remediation dryRun / rate limits / circuit breaker	features.enableProfiling / profilingPort
log level / format	enabling a previously-disabled exporter
	remediation.enabled false→true
	remediation.coordination.*

Restart-required changes now emit a ConfigReloadRestartRequiredwarning event naming each field and why. Per the ticket, an honest "this needs a rollout" is an acceptable outcome; pretending it applied is not.

4. reload.enabled was parsed and never read

A knob that silently does nothing is the same bug class. Now honored in both directions. It became *bool so absent (→ enabled, preserving today's behaviour for every existing deployment) is distinguishable from an explicit false, which now logs loudly that edits require a rollout.

Also

• Log level/format are re-applied on reload (routine incident action; the destination still needs a restart and says so).
• Every successful reload logs a line naming what changed:

  Config reload applied: monitors reconfigured=[dns-health] started=[] stopped=[]; remediation reconfigured=false; exporters reconfigured=false
  

243 did not need splitting — the scope stayed bounded once the root causes were identified.

246 — bind-ordering guard + liveness/readiness split

Ordering guard (both verified to fail when the ordering is inverted)

createExporters is now an explicit phase 1 (health server) / phase 2 (networked exporters), with two guards:

1. Behavioural — stubs phase 2 to block, then asserts the health endpoint already answers via runHealthCheck, the exact code path the kubelet exec probe runs.
2. AST lint — parses main.go and fails if any networked constructor is ordered ahead of startHealthServer. This catches someone inlining an exporter back in, which could otherwise slip past a test that stubs the seam.

I deliberately inverted the ordering to confirm both fail rather than passing vacuously:

--- FAIL: TestHealthEndpointServesBeforeNetworkedExporters
liveness probe exit code = 1, want 0...
--- FAIL: TestCreateExportersSourceOrdering
networked init "startNetworkedExportersFn" at main.go:561:53 runs BEFORE the health server...

Liveness vs readiness

Previously healthy was initialized true and never set false in production, so /healthz was correct only by accident, and /ready never reflected downstream failure at all.

• /healthz (liveness) consults only process-internal state, documented and tested to ignore dependency state. A broken API server must never restart the agent on the degraded nodes it exists to observe.
• /ready (readiness) gains AddReadinessCheck plus per-exporter SetDependencyStatus, fed from the detector's export loop.

Added 3-consecutive-failure hysteresis before a dependency counts: node-doctor is a DaemonSet, and one transient export error flipping every pod to NotReady would stall rolling updates fleet-wide for a blip that already healed. One success resets the counter.

Startup budget

Confirmed initialDelaySeconds 10 + (failureThreshold 20 x periodSeconds 5) = 110s. Because the health listener now provably binds in phase 1, it consumes ~none of that budget. Documented in values.yaml.template with a floor, and pinned by a chart contract test.

Not regressed

Probes remain exec against the per-pod unix socket. A new chart test fails if httpGet on 8080 is ever reintroduced, since that host port conflict still exists on a1pinode01 today. Another asserts liveness and readiness use distinct commands, and that startup uses liveness semantics (a downstream down at boot must not prevent start).

Chart

Edited values.yaml.template and configmap.yaml, regenerated values.yaml via make helm-generate; both committed. make helm-lint (which runs helm-verify-generated) passes.

Verification

• make build ✅
• make test ✅
• go test -race ./... ✅ (clean)
• go vet ./... ✅ · gofmt clean
• make helm-lint ✅

Task: #node-doctor-243
Task: #node-doctor-246

https://claude.ai/code/session_01Av4whriQf6KqJgRxGKQP7N

---

Follow-up: readiness must not blind Prometheus (added in f11cc76)

The readiness change interacted badly with the chart. The agent Service is selected by the ServiceMonitor and exists only for Prometheus discovery — no request traffic flows through it — but it did not set publishNotReadyAddresses, which defaults to false. Kubernetes therefore drops NotReady pods out of Endpoints, so a degraded node would stop being scraped and every node_doctor_* series for it would disappear.

Worse, silently: NodeDoctorNoMetrics is absent(node_doctor_monitor_uptime_seconds) — fleet-wide, firing only when every node stops reporting. A one- or five-node blackout raises nothing.

Fixed by setting publishNotReadyAddresses: true, with the reasoning recorded at both the template and values site so it isn't "cleaned up" later, plus a chart contract test (verified to fail when flipped to false). Readiness still gates rollouts and still surfaces degradation; it no longer destroys observability while doing so.

Peer discovery is unaffected — nothing in the Go code resolves this Service; it is referenced only by the ServiceMonitor and NOTES.txt.

Rollout deadlock question: it cannot deadlock — verified from source

Read pkg/controller/daemon/update.go (k8s v1.35.4) rather than relying on recollection. With maxSurge == 0 (our case — no surge, hostPort collision):

case!podutil.IsPodAvailable(oldPod, ds.Spec.MinReadySeconds, ...):
// the old pod isn't available, so it needs to be replacedallowedReplacementPods=append(allowedReplacementPods, oldPod.Name)
numUnavailable++casenumUnavailable>=maxUnavailable:
continue// only *available* old pods are budget-gated...oldPodsToDelete:=append(allowedReplacementPods, candidatePodsToDelete[:remainingUnavailable]...)

Old pods that are already unavailable are deleted unconditionally — maxUnavailable gates only candidatePodsToDelete, i.e. still-healthy old pods.

So a shared downstream failure (API server briefly unavailable, hitting all 13 pods at once) makes the rollout go faster for not-yet-updated pods: they are already unavailable, so replacing them costs no budget. The budget only throttles taking down pods that are still healthy — which is the behaviour you want — and unavailable new pods shrink remainingUnavailable, pausing further healthy-pod churn until they recover. That pause self-heals when the dependency does.

Notably this also rules out the nastier variant: "the shipped version is broken, every pod is NotReady, so I can't roll out the fix." Those pods are all in allowedReplacementPods and get replaced immediately.

Per-node metrics alert: recommend filing separately, not in this PR

absent(node_doctor_monitor_uptime_seconds) is genuinely inadequate for partial blackouts — but that is a pre-existing gap, not one this PR creates, and publishNotReadyAddresses: true removes the mechanism by which this PR would have widened it. So it is separable, and I recommend filing it. Reasoning:

• The naive forms are wrong.absent() per node needs templating per node (not viable). The metric offset 1h unless metric idiom false-fires on every legitimate node drain, scale-down, or autoscaler event — exactly the trap you flagged.
• The correct form needs a join to a "this node still exists" signal, roughly (node_doctor_monitor_uptime_seconds offset 1h unless node_doctor_monitor_uptime_seconds) and on(node) kube_node_info. That introduces a hard dependency on kube-state-metrics that this chart has nowhere else, plus label-alignment assumptions I cannot verify without cluster access (I confirmed our metric carries a node label; I cannot confirm the kube-state-metrics side from here, and I'm barred from touching the cluster).
• The for: window needs tuning against real rollout gaps, or every deploy pages. That wants observation data, not a guess.

Shipping an alert that false-fires on routine node lifecycle is worse than the status quo, because it trains people to ignore it. up{job="node-doctor"} == 0 is a cheaper partial-credit option (it disappears rather than firing when a node is removed) and may be the right first step — but it is still a real design decision deserving its own PR.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit:9c943bc63d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Detect per-minute remediation-limit changes

When an operator changes only remediation.maxRemediationsPerMinute, remediationEqual in pkg/reload/diff.go still considers the configurations equal because it compares only enabled, dry-run, the hourly limit, and circuit-breaker fields. Consequently this guard is false, ApplyConfig is never called, and the running token bucket keeps the old safety limit even though the new implementation explicitly supports reloading it.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Stop exporters when they are disabled

Handle the true-to-false transition as restart-required or actually stop/remove the exporter. The built-in Reload implementations merely replace their configuration and do not inspect Enabled, while the detector continues calling every instance in pd.exporters; therefore changing an HTTP, Kubernetes, or Prometheus exporter from enabled to disabled is reported as successfully reconfigured but it continues sending data or serving its listener.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Detect monitor remediation and dependency edits

Before relying on ComputeConfigDiff to drive monitor reinitialization, include all behavior-bearing monitor fields in its equality check. monitorsEqual currently omits Remediation and DependsOn, so an edit limited to a monitor's remediation strategy or dependency list yields no diff, skips the callback, and leaves monitorConfigIndex and dependency behavior at their previous values while the coordinator adopts the new file.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Apply log-only changes before adopting the config

When a ConfigMap edit changes only settings.logLevel or settings.logFormat, ConfigDiff.HasChanges() is false and ClassifyReload does not mark either field restart-required. This branch therefore returns without invoking handleConfigReload, so reapplyLoggingConfig never runs and the advertised live log-level/format change has no effect, even though the coordinator records the new config as current.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Feed webhook delivery failures into readiness

For the HTTP exporter, the error passed here reports only whether SubmitStatusRequest could enqueue work; actual webhook requests run asynchronously and failures are recorded later by the worker. Thus an unreachable or consistently failing webhook normally produces err == nil on every export and repeatedly clears exporter/http, so /ready remains healthy despite the downstream failure this new wiring is intended to expose.

Useful? React with 👍 / 👎.