METAL-1730: Add stream selection for multi-version support#175
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 (8)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (6)
WalkthroughAdds a stream dimension to image selection and serving: new ChangesStream-aware image selection and serving
sequenceDiagram
participant Client
participant ImageHandler
participant ImageSelection
participant ImageRetrieval
Client->>ImageHandler: ServeImage(key, arch, stream, ...)
ImageHandler->>ImageSelection: Lookup base by (stream, arch, initramfs)
alt Exact (stream,arch) found
ImageSelection->>ImageRetrieval: Return image file for (stream,arch)
ImageRetrieval-->>ImageSelection: Image data/path
else Fallback to default (empty) stream for arch
ImageSelection->>ImageSelection: Retry (default_stream, arch)
ImageSelection->>ImageRetrieval: Return image file
ImageRetrieval-->>ImageSelection: Image data/path
else Fallback to host architecture
ImageSelection->>ImageSelection: Retry (stream/default, host_arch)
ImageSelection->>ImageRetrieval: Return image file
ImageRetrieval-->>ImageSelection: Image data/path
else No base image
ImageSelection-->>ImageHandler: nil / InvalidBaseImageError
end
ImageSelection-->>ImageHandler: imageFile (includes stream)
ImageHandler-->>Client: served image URL/path
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/imagehandler/imagehandler.go`:
- Around line 417-419: HasImagesForArchitecture currently probes only the
empty/default stream and misses stream-qualified assets; update it to be
stream-aware by iterating over the set of known streams on imageFileSystem (e.g.
the method/property that lists available streams such as
availableStreams()/streams) and for each stream call getBaseImage(arch, stream,
false) and getBaseImage(arch, stream, true) (same pair of checks ServeImage
would use) and return true if any call returns non-nil; if the struct has no
stream-listing API, fall back to checking the empty stream plus any stream
values ServeImage accepts so the detection matches ServeImage behavior (refer to
HasImagesForArchitecture, getBaseImage and ServeImage).
In `@pkg/imageprovider/rhcos.go`:
- Around line 157-170: The function streamArchSpecificURL treats an empty arch
as a distinct architecture and produces a bogus “_” suffix; update
streamArchSpecificURL to normalize empty architecture to mean host by setting
arch to env.HostArchitecture() when arch == "" (or otherwise capture hostArch :=
env.HostArchitecture() and treat empty arch as hostArch) and then only append
the "_<arch>" suffix when arch != hostArch; reference streamArchSpecificURL and
env.HostArchitecture() when making this change.
🪄 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: ec6b8ae2-9fdb-4350-a82b-541bb604193d
📒 Files selected for processing (7)
cmd/static-server/main.gocmd/static-server/main_test.gopkg/imagehandler/imagefile.gopkg/imagehandler/imagefilesystem.gopkg/imagehandler/imagehandler.gopkg/imagehandler/imagehandler_test.gopkg/imageprovider/rhcos.go
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. |
There was a problem hiding this comment.
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 `@pkg/imageprovider/rhcos_test.go`:
- Around line 16-22: The import block in pkg/imageprovider/rhcos_test.go is not
goimports-formatted; move the standard library import "testing" to its own group
(first), add a blank line, then list the external imports
(github.com/metal3-io/baremetal-operator/pkg/imageprovider,
github.com/openshift/image-customization-controller/pkg/env, metav1
"k8s.io/apimachinery/pkg/apis/meta/v1") in the external group (alphabetize if
desired) or simply run `goimports -w pkg/imageprovider/rhcos_test.go` to
automatically fix the grouping and ordering.
🪄 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: Enterprise
Run ID: 9db30bca-4784-499c-82ce-72a69c7bf078
📒 Files selected for processing (8)
cmd/static-server/main.gocmd/static-server/main_test.gopkg/imagehandler/imagefile.gopkg/imagehandler/imagefilesystem.gopkg/imagehandler/imagehandler.gopkg/imagehandler/imagehandler_test.gopkg/imageprovider/rhcos.gopkg/imageprovider/rhcos_test.go
✅ Files skipped from review due to trivial changes (1)
- pkg/imagehandler/imagefile.go
🚧 Files skipped from review as they are similar to previous changes (6)
- cmd/static-server/main.go
- pkg/imagehandler/imagefilesystem.go
- cmd/static-server/main_test.go
- pkg/imageprovider/rhcos.go
- pkg/imagehandler/imagehandler.go
- pkg/imagehandler/imagehandler_test.go
Enable the image-customization-controller to serve different CoreOS base images (e.g., RHCOS 9 vs RHCOS 10) based on a stream label on PreprovisioningImage resources. Images are now indexed by (stream, architecture) and discovered from stream-prefixed filenames like ironic-python-agent-rhel-9.iso. The coreos.openshift.io/stream label is read from PPI metadata and falls back to the default stream when absent. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/lgtm |
|
/retest |
|
@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. |
3 participants
Enable the image-customization-controller to serve different CoreOS base images (e.g., RHCOS 9 vs RHCOS 10) based on a stream label on PreprovisioningImage resources. Images are now indexed by (stream, architecture) and discovered from stream-prefixed filenames like ironic-python-agent-rhel-9.iso. The coreos.openshift.io/stream label is read from PPI metadata and falls back to the default stream when absent.
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
New Features
Bug Fixes
Tests