docs: ADR-049 MaintenanceRequest (MR) — entry door for signaling incoming node maintenance - #1577
Open
jtschelling wants to merge 15 commits into
Open
docs: ADR-049 MaintenanceRequest (MR) — entry door for signaling incoming node maintenance#1577jtschelling wants to merge 15 commits into
jtschelling wants to merge 15 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe ADR defines the ChangesExternalFault remediation API
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
🌿 Fern Docs Preview: https://nvidia-preview-pull-request-1577.docs.buildwithfern.com/nvsentinel |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/designs/049-external-fault.md (1)
232-237: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd language identifiers to the fenced blocks.
markdownlint-cli2reports MD040 for the RBAC and sequence-diagram blocks. Usetextfor these fences.Also applies to: 241-243, 249-285, 289-303
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/designs/049-external-fault.md` around lines 232 - 237, Add the `text` language identifier to the fenced code blocks in the external fault design document, including the RBAC and sequence-diagram blocks referenced by the comment, while preserving their existing contents.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/designs/049-external-fault.md`:
- Around line 224-226: Update the external-fault design section describing
duplicate-node handling: retain the informer-backed lister check only as an
early rejection, remove claims that it prevents races, and specify an
API-server-enforced uniqueness claim or reconciler-side per-node leader
mechanism to guarantee a single active EF per node under concurrent creates.
- Around line 219-223: Update the external-fault validation design so opening
health events with isHealthy=true are rejected, and make the entire
spec.healthEvent immutable after creation rather than only nodeName. Ensure all
event fields, including agent, checkName, metadata, and related payload data,
remain unchanged for the clearing event.
- Around line 166-169: Update the operator-delete flow described under “Operator
delete” to coordinate fault retraction and queued child creation before removing
the cleanup finalizer. Ensure deletion emits or otherwise guarantees the
matching isHealthy=true recovery for an opening CUSTOM event, cancels queued
child creation with deduplication, and retains the finalizer until cancellation
succeeds and downstream processing guarantees no new child ERR can be created.
- Around line 173-192: Update the fault-remediation flow and event metadata
contract to include the originating ExternalFault UID alongside
externalFaultName, and require both name and UID to match before creating or
linking an ExternalRemediationRequest. When the referenced EF is missing or the
UID differs, do not create an unowned ERR; fail closed or retry according to the
existing reconciliation behavior.
- Around line 154-160: Update the ExternalFault reconciliation state machine to
explicitly retry opening whenever FaultReported is not True, including the
Unknown state after a failed initial emission. Define stable idempotency keys
using the ExternalFault UID plus the operation phase for both opening and
clearing, and use those keys when publishing downstream events so retries cannot
create duplicate ERRs.
---
Nitpick comments:
In `@docs/designs/049-external-fault.md`:
- Around line 232-237: Add the `text` language identifier to the fenced code
blocks in the external fault design document, including the RBAC and
sequence-diagram blocks referenced by the comment, while preserving their
existing contents.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0fb7556d-1748-4e88-8078-a7643cff1223
📒 Files selected for processing (1)
docs/designs/049-external-fault.md
Contributor
Merging this branch will increase overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/designs/049-external-fault.md`:
- Around line 29-31: Update the deployment requirements for Options 1 and 2 to
mandate leader election or an equivalent single-active-replica guarantee
whenever an EF host runs more than one replica. Explicitly cover Janitor’s
--leader-elect setting and the csp-health-monitor chart’s scalable replica
configuration, while preserving Option 3’s existing leader-election requirement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: db7fdddc-3445-4e45-a25d-a5572582ec94
📒 Files selected for processing (1)
docs/designs/049-external-fault.md
…nkage, webhook race wording, MD040)
natherz97
reviewed
Aug 11, 2026
natherz97
reviewed
Aug 11, 2026
| - **Pros:** health monitors are native emitters — `csp-health-monitor` already mounts the platform-connector socket and holds a `healthpub.Publisher`, so capability (2) is free. It also already detects CSP maintenance signals (AWS/GCP), the canonical EF trigger, so the component that observes external faults would also own their lifecycle — a good fit with EF's "synthetic monitor" framing. | ||
| - **Cons:** `csp-health-monitor` is a poll/emit loop, **not** a controller-runtime app — no manager, no CRD reconcilers, no webhook server, and no leader election (it runs as a multi-replica Deployment). Hosting EF here means grafting on the entire controller-runtime + webhook stack (capability 1). It also splits the EF↔ERR pair across components: `csp-health-monitor` would import janitor's CRD types and `Owns`-watch a janitor-owned CRD, and the EF webhook would either need a new server here or stay behind in janitor. | ||
|
|
||
| ### Option 3 — standalone component |
Contributor
There was a problem hiding this comment.
I'm working on an ADR which will add a validation-controller that reconciles a ValidationRequest: #1496. We could combine both of these validation and maintenance controllers into a single node lifecycle component. What do you think?
Contributor
Author
There was a problem hiding this comment.
i think thats a good idea. i've reworked the deployment logic for this ADR, i think it will fit nicely with the ValidationRequest reconciler. Having two open ADRs for this makes merge timing a bit difficult so i've intentionally not mentioned #1496 in this ADR
natherz97
reviewed
Aug 11, 2026
natherz97
reviewed
Aug 11, 2026
|
|
||
| EF only *emits* a health event; whether that event flows through quarantine → drain → ERR depends on the rest of the pipeline being configured to act on it. Two pieces must be in place for the happy path to fire, and neither is implied by the EF reconciler itself: | ||
|
|
||
| 1. **A `fault-quarantine` ruleset that matches EF-emitted events.** `fault-quarantine` cordons only events matching one of its rulesets; a generic external-remediation event (e.g. a CSP-maintenance `CUSTOM` event) matches none of the default agent/check-specific rulesets and is skipped, so no cordon/drain occurs. The EF-emitted event must carry fields that match an existing ruleset, or a dedicated ruleset for EF-originated events must be added. |
Contributor
There was a problem hiding this comment.
The fault-quarantine module is receiving a new ruleset which maps unhealthy events to a set of validation tests which will be run after an unquarantine event occurs for the given node. We could wire this functionality into EFs to support nodes being re-validated after the given maintenance completes. I think we can track that as a follow-up but would be useful in situations where CSPs perform maintenance without terminating the underlying node objects (such as with GCP GB200 maintenance).
…not forced to CUSTOM); child-agnostic clear on child CompletionTime
…Time; deletion-as-clear via finalizer; host in new lifecycle-manager component
jtschelling
changed the title
| | `spec.healthEvent.isHealthy` is `false` (an opening event must *raise* a fault, not clear one) | ✓ | ✓ | | ||
| | Node named by `nodeName` exists in the cluster | ✓ | — | | ||
| | `spec.healthEvent` is immutable (the whole event is frozen after creation) | — | ✓ | | ||
| | No other MaintenanceRequest for the same node | ✓ | — | |
Collaborator
There was a problem hiding this comment.
I'm not sure about these checks, we could have nodes have MRs starting at different times though right?
Contributor
Author
There was a problem hiding this comment.
we could have nodes have MRs starting at different times though right?
Do you mean node X has 2 MR's created at the same time, one starting at T and the other starting at T+1?
| - The MR path depends on pipeline wiring that does not exist purely for MR: a `fault-quarantine` ruleset that matches MR-emitted events, and `fault-remediation` association-label propagation on whatever child CR it creates (only the `CUSTOM` case additionally needs the not-yet-built ERR-producing action). These must land with, or before, MR. | ||
| - The webhook's duplicate and node-existence checks use an informer-backed lister — a best-effort guard, not a transactional uniqueness constraint. | ||
|
|
||
| ## Alternatives Considered |
Collaborator
There was a problem hiding this comment.
can we add an alternative to understand why we can't use KOM to handle this use case? Is it because it is too generic and representing this in CEL would be to complex?
Contributor
|
I think this is overlapping with different already available modules:
Is this really needed since we already have overlapping modules, can't we extend them? |
natherz97
reviewed
Aug 13, 2026
natherz97
reviewed
Aug 13, 2026
…letes it, no automatic cleanup
…ifecycle-manager vs kubernetes-object-monitor policy)
…o Decision/Consequences
…KOM object-sourced-event enhancement
Comment on lines
+156
to
+157
| startTime: "2026-05-13T03:00:00Z" | ||
| healthEvent: |
Contributor
There was a problem hiding this comment.
why do we need startTime in spec? does it represent when was the maintenance requested? won't that be equal to generatedTimestamp of CR?
Comment on lines
+172
to
+174
| generatedTimestamp: "2026-05-13T02:00:00Z" | ||
| id: he-mst-c6d92aa1-2f6e-4e8b-9e3d-b75f86b1aaaa | ||
| version: 1 |
Contributor
There was a problem hiding this comment.
I couldn't understand, Why do we need generatedTimestamp, ID and version in spec.healthevent? event IDs are automatically created whenever we insert event in DB
| **Negative / tradeoffs:** | ||
| - **A forgotten MR leaves a node cordoned indefinitely.** Nothing reclaims it, so MRs become an operational surface that needs monitoring (e.g. alert on MRs well past their `startTime`). This is the main cost of dropping automatic cleanup. | ||
| - Deleting an MR always emits the clear, even if the remediation it triggered is still in flight — the node un-cordons while that remediation continues independently. | ||
| - MRs are not retained after completion, so there is no built-in historical record of completed maintenance. |
Contributor
There was a problem hiding this comment.
since we are not automatically deleting the MR now, this point is not valid anymore, right?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds ADR-049, the design for the
MaintenanceRequest(MR) CRD and reconciler — the inbound counterpart toExternalRemediationRequest(ADR-040). An external system or operator creates an MR to tell NVSentinel that maintenance is incoming for a node and it should be prepared (cordoned, drained) before the work begins, without side-stepping the quarantine/drain pipeline.Key design points
spec.healthEventunchanged (the creator picksrecommendedAction) and injects the MR name + UID into metadata; the pipeline routes it to the matching remediation (CUSTOM/external-remediation-> ERR,RESTART_VM-> RebootNode, ...). MR carries astartTimerecording when the maintenance window opens.Status.CompletionTime, deletes the MR. Deleting the MR — automatically on completion, or manually by an operator — runs the finalizer, which emits a matchingisHealthy=trueevent to retract the fault. One clearing path, no owner-reference/GC machinery.HealthEventEmittedcondition; no retained/completed objects.lifecycle-manager. The reconciler needs a controller-runtime manager, a validating webhook, a platform-connector emitter, and read access to the maintenance CRs it watches. This fits neither janitor (a pure API controller) nor csp-health-monitor (a poll/emit loop), so it lives in a new dedicated component.isHealthy=trueopening event, freezesspec.healthEventafter creation, and best-effort-rejects a duplicate MR for a node.The ADR also documents the pipeline dependencies MR relies on (a fault-quarantine ruleset that matches MR-emitted events, and fault-remediation propagating the association label onto whatever child CR it creates).
Dependencies
external-remediationERR-producing action is only needed for theCUSTOMcase).🤖 Generated with Claude Code