fix: skip NodeEvaluation upsert when evaluation did not run#218
fix: skip NodeEvaluation upsert when evaluation did not run#218sahitya-chandra wants to merge 2 commits into
Conversation
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sahitya-chandra The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @sahitya-chandra. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
needs-ok-to-test
processNodeAgainstAllRules unconditionally wrote a NodeEvaluation entry for the node it just processed, even when evaluateRuleForNode returned an error before updateNodeEvaluationStatus could populate one. The zero-value NodeEvaluation either clobbered a valid prior entry or appended one with an empty NodeName. The CRD requires NodeName MinLength=1, so the API server rejected the whole status patch with 422, and the FailedNodes update bundled into the same patch was lost along with it. Skip the upsert when the in-memory rule has no evaluation for this node, and let the FailedNodes update through on its own. Add a regression test that fails Patch on the node, asserts FailedNodes is recorded, and asserts no empty NodeEvaluation slips into status.
sahitya-chandra
force-pushed
the
fix/empty-nodeevaluation-status-patch
branch
from
339a0ac to
169d820
Compare
k8s-ci-robot
removed
the
needs-rebase
|
/cc @AvineshTripathi could you please take a look at this PR whenever you get some free time? It has been open for quite a while now. |
| // Upsert the node's evaluation only after a successful evaluation. | ||
| // On the failure path evaluateRuleForNode returns before recording a | ||
| // fresh NodeEvaluation, so this must leave any existing persisted | ||
| // evaluation untouched and only persist FailedNodes below. |
There was a problem hiding this comment.
I view this more of a UX improvement than a bug fix --
- Currently if
evaluateRuleForNodefails for a node, it patches the rule status by overriding with NodeEvaluation{}, erasing previous (if existing) eval result. This change prevents from doing it, but the risk is possibly leaving a stale result for that rule. - We already capture the node failures at rule.status.failedNodes which should reflects recent evaluation failures for the node.
We have discussed moving the status object to a separate 'NodeReadinessEvaluation' CRD (in v1alpha2) to capture per rule results. This flow could look like --
processNodeAgainstAllRules {
NodeReadinessEvaluationCR {
// record result per rule
}
}
instead of double looping and x-updates we are currently doing here.
Assuming we move to a 1--1 evaluation result present for each Node evaluated by NodeReadinessController, how would the rule-evaluation be structured? and how should we record per rule failures in it?
@Karthik-K-N I think we should prioritize evaluating this to establish a cohesive observability for NRC ideally aligning it earlier with the mentorship timeline as there are some observability focus in it.
There was a problem hiding this comment.
If we have multiple rules per node and one among the rule evaluation fails we capture them in the ReadinessReport: https://github.com/kubernetes-sigs/node-readiness-controller/pull/133/changes#r2857725522
There was a problem hiding this comment.
Thanks @ajaysundark and @Karthik-K-N, I just went through #133, and the NodeReadinessRuleReport (nrrp) CRD makes sense to me. I agree a per-node report keyed by ruleName is cleaner for these results than the NodeEvaluations[] list inside the rule status, so I'm happy to align with that.
One small thing on the "UX or bug" point: I think it is a bit more than UX today. On the failure path, when the node has no prior cache NodeEvaluation (i.e. first evaluation), currEval is the zero value, so the patch carries NodeEvaluation{}. Its required fields are omitempty, so it serializes to {} and the apiserver rejects the whole Status().Patch with a 422. failedNodes is in that same patch, so it gets dropped too. So on that path the operator sees nothing, not even failedNodes, just a log line.
I checked this with envtest against the apiserver: on main the failed node ends with failedNodes empty, and with this change it persists. So I feel failedNodes only really captures the failure once we stop including the empty eval in the patch. They still go out in the same Status().Patch() call, but without the invalid {} entry the 422 no longer fires
On the stale result point, I agree. The idea here is just to keep the last good eval instead of erasing it, but I'm fine dropping the eval on failure instead if you prefer
Either way, I'm happy to keep this as a small v1alpha1 fix and leave the bigger change to the report work :)
4 participants
Description
The bug: When adding or removing a node's taint fails with a non-retryable error (for example an RBAC denial or a persistent API error),
evaluateRuleForNodereturns early without recording an evaluation for that node.processNodeAgainstAllRulesthen still tried to save aNodeEvaluationfor it, and with nothing recorded it saved an empty one. Every required field on that type usesomitempty, so an empty value serializes to{}and the API server rejects the entire status patch with a 422 (Required valuefornodeName,conditionResults,taintStatus, andlastEvaluationTime). The empty {} NodeEvaluation validation failure occurs when the node has no cached NodeEvaluation. If an existing cached entry is present, the old code avoids the empty object but can still overwrite fresher persisted status with stale cached evaluation data. TheFailedNodesupdate rides in that same patch, so it is dropped too. The failure then never appears inkubectl get nodereadinessrule -o yaml, and the only trace is a controller log line.The fix: Save the
NodeEvaluationonly when the evaluation succeeded. On the failure path, leave any existingNodeEvaluationuntouched. Since FailedNodes rides in the sameStatus().Patch()call, omitting the invalid empty NodeEvaluation prevents the 422 that was silently dropping it.Related Issue
Fixes #217
Type of Change
/kind bug
Testing
make testpasses locallymake lintpasses locallyPatchon the node, runsNodeReconciler.Reconcile, and asserts that theFailedNodesentry lands, no emptyNodeEvaluationslipped in, and an unrelated pre-existingNodeEvaluationwas preserved. Without the fix, the test fails on the empty-NodeNameassertionTaintStatus=Absent, the cached rule snapshot has staleTaintStatus=Present, evaluation fails, and the persisted entry must stayAbsentChecklist
make testpassesmake lintpassesDoes this PR introduce a user-facing change?