OCPBUGS-77066: SRIOV per-arch kustomize images#6267
OCPBUGS-77066: SRIOV per-arch kustomize images#6267pacevedom wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
jira/valid-reference
|
@pacevedom: This pull request references Jira Issue OCPBUGS-77066, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
📝 WalkthroughWalkthroughThis PR refactors SR-IOV operator configuration to support architecture-specific deployments using Kustomize overlays. The base operator deployment is simplified, while new per-architecture kustomization files inject environment variables and image digests. Build and rebase scripts are updated to apply these overlays. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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. Comment |
|
@pacevedom: This pull request references Jira Issue OCPBUGS-77066, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pacevedom 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
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/auto-rebase/rebase_sriov.sh (1)
400-412: Globals/_/-/gsed operates on image URL values too.The substitution is applied to the entire JSON string including image reference values. Any image URL containing an underscore (technically valid) would be silently corrupted. Scoping to keys only is safer:
♻️ Suggested alternative using jq
- images_json=$(yq " + images_json=$(yq -o json " ( [.spec.template.spec.containers[].env[] | select(.name | test(\"_IMAGE$\")) | {\"key\": (.name | downcase), \"value\": .value}] | from_entries ) as \$env_images | ( .spec.template.spec.containers[] | {(.name): .image} ) as \$main_images | - (\$env_images + \$main_images) - " "${STAGING_SRIOV}/${OPERATOR_FILENAME}" -o json | sed 's/"sriov_\([^"]*\)_\([^"]*\)"/"sriov-\1-\2"/g; s/_/-/g') + (\$env_images + \$main_images) + " "${STAGING_SRIOV}/${OPERATOR_FILENAME}" | jq 'with_entries(.key |= gsub("_"; "-"))')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/auto-rebase/rebase_sriov.sh` around lines 400 - 412, The current pipeline assigns images_json but then runs sed 's/_/-/g' which mangles underscores inside image values; remove that global sed and instead post-process the JSON with jq to only rewrite object keys (e.g. use with_entries and gsub to replace "_" with "-" and also gsub("sriov_(\\w+)_(\\w+)"; "sriov-\\1-\\2") for the sriov pattern). Update the pipeline that builds images_json (the yq ... | sed ...) to pipe into jq and perform key-only transformations so image URL values (from STAGING_SRIOV and OPERATOR_FILENAME processing) are left unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/auto-rebase/rebase_sriov.sh`:
- Around line 470-474: The current heredoc unconditionally splits
resolved_images["sriov-network-operator"] using %@* and #*@ which breaks if the
image has no '@' (tag-only). Update the write to ${kustomization_file} to detect
if the string contains '@' (check resolved_images["sriov-network-operator"] for
'@') and only emit the digest line (and use the %@* / #*@ splits) when present;
otherwise emit newName as the full image and omit the digest entry. Apply this
guard around the block that writes to kustomization_file and reference the
resolved_images["sriov-network-operator"] variable and the heredoc that creates
the kustomization content.
---
Nitpick comments:
In `@scripts/auto-rebase/rebase_sriov.sh`:
- Around line 400-412: The current pipeline assigns images_json but then runs
sed 's/_/-/g' which mangles underscores inside image values; remove that global
sed and instead post-process the JSON with jq to only rewrite object keys (e.g.
use with_entries and gsub to replace "_" with "-" and also
gsub("sriov_(\\w+)_(\\w+)"; "sriov-\\1-\\2") for the sriov pattern). Update the
pipeline that builds images_json (the yq ... | sed ...) to pipe into jq and
perform key-only transformations so image URL values (from STAGING_SRIOV and
OPERATOR_FILENAME processing) are left unchanged.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
assets/optional/sriov/deploy/operator.yamlassets/optional/sriov/kustomization.aarch64.yamlassets/optional/sriov/kustomization.x86_64.yamlassets/optional/sriov/release-sriov-aarch64.jsonassets/optional/sriov/release-sriov-x86_64.jsonpackaging/rpm/microshift.specscripts/auto-rebase/assets_sriov.yamlscripts/auto-rebase/rebase_sriov.sh
| cat > "${kustomization_file}" <<EOF | ||
| images: | ||
| - name: quay.io/openshift/sriov-network-operator | ||
| newName: ${resolved_images["sriov-network-operator"]%@*} | ||
| digest: ${resolved_images["sriov-network-operator"]#*@} |
There was a problem hiding this comment.
%@* / #*@ expansions assume a digest separator is present.
If resolved_images["sriov-network-operator"] has no @ (tag-based fallback ref), both newName and digest would get the full image string. A small guard prevents generating a malformed kustomization:
🛡️ Suggested guard
+ local operator_img="${resolved_images["sriov-network-operator"]}"
+ if [[ "${operator_img}" != *@* ]]; then
+ >&2 echo "ERROR: sriov-network-operator image has no digest: ${operator_img}"
+ return 1
+ fi
+
cat > "${kustomization_file}" <<EOF
images:
- name: quay.io/openshift/sriov-network-operator
- newName: ${resolved_images["sriov-network-operator"]%@*}
- digest: ${resolved_images["sriov-network-operator"]#*@}
+ newName: ${operator_img%@*}
+ digest: ${operator_img#*@}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cat > "${kustomization_file}" <<EOF | |
| images: | |
| - name: quay.io/openshift/sriov-network-operator | |
| newName: ${resolved_images["sriov-network-operator"]%@*} | |
| digest: ${resolved_images["sriov-network-operator"]#*@} | |
| local operator_img="${resolved_images["sriov-network-operator"]}" | |
| if [[ "${operator_img}" != *@* ]]; then | |
| >&2 echo "ERROR: sriov-network-operator image has no digest: ${operator_img}" | |
| return 1 | |
| fi | |
| cat > "${kustomization_file}" <<EOF | |
| images: | |
| - name: quay.io/openshift/sriov-network-operator | |
| newName: ${operator_img%@*} | |
| digest: ${operator_img#*@} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/auto-rebase/rebase_sriov.sh` around lines 470 - 474, The current
heredoc unconditionally splits resolved_images["sriov-network-operator"] using
%@* and #*@ which breaks if the image has no '@' (tag-only). Update the write to
${kustomization_file} to detect if the string contains '@' (check
resolved_images["sriov-network-operator"] for '@') and only emit the digest line
(and use the %@* / #*@ splits) when present; otherwise emit newName as the full
image and omit the digest entry. Apply this guard around the block that writes
to kustomization_file and reference the
resolved_images["sriov-network-operator"] variable and the heredoc that creates
the kustomization content.
There was a problem hiding this comment.
The sriov images are coming from a released bundle, which is always using image digests and therefore the @ is always present. If the digests were dropped there would be deeper changes in other parts of the test framework, and these would fail in CI jobs.
There was a problem hiding this comment.
@pacevedom, thanks for the clarification! That makes sense—if the bundle always provides digest-based references and CI would catch any deviation, the guard isn't necessary. I appreciate the context.
|
/verified by CI |
openshift-ci-robot
added
the
verified
|
@pacevedom: 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. |
|
@pacevedom: all tests passed! 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. |
openshift-merge-robot
added
the
needs-rebase
|
PR needs rebase. 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. |
3 participants
$ du -ksh /tmp/images/* 1.5G /tmp/images/sriov-arm 1.6G /tmp/images/sriov-x86Summary by CodeRabbit
New Features
Chores