METAL-1731: baremetal: Add coreos.openshift.io/stream label to BMH and hostSelector#10502
METAL-1731: baremetal: Add coreos.openshift.io/stream label to BMH and hostSelector#10502honza wants to merge 1 commit intoopenshift:mainfrom
Conversation
Set the coreos.openshift.io/stream label on all generated BareMetalHost objects (control-plane, worker, and arbiter) based on the install-config's OSImageStream setting, defaulting to rhel-9. Configure HostSelector on BareMetalMachineProviderSpec so MachineSets only claim BMHs with the matching stream label. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
honza
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@honza: This pull request references METAL-1731 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. |
WalkthroughOS image stream labeling is introduced throughout the baremetal provisioning pipeline. A helper function selects the configured OS image stream, and this value is attached as labels to BareMetalHost objects and used in HostSelector label matching for machines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (8 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.11.4)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
@honza: This pull request references METAL-1731 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/asset/machines/baremetal/machines.go (1)
82-84: Use the shared stream label constant to avoid key drift.Functionally correct, but this literal duplicates
streamLabelKeydefined inpkg/asset/machines/baremetal/hosts.go. Reusing the constant keeps selector/label contracts safer over time.♻️ Proposed refactor
HostSelector: baremetalprovider.HostSelector{ MatchLabels: map[string]string{ - "coreos.openshift.io/stream": stream, + streamLabelKey: stream, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/baremetal/machines.go` around lines 82 - 84, Replace the literal label key "coreos.openshift.io/stream" in the MatchLabels map with the shared constant streamLabelKey (defined in pkg/asset/machines/baremetal/hosts.go) to avoid key drift; update the MatchLabels entry in machines.go to use streamLabelKey so both selector and label use the same canonical constant.pkg/asset/machines/baremetal/hosts_test.go (1)
32-790: Add one explicit non-default stream test case (rhel-10).Current updates validate the default path well, but there’s no direct assertion that
config.OSImageStreamoverride propagates to host labels. A dedicatedrhel-10scenario would lock down the new feature behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/machines/baremetal/hosts_test.go` around lines 32 - 790, Add a focused test case that verifies an OS image stream override (rhel-10) is propagated to host labels: create a new scenario (e.g. "stream-rhel-10") in the test table that uses machines(machine("machine-0")) and a Config built with the OS image stream override (set Config.OSImageStream or use config().withOSImageStream("rhel-10") / configHosts helper if available) and a hostType("master-0").bmc(...).role("master"), then assert ExpectedSetting hosts include label "coreos.openshift.io/stream" with value "rhel-10" (and corresponding secret/userData/consumerRef as in other control-plane tests) so the override is explicitly covered by host label assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/asset/machines/baremetal/hosts_test.go`:
- Around line 32-790: Add a focused test case that verifies an OS image stream
override (rhel-10) is propagated to host labels: create a new scenario (e.g.
"stream-rhel-10") in the test table that uses machines(machine("machine-0")) and
a Config built with the OS image stream override (set Config.OSImageStream or
use config().withOSImageStream("rhel-10") / configHosts helper if available) and
a hostType("master-0").bmc(...).role("master"), then assert ExpectedSetting
hosts include label "coreos.openshift.io/stream" with value "rhel-10" (and
corresponding secret/userData/consumerRef as in other control-plane tests) so
the override is explicitly covered by host label assertions.
In `@pkg/asset/machines/baremetal/machines.go`:
- Around line 82-84: Replace the literal label key "coreos.openshift.io/stream"
in the MatchLabels map with the shared constant streamLabelKey (defined in
pkg/asset/machines/baremetal/hosts.go) to avoid key drift; update the
MatchLabels entry in machines.go to use streamLabelKey so both selector and
label use the same canonical constant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 74800e99-5d37-4905-8d0a-17a5d54b31e4
📒 Files selected for processing (4)
pkg/asset/machines/baremetal/hosts.gopkg/asset/machines/baremetal/hosts_test.gopkg/asset/machines/baremetal/machines.gopkg/asset/machines/baremetal/machinesets.go
|
@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
Set the coreos.openshift.io/stream label on all generated BareMetalHost objects (control-plane, worker, and arbiter) based on the install-config's OSImageStream setting, defaulting to rhel-9. Configure HostSelector on BareMetalMachineProviderSpec so MachineSets only claim BMHs with the matching 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