feat(fault-quarantine): gate events for opted-out nodes via skipNodeL… - #1572
feat(fault-quarantine): gate events for opted-out nodes via skipNodeL…#1572cbumb wants to merge 2 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughFault-quarantine now skips events for nodes matching configured label key/value pairs. Helm and TOML configuration support the labels. Metrics and tracing record skipped events. End-to-end tests cover filtering and ExternalRemediationRequest lifecycle transitions. ChangesNode-label suppression
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant HealthEvent
participant ProcessEvent
participant NodeInformer
participant EventsSkippedNodeLabel
HealthEvent->>ProcessEvent: submit node health event
ProcessEvent->>NodeInformer: read cached node labels
NodeInformer-->>ProcessEvent: return labels
ProcessEvent->>EventsSkippedNodeLabel: increment skip counter
ProcessEvent-->>HealthEvent: drop matching event
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
fault-quarantine/pkg/reconciler/reconciler.go (1)
531-535: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a descriptive loop variable.
Rename
sltoskipNodeLabel. This name identifies the configured value used for the label comparison and metric.As per coding guidelines, “Use meaningful variable names in Go code (e.g.,
syncedoverokfor cache sync checks).”🤖 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 `@fault-quarantine/pkg/reconciler/reconciler.go` around lines 531 - 535, Rename the loop variable in the SkipNodeLabels iteration from sl to skipNodeLabel, and update its Key and Value references in the label comparison, logging, and metric call while preserving the existing behavior.Source: Coding guidelines
fault-quarantine/pkg/reconciler/reconciler_e2e_test.go (1)
6722-6722: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required test-name pattern.
Add an explicit scenario and expected behavior to each test name. Add exported-function comments for the four new
fault-quarantinetest functions.
fault-quarantine/pkg/reconciler/reconciler_e2e_test.go#L6722-L6722: rename to include that the unhealthy event is skipped.fault-quarantine/pkg/reconciler/reconciler_e2e_test.go#L6788-L6788: rename to include that the healthy event does not uncordon.fault-quarantine/pkg/reconciler/reconciler_e2e_test.go#L6887-L6887: rename to include that an empty configuration quarantines the node.fault-quarantine/pkg/reconciler/reconciler_e2e_test.go#L6945-L6945: rename to include that a nonmatching value quarantines the node.tests/managed_optout_test.go#L40-L40: rename to theTestFunctionName_Scenario_ExpectedBehaviorpattern.As per coding guidelines, “Name tests descriptively following the pattern
TestFunctionName_Scenario_ExpectedBehaviorin Go” and “Include function comments for exported Go functions.”🤖 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 `@fault-quarantine/pkg/reconciler/reconciler_e2e_test.go` at line 6722, Rename the four tests at fault-quarantine/pkg/reconciler/reconciler_e2e_test.go:6722-6722, 6788-6788, 6887-6887, and 6945-6945 to follow TestFunctionName_Scenario_ExpectedBehavior, explicitly describing respectively the skipped unhealthy event, healthy event not uncordoning, empty configuration quarantining the node, and nonmatching value quarantining the node; add exported-function comments for each. Rename the test at tests/managed_optout_test.go:40-40 to the same descriptive pattern, preserving its scenario and expected behavior.Source: Coding guidelines
🤖 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 `@distros/kubernetes/nvsentinel/charts/fault-quarantine/values.yaml`:
- Around line 70-76: Remove the hard-coded managed=false CEL filters from the
rule templates referenced by the skipNodeLabels configuration, including the
expressions around NodeEventRules, quarantine, uncordon, and related actions.
Ensure the managed label exclusion is enforced only through skipNodeLabels, so
setting it to an empty list removes the exclusion while preserving other
configured label filters.
In `@docs/designs/040-external-remediation-request.md`:
- Around line 305-311: Update the remaining release-taint timing section to
remove references to cluster-scope monitors stopping emission and any
release-taint guard handling late events. Describe that cluster-scope monitors
continue emitting events, while fault-quarantine centrally checks configured
skipNodeLabels and silently drops matching healthy or unhealthy events before
state changes.
In `@fault-quarantine/pkg/reconciler/reconciler_e2e_test.go`:
- Around line 6836-6839: Update each Eventually callback in
fault-quarantine/pkg/reconciler/reconciler_e2e_test.go at lines 6836-6839,
6939-6942, and 7000-7003 to check the Nodes().Get error before accessing
node.Spec.Unschedulable; return err == nil && node.Spec.Unschedulable in all
three sites.
In `@fault-quarantine/pkg/reconciler/reconciler.go`:
- Around line 525-529: The ownership decision in the reconciler’s node lookup
must not rely solely on the potentially stale NodeInformer cache. In the
state-changing event path around GetNode, perform an authoritative or causally
synchronized label/ownership check after the ExtRR PATCH succeeds, and use that
result to decide whether to skip processing; preserve the existing behavior for
nodes confirmed as owned or released.
---
Nitpick comments:
In `@fault-quarantine/pkg/reconciler/reconciler_e2e_test.go`:
- Line 6722: Rename the four tests at
fault-quarantine/pkg/reconciler/reconciler_e2e_test.go:6722-6722, 6788-6788,
6887-6887, and 6945-6945 to follow TestFunctionName_Scenario_ExpectedBehavior,
explicitly describing respectively the skipped unhealthy event, healthy event
not uncordoning, empty configuration quarantining the node, and nonmatching
value quarantining the node; add exported-function comments for each. Rename the
test at tests/managed_optout_test.go:40-40 to the same descriptive pattern,
preserving its scenario and expected behavior.
In `@fault-quarantine/pkg/reconciler/reconciler.go`:
- Around line 531-535: Rename the loop variable in the SkipNodeLabels iteration
from sl to skipNodeLabel, and update its Key and Value references in the label
comparison, logging, and metric call while preserving the existing behavior.
🪄 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: fea49da3-0299-4388-b667-b4fa810f1d29
📒 Files selected for processing (8)
distros/kubernetes/nvsentinel/charts/fault-quarantine/templates/configmap.yamldistros/kubernetes/nvsentinel/charts/fault-quarantine/values.yamldocs/designs/040-external-remediation-request.mdfault-quarantine/pkg/config/config.gofault-quarantine/pkg/metrics/metrics.gofault-quarantine/pkg/reconciler/reconciler.gofault-quarantine/pkg/reconciler/reconciler_e2e_test.gotests/managed_optout_test.go
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
…abels (NKX-13877)
github-actions
Bot
added
area/docs
area/deployment
area/fault-management
area/tests
size/XL
labels
|
🌿 Fern Docs Preview: https://nvidia-preview-pull-request-1572.docs.buildwithfern.com/nvsentinel |
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
|
|
why can't this be dropped at the platform connector layer since that has access to the node labels IIRC? |
|
we could always cache this specific label for a shorter TTL. We have to keep in mind that the system will be eventually consistent, and we can't fire off a get call for every health event. The FQ also has an informer which is backed by a cache |
Yes both are eventually consistent. The difference is the lag. If I am not wrong FQ's informer uses a watch, so the API server pushes label changes in sub-second. A TTL cache is poll-based, so the lag equals the full TTL no matter how short we set it. Even at 30s TTL, that's 30 seconds of wrong behavior on every label flip vs under a second with the informer right? And I also confirmed with Ajay that informers on a DaemonSet are wasteful, the only option in platform-connector would be the TTL cache. FQ is a single Deployment, so one informer is fine. Both approaches work, but I thought the informer in FQ gives tighter consistency with less infrastructure cost. |
yes, but there is no guarantee. a watch drop can also result in a larger lag. Yes, we can't do informers in the daemonset it will have to be TTL based. Now that I think about it more, I'm not sure if the platform connector is the right place either. We also have to keep in mind that the number of cluster wide monitors are limited to currently HEA, CSP and KOM. The HEA case is an easy one because it doesn't publish a healthy event, so dropping the event is good enough. The CSP and KOM will need some thought because as I understand it KOM keeps state on the node object so dropping events out of it will cause the state to go out of sync with reality. The CSP health monitor also has some state that it maintains in the database. So, I would like to understand what the implications of having the checks in the monitor itself and making sure state logic if any accounts for this? |
Yeah that's the thing, each monitor handles state differently. KOM has state on the node object, CSP has state in the database, and HEA is the only clean one. Accounting for all of that means three separate implementations with different state cleanup logic. That's why I went with fault-quarantine. Monitors keep running normally and their internal state stays consistent. Events still flow through and get stored for audit. FQ just decides not to act on opted-out nodes. Nothing goes out of sync, and also future monitors get the same behavior without opting in. We also dont need to worry about someone not adding the logic in the future monitors. |
correct, this is exactly why I think it has to be per health monitor. If KOM has state saying that the node is bad and it uses that to dedup and we drop the unhealthy event then the node will never be marked once it returns. Same with the case of the CSP health monitor. I don't think we can account for these types of nuances centrally unless we build a backlog of unprocessed events and then reprocess them like cold start, but that is going to get complex quickly |
|
Adding to platform-connector or fault-quarantine are the only approaches which enforce comprehensive coverage for all health events, otherwise any health-monitor which is added would need knowledge of this label. Right now the fault-quarantine rule-set is the mechanism we have to indicate whether a given unhealthy event should contribute to a node's quarantine session. We do not have an equivalent to apply to healthy events so this PR is the first step that applies filtering across all events (I think we could add a full rule-set evaluation for healthy events or for all events later if we need functionality beyond node labels). Independent of this use-case it does seem like we should have this equivalent for healthy events. I'm trying to think what we want to do in the short-term since if we added this functionality to the global monitors without any optimizations about when it sends events for a node (such as queuing events or force-sending a healthy event when the label is removed), we'd get the same behavior as keeping it in fault-quarantine with the tradeoff of having to implement it 3 times. |
That's a fair concern. But I think the window is pretty narrow. After external remediation, the fault should normally be fixed, so the monitor would see the state change and publish a healthy event, which clears the dedup. The case where this breaks is if the fault persists after remediation AND only a cluster-scope monitor can detect it AND its dedup prevents re-publishing. That's a pretty specific edge case right? Also If I am not wrong DaemonSet monitors cold-start when the managed label is removed, so they re-detect everything fresh. This only applies to the cluster-scope ones. And if we do put the check in each monitor, we'd need to build that "remember to re-publish later" logic into each one differently, which is the same state complexity you were calling out earlier. If we're really worried about the re-detection case, I think a simpler path would be to add something in FQ that re-processes stored events when a node's skip label is removed, rather than building custom state management into each monitor. But I'd treat that as a follow-up if it turns out to be a real problem. |
I think the last point coincides with Nathan's comment |
|
if we would like to break this into two parts that is fine with me, we could start off with dropping the health events in a central place and then as a follow up we can have per health monitor handling if required. For this as well, I'm not sure if FQ is the right home for this logic because this means we drop the events after some part of the system and some downstream consumers have already consumed it. Specifically, this would mean that node conditions, the event exporter and the health event analyzer and anything downstream of these such as an observability stack or an analytics pipeline. Dropping it in the earliest possible place I think would be the most ideal so that we never end up in a state where part of the system emit signals while others don't. Previously this not much of a concern because the behaviour was consistent between per node health monitors and cluster wide health monitors. But now with the more recent changes the per node health monitor are evicted which would mean they will not suffer the similar issue. We could add a delayed processing strategy to the health events which would work as well, but that is something we'd probably want to do as a follow up if we have more use cases outside of this. I agree that having this per health monitor does complicate the logic, but in an ideal world we should have minimal number of cluster wide health monitors since most of the faults can be detected on a per node level. With that said, I also think it would be simpler than having to do event replays in FQ given that right now the biggest source for event replay would likely be the CSP health monitor since those deals with times for maintenance and it is entirely possible that a future maint notification comes while a node is being rebooted and those can't be dropped. Similar case would be with the KOM which monitoring node conditions that are set by an external entity in response to a change outside of the node/cluster. |
4 participants
Summary
Add a configurable
skipNodeLabelslist to fault-quarantine that silently drops all events (healthy and unhealthy) for nodes carrying matching labels. This prevents opted-out nodes (e.g. nodes under external remediation via ExtRR) from being quarantined or uncordoned by fault-quarantine. Default config ships withnvsentinel.dgxc.nvidia.com/managed=false. Updates ADR-040 to reflect the fault-quarantine approach for cluster-scope monitor gating.Known tradeoff: Because the gate is in fault-quarantine rather than at the monitor/platform-connector level, node status conditions set by platform-connectors may become stale for nodes under an ERR. This is a reasonable tradeoff since events should still be "observed, recorded, and exported as usual" per ADR-040, and the guard only applies to state-changing actions (quarantine, uncordon, annotation updates).
Type of Change
Component(s) Affected
Testing
Checklist
Summary by CodeRabbit
New Features
Documentation
Tests