fix: consistent dry-run behavior between NodeController and RuleContr… - #422
fix: consistent dry-run behavior between NodeController and RuleContr…#422Saloni3494 wants to merge 1 commit into
Conversation
✅ Deploy Preview for node-readiness-controller canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Saloni3494 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 |
kubernetes-prow
Bot
added
cncf-cla: yes
|
Hi @Saloni3494. 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. |
kubernetes-prow
Bot
added
the
size/M
|
Hi @Saloni3494, Thanks for your PR. I see you have created multiple large PRs against the repo without the discussion context. If you are interested in working on this / other issues, I'd welcome you to bring this to our slack channel / biweekly sync to align on the directions first. I also recommend you to review the kubernetes contribution policy guidance here: https://github.com/kubernetes/community/blob/main/contributors/guide/pull-requests.md#ai-guidance. We love to have you work with us, but it'd help us if you engage with us and work on these PRs than one-shotting them! |
|
Thank you for the feedback, Ajay! I really appreciate you taking the time to review my PRs and share the contribution guidelines. I understand your point about aligning on the direction before working on larger changes. I’ll review the Kubernetes contribution policy and will make sure to engage in the Slack channel/bisweekly syncs before taking up similar issues going forward. I’d definitely like to continue contributing to this project and work more closely with the community. I’ll also revisit my current PRs based on the discussion and guidance from the maintainers. Thanks again for pointing me in the right direction! |
There was a problem hiding this comment.
Left a few comments. The bigger thing is direction.
There are two shapes in #54. This takes the minimal one, keep processDryRun separate and call it from the NodeReconciler. The other, which AvineshTripathi and farazmd each landed on independently, unifies them, let evaluateRuleForNode handle both modes, gate only the taint write on dryRun, and update stats by delta.
That second shape also gets you nodeEvaluations for dry-run rules, drops the duplicated aggregation logic, and is O(1) per node event instead of a full node scan.
Nothing was ever decided there though. Could you drop a note on #54 with which shape you'd like to take? Would rather agree that up front than have you rework this twice.
| patch := client.MergeFrom(latestRule.DeepCopy()) | ||
| latestRule.Status.DryRunResults = rule.Status.DryRunResults | ||
| latestRule.Status.ObservedGeneration = rule.Status.ObservedGeneration | ||
| return r.Status().Patch(ctx, latestRule, patch) |
There was a problem hiding this comment.
This fires on every node event regardless of whether DryRunResults actually changed, so a no-op merge patch still costs a full API round trip per node event per dry-run rule.
Could we compare against latestRule.Status.DryRunResults first and skip the patch when they're equal? We already do this kind of no-op-write suppression in the readiness condition reporter for the same scale reason (documented here https://node-readiness-controller.sigs.k8s.io/user-guide/concepts.html#optimizing-node-status-writes).
| "node", node.Name, "rule", rule.Name) | ||
|
|
||
| nodeList := &corev1.NodeList{} | ||
| if err := r.List(ctx, nodeList); err != nil { |
There was a problem hiding this comment.
This List runs inside the per-rule loop, so N matching dry-run rules means N cluster-wide lists per node event. It's a cache read, but controller-runtime still deep-copies every Node, and processDryRun then walks all of them. During a rolling upgrade that's O(nodes²).
Two fixes:
- move the List out of the rule loop so it runs once per node event
- pass the rule's nodeSelector via
client.MatchingLabelsso we only copy matching nodes
| continue | ||
| } | ||
|
|
||
| err := retry.RetryOnConflict(retry.DefaultRetry, func() error { |
There was a problem hiding this comment.
This patches the live rule with state read from the cache. If someone flips dryRun: false while a node reconcile is in flight, we can end up writing dryRunResults back onto a now-enforcing rule and reverting observedGeneration. Status is a subresource and we use GenerationChangedPredicate, so RuleReconciler never re-runs to fix it.
Can we re-check latestRule.Spec.DryRun inside the retry closure, and skip the ObservedGeneration write here?
| cmd := exec.Command("kubectl", "get", "nodereadinessrule", "dryrun-test-rule", "-o", "jsonpath={.status.dryRunResults.taintsToAdd}") | ||
| output, _ := utils.Run(cmd) | ||
| return output | ||
| }, 30*time.Second, 2*time.Second).Should(Equal("1")) |
There was a problem hiding this comment.
Unless I'm misreading the marshalling, taintsToAdd is a *int32 with omitempty, which only drops nil, and processDryRun always sets the pointer. So a zero count still serializes as "taintsToAdd": 0 and jsonpath returns "0" here, not empty.
| }, 30*time.Second, 2*time.Second).Should(Equal("1")) | |
| }, 30*time.Second, 2*time.Second).Should(Equal("0")) |
Optional, but summary might be a stronger signal than a count dropping to zero, it goes from "would add 1 taints" to "No changes needed"
|
@yindia Thanks for the detailed review and for laying out the two architectural options so clearly. I agree that the second approach, unifying the logic under evaluateRuleForNode to handle both modes and updating the stats by delta, is the stronger direction. The Before I start rewriting the PR, I wanted to check if we have consensus to officially proceed with this unified shape? I'm ready to refactor the code to match this direction once everyone is aligned. |
|
Before you start the rewrite, let's move this to Slack or the issue thread so the maintainers can chime in. Once everyone agrees on the approach, go ahead with the refactor. |
3 participants
What type of PR is this?
What this PR does / why we need it:
This PR fixes inconsistent Dry-Run evaluation behavior between the
RuleControllerandNodeController.Previously: When
DryRunwas set totrue,NodeControllerwould completely skip rule evaluation for any node updates (continue). This resulted in staleDryRunResultsbeing reported in the Rule's status when a Node's conditions transitioned, as the results would only update when the Rule itself was modified.Now:
NodeControllercatches the dry-run rules, safely evaluates them against all applicable nodes (viaprocessDryRun), and persists the refreshedDryRunResultssummary to theNodeReadinessRulestatus, giving real-time visibility into what actions would occur without actually applying the taints.An E2E test verifying this live-update behavior has also been added.
Which issue(s) this PR fixes:
Fixes #54
Special notes for your reviewer:
The update to the rule's status is done safely using
RetryOnConflictwithin theNodeControllerloop.Does this PR introduce a user-facing change?: