OCPBUGS-83863: Remove rhel8 CNI binary logic, fall back to default paths#2967
OCPBUGS-83863: Remove rhel8 CNI binary logic, fall back to default paths#2967sdodson wants to merge 2 commits into
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. |
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sdodson The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![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 |
Replace hardcoded rhel8/rhel9 case statements with dynamic OS version
detection in both cnibincopy scripts:
- multus.yaml: Consolidate RHEL8_SOURCE_DIRECTORY, RHEL9_SOURCE_DIRECTORY,
and DEFAULT_SOURCE_DIRECTORY into a single SOURCE_DIRECTORY. The script
detects the RHEL major version at runtime and probes for a
version-specific directory (both standard and flat layouts), falling
back to SOURCE_DIRECTORY with a warning when none exists.
- 008-script-lib.yaml (OVN): Try /usr/libexec/cni/rhel${rhelmajor} first,
fall back to /usr/libexec/cni/ with a warning.
Both scripts also update the Fedora CoreOS default rhelmajor from 8 to 9.
This unblocks removing rhel8 build stages from upstream images and is
forwards-compatible with future RHEL versions without any CNO changes.
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
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
Drop OS detection and version-specific directory probing from both cnibincopy scripts entirely. Binaries are now expected at the default paths (/usr/src/.../bin/, /bondcni/, /usr/libexec/cni/) with no RHEL version subdirectory fallback. 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. Also removes the os-release host volume mount from all cnibincopy init containers and the multus DaemonSet, since OS detection is no longer performed. rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
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
|
@sdodson: The following tests 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. |
2 participants
Summary
casestatements in bothcnibincopyscripts with dynamic OS version detectionRHEL8_SOURCE_DIRECTORY,RHEL9_SOURCE_DIRECTORY, andDEFAULT_SOURCE_DIRECTORYenv vars into a singleSOURCE_DIRECTORYrhelmajorfrom8to9multus.yaml
The script detects the RHEL major version at runtime and probes for a version-specific directory in two layouts:
rhel<N>inserted before the last path component (e.g.,/usr/src/multus-cni/rhel9/bin/)rhel<N>as a subdirectory (e.g.,/bondcni/rhel9/)Falls back to
SOURCE_DIRECTORYwith a warning when neither exists.008-script-lib.yaml (OVN)
Tries
/usr/libexec/cni/rhel${rhelmajor}first, falls back to/usr/libexec/cni/with a warning.This unblocks removing rhel8 build stages from upstream images (openshift/ovn-kubernetes#3149, openshift/multus-cni#285) and is forwards-compatible with future RHEL versions without any CNO changes.
Test plan
Summary by CodeRabbit
Chores
Bug Fixes