OCPBUGS-77532: Fix premature node schedulability marking in taint checking

[davidesalerno]

Summary

• Fix tolerationsTolerateTaints() to require all taints on a node to be tolerated (not just any one) before considering the node schedulable
• Add comprehensive unit tests for the taint-toleration matching logic

Context

The tolerationsTolerateTaints function in daemonsetUpdateIsSafe was returning true as soon as it found any single taint tolerated by any toleration. This meant a node with multiple taints (e.g. node-role.kubernetes.io/master and node.kubernetes.io/disk-pressure) would be considered schedulable even if only the master taint was tolerated, violating standard Kubernetes scheduling semantics.

The fix ensures every taint on a node must be tolerated by at least one toleration before returning true.

See also: #459 (review)

Test plan

• Unit tests added for tolerationsTolerateTaints covering:
  • No taints (returns true)
  • Single taint tolerated / not tolerated
  • Multiple taints: all tolerated, only some tolerated (bug scenario), none tolerated
  • Wildcard toleration matching all taints
• make build passes
• make test passes
• make verify passes

🤖 Assisted by Claude Code via /jira:solve OCPBUGS-77532

Summary by CodeRabbit

Release Notes

• Bug Fixes

  • Corrected taint tolerance evaluation for DNS DaemonSet pod scheduling to ensure accurate assessment of node compatibility.

• Tests

  • Added comprehensive test coverage for taint tolerance validation across multiple scenarios.

[openshift-ci]

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

[openshift-ci-robot]

@davidesalerno: This pull request references Jira Issue OCPBUGS-77532, which is invalid:

• expected the bug to target the "4.22.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Summary

• Fix tolerationsTolerateTaints() to require all taints on a node to be tolerated (not just any one) before considering the node schedulable
• Add comprehensive unit tests for the taint-toleration matching logic

Context

The tolerationsTolerateTaints function in daemonsetUpdateIsSafe was returning true as soon as it found any single taint tolerated by any toleration. This meant a node with multiple taints (e.g. node-role.kubernetes.io/master and node.kubernetes.io/disk-pressure) would be considered schedulable even if only the master taint was tolerated, violating standard Kubernetes scheduling semantics.

The fix ensures every taint on a node must be tolerated by at least one toleration before returning true.

See also: #459 (review)

Test plan

• Unit tests added for tolerationsTolerateTaints covering:
• No taints (returns true)
• Single taint tolerated / not tolerated
• Multiple taints: all tolerated, only some tolerated (bug scenario), none tolerated
• Wildcard toleration matching all taints
• make build passes
• make test passes
• make verify passes

🤖 Generated with Claude Code via /jira:solve OCPBUGS-77532

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

Walkthrough

Updated the tolerationsTolerateTaints function to correctly evaluate whether all taints are tolerated by checking if each taint has a matching toleration, inverting the previous logic. Added comprehensive unit tests covering multiple scenarios including no taints, single and multiple taints, empty tolerations, and wildcard tolerations.

Changes

Cohort / File(s)	Summary
Taint Tolerance Logic pkg/operator/controller/controller_dns_daemonset.go	Refactored tolerationsTolerateTaints to verify all taints are tolerated by iterating through each taint and immediately returning false if any lacks a matching toleration. Removed early-exit condition for empty taints and inverted final return logic.
Test Coverage pkg/operator/controller/controller_dns_daemonset_test.go	Added TestTolerationsTolerateTaints with comprehensive subtests covering: no taints, single tolerated/untolerated taint, multiple taints (all/partial toleration), empty tolerations list, and wildcard toleration matching.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 9 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (9 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title directly describes the main fix: correcting premature node schedulability marking in taint checking, which aligns with the core change to tolerationsTolerateTaints logic.
Stable And Deterministic Test Names	✅ Passed	Test file uses standard Go testing framework with static, non-dynamic test names in t.Run() subtests.
Test Structure And Quality	✅ Passed	Custom check for Ginkgo test code is not applicable to this standard Go unit test using testing.T framework for a utility function.
Microshift Test Compatibility	✅ Passed	PR adds standard Go unit test using *testing.T, not Ginkgo e2e tests. Custom check is specific to Ginkgo e2e tests only.
Single Node Openshift (Sno) Test Compatibility	✅ Passed	The added test TestTolerationsTolerateTaints is a standard Go unit test using the testing package, not a Ginkgo e2e test. No Ginkgo test constructs are present.
Topology-Aware Scheduling Compatibility	✅ Passed	The PR fixes the tolerationsTolerateTaints utility function to properly validate that all taints on a node are tolerated. No topology-incompatible scheduling constraints are introduced.
Ote Binary Stdout Contract	✅ Passed	This PR modifies operator controller code, not OTE test binary code. No stdout writes or test suite setup functions are present in the changes.
Ipv6 And Disconnected Network Test Compatibility	✅ Passed	The test added is a standard Go unit test using the testing package, not a Ginkgo e2e test. The custom check applies only to Ginkgo e2e tests (It(), Describe(), Context(), When(), etc.), and no such patterns were found in this PR.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign miciah for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[openshift-ci-robot]

@davidesalerno: This pull request references Jira Issue OCPBUGS-77532, which is invalid:

• expected the bug to target the "5.0.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Summary

• Fix tolerationsTolerateTaints() to require all taints on a node to be tolerated (not just any one) before considering the node schedulable
• Add comprehensive unit tests for the taint-toleration matching logic

Context

The tolerationsTolerateTaints function in daemonsetUpdateIsSafe was returning true as soon as it found any single taint tolerated by any toleration. This meant a node with multiple taints (e.g. node-role.kubernetes.io/master and node.kubernetes.io/disk-pressure) would be considered schedulable even if only the master taint was tolerated, violating standard Kubernetes scheduling semantics.

The fix ensures every taint on a node must be tolerated by at least one toleration before returning true.

See also: #459 (review)

Test plan

• Unit tests added for tolerationsTolerateTaints covering:
• No taints (returns true)
• Single taint tolerated / not tolerated
• Multiple taints: all tolerated, only some tolerated (bug scenario), none tolerated
• Wildcard toleration matching all taints
• make build passes
• make test passes
• make verify passes

🤖 Assisted by Claude Code via /jira:solve OCPBUGS-77532

Summary by CodeRabbit

Release Notes

• Bug Fixes

• Corrected taint tolerance evaluation for DNS DaemonSet pod scheduling to ensure accurate assessment of node compatibility.

• Tests

• Added comprehensive test coverage for taint tolerance validation across multiple scenarios.

Instructions 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 openshift-eng/jira-lifecycle-plugin repository.

[coderabbitai]

🧹 Nitpick comments (1)

pkg/operator/controller/controller_dns_daemonset_test.go (1)

891-899: Optional: add an explicit PreferNoSchedule case to lock intended behavior.

Current cases focus on NoSchedule/NoExecute. Adding one PreferNoSchedule case would document whether this helper should treat it as strictly untolerated or still acceptable for “safe update” purposes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/operator/controller/controller_dns_daemonset_test.go` around lines 891 -
899, Add a new test case in controller_dns_daemonset_test.go alongside the
existing cases (the table that includes the case named "wildcard toleration
tolerates all taints") that exercises a taint with Effect:
corev1.TaintEffectPreferNoSchedule and a toleration set that should indicate how
the helper treats PreferNoSchedule for safe updates; specify the tolerations and
taints explicitly and add the expected boolean (true if PreferNoSchedule should
be considered tolerated for safe updates, or false if it should be treated as
untolerated), so the intended behavior for PreferNoSchedule is documented and
enforced by the test harness that uses the same test helpers and assertions as
the surrounding cases.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/operator/controller/controller_dns_daemonset_test.go`:
- Around line 891-899: Add a new test case in controller_dns_daemonset_test.go
alongside the existing cases (the table that includes the case named "wildcard
toleration tolerates all taints") that exercises a taint with Effect:
corev1.TaintEffectPreferNoSchedule and a toleration set that should indicate how
the helper treats PreferNoSchedule for safe updates; specify the tolerations and
taints explicitly and add the expected boolean (true if PreferNoSchedule should
be considered tolerated for safe updates, or false if it should be treated as
untolerated), so the intended behavior for PreferNoSchedule is documented and
enforced by the test harness that uses the same test helpers and assertions as
the surrounding cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 12ac8b68-358c-4189-a12f-696ea67649b5

📥 Commits

Reviewing files that changed from the base of the PR and between 3d21411 and 2999248.

📒 Files selected for processing (2)

• pkg/operator/controller/controller_dns_daemonset.go
• pkg/operator/controller/controller_dns_daemonset_test.go

[openshift-ci]

@davidesalerno: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions 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. I understand the commands that are listed here.

[grzpiotrowski]

/assign

[bentito]

/assign @grzpiotrowski