OCPBUGS-83863: Remove version-specific CNI binary copy logic#2967
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@sdodson: This pull request references Jira Issue OCPBUGS-83863, which is valid. 3 validation(s) were run on this bug
The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughRemoved OS/version-based CNI source selection and replaced it with a single required ChangesMultus manifests
OVN Kubernetes script lib & cni copy
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bindata/network/multus/multus.yaml (1)
24-26: Also validate thatDEFAULT_SOURCE_DIRECTORYis an existing directory.Right now only variable presence is validated. Add a
-dcheck so failures are explicit and happen before copy-time exits.Suggested patch
if [ -z "$DEFAULT_SOURCE_DIRECTORY" ]; then log "FATAL ERROR: You must set the DEFAULT_SOURCE_DIRECTORY env variable" exit 1 fi +if [ ! -d "$DEFAULT_SOURCE_DIRECTORY" ]; then + log "FATAL ERROR: DEFAULT_SOURCE_DIRECTORY ($DEFAULT_SOURCE_DIRECTORY) does not exist" + exit 1 +fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/network/multus/multus.yaml` around lines 24 - 26, The script currently checks only for presence of DEFAULT_SOURCE_DIRECTORY; modify the validation around that variable (the if block referencing DEFAULT_SOURCE_DIRECTORY and the log function) to also verify it is an existing directory (use a -d style check), and if the check fails call log with a clear fatal message including the variable name and then exit 1 so failures occur immediately before any copy operations.bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)
500-503: Prefer deriving FCOS major dynamically instead of hardcoding9.Line 502 hardcodes Fedora CoreOS to RHEL9, which means this path won’t pick
rhel10+directories without another code change. Consider deriving/probing dynamically to keep the new fallback logic future-proof.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml` around lines 500 - 503, The fedora) branch currently hardcodes rhelmajor=9 for FCOS (VARIANT_ID == "coreos"); replace that hardcoded assignment with runtime detection: inside the fedora) block (where VARIANT_ID and rhelmajor are used) probe the host image to derive the RHEL major version (e.g., parse /etc/os-release fields like VERSION_ID/ID_LIKE or run an rpm macro query such as rpm -E '%{rhel}' as a fallback) and set rhelmajor to the detected major number, falling back to a sensible default only if detection fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bindata/network/multus/multus.yaml`:
- Around line 24-26: The script currently checks only for presence of
DEFAULT_SOURCE_DIRECTORY; modify the validation around that variable (the if
block referencing DEFAULT_SOURCE_DIRECTORY and the log function) to also verify
it is an existing directory (use a -d style check), and if the check fails call
log with a clear fatal message including the variable name and then exit 1 so
failures occur immediately before any copy operations.
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 500-503: The fedora) branch currently hardcodes rhelmajor=9 for
FCOS (VARIANT_ID == "coreos"); replace that hardcoded assignment with runtime
detection: inside the fedora) block (where VARIANT_ID and rhelmajor are used)
probe the host image to derive the RHEL major version (e.g., parse
/etc/os-release fields like VERSION_ID/ID_LIKE or run an rpm macro query such as
rpm -E '%{rhel}' as a fallback) and set rhelmajor to the detected major number,
falling back to a sensible default only if detection fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 899d45c2-d48a-447b-a74a-f98028b409f0
📒 Files selected for processing (2)
bindata/network/multus/multus.yamlbindata/network/ovn-kubernetes/common/008-script-lib.yaml
sdodson
force-pushed
the
OCPBUGS-83863-remove-rhel8-fallback
branch
from
71efd9d to
a4f2c56
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindata/network/multus/multus.yaml`:
- Around line 415-416: The SOURCE_DIRECTORY environment for the bond-cni-plugin
is currently set to the EL9-specific path "/bondcni/rhel9/" which causes the
resolver to construct paths like "/bondcni/rhel${rhelmajor}/rhel9"; change
SOURCE_DIRECTORY to the unversioned base path "/bondcni/" (or alternatively keep
explicit per-RHEL wiring) so the resolver derives the correct per-release
subpaths instead of always falling back to rhel9; update the env var named
SOURCE_DIRECTORY in the bond-cni-plugin container spec accordingly.
🪄 Autofix (Beta)
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: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88d2ddfe-14cd-40c7-9bc4-3a3bd9a94435
📒 Files selected for processing (2)
bindata/network/multus/multus.yamlbindata/network/ovn-kubernetes/common/008-script-lib.yaml
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/testwith e2e-gcp-ovn openshift/ovn-kubernetes#3149 openshift/multus-cni#285 |
|
@sdodson, |
|
/testwith openshift/cluster-network-operator/master/e2e-gcp-ovn openshift/ovn-kubernetes#3149 openshift/multus-cni#285 |
|
/testwith openshift/cluster-network-operator/master/e2e-gcp-ovn openshift/ovn-kubernetes#3149 openshift/multus-cni#285 openshift/route-override-cni#66 openshift/bond-cni#113 openshift/egress-router-cni#100 openshift/containernetworking-pluigins#228 openshift/whereabouts-cni#405 |
|
@sdodson, |
|
/testwith openshift/cluster-network-operator/master/e2e-gcp-ovn openshift/ovn-kubernetes#3149 openshift/multus-cni#285 openshift/route-override-cni#66 openshift/bond-cni#113 openshift/egress-router-cni#100 openshift/containernetworking-plugins#228 openshift/whereabouts-cni#405 |
sdodson
force-pushed
the
OCPBUGS-83863-remove-rhel8-fallback
branch
from
a4f2c56 to
f5f6667
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bindata/network/multus/multus.yaml (1)
71-73: Normalize the fallback path before reusing it.This branch keeps
SOURCE_DIRECTORYverbatim, so a future caller that passes/usr/src/plugins/bininstead of/usr/src/plugins/bin/will make Line 82 copy thebindirectory itself rather than its contents. Using the already-trimmed value here avoids making the trailing slash part of the script’s API.♻️ Proposed fix
if [ -z "$sourcedir" ]; then log "WARNING: No version-specific directory found for rhel${rhelmajor}, using ${SOURCE_DIRECTORY}" - sourcedir="${SOURCE_DIRECTORY}" + sourcedir="${default_trimmed}/" fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindata/network/multus/multus.yaml` around lines 71 - 73, When falling back to SOURCE_DIRECTORY for sourcedir, normalize the path by removing any trailing slash before assigning it so callers passing either "/usr/src/plugins/bin" or "/usr/src/plugins/bin/" behave the same; update the assignment of sourcedir in the branch that checks if [ -z "$sourcedir" ] to use the already-trimmed value (or compute a trimmed version of SOURCE_DIRECTORY) so subsequent uses of sourcedir (e.g., the copy logic later) operate on the directory contents rather than the directory name.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bindata/network/multus/multus.yaml`:
- Around line 71-73: When falling back to SOURCE_DIRECTORY for sourcedir,
normalize the path by removing any trailing slash before assigning it so callers
passing either "/usr/src/plugins/bin" or "/usr/src/plugins/bin/" behave the
same; update the assignment of sourcedir in the branch that checks if [ -z
"$sourcedir" ] to use the already-trimmed value (or compute a trimmed version of
SOURCE_DIRECTORY) so subsequent uses of sourcedir (e.g., the copy logic later)
operate on the directory contents rather than the directory name.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8d7eed9b-550f-42ae-a699-05e7a2f2429b
📒 Files selected for processing (2)
bindata/network/multus/multus.yamlbindata/network/ovn-kubernetes/common/008-script-lib.yaml
There was a problem hiding this comment.
🧹 Nitpick comments (2)
bindata/network/ovn-kubernetes/common/008-script-lib.yaml (1)
481-491: 💤 Low valueOutdated function docstring.
The docstring on lines 481-486 states the function "detects the host OS" and requires the
/hostvolume mount, but the implementation now copies from a fixed path without any OS detection. Update the comment to reflect the simplified behavior.📝 Suggested docstring update
- # cni-bin-copy() detects the host OS and copies the correct shim binary to - # the CNI binary directory. - # - # Requires the following volume mounts: - # /host - # /cni-bin-dir + # cni-bin-copy() copies the CNI shim binary to the CNI binary directory. + # + # Requires the following volume mounts: + # /cni-bin-dir cni-bin-copy() {🤖 Prompt for 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. In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml` around lines 481 - 491, The comment above the cni-bin-copy() function is outdated: it claims the function detects the host OS and requires a /host mount, but the implementation simply copies a fixed shim from /usr/libexec/cni/ovn-k8s-cni-overlay to /cni-bin-dir/. Update the docstring for cni-bin-copy() to remove any mention of OS detection and the /host volume, and instead state that it copies the fixed shim from /usr/libexec/cni/ovn-k8s-cni-overlay into /cni-bin-dir/ (and list only the /cni-bin-dir/ mount as required).bindata/network/multus/multus.yaml (1)
8-9: 💤 Low valueOutdated ConfigMap description.
The annotation states "copy CNI binaries based on host OS" but the script no longer performs OS detection—it copies directly from
SOURCE_DIRECTORY.📝 Suggested description update
annotations: kubernetes.io/description: | - This is a script used to copy CNI binaries based on host OS + This is a script used to copy CNI binaries from a specified source directory🤖 Prompt for 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. In `@bindata/network/multus/multus.yaml` around lines 8 - 9, Update the kubernetes.io/description annotation in multus.yaml to reflect current behavior: the script no longer detects host OS but copies CNI binaries directly from SOURCE_DIRECTORY; edit the description text (kubernetes.io/description) to remove references to "based on host OS" and instead state it copies CNI binaries from SOURCE_DIRECTORY without OS detection.
🤖 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.
Nitpick comments:
In `@bindata/network/multus/multus.yaml`:
- Around line 8-9: Update the kubernetes.io/description annotation in
multus.yaml to reflect current behavior: the script no longer detects host OS
but copies CNI binaries directly from SOURCE_DIRECTORY; edit the description
text (kubernetes.io/description) to remove references to "based on host OS" and
instead state it copies CNI binaries from SOURCE_DIRECTORY without OS detection.
In `@bindata/network/ovn-kubernetes/common/008-script-lib.yaml`:
- Around line 481-491: The comment above the cni-bin-copy() function is
outdated: it claims the function detects the host OS and requires a /host mount,
but the implementation simply copies a fixed shim from
/usr/libexec/cni/ovn-k8s-cni-overlay to /cni-bin-dir/. Update the docstring for
cni-bin-copy() to remove any mention of OS detection and the /host volume, and
instead state that it copies the fixed shim from
/usr/libexec/cni/ovn-k8s-cni-overlay into /cni-bin-dir/ (and list only the
/cni-bin-dir/ mount as required).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: ad37a26c-b2a6-418c-8c87-3fd0b41857ec
📒 Files selected for processing (2)
bindata/network/multus/multus.yamlbindata/network/ovn-kubernetes/common/008-script-lib.yaml
|
/retest-required |
|
All of the previous runs happened while Insights and metal clusters were broken. |
Drop OS detection and version-specific directory probing from both cnibincopy scripts (multus.yaml and 008-script-lib.yaml) entirely. multus.yaml: - Consolidate RHEL8_SOURCE_DIRECTORY, RHEL9_SOURCE_DIRECTORY, and DEFAULT_SOURCE_DIRECTORY env vars into a single SOURCE_DIRECTORY - Remove os-release host volume mount from all cnibincopy init containers and the multus DaemonSet - Binaries are now copied directly from the default paths (e.g. /usr/src/multus-cni/bin/, /bondcni/, /usr/src/plugins/bin/) 008-script-lib.yaml (OVN): - Remove os-release sourcing and RHEL version case statements - Copy ovn-k8s-cni-overlay directly from /usr/libexec/cni/ This unblocks removing rhel8 build stages from upstream images (openshift/ovn-kubernetes#3149, openshift/multus-cni#285). By the time version-specific paths would be needed again (RHEL 11+), all in-cluster components will use native FIPS, making this logic permanently unnecessary. rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
sdodson
changed the title
sdodson
force-pushed
the
OCPBUGS-83863-remove-rhel8-fallback
branch
from
a5c883c to
5b39e01
Compare
The cnibincopy.sh script uses cp -r to copy all entries from SOURCE_DIRECTORY into a temp upgrade directory, then mv -f to atomically move them to the destination. When SOURCE_DIRECTORY contains subdirectories (e.g. /bondcni/ has rhel8/ and rhel9/ subdirs), the mv fails with "File exists" if those directories already exist at the destination from a previous pod run (e.g. after a node reboot). This caused bond-cni-plugin to CrashLoopBackOff on master nodes during IPsec cluster installs. Replace cp -r with find + cp to copy only non-directory entries (regular files, symlinks), skipping subdirectories that CNI containers don't need. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
|
/retest-required |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
| rm -Rf $UPGRADE_DIRECTORY | ||
| mkdir -p $UPGRADE_DIRECTORY | ||
| cp -r --remove-destination ${sourcedir}* $UPGRADE_DIRECTORY | ||
| find "${sourcedir}" -maxdepth 1 -mindepth 1 ! -type d -exec cp --remove-destination {} "${UPGRADE_DIRECTORY}/" \; |
There was a problem hiding this comment.
This command is pretty non-obvious, and also very specific to right now, where we know that ${sourcedir} contains subdirectories that we don't care about. Once we update the other modules to stop building the versioned binaries, this fix will be unnecessary. Can we just leave it out?
| - name: DEFAULT_SOURCE_DIRECTORY | ||
| value: "/bondcni/rhel9/" | ||
| - name: SOURCE_DIRECTORY | ||
| value: "/bondcni/" |
There was a problem hiding this comment.
right, ok, although we weren't using it before, the bond-cni image does include a "default" build at the top level
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danwinship, sdodson The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@sdodson: This PR has been marked as verified by DetailsIn response to this:
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. |
|
/retest-required |
|
/skip |
|
@sdodson: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/retest-required |
|
@sdodson: Jira Issue OCPBUGS-83863: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-83863 has not been moved to the MODIFIED state. This PR is marked as verified. If the remaining PRs listed above are marked as verified before merging, the issue will automatically be moved to VERIFIED after all of the changes from the PRs are available in an accepted nightly payload. DetailsIn response to this:
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. |
3 participants
Summary
/host/etc/os-releasesourcing, RHEL version case statements) from both cnibincopy scriptsRHEL8_SOURCE_DIRECTORY,RHEL9_SOURCE_DIRECTORY, andDEFAULT_SOURCE_DIRECTORYenv vars into a singleSOURCE_DIRECTORYin multus.yamlovn-k8s-cni-overlaydirectly from/usr/libexec/cni/instead of probing version-specific subdirectoriesos-releasehost volume mount from all cnibincopy init containers and the multus DaemonSetBy the time version-specific paths would be needed again (RHEL 11+), all in-cluster components will use native FIPS, making this logic permanently unnecessary.
This unblocks removing rhel8 build stages from upstream images (openshift/ovn-kubernetes#3149, openshift/multus-cni#285).
Test plan
Summary by CodeRabbit