Inconsistent Dry-Run Behavior Between Controllers

[AvineshTripathi]

Dry-run behavior differs between RuleController and NodeController

Current Behavior

• RuleController evaluates dry-run rules and reports what would happen(here: https://github.com/kubernetes-sigs/node-readiness-controller/blob/main/internal/controller/nodereadinessrule_controller.go#L544).
• NodeController skips dry-run rules entirely, producing no output when nodes change(here: https://github.com/kubernetes-sigs/node-readiness-controller/blob/main/internal/controller/node_controller.go#L88).

Problem
When node labels or conditions change, we will get no visibility into whether rules would match or what taint operations would occur.

Question: is that by design? If no we should evaluate in nodeController too when we do dryRun=True and update the nodeStatus maybe(here: https://github.com/kubernetes-sigs/node-readiness-controller/blob/main/internal/controller/nodereadinessrule_controller.go#L350)

[Karthik-K-N]

I am not sure whats the initial idea was, But I feel its better to update the node-controller to handle the dry run case.
As of now, Since Rule controller doesn't watch for node object, If we want to see the dryRun results immediately, need to re trigger the rule once the node label changes.

[AvineshTripathi]

I think we can deprecate the standalone processDryRun and unify dry-run handling via evaluateRuleForNode. The function now returns an EvaluationOutcome, which captures both real effects and dry-run intent (e.g. WouldAddTaint, WouldRemoveTaint). A new helper, updateDryRunStats(stats, oldEval, newOutcome), updates counters using delta logic by removing the old node contribution and adding the new one.

something like

type EvaluationOutcome struct {
    Error             error
    TaintAdded        bool
    TaintRemoved      bool
    ConditionsMissing bool
    // Dry-run specific flags
    WouldAddTaint     bool
    WouldRemoveTaint  bool
}

RuleController rebuilds dry-run stats from scratch by evaluating all nodes (source of truth), while NodeController performs incremental updates during node reconciles by re-evaluating the node and applying the delta. This ensures consistent behavior, avoids skipped evaluations, and keeps dry-run logic centralized.

[ajaysundark]

Thanks for raising this issue, @AvineshTripathi. The original intent with dry-run was to keep it simple - evaluate the impact before a rule deployment. That's why it wasn't handled on Node updates. My thoughts were that the 'dryrun' results were useful only just before the deployment. I agree, it is not perfect and will be a good improvement to have a continuous evaluation / update for node changes as well. We could keep this as a backlog and could plan for it after the initial release. wdyt?

[AvineshTripathi]

@ajaysundark sgtm, thanks!

[farazmd]

Hi! I'd like to work on this if it's open. Happy to follow the approach discussed above.

[farazmd]

@ajaysundark, @AvineshTripathi

Looking at the code, the evaluateRuleForNode needs the switch block here to be wrapped around !rule.Spec.DryRun to perform actions only when DryRun=false.

Since we need to return EvaluationOutcome - I think it makes sense to handle updateNodeEvaluationStatus in the calling functions, along with the helpers to manage dryrun status. wdyt?

[sahitya-chandra]

I noticed there is already a commit addressing this in aramase/node-readiness-controller: aramase@26af2c4

It seems to re-evaluate dry-run rules from the NodeReconciler and update status.dryRunResults, with tests for node condition changes

Is someone already planning to open an upstream PR for this? If not, I’d be happy to help by opening a PR based on that
approach, with attribution preserved

[Saloni3494]

@yindia and maintainers, bringing the architectural discussion over from PR #422 as suggested.

Regarding the fix for the inconsistent dry-run behavior, yindia outlined two potential approaches. 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 O(1) scaling per node event and the removal of duplicated aggregation logic make much more sense than relying on full node scans.

Before I start rewriting the PR, I wanted to check if we have consensus here to officially proceed with this unified shape? I'm ready to refactor the code to match this direction once everyone is aligned.