Check rule generation before updating the staus #44
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Karthik-K-N 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 |
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XS
✅ Deploy Preview for node-readiness-controller canceled.
|
| return err | ||
| } | ||
|
|
||
| if latestRule.GetGeneration() != rule.GetGeneration() { |
There was a problem hiding this comment.
The core of the issue seems to me that node-reconciler overwrites correct (recent) values with stale/empty values from cache.
For instance:
- T0: NodeReconciler starts rule.gen=1
- T1: RuleReconciler updates rule.gen=2
- T2: NodeReconciler updates with copying rule from cache?
I think instead of calling the full updateRuleStatus(), NodeReconciler should use Patch ONLY the fields it modifies.
There was a problem hiding this comment.
Let me try this and see if it helps.
There was a problem hiding this comment.
NodeReconciler should use Patch ONLY the fields it modifies.
This will be the best solution of all.
There was a problem hiding this comment.
My chances here: ajaysundark@0f1b4fc. Haven't gotten a chance to test this fully yet
There was a problem hiding this comment.
#49 for your review fixing the status updates.
rule generation mismatch suggests the node reconciliation is dealing with a older rule and next rule-reconciliation should handle this pending node update anyway; so, including this check makes sense (could be an optimization to save some redundant reconciliation cycles).
|
|
||
| if latestRule.GetGeneration() != rule.GetGeneration() { | ||
| log.V(4).Info("Rule generation mismatch during status update, avoiding retry to let new reconciliation handle it") | ||
| return nil |
There was a problem hiding this comment.
I think returning here silently is problematic as it would skip the node update entirely..
3 participants
Since both node controller and node readiness controller trying to update the status of rule, There may be conflict where the status updated by the NRR controller overriden by node controller, this tries to match the rule generation and make sure both are at the same level before updating the status,
From Error logs
Fixes: #43