fix: reconcile retry on rule processing errors#222
fix: reconcile retry on rule processing errors#222dorodb-web22 wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dorodb-web22 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 |
|
Welcome @dorodb-web22! |
|
Hi @dorodb-web22. 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 Regular contributors should join the org to skip this step. 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. |
Karthik-K-N
left a comment
There was a problem hiding this comment.
Nice addition, Thank you
| }) | ||
|
|
||
| Context("when rule status patch fails during node reconciliation", func() { | ||
| It("should return an error so the workqueue requeues", func() { |
There was a problem hiding this comment.
we are just checking if there is expected error and we are never checking workqueue length, its assumption that it requeue.
Do you think its better to rename test?
There was a problem hiding this comment.
Good point! i renamed it to "should return an error when rule status patch fails". since we're just checking the error returned rather than inspecting the workqueue directly, this is definitely more accurate.
|
/ok-to-test |
Description
processNodeAgainstAllRuleshad no return value, soReconcilealways returnedctrl.Result{}, nileven when rule evaluation or status patch calls failed. This caused the workqueue to treat every reconciliation as successful, skipping backoff and requeue on transient errors. Node taint state could drift silently until an unrelated event triggered a new reconcile.This change makes
processNodeAgainstAllRulesreturn an aggregated error viaerrors.Joinand propagates it throughReconcile, consistent with howRuleReconciler.processAllNodesForRulealready handles errors.Adds a regression test that simulates a status patch failure using the interceptor pattern and verifies
Reconcilereturns a non-nil error.Related Issue
Fixes #220
Type of Change
/kind bug
Testing
make test
make lint
Checklist
Does this PR introduce a user-facing change?
NONE