Skip to content

OCPBUGS-105445: Fix shell arithmetic comparison in multus daemonset - #7170

Closed
ggiguash wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:fix/OCPBUGS-105445-multus-shell-syntax
Closed

OCPBUGS-105445: Fix shell arithmetic comparison in multus daemonset#7170
ggiguash wants to merge 1 commit into
openshift:mainfrom
redhat-chai-bot:fix/OCPBUGS-105445-multus-shell-syntax

Conversation

@ggiguash

@ggiguash ggiguash commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

The init container's timeout check used if $(( now - start > 5 * 60 )) which performs arithmetic expansion and returns the result as a string that if interprets as a command name. This causes the timeout logic to never trigger correctly.

Replace with POSIX test syntax if [ "$((now - start))" -gt 300 ] which properly compares the elapsed seconds against the 300-second (5 minute) threshold.

OCPBUGS-105445

Summary by CodeRabbit

  • Bug Fixes
    • Improved Multus DaemonSet timeout handling to reliably enforce the five-minute wait limit.

The init container's timeout check used `if $(( now - start > 5 * 60 ))`
which performs arithmetic expansion and returns the result as a string
that `if` interprets as a command name. This causes the timeout logic
to never trigger correctly.

Replace with POSIX `test` syntax `if [ "$((now - start))" -gt 300 ]`
which properly compares the elapsed seconds against the 300-second
(5 minute) threshold.

OCPBUGS-105445
@openshift-ci-robot openshift-ci-robot added jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. labels Aug 10, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@ggiguash: This pull request references Jira Issue OCPBUGS-105445, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state New, which is one of the valid states (NEW, ASSIGNED, POST)

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

Details

In response to this:

The init container's timeout check used if $(( now - start > 5 * 60 )) which performs arithmetic expansion and returns the result as a string that if interprets as a command name. This causes the timeout logic to never trigger correctly.

Replace with POSIX test syntax if [ "$((now - start))" -gt 300 ] which properly compares the elapsed seconds against the 300-second (5 minute) threshold.

OCPBUGS-105445

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.

@openshift-ci
openshift-ci Bot requested review from agullon and pmtk August 10, 2026 08:35
@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2026
@coderabbitai

coderabbitai Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Walkthrough

The Multus DaemonSet now calculates elapsed wait time with arithmetic expansion before applying the 300-second timeout.

Changes

Multus timeout handling

Layer / File(s) Summary
Update wait-loop timeout expression
assets/components/multus/06-daemonset.yaml
The CNI configuration wait loop now uses "$((now - start))" to enforce the 300-second timeout.

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

Suggested labels: ready-for-human-review


Caution

Pre-merge checks failed

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

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Container-Privileges ❌ Error The DaemonSet manifest sets hostNetwork: true (line 27) and privileged: true for kube-multus (line 83). Remove these settings if they are not required. If they are required for Multus, document the security justification and scope the exception.
✅ Passed checks (14 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the shell arithmetic comparison fix in the Multus DaemonSet.
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.
Stable And Deterministic Test Names ✅ Passed The PR changes only a Multus DaemonSet YAML timeout expression; it adds or changes no Ginkgo test titles.
Test Structure And Quality ✅ Passed PASS: The PR changes only one YAML shell condition and adds or modifies no Ginkgo It blocks, so the listed Ginkgo test-quality checks do not apply.
Microshift Test Compatibility ✅ Passed The commit changes only a Multus DaemonSet YAML timeout expression; it adds no Ginkgo e2e tests, so MicroShift test compatibility does not apply.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The only change is a shell timeout expression in a Multus DaemonSet YAML file; no new Ginkgo e2e tests or multi-node assumptions were added.
Topology-Aware Scheduling Compatibility ✅ Passed The only change fixes shell timeout arithmetic. It adds no scheduling constraint; the existing Linux nodeSelector and wildcard toleration are unchanged.
Ote Binary Stdout Contract ✅ Passed The PR changes only a Multus YAML shell condition; no OTE binary, suite setup, or process-level stdout write is added or modified.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The PR changes only one shell condition in a Multus DaemonSet YAML; it adds no Ginkgo e2e tests or networking assumptions to assess.
No-Weak-Crypto ✅ Passed The only added line changes shell timeout arithmetic to a numeric test; the PR patch contains no weak-crypto algorithms, crypto APIs, custom crypto, or secret comparisons.
No-Sensitive-Data-In-Logs ✅ Passed The patch only changes the timeout comparison. Existing logs contain timestamps and static CNI paths, with no passwords, tokens, API keys, PII, session IDs, or customer data.
✨ 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.

@coderabbitai coderabbitai Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Aug 10, 2026
@openshift-ci-robot

Copy link
Copy Markdown

@ggiguash: This pull request references Jira Issue OCPBUGS-105445, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target version (5.0.0) matches configured target version for branch (5.0.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, POST)
Details

In response to this:

The init container's timeout check used if $(( now - start > 5 * 60 )) which performs arithmetic expansion and returns the result as a string that if interprets as a command name. This causes the timeout logic to never trigger correctly.

Replace with POSIX test syntax if [ "$((now - start))" -gt 300 ] which properly compares the elapsed seconds against the 300-second (5 minute) threshold.

OCPBUGS-105445

Summary by CodeRabbit

  • Bug Fixes
  • Improved Multus DaemonSet timeout handling to reliably enforce the five-minute wait limit.

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 coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@assets/components/multus/06-daemonset.yaml`:
- Line 64: Move the timeout change from the rebase-managed generated manifest
into its upstream Multus component source, then regenerate
assets/components/multus/06-daemonset.yaml from that source. Do not manually
edit any manifest under assets/; preserve the updated timeout expression in the
regenerated output.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Pro Plus

Run ID: 0a3ac142-0b42-4a09-83ee-0c90bd148ddc

📥 Commits

Reviewing files that changed from the base of the PR and between 81f1a4a and b403ff7.

📒 Files selected for processing (1)
  • assets/components/multus/06-daemonset.yaml

Comment thread assets/components/multus/06-daemonset.yaml

@pacevedom pacevedom left a comment

Copy link
Copy Markdown
Contributor

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 added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2026
@openshift-ci

openshift-ci Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ggiguash, pacevedom

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

The pull request process is described 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

@openshift-ci

openshift-ci Bot commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

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

@ggiguash

Copy link
Copy Markdown
Contributor Author

Closing due to an unsigned commit. Will reopen from a private branch.
/close

@openshift-ci openshift-ci Bot closed this Aug 11, 2026
@openshift-ci

openshift-ci Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@ggiguash: Closed this PR.

Details

In response to this:

Closing due to an unsigned commit. Will reopen from a private branch.
/close

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.

@openshift-ci-robot

Copy link
Copy Markdown

@ggiguash: This pull request references Jira Issue OCPBUGS-105445. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state.

Details

In response to this:

The init container's timeout check used if $(( now - start > 5 * 60 )) which performs arithmetic expansion and returns the result as a string that if interprets as a command name. This causes the timeout logic to never trigger correctly.

Replace with POSIX test syntax if [ "$((now - start))" -gt 300 ] which properly compares the elapsed seconds against the 300-second (5 minute) threshold.

OCPBUGS-105445

Summary by CodeRabbit

  • Bug Fixes
  • Improved Multus DaemonSet timeout handling to reliably enforce the five-minute wait limit.

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.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-bug Indicates that a referenced Jira bug is valid for the branch this PR is targeting. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants