docs(mgmt-agent): add node-health controller design doc (AROSLSRE-1588) - #6298
docs(mgmt-agent): add node-health controller design doc (AROSLSRE-1588)#6298raelga wants to merge 8 commits into
Conversation
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Adds a new design document for a planned management-cluster “node healer” controller and links it from the docs index. The doc describes the problem signal (kubelet Events on nodes that remain Ready), a declarative detector model, safety guardrails, and a phased rollout (label → cordon), with draining explicitly deferred.
Changes:
- Add
docs/node-healer.mddesign doc describing detection, tracking, safety guards, and phased actions. - Link the new doc from
docs/README.mdfor discoverability.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| docs/README.md | Adds an index entry pointing readers to the new node healer design doc. |
| docs/node-healer.md | Introduces the node healer design, including detectors/tracker model, safeguards, and phased remediation approach. |
Comments suppressed due to low confidence (2)
docs/node-healer.md:231
mgmt-agent/values.yamlcurrently has no node-healer defaults, so linking to it as the source of the rendered default config is misleading in this PR. Consider rephrasing to indicate this is the intended location once the follow-up implementation PR lands.
The rendered default (see [`mgmt-agent/values.yaml`](../mgmt-agent/values.yaml))
ships **disabled and in dry-run**, so enabling the healer in an environment is an
explicit, per-environment decision.
docs/node-healer.md:265
- This section points readers at
mgmt-agent/pkg/controller/nodehealerfor the implementation, but that directory does not exist in the repo state for this design-only PR. Marking it as a planned location (or follow-up PR) prevents broken links/confusion for readers reviewing this PR in isolation.
- Controller, detectors, tracker, labeler, metrics:
[`mgmt-agent/pkg/controller/nodehealer`](../mgmt-agent/pkg/controller/nodehealer).
e02c9fa to
010a142
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
docs/node-healer.md:13
- This doc says the controller "is implemented" and links to
../mgmt-agent/pkg/controller/nodehealer, but that path doesn't exist in the repo at this commit (the link is currently broken) and the PR description says the implementation is in a follow-up PR. Suggest switching to future tense and removing the dead markdown link (keep the path as inline code).
This is a design document. The controller is implemented in this repository under
[`mgmt-agent/pkg/controller/nodehealer`](../mgmt-agent/pkg/controller/nodehealer)
and ships as part of the `mgmt-agent` deployment. It is delivered in phases so we
can put detection into production and confirm it behaves before any node is ever
made unschedulable. See [Monitoring](monitoring.md) for how the metrics it emits
docs/node-healer.md:336
- This section repeats the same
mgmt-agent/pkg/controller/nodehealermarkdown link, but the target path doesn't exist in the repository at this commit, so the link is dead. Consider changing it to inline code until the implementation lands.
- Controller, detectors, tracker, labeler, metrics:
[`mgmt-agent/pkg/controller/nodehealer`](../mgmt-agent/pkg/controller/nodehealer).
- Rendered default config and the watched ConfigMap:
010a142 to
52d56ed
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
docs/node-healer.md:13
- The doc claims the controller is already implemented at
mgmt-agent/pkg/controller/nodehealer, but that package path does not exist in this PR. Since this PR is design-only and the implementation is called out as a follow-up elsewhere, reword this section to avoid a broken link / incorrect implementation claim.
This is a design document. The controller is implemented in this repository under
[`mgmt-agent/pkg/controller/nodehealer`](../mgmt-agent/pkg/controller/nodehealer)
and ships as part of the `mgmt-agent` deployment. It is delivered in phases so we
can put detection into production and confirm it behaves before any node is ever
made unschedulable. See [Monitoring](monitoring.md) for how the metrics it emits
docs/node-healer.md:5
- Spelling/terminology is inconsistent with other docs in this repo, which use US English (e.g., "labeling", "labeled"). Consider switching this doc to "labeling"/"labeled"/"unlabeled" for consistency (ideally via a global search/replace).
them out of service in phases: first by **labelling** them, then by marking them
d12f7eb to
aa3771d
Compare
…ording (AROSLSRE-1588)
|
@copilot Suppressed comment reviewed and applied. "un-done" is now "undone". I also corrected the off-switch bullet in the same push. The
That came out of the matching review on the implementation PR Azure/ARO-HCP#6284, where the same claim lived in the No further action or commit needed from you, this is resolved. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (2)
docs/controllers/node-health.md:1
- The top-level heading uses sentence case ("Node health") while other docs in this repo typically use title case (e.g., "# Monitoring", "# Pre-Merge Node Rollout Check"). Consider switching this heading to title case for consistency, which also aligns with the index entry in docs/README.md.
# Node health
docs/README.md:163
- The new entry title ("Node health") is in sentence case, but the rest of this section uses title case for link text. Consider renaming the link text to "Node Health" to match surrounding conventions (and the document title).
- [Node health](controllers/node-health.md)
|
@copilot Both applied. I checked the convention before changing it: every other top-level heading under The document heading is now No further action or commit needed from you, this is resolved. |
…f switch (AROSLSRE-1588)
…n (AROSLSRE-1588)
|
I went back over the review comments on this doc and re-checked the design against each one, since the doc has been reworked twice since then (mitigation split out, then detection-only). Two had drifted back in presentation, and I fixed both:
The rest still hold. Detection is hard-coded Go with a single boolean of config, the reconcile is driven by the kubelet Event that crosses the threshold rather than a short relist, Event data is joined with Pod state (the dwell comes from the The periodic sweep that remains is not a relist for detection. It exists because a threshold that becomes true purely because the dwell elapsed produces no new Event, and because a label left on a node that stopped being a candidate while the controller was down still needs retiring. It is scoped to the union of the SWIFT-v2 nodes and the labeled nodes rather than every node. |
…SLSRE-1588) Record the matched signature in the labeling contract and on the detection counter, and state why it stays an annotation: the wedged label is the whole selectable surface, and mitigation keys on the detector rather than on the failure mode inside a detector family.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/controllers/node-health.md:527
- The RBAC section says the ServiceAccount only needs
get/list/watchon Events, but earlier the doc states the controller emits a Kubernetes Event (NodeHealthLabeled). Creating Events requirescreate(and typically not just read verbs), so the RBAC summary is internally inconsistent.
The controller's ServiceAccount needs only: `get`/`list`/`watch` on Nodes, Pods,
and Events, `patch` on Nodes (to set/remove the label and annotations), and
`get`/`list`/`watch` on its own ConfigMap. Pods are
The summary listed Events as read-only and left out the verbs the controller actually uses. It now matches the ClusterRole: get on Nodes for the live re-check, and create/patch on Events for the recorder.
|
@copilot Correct, the RBAC summary was wrong, and it was wrong in more than the one way you flagged. Fixed in 7bdaa5b. The controller does create Events, so read-only verbs on Events were wrong. It also needs The paragraph now matches the ClusterRole in the chart exactly: No further action or commit needed from you, this is resolved. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
docs/controllers/node-health.md:311
- The Kubernetes version reference here looks incorrect: PodReadyToStartContainers moved to beta in 1.29 (enabled by default), but it became GA in 1.36 (not 1.37). Keeping this accurate matters because the controller’s safety logic is justified based on feature availability in specific cluster versions.
entirely (`PodReadyToStartContainers` is beta and on by default from Kubernetes 1.29
and GA in 1.37, so on the 1.35 management clusters it is beta-but-default-on, verified
present before enablement), so a node is never labeled wedged purely because the
AROSLSRE-1588
What
Adds a design doc,
docs/controllers/node-health.md, for the node-health controller: a management-cluster controller that detects "Ready but broken" nodes from kubelet Events and labels them so a separate mitigation controller can drive them out of service. It documents the problem (SWIFT v2 delegated-NIC nodes that stayReadywhile the kubelet cannot create pod sandboxes), the detection approach (hard-coded, modular Go detectors that share a common toolkit, driven by shared informers with an Event indexer keyed by node, computed as a pure function of node state), the load-bearing discriminator (zero successful pod-sandbox creation across the window, which separates a hard-wedge from a harmless flap), the floor and dwell measured per pod from durable Pod state, the safety guards (enabledand a Node-Ready precondition), and the scope split: this controller only detects and labels/unlabels, while every disruptive action (cordon, taint, evict, delete) belongs to a separate mitigation controller left as future work. It also records the deliberate decision to keep detection thresholds hard-coded in each detector's Go file rather than in a ConfigMap. Linked fromdocs/README.md.Why
These wedged nodes are invisible to every normal health signal: the node stays
Ready, theMultitenantPodNetworkConfig/NodeNetworkConfigCRDs stay populated, and no NodeCondition flips, yet every pod scheduled there fails withFailedCreatePodSandBox. Today an SRE has to notice the pattern and manually cordon and drain the node. This design records the detection approach and a reversible rollout (label in non-prod, then in prod), with all disruptive mitigation owned by a separate controller, so the implementation has an agreed reference and each step can be validated in production before the next is enabled.Testing
Docs-only change. All embedded Mermaid diagrams were validated with the Mermaid parser. No code or pipeline behavior changes.
Special notes for your reviewer
This is the design only. The controller (detection plus label/unlabel) is implemented in a separate PR, #6284; this doc is split out so the design can be reviewed on its own.
PR Checklist