[#376] fix: clear stale FailedNodes entry in node reconciler after successful evaluation - #430
Conversation
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: tejassinghbhati 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 @tejassinghbhati. 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. |
ec411f2 to
1c09752
Compare
…l evaluation After a transient evaluation error, processNodeAgainstAllRules was permanently re-asserting the stale FailedNodes entry for the node on every subsequent reconcile, even when evaluateRuleForNode succeeded. The status patch loop rebuilt FailedNodes in two passes: the first dropped this node's entry from the API server's current state, and the second unconditionally re-appended whatever was in the cached rule copy (rule.Status.FailedNodes). Because a successful evaluation never cleared the cached copy's FailedNodes field, pass 2 actively undid the cleanup done by pass 1. The fix gates the re-append on evalErr != nil, mirroring the equivalent logic already present in processAllNodesForRule (rule reconciler path), which uses an else branch to only clear on success. This is the unfixed mirror of the processAllNodesForRule fix: that fix landed only in nodereadinessrule_controller.go and the NodeReconciler path in node_controller.go was never updated. Also adds a regression test using a fake client with WithStatusSubresource, which seeds a stale FailedNodes entry, triggers a successful reconcile, and asserts the entry is absent afterwards. Signed-off-by: Tejas Singh Bhati <tejassinghbhati077@gmail.com>
1c09752 to
87ec116
Compare
|
PR needs rebase. 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. |
|
Duplicate Of #381 /close |
|
@ajaysundark: Closed this PR. DetailsIn response to this:
Instructions 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. |
Summary
processNodeAgainstAllRulespermanently re-asserts a staleStatus.FailedNodesentry for a node on every reconcile, even afterevaluateRuleForNodesucceeds, until the rule is manually edited.This is the unfixed mirror of the fix that landed for
processAllNodesForRule— that fix only touchednodereadinessrule_controller.go. The node reconciler path innode_controller.gowas never updated with the equivalent clearing logic.Root cause
The status patch loop rebuilds
FailedNodesin two passes. Pass 1 correctly drops this node's current entry from the API server's state. Pass 2 unconditionally re-appends whatever the cachedrule.Status.FailedNodesholds for the node — regardless of whether the evaluation just succeeded. Because a successful evaluation never clears the cached copy'sFailedNodesfield, pass 2 actively re-asserts the stale entry on every node reconcile.Fix
Gate pass 2 on
evalErr != nil, matching theelsebranch already present inprocessAllNodesForRule:Testing
Added a regression test in
node_controller_test.gousing a fake client withWithStatusSubresource:FailedNodesentry for the node to simulate a prior transient error.processNodeAgainstAllRuleswith the node satisfying all conditions (evaluation succeeds).FailedNodesentry is absent from the patched rule status.go build ./...passes cleanly. The controller test suite requiresenvtestbinaries (etcd / kube-apiserver) which are unavailable locally on Windows; CI runs onubuntu-latestwhere they are present.Related Issue
Fixes #376
Type of Change
/kind bug
Checklist
make testpasses (CI; envtest not available locally on Windows)go build ./...passesDoes this PR introduce a user-facing change?