METAL-1730: Add stream symlinks for multi-stream support#83
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe ChangesStream-Prefixed Symlink Generation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: honza 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/copy-metal (1)
131-134: Avoid hard-coded stream mapping in this loop.This works for 9/10, but it will drift when new streams are added. Prefer deriving
streamdirectly fromLine 126’s JSON.Refactor idea
- case "${prefix}" in - coreos) stream="rhel-9" ;; - coreos10) stream="rhel-10" ;; - esac + stream="$(jq -r '.stream // empty' "${stream_json}")" + if [ -z "${stream}" ]; then + echo "Failed to read stream from ${stream_json}" >&2 + continue + fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/copy-metal` around lines 131 - 134, The hard-coded case mapping for the variable stream (inside the case on prefix) should be replaced by extracting the stream value from the parsed JSON produced earlier (the variable holding the JSON parsed at Line 126), so that stream is derived dynamically for the current prefix instead of using fixed coreos/coreos10 branches; locate the case statement that matches prefix and replace it with a lookup from that JSON (e.g., use jq or shell-parsed fields to get the stream for the current prefix) so prefix -> stream comes directly from the parsed JSON variable rather than hard-coded values.
🤖 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/copy-metal`:
- Around line 142-145: The current ln -f -s commands that create aarch64
symlinks (using variables prefix, stream, DEST_DIR and conditional on arch ==
"x86_64") can produce dangling symlinks when the source artifacts are missing;
change the logic to test for the existence of each source file (e.g., [ -f
"${prefix}-aarch64.iso" ] and [ -f "${prefix}-aarch64-initrd.img" ]) before
creating the corresponding symlink so links are only created when the real
artifact exists; apply the same existence checks to the other aarch64 symlink
block referenced (lines creating ironic-python-agent-..._aarch64.iso and
..._aarch64.initramfs) to avoid broken stream/arch discovery at runtime.
---
Nitpick comments:
In `@scripts/copy-metal`:
- Around line 131-134: The hard-coded case mapping for the variable stream
(inside the case on prefix) should be replaced by extracting the stream value
from the parsed JSON produced earlier (the variable holding the JSON parsed at
Line 126), so that stream is derived dynamically for the current prefix instead
of using fixed coreos/coreos10 branches; locate the case statement that matches
prefix and replace it with a lookup from that JSON (e.g., use jq or shell-parsed
fields to get the stream for the current prefix) so prefix -> stream comes
directly from the parsed JSON variable rather than hard-coded values.
🪄 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: b089df0b-e655-48c1-b845-deb286822f63
📒 Files selected for processing (2)
Dockerfilescripts/copy-metal
honza
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@honza: This pull request references METAL-1730 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. 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 |
|
/lgtm The bot's suggestion is worth addressing but it doesn't seem critical to me. |
Create ironic-python-agent-{stream} symlinks (e.g.,
ironic-python-agent-rhel-9.iso, ironic-python-agent-rhel-10.iso)
for all available CoreOS versions. These allow the
image-customization-controller to discover and serve different
CoreOS base images based on the coreos.openshift.io/stream label.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
/verified by sgoveas link |
openshift-ci-robot
added
the
verified
|
@sgoveas: 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. |
|
@honza: 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. |
5 participants
Create ironic-python-agent-{stream} symlinks (e.g., ironic-python-agent-rhel-9.iso, ironic-python-agent-rhel-10.iso) for all available CoreOS versions. These allow the image-customization-controller to discover and serve different CoreOS base images based on the coreos.openshift.io/stream label.
How it works
The
coreos.openshift.io/streamlabel on a BareMetalHost determineswhich kernel, initrd, and rootfs files are used during PXE boot. For
example, a BMH labeled
coreos.openshift.io/stream: rhel-10will PXEboot with
ironic-python-agent-rhel-10.kernel,ironic-python-agent-rhel-10.initramfs, andironic-python-agent-rhel-10.rootfsinstead of the unqualified defaults.The per-node
coreos.live.rootfs_urlkernel parameter set via ICC'sExtraKernelParamsoverrides the global one from ironic-image becausedracut's
getargreturns the last value for duplicate parameters.Changes across repos
installer (openshift/installer#10502)
—
pkg/asset/machines/baremetal/: Sets thecoreos.openshift.io/streamlabel on all generated BareMetalHost objects (control-plane, worker,
arbiter) based on the install-config's
OSImageStreamsetting, defaultingto
rhel-9. ConfigureshostSelector.matchLabelsonBareMetalMachineProviderSpec so MachineSets only claim BMHs with the
matching stream label.
cluster-baremetal-operator (openshift/cluster-baremetal-operator#590)
—
provisioning/image_customization.go: Passes three new environmentvariables to the ICC container:
DEPLOY_KERNEL(path to the kernel file),IMAGE_SHARED_DIR(path to the shared images directory wherestream-prefixed files are discovered), and
IRONIC_ROOTFS_URL(base URLfor the rootfs served by httpd). These enable ICC to discover multi-stream
images and construct per-node kernel/rootfs URLs.
machine-os-images (openshift/machine-os-images#83)
—
scripts/copy-metal: Creates stream-prefixed IPA symlinks (e.g.ironic-python-agent-rhel-9.iso,ironic-python-agent-rhel-10.kernel)for all available CoreOS versions, allowing ICC to discover images by
stream. Also fixes missing initramfs symlinks in the
fixed=true(PXE)block — without these, a stream-specific kernel would be paired with the
default-stream initrd, causing a version mismatch that crashes immediately
on PXE boot.
image-customization-controller (openshift/image-customization-controller#175)
—
pkg/imagehandler/,pkg/imageprovider/rhcos.go: Adds OS streamselection so ICC can serve different CoreOS base images based on the
coreos.openshift.io/streamlabel. Images are indexed by (stream,architecture) and discovered from stream-prefixed filenames. Key changes:
streamArchSpecificURL()transforms the base rootfs URL into a stream-and arch-specific URL
BuildImage()passes the stream toServeKernel()and uses thestream-specific rootfs URL in
ExtraKernelParamsimageKey()includes the stream in the cache key so that changing aBMH's stream label invalidates the cached image
Summary by CodeRabbit