feat(mgmt-agent): add node-health detection and labeling controller (AROSLSRE-1588)

[raelga]

Relates to AROSLSRE-1588

Implements the design in #6298 (docs/controllers/node-health.md).

What

Adds a node-health controller to mgmt-agent. It watches management-cluster Nodes, Pods, and kubelet Pod Events through shared informers and, when a SWIFT-v2 node is Ready but wedged, labels the node (node-health.aro-hcp.azure.com/status=wedged, plus detector/reason/observed-at annotations) so a separate mitigation controller can act on it. This controller does detection plus a label/unlabel flow only, it does not cordon, drain, taint, or delete nodes; all disruptive mitigation is a separate controller.

Detection is hard-coded and modular, not config-driven:

• Each fault family is a Go detector over a shared toolkit: event-signature match, a sustained-storm floor, a per-pod dwell, and the load-bearing zero-successful-start discriminator that separates a hard wedge (VF gone) from a flap.
• The reconcile core is a pure function decide(node, events, pods, now, observedSince, lastSuccessAt) with no API access, fed from a pod-by-node indexer and an event-by-node indexer keyed on Event.Source.Host, so it is exhaustively table-tested. Events are correlated to a specific Pod by InvolvedObject.UID, never by name, and are used only to classify failing pods, never counted.
• The floor and dwell come from durable Pod state: a pod counts toward the floor only once its own PodReadyToStartContainers=False condition has held past the dwell, so one long-stuck pod plus a burst of brand-new ones does not fire. Success is a recorded per-node history of PodReadyToStartContainers=True transitions, so it survives Event and Pod garbage collection.
• The one registered detector is the Azure SWIFT-v2 delegated-NIC teardown, where a node stays Ready but every pod sandbox fails with no such network interface / network is unreachable / mtpnc is not ready / DHCP-discover timeouts.
• Scope is the AKS-managed label kubernetes.azure.com/podnetwork-swiftv2-enabled=true, not a node-name allow-list.

Runtime configuration is a single operational switch delivered via a watched ConfigMap (mgmt-agent-node-health, hot-reload): enabled, a hard off switch. Detection thresholds are deliberately hard-coded as constants in each detector's Go file, not configured, so the whole detector stays a pure, exhaustively testable unit. The controller ships disabled by default.

Why

The SWIFT-v2 delegated-NIC teardown leaves a node Ready while every new pod fails to get a sandbox, so the scheduler keeps placing work on a node that cannot run it. We hit this in production (australiaeast, uksouth) and mitigated it by hand. Labeling the node lets an out-of-band consumer drain or replace it automatically. Hard-coding the detection (instead of a config-driven engine) keeps the thresholds and the zero-success rule under code review and test, where they belong, and avoids a live production lever on a safety trigger.

Testing

• go build ./..., go vet ./... clean on the mgmt-agent module.
• go test ./pkg/controller/nodehealth/...: table-driven decide() cases (NotReady, no-detector, sub-floor, some-success flap, sustained wedge, per-pod dwell not met, one-old-plus-new-does-not-fire, event-UID correlation, recorded-success recovery), the recorded per-node success history (SuccessAt), plus config parse/validate and labeler apply/clear/idempotency.
• controller_test.go: white-box controller tests covering enable-gated enqueue, per-node observation and success recording across pod add/update/delete, hot-enable observation reset, success pruning past the max window, the informer index funcs, and end-to-end syncHandler wedge/recovery. Package coverage 55.8%, detectors 78.1%.
• Helm fixture regenerated (make update-helm-fixtures).
• Live-validated on a personal dev cluster (not CSPR): clean startup and informer sync, config hot-reload, disabled=no-op, SWIFT-v2 scoping, end-to-end wedge labeling (label + detector/reason/observed-at annotations + NodeHealthLabelled Event), and recovery/unlabel.

Metrics

This PR adds a nodehealth Prometheus subsystem (all ALPHA), exposed on the existing mgmt-agent /metrics endpoint: nodehealth_detections_total{detector}, nodehealth_label_actions_total{action,result}, and nodehealth_wedged_nodes.

Before: no nodehealth_* series exist (the subsystem is new in this PR).

After (scraped from the leader mgmt-agent pod on the dev cluster, after the live wedge + recovery test):

# HELP nodehealth_detections_total [ALPHA] Number of times a detector caused a node to be labelled wedged, counted on the label transition rather than on every re-evaluation.
# TYPE nodehealth_detections_total counter
nodehealth_detections_total{detector="swift-vf-teardown"} 1
# HELP nodehealth_label_actions_total [ALPHA] Number of label/unlabel actions, by action and result.
# TYPE nodehealth_label_actions_total counter
nodehealth_label_actions_total{action="label",result="success"} 1
nodehealth_label_actions_total{action="unlabel",result="success"} 1
# HELP nodehealth_wedged_nodes [ALPHA] Number of nodes currently carrying the wedged health label, as observed by the node-health controller. Reported as 0 while the controller is disabled.
# TYPE nodehealth_wedged_nodes gauge
nodehealth_wedged_nodes 0

The counters recorded exactly one label and one unlabel over the synthetic wedge/recovery cycle; wedged_nodes is back to 0 after cleanup.

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: raelga

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [raelga]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[raelga]

/retitle WIP feat(mgmt-agent): add generic node-healer controller (AROSLSRE-1588)

[raelga]

/retitle WIP - DO NOT REVIEW YET - feat(mgmt-agent): add generic node-healer controller (AROSLSRE-1588)

[Copilot]

Pull request overview

Adds a new mgmt-agent “node-healer” controller that watches kubelet Pod Events for configured failure signatures and, when thresholds/dwell are met for allow-listed nodes, performs a safety-gated remediation workflow (cordon → drain → delete) to trigger AKS reprovisioning. The controller is configured via a watched ConfigMap (hot-reload) and ships disabled + dry-run by default, with new Prometheus metrics and unit test coverage.

Changes:

• Introduces the nodehealer controller package (config parsing/validation, signal tracking, evaluation, remediation, and metrics) plus unit tests.
• Wires the controller into mgmt-agent leader election, adds Helm templates/values, default ConfigMap, and RBAC needed to cordon/drain/delete nodes.
• Regenerates Helm golden fixture to include the new ConfigMap and deployment args.

Reviewed changes

Copilot reviewed 18 out of 18 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
mgmt-agent/zz_fixture_TestHelmTemplate_dev_westus3_mgmt_1_mgmt_agent.yaml	Updated Helm golden fixture to include node-healer ConfigMap, args, and RBAC deltas.
mgmt-agent/values.yaml	Adds default node-healer chart values (disabled + dry-run default config).
mgmt-agent/pkg/controller/nodehealer/config.go	Implements YAML config model, defaults, compilation (regex), and validation.
mgmt-agent/pkg/controller/nodehealer/config_test.go	Unit tests for config parse/override/validation/allow-list behavior.
mgmt-agent/pkg/controller/nodehealer/consts.go	Shared constants for patch fieldManager/annotations (and related controller constants).
mgmt-agent/pkg/controller/nodehealer/controller.go	Core controller loop (queue, resync, evaluation, remediation orchestration, hot-reload).
mgmt-agent/pkg/controller/nodehealer/detector.go	Node evaluation logic (safety gating + detector firing → remediation selection).
mgmt-agent/pkg/controller/nodehealer/detector_test.go	Unit tests for evaluation guards and firing/remediation behavior.
mgmt-agent/pkg/controller/nodehealer/metrics.go	Prometheus metrics definitions + registration helper.
mgmt-agent/pkg/controller/nodehealer/remediation.go	Remediation engine (cordon/drain/delete, gating, events/metrics).
mgmt-agent/pkg/controller/nodehealer/remediation_test.go	Unit tests for remediation actions (including dry-run and pod filtering).
mgmt-agent/pkg/controller/nodehealer/signals.go	Event-informer signal ingestion + sliding-window tracking logic.
mgmt-agent/pkg/controller/nodehealer/signals_test.go	Unit tests for tracker window/dwell/forget semantics.
mgmt-agent/deploy/templates/node-healer-config.yaml	New Helm template to render the watched node-healer ConfigMap.
mgmt-agent/deploy/templates/deployment.yaml	Adds node-healer configmap/key args to mgmt-agent deployment.
mgmt-agent/deploy/templates/clusterrole.yaml	Expands RBAC to support node patch/delete and pod delete for drain.
mgmt-agent/cmd/options.go	Wires node-healer informers, configmap watcher, recorder, and controller startup under leader election.
mgmt-agent/cmd/cmd.go	Updates CLI help text documenting the new controller.

Comments suppressed due to low confidence (1)

mgmt-agent/pkg/controller/nodehealer/remediation.go:201

• The remediation parameter is unused in drain(), which is likely to be reported by the enabled golangci-lint unused linter. Use it or rename it to _.

func (r *Remediator) drain(ctx context.Context, name string, dryRun bool, remediation string) error {

[raelga]

Force-pushed (WIP): rescoped this PR to phase 1 — detection + labelling only (per review feedback to ship the smallest useful increment first). Removed the cordon/drain/delete remediation path, the Confirmer/Scaler hooks, and the concurrency/cooldown/pre-scale safety machinery; the controller now sets/clears a node-healer.aro-hcp.azure.com/status=wedged label and reconciles it both ways. RBAC shrank to nodes get/list/watch/update/patch + events. Helm fixture regenerated.

Second push in this batch addresses the Copilot review: removed a dead const, made SetConfig update tracker retention on hot-reload, and shut the event broadcaster down on ctx.Done(). Still WIP — do not review yet.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

Comments suppressed due to low confidence (1)

mgmt-agent/pkg/controller/nodehealer/controller.go:125

• Tracker retention is computed only once from the initial config (Default) and never updated on hot-reload. If a ConfigMap reload increases a detector window or dwell, the tracker will keep pruning history too aggressively and firings/dwell timing will be incorrect until enough new history accumulates (or may never fire as expected). Update retention when applying a new config.

// SetConfig atomically replaces the configuration. The provided config must
// already be compiled and validated (use Parse). It also updates the tracker's
// retention so a hot-reload that widens a detector window/dwell does not prune
// older signals prematurely.
func (c *NodeHealer) SetConfig(cfg Config) {
	c.config.Store(&cfg)

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.

[raelga]

Pushed c4f2c1d addressing the latest Copilot review round (all in-scope):

• controller.go resyncAll: reset the wedgedNodes gauge to 0 on the disabled path so it can't linger stale after a hot-disable.
• clusterrole.yaml: dropped the unused update verb on nodes (the labeler only uses Nodes().Patch); kept get/list/watch/patch. Helm fixture regenerated to match.
• signals_test.go: renamed the unused dwellIgnored param to _.

Build, vet, and go test ./pkg/controller/nodehealer/... green. No behavior change beyond the gauge reset.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.

Comments suppressed due to low confidence (1)

mgmt-agent/deploy/templates/clusterrole.yaml:17

• The node-healer uses a MergePatch to modify nodes (Nodes().Patch), so the ClusterRole does not appear to need the broader 'update' verb on nodes. Dropping it would reduce privileges (least-privilege) while still supporting the current implementation.

  verbs:
  - get
  - list
  - watch
  - patch
- apiGroups:

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

mgmt-agent/pkg/controller/nodehealth/controller.go:247

• resyncAll() lists and enqueues every node on each tick. Without the node-name allow-list mentioned in the PR description, this can cause unnecessary steady-state work (per-node gather scans pods and events) and elevated API/informer cache churn once enabled. Consider scoping resync/enqueue to only nodes that can ever be candidates (e.g., name allow-list and/or the SWIFT-v2 label).

	for _, n := range nodes {
		if n.Labels[labelKey] == labelValue {
			wedged++
		}
		c.enqueue(n.Name)

mgmt-agent/pkg/controller/nodehealth/config.go:25

• The PR description says detectors/failure modes are declarative and configurable, but this package explicitly documents that detection is hard-coded in Go and that runtime config is only {enabled,dryRun}. Please align the PR description with the implemented behavior (or implement the config-driven detector engine described).

// Detection is hard-coded and modular: each fault family is a Go detector that
// reuses a shared toolkit (event-signature match, a sustained-storm floor, and
// the load-bearing zero-successful-start check) and is a pure function of the
// node's current state, so it is exhaustively unit-tested with no API server.
// The only runtime configuration is the two operational switches the rollout
// needs, enabled and dryRun; there is no config-driven detection engine.

mgmt-agent/deploy/templates/clusterrole.yaml:29

• RBAC: the new node-health controller uses shared informers/listers for pods/events and does not make direct GET calls. The added get verb on pods/events increases permissions without an apparent need; list/watch (for informers) plus create/patch/update (for event recording) should be sufficient.

- apiGroups:
  - ""
  resources:
  - pods
  verbs:
  - get

[raelga]

@copilot thanks for the review. Pushed cc7672d to address the two low-confidence points, and a note on the inline comments:

Addressed in code:

• RBAC get on pods/events (clusterrole): removed. The controller reads pods and events only through informer listers/indexers (list+watch) and records events via create/patch/update; it never issues an API get. Dropped the unused verb from both grants and regenerated the TestHelmTemplate fixture.
• resyncAll scope (controller.go): scoped the periodic sweep to nodes carrying kubernetes.azure.com/podnetwork-swiftv2-enabled=true, so we no longer enqueue and gather every node each interval. Correctness is unchanged, non-SWIFT-v2 nodes already resolved to DecisionUnknown and were never mutated.

Inline comments (label name / node-name allow-list) are stale: the PR description was reworked before this review and already matches the code, it documents the node-health.aro-hcp.azure.com/status=wedged label, hard-coded (not config-driven) detection, and scope by the SWIFT-v2 label rather than a aks-userswft* name allow-list. So there is no description-vs-code drift to fix. Note also that a node can only ever be labelled on DecisionWedged, which requires the detector's appliesTo=isSwiftV2Node, so unintended nodes cannot be labelled regardless of resync scope.

No further action or commit needed from you, this is resolved.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

mgmt-agent/cmd/cmd.go:53

• This section adds a third controller (node-health), but the earlier help text still says “mgmt-agent runs two controllers under a single leader election”. Please update that introductory sentence to “three controllers” to keep the CLI help consistent.

3. Node-health controller: watches kubelet Pod Events for hard-coded failure
   signatures (e.g. SWIFT delegated-NIC teardown that leaves a node Ready but
   unable to start pods) and, when a SWIFT-v2 node crosses a detector threshold,
   sets a health label on the node (and clears it when the node recovers) so an
   out-of-band consumer can act on it. It watches only SWIFT-v2 nodes and ships
   disabled by default.

mgmt-agent/pkg/controller/nodehealth/labeler.go:68

• label() short-circuits solely on the label being present, so it won’t restore/repair the detector/reason/observed-at annotations if they’re missing or drifted (e.g., manual edit). That can leave a node marked wedged without the explanatory metadata the controller expects to publish.

	if node.Labels[labelKey] == labelValue {
		logger.V(4).Info("node already labelled wedged")
		return false, nil
	}

mgmt-agent/pkg/controller/nodehealth/metrics.go:35

• This PR introduces new Prometheus series under the nodehealth subsystem. CONTRIBUTING.md Pull Request Standards require before/after screenshots for metrics changes; please add a /metrics scrape snippet or equivalent before/after evidence to the PR description.

const metricsSubsystem = "nodehealth"

var (
	detectionsTotal = metrics.NewCounterVec(
		&metrics.CounterOpts{
			Subsystem:      metricsSubsystem,
			Name:           "detections_total",
			Help:           "Number of times a detector caused a node to be labelled wedged, counted on the label transition rather than on every re-evaluation.",
			StabilityLevel: metrics.ALPHA,
		},
		[]string{"detector"},
	)

[raelga]

@copilot thanks, addressed the re-review:

• CLI help "two controllers" (cmd.go): fixed in e4534f6. The intro now says "three controllers", matching the numbered list that already includes the node-health controller.
• Metrics before/after evidence (metrics.go): added a "Metrics" section to the PR description per CONTRIBUTING §5. Before, no nodehealth_* series exist (the subsystem is new here). After, a real /metrics scrape from the leader mgmt-agent pod on the dev cluster shows all four ALPHA series with the values from the live wedge+recovery test (detections_total{detector="swift-vf-teardown"} 1, label_actions_total{action="label"/"unlabel",result="success"} 1, wedged_nodes 0, dry_run 0).
• labeler.go annotation repair on drift: leaving as-is by design for Phase 1. label() and unlabel() always write and remove the label and its detector/reason/observed-at annotations together in a single patch, so under controller operation they never drift. The only way to get a label without its annotations is a manual edit of controller-owned metadata, which is out of scope here; the controller re-establishes the full set on the next wedge transition. A self-heal that reconciles annotation drift on an already-labelled node would add a per-reconcile compare/patch for no operational case we have, so I'd keep it out of this PR.

No further action or commit needed from you, this is resolved.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

mgmt-agent/pkg/controller/nodehealth/labeler.go:61

• label() ignores snap.reason (the detector’s human-readable explanation) and instead stores/logs only snap.reasonString() (counts). This makes the annotationReason value inconsistent with its own docs ("human-readable explanation") and leaves snap.reason effectively unused outside tests. Consider preferring snap.reason when it’s set (falling back to reasonString() only when empty).

	reason := snap.reasonString()
	patch, err := metadataPatch(
		map[string]*string{labelKey: ptr(labelValue)},
		map[string]*string{
			annotationDetector:   ptr(detector),

mgmt-agent/pkg/controller/nodehealth/metrics.go:33

• This PR adds new /metrics series under the nodehealth_ subsystem. Per CONTRIBUTING.md Pull Request Standards, changes that affect metrics must include before/after screenshots (a /metrics scrape excerpt is usually sufficient if there’s no dashboard yet). Please add that evidence to the PR description before approval.

const metricsSubsystem = "nodehealth"

var (
	detectionsTotal = metrics.NewCounterVec(
		&metrics.CounterOpts{
			Subsystem:      metricsSubsystem,
			Name:           "detections_total",
			Help:           "Number of times a detector caused a node to be labelled wedged, counted on the label transition rather than on every re-evaluation.",
			StabilityLevel: metrics.ALPHA,
		},

[raelga]

/test images

Retesting: this is an environmental flake unrelated to this PR's diff. Job pull-ci-Azure-ARO-HCP-main-images/2082734287295090688 failed only in the aro-hcp-oc-mirror image build, at a Dockerfile step that downloads a binary from an external mirror:

RUN curl -sfL https://mirror.openshift.com/pub/openshift-v4/clients/ocp/${OC_MIRROR_4_18_VERSION}/oc-mirror.tar.gz ...
-> build aro-hcp-oc-mirror-amd64 failed with DockerBuildFailed

This PR only changes mgmt-agent (Go source, RBAC, docs) and cannot affect the oc-mirror Dockerfile. The aro-hcp-mgmt-agent image built successfully in the same run (digest sha256:53f7559925440b4eef6fde13258c6f55fe7f706a017f37769be10085d908289f). The failure is a transient external-fetch error from mirror.openshift.com, so re-running.

[raelga]

/test images

Still an environmental flake, retesting once more. The retest (build 2082738534065639424) failed again in the aro-hcp-oc-mirror image, but at a different external download step:

STEP 11/11: RUN curl -sfL https://github.com/mikefarah/yq/... -o yq.tar.gz ...
error: build error: ... exit status 6

curl exit 6 is "couldn't resolve host" (DNS), and the first failure was a different external download (mirror.openshift.com). Different steps failing across runs = non-deterministic build-node DNS/egress, not a deterministic build bug.

Evidence it is not this PR:

• image-sync/oc-mirror/Dockerfile is byte-identical to upstream/main (empty diff), so main and other PRs build the same image.
• ci/prow/images is green on the 8 most recent open PRs.
• This PR only changes mgmt-agent; its aro-hcp-mgmt-agent image built successfully in both runs.

If this recurs, I'll switch to periodic backoff rather than spam retests.

[raelga]

/test images

Periodic backoff retest (1/4). ci/prow/images keeps failing only in the aro-hcp-oc-mirror image on external curl downloads (mirror.openshift.com / github.com, exit 6 = DNS). The Dockerfile is identical to main, main's postsubmit images is green, and this PR only touches mgmt-agent. Treating as build-cluster egress/DNS flake; re-running on a fresh node.

[raelga]

/test images

Periodic backoff retest (2/4). ci/prow/images keeps failing only in the aro-hcp-oc-mirror image on external curl downloads (mirror.openshift.com / github.com, exit 6 = DNS). The Dockerfile is identical to main, main's postsubmit images is green, and this PR only touches mgmt-agent. Treating as build-cluster egress/DNS flake; re-running on a fresh node.

[raelga]

/test images

Periodic backoff retest (3/4). ci/prow/images keeps failing only in the aro-hcp-oc-mirror image on external curl downloads (mirror.openshift.com / github.com, exit 6 = DNS). The Dockerfile is identical to main, main's postsubmit images is green, and this PR only touches mgmt-agent. Treating as build-cluster egress/DNS flake; re-running on a fresh node.

[raelga]

/test images

Retesting because this is a CI network failure outside this PR's mgmt-agent change. The images job 2082756282724388864 built the new aro-hcp-mgmt-agent image successfully, then the unrelated aro-hcp-oc-mirror build failed downloading yq:

curl -sfL https://github.com/mikefarah/yq/... -zvxf yq.tar.gz
exit status 6

Another open PR's images job failed during the same period on DNS resolution while downloading the Bicep CLI, so this is not specific to this diff.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

mgmt-agent/pkg/controller/nodehealth/metrics.go:32

• This PR adds new nodehealth_* Prometheus series, which triggers the repo PR standard to include before/after screenshots for metrics changes (CONTRIBUTING.md Pull Request Standards §5). The PR description currently includes a text scrape, but not screenshots; please add the required screenshots (e.g., /metrics in the browser or a Grafana panel) before approval.

		&metrics.CounterOpts{
			Subsystem:      metricsSubsystem,
			Name:           "detections_total",
			Help:           "Number of times a detector caused a node to be labelled wedged, counted on the label transition rather than on every re-evaluation.",
			StabilityLevel: metrics.ALPHA,

mgmt-agent/pkg/controller/nodehealth/config.go:25

• The PR description says runtime config includes both enabled and dryRun, and the Metrics section shows a nodehealth_dry_run gauge. In this implementation, the config only supports enabled (no dryRun field) and the metrics registered are detections/label_actions/wedged_nodes only. Please align the PR description (and example scrape) with the actual shipped behavior, or add the missing dry-run config+metric if that’s still a requirement.

// Detection is hard-coded and modular: each fault family is a Go detector that
// reuses a shared toolkit (event-signature match, a sustained-storm floor, and
// the load-bearing zero-successful-start check) and is a pure function of the
// node's current state, so it is exhaustively unit-tested with no API server.
// The only runtime configuration is the single operational switch the rollout
// needs, enabled; there is no config-driven detection engine.

[Copilot]

Pull request overview

Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

mgmt-agent/pkg/controller/nodehealth/config.go:42

• The PR description states the node-health ConfigMap supports both enabled and dryRun, but the implementation only defines/loads a single enabled boolean (no dryRun field or handling). Please either update the PR description to match the implemented config surface, or add the missing dryRun flag to the config + controller wiring if it’s intended to ship in Phase 1.

// Config holds the node-health controller's operational switch, delivered
// via a watched ConfigMap so it can be flipped without a redeploy. Detection
// logic, thresholds, and the SWIFT-v2 node scoping are hard-coded, not
// configured.
type Config struct {
	// Enabled is a hard off switch. When false the controller still runs its
	// informers but records no state, enqueues nothing, and takes no action.
	Enabled bool `json:"enabled"`
}

mgmt-agent/pkg/controller/nodehealth/metrics.go:35

• This PR introduces new nodehealth_* metrics. CONTRIBUTING.md Pull Request Standards require before/after screenshots for metrics/observability changes; the PR should include screenshots (not just text output) demonstrating the new series on /metrics or in a dashboard/panel.

	detectionsTotal = metrics.NewCounterVec(
		&metrics.CounterOpts{
			Subsystem:      metricsSubsystem,
			Name:           "detections_total",
			Help:           "Number of times a detector caused a node to be labelled wedged, counted on the label transition rather than on every re-evaluation.",
			StabilityLevel: metrics.ALPHA,
		},
		[]string{"detector"},
	)

mgmt-agent/pkg/controller/nodehealth/metrics.go:55

• The PR description’s metrics section includes a nodehealth_dry_run gauge, but the node-health metrics registered in this PR do not define or register a dry_run metric. Please either remove it from the PR description/example scrape, or add/register the gauge (and ensure it is set from config/state).

const metricsSubsystem = "nodehealth"

var (
	detectionsTotal = metrics.NewCounterVec(
		&metrics.CounterOpts{
			Subsystem:      metricsSubsystem,
			Name:           "detections_total",
			Help:           "Number of times a detector caused a node to be labelled wedged, counted on the label transition rather than on every re-evaluation.",
			StabilityLevel: metrics.ALPHA,
		},
		[]string{"detector"},
	)

	labelActionsTotal = metrics.NewCounterVec(
		&metrics.CounterOpts{
			Subsystem:      metricsSubsystem,
			Name:           "label_actions_total",
			Help:           "Number of label/unlabel actions, by action and result.",
			StabilityLevel: metrics.ALPHA,
		},
		[]string{"action", "result"},
	)

	wedgedNodes = metrics.NewGauge(
		&metrics.GaugeOpts{
			Subsystem:      metricsSubsystem,
			Name:           "wedged_nodes",
			Help:           "Number of nodes currently carrying the wedged health label, as observed by the node-health controller. Reported as 0 while the controller is disabled.",
			StabilityLevel: metrics.ALPHA,
		},
	)
)

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

mgmt-agent/pkg/controller/nodehealth/config.go:42

• The PR description states node-health has two hot-reloadable switches (enabled + dryRun) and exposes a nodehealth_dry_run metric, but the shipped configuration (Config) only supports Enabled and there is no dry-run metric in the nodehealth subsystem. Please either implement the dryRun switch/metric as described or update the PR description to match the current Phase 1 scope (enabled-only + no dry-run metric).

// Config holds the node-health controller's operational switch, delivered
// via a watched ConfigMap so it can be flipped without a redeploy. Detection
// logic, thresholds, and the SWIFT-v2 node scoping are hard-coded, not
// configured.
type Config struct {
	// Enabled is a hard off switch. When false the controller still runs its
	// informers but records no state, enqueues nothing, and takes no action.
	Enabled bool `json:"enabled"`
}

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (4)

mgmt-agent/cmd/cmd.go:53

• The help text says the node-health controller "watches only SWIFT-v2 nodes", but the implementation starts cluster-wide Node/Pod/Event informers and only acts (labels/unlabels) when a detector applies. Please adjust this wording to avoid implying informer-level filtering.

3. Node-health controller: watches kubelet Pod Events for hard-coded failure
   signatures (e.g. SWIFT delegated-NIC teardown that leaves a node Ready but
   unable to start pods) and, when a SWIFT-v2 node crosses a detector threshold,
   sets a health label on the node (and clears it when the node recovers) so an
   out-of-band consumer can act on it. It watches only SWIFT-v2 nodes and ships
   disabled by default.

mgmt-agent/pkg/controller/nodehealth/detectors/swift_vf.go:28

• This comment says the controller "uses it to scope its node informer" to SWIFT-v2 nodes, but the controller currently does not apply a label selector to the Node informer (it only scopes the periodic resync sweep / gauge). Please reword to match the actual scoping mechanism.

	// SwiftV2LabelKey is the AKS-managed label marking a node as SWIFT-v2
	// delegated-NIC capable. A node without a delegated secondary NIC cannot
	// suffer a VF teardown, so this label scopes the swift-vf-teardown detector.
	// It is exported because the controller uses it to scope its node informer to
	// the only nodes any detector can apply to today.

mgmt-agent/pkg/controller/nodehealth/detectors/README.md:112

• The README states the controller "scopes its node informer" to SWIFT-v2 nodes, but the current wiring uses a cluster-wide Node informer (it only scopes the periodic resync sweep to SWIFT-v2). Please adjust this to avoid misleading future readers/operators.

`SwiftV2LabelKey`/`Value` are exported because the controller scopes its node
informer to SWIFT-v2 nodes (the only nodes any detector applies to today), so it
does not waste work watching nodes nothing can fire on.

mgmt-agent/values.yaml:25

• PR description claims a hot-reloadable dryRun switch and a nodehealth_dry_run metric, but the chart values/config currently expose only enabled: false. Either implement dryRun end-to-end (values/config parsing + metric) or update the PR description/metrics section so the documented operational surface matches what ships.

  # thresholds, and the SWIFT-v2 node scoping are hard-coded; the only runtime
  # config is this switch. Defaults to disabled; enable per environment or by
  # editing the ConfigMap live (hot-reload).
  config:
    enabled: false

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (5)

mgmt-agent/pkg/controller/nodehealth/config.go:56

• Config parsing uses yaml.Unmarshal, which will silently ignore unknown fields. That makes typos (or fields mentioned elsewhere, like a future “dryRun”) look like they applied but actually fall back to defaults. Using UnmarshalStrict helps fail fast on misconfiguration and avoids “enabled stayed false” surprises.

		if err := yaml.Unmarshal(data, &cfg); err != nil {

mgmt-agent/pkg/controller/nodehealth/controller.go:253

• recordPodSuccess() updates the per-node successAt map even when the controller is disabled. While disabled, observation() is never called so entries are never pruned, and over time this can grow without bound (and it also contradicts Config.Enabled’s “records no state” contract). Gate success recording on Enabled so disabled mode stays low-impact and bounded.

func (c *Controller) recordPodSuccess(obj interface{}) {
	if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok {
		obj = tombstone.Obj
	}
	pod, ok := obj.(*corev1.Pod)

mgmt-agent/pkg/controller/nodehealth/controller.go:185

• This comment says pod handlers “still record successes” while disabled, but that behavior conflicts with the Enabled contract in config.go and also risks unbounded successAt growth while disabled. If you gate recordPodSuccess on Enabled, this comment should be updated to avoid misleading future maintainers/operators.

This issue also appears on line 249 of the same file.

	// A disabled->enabled transition restarts observation: while disabled the
	// pod handlers still record successes, but the observedSince warm-up must
	// begin now so the success signal is treated as indeterminate until a full
	// window has been watched under the enabled controller.

mgmt-agent/pkg/controller/nodehealth/config.go:42

• The PR description calls out a hot-reload ConfigMap with both enabled and dryRun, and lists a nodehealth_dry_run metric. In this implementation, Config only contains enabled and the nodehealth metrics registered are detections_total/label_actions_total/wedged_nodes. Either implement dryRun end-to-end (config, wiring, metric) or update the PR description/contract to match what actually shipped.

This issue also appears on line 56 of the same file.

type Config struct {
	// Enabled is a hard off switch. When false the controller still runs its
	// informers but records no state, enqueues nothing, and takes no action.
	Enabled bool `json:"enabled"`
}

mgmt-agent/pkg/controller/nodehealth/metrics.go:31

• This PR introduces new /metrics series under the nodehealth_* subsystem. Per CONTRIBUTING.md Pull Request Standards, graph/UI/metrics changes require before/after screenshots; please add them to the PR description (e.g., a before/after /metrics scrape view or a dashboard/panel screenshot once available).

	detectionsTotal = metrics.NewCounterVec(
		&metrics.CounterOpts{
			Subsystem:      metricsSubsystem,
			Name:           "detections_total",
			Help:           "Number of times a detector caused a node to be labelled wedged, counted on the label transition rather than on every re-evaluation.",

[Copilot]

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (5)

mgmt-agent/pkg/controller/nodehealth/controller.go:210

• resyncAll() intentionally scopes to SWIFT-v2 nodes to avoid wasted work, but enqueueNodeObject() still enqueues every Node update when Enabled. Since Node objects update frequently, this can drive substantial reconcile/gather churn for nodes that no detector can ever apply to (today). Consider scoping node-driven enqueues to the current applicability label (or otherwise gating enqueues) so Enabled doesn’t amplify steady-state load cluster-wide.

func (c *Controller) enqueueNodeObject(obj interface{}) {
	if name, ok := nodeName(obj); ok {
		c.enqueue(name)
	}
}

mgmt-agent/pkg/controller/nodehealth/controller.go:256

• recordPodSuccess() updates the per-node successAt map even when the controller is disabled. That both contradicts Config.Enabled's doc comment (“records no state”) and can cause unnecessary CPU/memory churn while the controller ships disabled by default. Gate success recording on Enabled so disabled mode is truly low-impact.

func (c *Controller) recordPodSuccess(obj interface{}) {
	if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok {
		obj = tombstone.Obj
	}
	pod, ok := obj.(*corev1.Pod)

mgmt-agent/pkg/controller/nodehealth/controller.go:185

• The disabled->enabled transition comment says pod handlers record successes while disabled, but (with success recording gated by Enabled) this becomes inaccurate. Please update this comment so it reflects the actual behavior: observation warm-up restarts on enable so the controller requires a full window before trusting the success signal.

	// A disabled->enabled transition restarts observation: while disabled the
	// pod handlers still record successes, but the observedSince warm-up must
	// begin now so the success signal is treated as indeterminate until a full
	// window has been watched under the enabled controller.

mgmt-agent/pkg/controller/nodehealth/consts.go:21

• ControllerName's doc comment claims it also feeds the event recorder component and log fields, but it is only used for the workqueue name. This mismatch is misleading (and was likely left behind after removing the unused recorder-component const). Update the comment to match actual usage, or wire ControllerName into the recorder/logger setup.

	// ControllerName is the single source of truth for this controller's name.
	// It feeds the workqueue name (which surfaces as a Prometheus label), the
	// event recorder component, and log fields, so those never drift.

mgmt-agent/pkg/controller/nodehealth/metrics.go:28

• The PR description claims two runtime switches (enabled + dryRun) and a nodehealth_dry_run metric, but the implementation currently has only Config.Enabled and no dry-run metric/gauge. Please either update the PR description to match the implemented Phase-1 scope, or add the dryRun switch + metric (which will require changes across config parsing, Helm values/templates, and controller metrics wiring).

const metricsSubsystem = "nodehealth"

var (
	detectionsTotal = metrics.NewCounterVec(
		&metrics.CounterOpts{

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated no new comments.

Suppressed comments (7)

mgmt-agent/pkg/controller/nodehealth/detectors/production_evidence_test.go:72

• This hard-coded node name appears to be a real production identifier, but it is not required for the detector logic exercised by these tests. Consider switching to a clearly-redacted/example value to avoid committing production environment details.

			Name: "aks-userswft2-40171262-vmss000007",

mgmt-agent/pkg/controller/nodehealth/detectors/production_evidence_test.go:107

• This hard-coded namespace value appears to be a real production namespace, but namespace does not affect the detector logic in this test. Consider replacing it with a clearly-redacted/example value.

	ns := "ocm-arohcpprod-2rn5j76cjp7rhpkjbkgh4549ssub4at2-e9z7v8c3g8f5s0q"

mgmt-agent/pkg/controller/nodehealth/detectors/production_evidence_test.go:141

• This EventSource.Host value appears to be a real production node identifier, but Source.Host is not used by the detector logic exercised in this test. Consider replacing it with the same redacted/example host used elsewhere in this file.

			Source:         corev1.EventSource{Host: "aks-userswft2-40171262-vmss000007"},

mgmt-agent/pkg/controller/nodehealth/detectors/production_evidence_test.go:166

• This hard-coded host constant appears to be a real production node identifier, but the specific value is not required for the detector logic in this test. Consider using a redacted/example host value.

	const host = "aks-userswft2-40171262-vmss000007"

mgmt-agent/pkg/controller/nodehealth/controller.go:59

• The Controller doc comment says it has "two operational switches", but the current Config only exposes a single switch (Enabled). This mismatch is likely to confuse future maintainers and operators reading godoc.

// Controller is the level-driven controller that ties detection (the pure
// detectors.Decide function, fed from shared informers) to a label/unlabel
// reconcile of wedged nodes. Its two operational switches are hot-reloadable
// via SetConfig.

mgmt-agent/pkg/controller/nodehealth/config.go:41

• Config.Enabled is documented as "records no state", but the implementation records per-node success timestamps even while disabled (recordPodSuccess is not gated). Either gate the recording on Enabled, or adjust this comment so it matches the actual behavior.

	// Enabled is a hard off switch. When false the controller still runs its
	// informers but records no state, enqueues nothing, and takes no action.
	Enabled bool `json:"enabled"`

mgmt-agent/pkg/controller/nodehealth/detectors/production_evidence_test.go:30

• This comment embeds specific production cluster/node identifiers. Since the tests only need the message strings and SWIFT-related node shape (not the real host/cluster names), consider redacting these identifiers to avoid unnecessarily committing production environment details.

This issue also appears in the following locations of the same file:

• line 72
• line 107
• line 141
• line 166

// This file pins the detector against evidence captured from a real wedged
// production node (aks-userswft2-40171262-vmss000007, prod-uksouth-mgmt-1,
// 2026-07-27). The strings and the node shape are taken verbatim from that
// capture, so a future edit to the signatures or the applicability label that
// would have missed the real incident fails here.