METAL-1720: Add IRONIC_ROOTFS_URL env var for multi-arch rootfs override#583
METAL-1720: Add IRONIC_ROOTFS_URL env var for multi-arch rootfs override#583honza wants to merge 1 commit intoopenshift:mainfrom
Conversation
The image-customization-controller needs the base rootfs URL to construct arch-specific rootfs URLs for non-host architectures. Without this, aarch64 workers on an x86_64 cluster download the wrong rootfs during PXE boot. ICC uses this URL to set ExtraKernelParams with an arch-specific rootfs URL (e.g. ironic-python-agent_aarch64.rootfs). BMO appends this after %default% in kernel_append_params, and since the last coreos.live.rootfs_url= on the kernel command line wins, the correct rootfs is downloaded. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@honza: This pull request references METAL-1720 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 "4.22.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. |
WalkthroughAdded Ironic rootfs URL configuration to the provisioning module. Introduced new constants and a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes ✨ 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.
🧹 Nitpick comments (1)
provisioning/image_customization_test.go (1)
75-76: LGTM: Test expectations correctly updated.All test scenarios properly include the new
IRONIC_ROOTFS_URLenvironment variable with the expected value. The dual-stack test (container3) correctly expects only the first (IPv4) address in the rootfs URL.Optional: Consider adding a dedicated
TestGetRootfsURLfunction to cover edge cases (empty list, IPv6-only) similar to the existingTestGetUrlFromIP.Also applies to: 98-99, 124-125, 147-148, 171-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@provisioning/image_customization_test.go` around lines 75 - 76, Add a new focused test function (e.g., TestGetRootfsURL) in provisioning/image_customization_test.go that exercises the logic which sets IRONIC_ROOTFS_URL (referencing the IRONIC_ROOTFS_URL env var and the same helpers used by existing TestGetUrlFromIP) and covers edge cases: empty IP list, IPv6-only addresses, IPv4-only, and dual-stack where the function should pick the IPv4 address; assert the constructed rootfs URL matches expected values for each scenario and mirror patterns used in the existing TestGetUrlFromIP to keep consistency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@provisioning/image_customization_test.go`:
- Around line 75-76: Add a new focused test function (e.g., TestGetRootfsURL) in
provisioning/image_customization_test.go that exercises the logic which sets
IRONIC_ROOTFS_URL (referencing the IRONIC_ROOTFS_URL env var and the same
helpers used by existing TestGetUrlFromIP) and covers edge cases: empty IP list,
IPv6-only addresses, IPv4-only, and dual-stack where the function should pick
the IPv4 address; assert the constructed rootfs URL matches expected values for
each scenario and mirror patterns used in the existing TestGetUrlFromIP to keep
consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7ad080de-3841-4840-bc9e-8366db6b7d8a
📒 Files selected for processing (2)
provisioning/image_customization.goprovisioning/image_customization_test.go
|
/verified later @sgoveas |
openshift-ci-robot
added
verified-later
verified
|
@sgoveas: This PR has been marked to be verified later 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. |
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
@honza: The following test 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. |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
3 participants
Follow-up to #581
The image-customization-controller needs the base rootfs URL to construct arch-specific rootfs URLs for non-host architectures. Without this, aarch64 workers on an x86_64 cluster download the wrong rootfs during PXE boot.
ICC uses this URL to set ExtraKernelParams with an arch-specific rootfs URL (e.g. ironic-python-agent_aarch64.rootfs). BMO appends this after %default% in kernel_append_params, and since the last coreos.live.rootfs_url= on the kernel command line wins, the correct rootfs is downloaded.