Skip to content

Remove NoSchedule effect from tolerations - #3105

Open
ssonigra wants to merge 1 commit into
openshift:masterfrom
ssonigra:patch-1
Open

Remove NoSchedule effect from tolerations#3105
ssonigra wants to merge 1 commit into
openshift:masterfrom
ssonigra:patch-1

Conversation

@ssonigra

@ssonigra ssonigra commented Aug 5, 2026

Copy link
Copy Markdown

Bug: https://redhat.atlassian.net/browse/OCPBUGS-77772

What

Removes the effect: NoSchedule restriction from the whereabouts-reconciler DaemonSet toleration in bindata/network/multus/multus.yaml, leaving just operator: Exists.

Why

The whereabouts-reconciler DaemonSet is the only Multus DaemonSet in multus.yaml that specifies effect: NoSchedule on its toleration. All other Multus DaemonSets (kube-multus, multus-additional-cni-plugins, multus-networkpolicy) use a catch-all operator: Exists toleration with no effect restriction.

The NoSchedule restriction prevents the whereabouts-reconciler pods from being scheduled on nodes with NoExecute or PreferNoSchedule taints, causing IP address reconciliation gaps on those nodes.

How to verify

  1. Apply a NoExecute taint to a worker node:
    oc adm taint nodes <node-name> test-taint=true:NoExecute
    
  2. Verify the whereabouts-reconciler pod is running on the tainted node:
    oc get pods -n openshift-multus -o wide | grep whereabouts-reconciler
    
  3. Confirm all other Multus DaemonSet pods continue to run normally:
    oc get ds -n openshift-multus
    

Behavioral impact

  • Before: whereabouts-reconciler pods only tolerate NoSchedule taints; pods are not scheduled on nodes with NoExecute or PreferNoSchedule taints.
  • After: whereabouts-reconciler pods tolerate all taints, matching the behavior of every other Multus DaemonSet.

Upgrade/Rollback

  • Upgrade: The CNO will reconcile the DaemonSet and roll out the updated toleration. Pods on previously untolerated nodes will be scheduled automatically.
  • Rollback: Reverting restores the NoSchedule restriction. Pods on nodes with other taint effects will be evicted by the scheduler.

Container privileges note

The hostNetwork: true, hostPID: true, and privileged: true settings flagged by static analysis are pre-existing and required by the Multus network stack — they are not introduced by this change.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: LGTM mode

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Summary by CodeRabbit

  • Bug Fixes
    • Updated scheduling tolerations for the whereabouts reconciler to improve workload placement behavior.

Walkthrough

The Multus manifest removes the NoSchedule toleration effect from the whereabouts-reconciler DaemonSet. The Exists operator toleration remains.

Changes

Multus toleration update

Layer / File(s) Summary
Update whereabouts-reconciler toleration
bindata/network/multus/multus.yaml
The NoSchedule toleration effect was removed. The Exists operator toleration remains.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: taanyas, pperiyasamy


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors, 1 warning)

Check name Status Explanation Resolution
Pr Quality ❌ Error The rationale, issue link, impact, rollback, and one-line scope are clear, but “How to verify” lists only manual oc commands and no automated CI lanes or platforms. Add a Testing section that names the automated CNO CI jobs and platforms covering this functional manifest change; do not rely only on manual cluster commands.
Unit Tests For Go Changes ❌ Error The PR modifies bindata/network/multus/multus.yaml but changes no *_test.go files, so the bindata template test requirement is not met. Add or modify a corresponding *_test.go file, or document the valid trivial-change testing justification under “How to verify it” and then use @coderabbitai ignore pre-merge checks after other checks pass.
Topology-Aware Scheduling Compatibility ⚠️ Warning The DaemonSet now has an effect-less Exists toleration, which matches the TNA arbiter's NoSchedule taint; its only selector is linux, with no arbiter exclusion. Add required node affinity excluding node-role.kubernetes.io/arbiter, while retaining only the taint tolerations needed for eligible nodes.
✅ Passed checks (21 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Commit Message Quality ✅ Passed The PR has one non-merge commit with a scoped, descriptive subject, a rationale and bug reference in the body, and one self-contained YAML change.
E2e Tests For Feature Changes ✅ Passed The PR changes only bindata/network/multus/multus.yaml; no Go files under pkg/ or cmd/ were modified, so this Go-specific E2E check is not applicable.
Rbac Least Privilege ✅ Passed The commit changes only one toleration line in multus.yaml; no ClusterRole, Role, or RBAC rules are added or modified.
Docs For Feature And Behavior Changes ✅ Passed The PR removes one toleration field to fix whereabouts-reconciler scheduling; the parent diff confirms a one-line manifest bug fix, and the check explicitly passes bug fixes without docs changes.
Stale Project Docs And Config ✅ Passed The commit changes only one toleration line; scoped docs and config contain no stale whereabouts-reconciler or NoSchedule references, and existing general statements remain accurate.
Go And Test Code Quality ✅ Passed The pull request changes only bindata/network/multus/multus.yaml; the HEAD^..HEAD diff contains no modified Go files, so none of the nine Go/test checks apply.
Ai-Generated Code Smell ✅ Passed The parent-to-current diff removes only effect: NoSchedule from an existing YAML toleration; it adds no comments, tests, variables, verbose logic, or AI references.
Stable And Deterministic Test Names ✅ Passed The PR changes only one YAML toleration line and adds no Ginkgo test declarations or test titles, so no unstable test name is introduced.
Test Structure And Quality ✅ Passed The pull request changes only one YAML line; it adds or modifies no Ginkgo tests, so the listed test-quality requirements are not applicable.
Microshift Test Compatibility ✅ Passed The pull request changes only one YAML line in multus.yaml and adds no Ginkgo e2e tests, so the MicroShift test compatibility check is not applicable.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The pull request changes only one YAML toleration line and adds no Ginkgo e2e tests, so the SNO compatibility check is not applicable.
Ote Binary Stdout Contract ✅ Passed The patch changes only one YAML toleration line; it adds no OTE binary, main, suite setup, or stdout/logging code, so the stdout contract is not applicable.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The commit changes only one YAML line in multus.yaml and adds no Ginkgo e2e tests, IPv4 assumptions, or external connectivity requirements.
No-Weak-Crypto ✅ Passed The PR patch only removes effect: NoSchedule; it adds no weak crypto, custom crypto, or secret comparisons. Existing md5sum code is unchanged from the parent.
Container-Privileges ✅ Passed The PR only deletes effect: NoSchedule; it adds no privilege-related settings. Existing hostNetwork, hostPID, and privileged declarations are unchanged from the parent commit.
No-Sensitive-Data-In-Logs ✅ Passed The commit only deletes effect: NoSchedule from a toleration; no logging statements or log payloads change, and no sensitive data is newly exposed in logs.
Title check ✅ Passed The title uses the imperative mood, stays under 72 characters, and accurately describes the toleration change.
Description check ✅ Passed The description clearly explains the change, rationale, verification steps, and behavioral impact.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@openshift-ci
openshift-ci Bot requested review from pperiyasamy and taanyas August 5, 2026 08:52
@ssonigra

ssonigra commented Aug 5, 2026

Copy link
Copy Markdown
Author

@coderabbitai regarding the pre-merge check findings:

  • Container-Privileges: hostNetwork: true, hostPID: true, and privileged: true are pre-existing settings required by the Multus network stack. They are not introduced by this PR.

  • Topology-Aware Scheduling: The catch-all operator: Exists toleration is intentional and consistent with all other Multus DaemonSets in this file (kube-multus, multus-additional-cni-plugins, multus-networkpolicy). The whereabouts-reconciler was the only outlier.

  • Unit Tests: This is a single-line YAML config fix (removing effect: NoSchedule) to align with existing DaemonSet conventions. There is no new Go logic to test. Please ignore this pre-merge check.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="bindata/network/multus/multus.yaml"

printf '%s\n' '=== Changed file and PR diff ==='
git diff -- "$file"
git diff --unified=20 HEAD^ HEAD -- "$file" || true

printf '%s\n' '=== DaemonSet names and toleration contexts ==='
rg -n -A24 -B8 'name: (whereabouts-reconciler|kube-multus|multus-additional-cni-plugins|multus-networkpolicy)|tolerations:|effect: NoSchedule|hostNetwork: true|hostPID: true|privileged: true' "$file"

Length of output: 11825


@ssonigra Verified.

The PR changes only the whereabouts-reconciler toleration in bindata/network/multus/multus.yaml.

The operator: Exists toleration now matches the other Multus DaemonSets. The privileged settings are unchanged. New Go unit tests do not apply to this YAML-only convention alignment.

I will ignore the pre-merge check findings.


✏️ Learnings added
Learnt from: ssonigra
URL: https://github.com/openshift/cluster-network-operator/pull/3105

Timestamp: 2026-08-05T08:59:27.347Z
Learning: In `bindata/network/multus/multus.yaml`, Multus DaemonSets use the catch-all toleration `operator: Exists` without an `effect`. The `whereabouts-reconciler` DaemonSet must follow this convention. The Multus network stack requires its existing host namespace and privileged container settings; do not flag unchanged settings as introduced risk in narrow manifest changes.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

You are interacting with an AI system.

Removed NoSchedule effect from tolerations in multus.yaml.
As per bug https://redhat.atlassian.net/browse/OCPBUGS-77772 , the NoSchedule Effect is causing an issue with placement of the pods on the nodes where taint NoSchedule is added also as per the standard the toleration is only operator: Exists for all the other pods.

Signed-off-by: Saurab Sonigra <ssonigra@redhat.com>
@coderabbitai

coderabbitai Bot commented Aug 5, 2026

Copy link
Copy Markdown

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

@ssonigra

ssonigra commented Aug 5, 2026

Copy link
Copy Markdown
Author

I have tested the fix and its working, correctly as expected post the fix.

@taanyas taanyas left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

lgtm

@openshift-ci

openshift-ci Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ssonigra, taanyas
Once this PR has been reviewed and has the lgtm label, please assign jcaamano 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:

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

@ssonigra

ssonigra commented Aug 5, 2026

Copy link
Copy Markdown
Author

/assign jcaamano

@ssonigra

ssonigra commented Aug 6, 2026

Copy link
Copy Markdown
Author

/test 5.0-upgrade-from-stable-4.22-e2e-gcp-ovn-upgrade

@openshift-ci

openshift-ci Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@ssonigra: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants