Skip to content

fix: consistent dry-run behavior between NodeController and RuleContr… #422

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Saloni3494 wants to merge 1 commit into
base: main
Choose a base branch
from
+49 −11
Open

fix: consistent dry-run behavior between NodeController and RuleContr… #422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions internal/controller/node_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,10 +140,41 @@ func (r *RuleReadinessController) processNodeAgainstAllRules(ctx context.Context
continue
}

// Skip if dry run
// Handle dry run
if rule.Spec.DryRun {
log.Info("Skipping rule - dry run mode",
log.Info("Evaluating rule - dry run mode",
"node", node.Name, "rule", rule.Name)

nodeList := &corev1.NodeList{}
if err := r.List(ctx, nodeList); err != nil {

@yindia yindia Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.MatchingLabels so we only copy matching nodes

log.Error(err, "Failed to list nodes for dry run evaluation", "rule", rule.Name)
errs = append(errs, err)
continue
}

if err := r.processDryRun(ctx, rule, nodeList); err != nil {
log.Error(err, "Failed to process dry run for node",
"node", node.Name, "rule", rule.Name)
errs = append(errs, err)
continue
}

err := retry.RetryOnConflict(retry.DefaultRetry, func() error {

@yindia yindia Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

latestRule := &readinessv1alpha1.NodeReadinessRule{}
if err := r.Get(ctx, client.ObjectKey{Name: rule.Name}, latestRule); err != nil {
return err
}
patch := client.MergeFrom(latestRule.DeepCopy())
latestRule.Status.DryRunResults = rule.Status.DryRunResults
latestRule.Status.ObservedGeneration = rule.Status.ObservedGeneration
return r.Status().Patch(ctx, latestRule, patch)

@yindia yindia Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

})

if err != nil {
log.Error(err, "Failed to update rule status after dry run evaluation",
"node", node.Name, "rule", rule.Name)
errs = append(errs, err)
}
continue
}

Expand Down
25 changes: 16 additions & 9 deletions test/e2e/e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -543,15 +543,22 @@ status:
}, 10*time.Second, 2*time.Second).Should(BeTrue())

By("verifying rule has dry-run results showing what would happen")
Eventually(func() bool {
cmd := exec.Command("kubectl", "get", "nodereadinessrule", "dryrun-test-rule", "-o", "jsonpath={.status.dryRunResults}")
output, err := utils.Run(cmd)
if err != nil {
return false
}
// Check that dry run results exist and contain the node
return len(output) > 0
}, 30*time.Second, 2*time.Second).Should(BeTrue())
Eventually(func() string {
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"))

@yindia yindia Aug 19, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
}, 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"


By("updating node condition to True")
err = patchNodeCondition(nodeName, "TestReady", "True")
Expect(err).NotTo(HaveOccurred())

By("verifying rule dry-run results update to reflect the change")
Eventually(func() string {
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(BeEmpty())

By("cleaning up test resources")
exec.Command("kubectl", "delete", "node", nodeName).Run()
Expand Down