METAL-1730: Add stream selection for multi-version support#175
METAL-1730: Add stream selection for multi-version support#175honza wants to merge 1 commit intoopenshift:mainfrom
Conversation
WalkthroughThe changes introduce stream-aware image handling throughout the codebase. A new Changes
Sequence DiagramsequenceDiagram
participant Client
participant ImageHandler
participant ImageSelection
participant ImageRetrieval
Client->>ImageHandler: ServeImage(arch, stream, ...)
ImageHandler->>ImageSelection: Lookup image by (stream, arch)
alt Exact match found
ImageSelection->>ImageRetrieval: Get image file
ImageRetrieval-->>ImageSelection: Image data
else Fallback to default stream
ImageSelection->>ImageSelection: Retry with (default_stream, arch)
ImageSelection->>ImageRetrieval: Get image file
ImageRetrieval-->>ImageSelection: Image data
else Fallback to host architecture
ImageSelection->>ImageSelection: Retry with (stream/default, host_arch)
ImageSelection->>ImageRetrieval: Get image file
ImageRetrieval-->>ImageSelection: Image data
end
ImageSelection-->>ImageHandler: Image file with stream metadata
ImageHandler-->>Client: Image path/URL
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 9 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (9 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 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
| func (f *imageFileSystem) HasImagesForArchitecture(arch string) bool { | ||
| return f.getBaseImage(arch, false) != nil && f.getBaseImage(arch, true) != nil | ||
| return f.getBaseImage(arch, "", false) != nil && f.getBaseImage(arch, "", true) != nil | ||
| } |
There was a problem hiding this comment.
Make architecture support detection stream-aware.
This still probes only the legacy/default stream (stream=""). If an architecture is available only via stream-qualified assets such as *-rhel-10.{iso,initramfs}, HasImagesForArchitecture returns false even though ServeImage(..., "rhel-10", ...) would work. That can cause the provider to reject a supported architecture before build time.
Suggested direction
func (f *imageFileSystem) HasImagesForArchitecture(arch string) bool {
- return f.getBaseImage(arch, "", false) != nil && f.getBaseImage(arch, "", true) != nil
+ streams := map[string]struct{}{"": {}}
+
+ addStreams := func[T any](files map[streamArchKey]T) {
+ for key := range files {
+ if key.arch == arch || (arch == env.HostArchitecture() && key.arch == hostArchitectureKey) {
+ streams[key.stream] = struct{}{}
+ }
+ }
+ }
+
+ addStreams(f.isoFiles)
+ addStreams(f.initramfsFiles)
+
+ for stream := range streams {
+ if f.getBaseImage(arch, stream, false) != nil && f.getBaseImage(arch, stream, true) != nil {
+ return true
+ }
+ }
+
+ return false
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/imagehandler/imagehandler.go` around lines 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).
| func streamArchSpecificURL(baseURL, stream, arch string) string { | ||
| u, err := url.Parse(baseURL) | ||
| if err != nil { | ||
| // Fallback for unparseable URLs - shouldn't happen in practice | ||
| return baseURL | ||
| } | ||
| ext := path.Ext(u.Path) | ||
| base := strings.TrimSuffix(u.Path, ext) | ||
| u.Path = fmt.Sprintf("%s_%s%s", base, arch, ext) | ||
| if stream != "" { | ||
| base = fmt.Sprintf("%s-%s", base, stream) | ||
| } | ||
| if arch != env.HostArchitecture() { | ||
| base = fmt.Sprintf("%s_%s", base, arch) | ||
| } |
There was a problem hiding this comment.
Normalize empty host architecture before rewriting the rootfs URL.
Everywhere else in this flow, an empty architecture means “host”. This helper treats "" as a non-host architecture and produces a path like ..._.rootfs. If ImageData.Architecture is blank for host nodes, the generated coreos.live.rootfs_url becomes invalid.
Suggested fix
func streamArchSpecificURL(baseURL, stream, arch string) string {
+ if arch == "" {
+ arch = env.HostArchitecture()
+ }
+
u, err := url.Parse(baseURL)
if err != nil {
// Fallback for unparseable URLs - shouldn't happen in practice
return baseURL
}
@@
if stream != "" {
base = fmt.Sprintf("%s-%s", base, stream)
}
if arch != env.HostArchitecture() {
base = fmt.Sprintf("%s_%s", base, arch)
}📝 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.
| func streamArchSpecificURL(baseURL, stream, arch string) string { | |
| u, err := url.Parse(baseURL) | |
| if err != nil { | |
| // Fallback for unparseable URLs - shouldn't happen in practice | |
| return baseURL | |
| } | |
| ext := path.Ext(u.Path) | |
| base := strings.TrimSuffix(u.Path, ext) | |
| u.Path = fmt.Sprintf("%s_%s%s", base, arch, ext) | |
| if stream != "" { | |
| base = fmt.Sprintf("%s-%s", base, stream) | |
| } | |
| if arch != env.HostArchitecture() { | |
| base = fmt.Sprintf("%s_%s", base, arch) | |
| } | |
| func streamArchSpecificURL(baseURL, stream, arch string) string { | |
| if arch == "" { | |
| arch = env.HostArchitecture() | |
| } | |
| u, err := url.Parse(baseURL) | |
| if err != nil { | |
| // Fallback for unparseable URLs - shouldn't happen in practice | |
| return baseURL | |
| } | |
| ext := path.Ext(u.Path) | |
| base := strings.TrimSuffix(u.Path, ext) | |
| if stream != "" { | |
| base = fmt.Sprintf("%s-%s", base, stream) | |
| } | |
| if arch != env.HostArchitecture() { | |
| base = fmt.Sprintf("%s_%s", base, arch) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/imageprovider/rhcos.go` around lines 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.
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. |
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>
|
@honza: 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
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