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?