METAL-1729: Add dual CoreOS version support for RHCOS 9 and 10#81
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@elfosardo: This pull request references METAL-1729 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds a COREOS_VERSIONS build/config variable and exposes it in the image; refactors fetch_image.sh to support multiple CoreOS versions with per-version stream JSON and ISO naming; updates scripts/copy-metal to select/extract per-version artifacts and adjust symlink/cleanup targets. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elfosardo 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 6: In the podman build recipe update the build-arg invocation to quote
the Makefile variables so their values are passed as single arguments: wrap
DIRECT_DOWNLOAD and COREOS_VERSIONS in quotes in the podman build command (i.e.,
change the --build-arg uses of DIRECT_DOWNLOAD and COREOS_VERSIONS to use
"$(DIRECT_DOWNLOAD)" and "$(COREOS_VERSIONS)") so multi-word values remain a
single --build-arg value.
In `@scripts/copy-metal`:
- Around line 12-21: After deriving COREOS_VERSION and coreos_name, verify that
the bundle actually contains files matching the expected pattern (e.g.,
"${coreos_name}-"*) before creating symlinks; if no matches are found, print an
error mentioning the requested COREOS_VERSION and exit non‑zero. Locate the code
that sets coreos_name and add a check (using the same source/bundle directory
variable used elsewhere in the script) to glob for "${coreos_name}-"* entries,
fail fast when the glob is empty, and only proceed to create symlinks when at
least one matching artifact exists.
- Around line 66-73: The oc image extract command is allowed to fail due to the
"|| echo ..." fallback which bypasses set -e and causes jq to write the ISO
checksum even when the aarch64 ISO extract failed; change the flow so the oc
image extract (the command invoking oc image extract with --path and
--filter-by-os for "${MACHINE_OS_IMAGES_IMAGE}" and variables ISO_DIR_WRITABLE
and iso_file) is executed inside an if/then check and only when that command
succeeds run the jq line that writes "${ISO_DIR_WRITABLE}/${iso_file}.sha256";
ensure failures still log the existing failure message and do not proceed to
write the checksum.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3dbaafc5-9be0-4613-af55-1afd0a0b4750
📒 Files selected for processing (4)
DockerfileMakefilefetch_image.shscripts/copy-metal
elfosardo
force-pushed
the
dual-stream
branch
2 times, most recently
from
73defd5 to
0dba9a6
Compare
|
@elfosardo: This pull request references METAL-1729 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. |
1 similar comment
|
@elfosardo: This pull request references METAL-1729 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/copy-metal (1)
63-85:⚠️ Potential issue | 🟠 MajorDon’t publish the arm64 ISO link when extraction was skipped or failed.
Line 84 explicitly skips the aarch64 ISO extract when
MACHINE_OS_IMAGES_IMAGEis unset, and Lines 79-80 only log extract failures. Line 119 still createsironic-python-agent_aarch64.iso, so x86_64 custom runs can expose a broken link or keep serving a stale ISO from a reused${DEST_DIR}. Clean up${iso_file}/.sha256on failure and only create the arm64 ISO symlink aftercopy-isohas actually produced${coreos_name}-aarch64.iso.Suggested hardening
for prefix in coreos coreos10; do stream_json="/coreos/${prefix}-stream.json" if [ ! -f "${stream_json}" ]; then continue fi iso_file="${prefix}-aarch64.iso" if oc image extract \ --certificate-authority=/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem \ --registry-config=/run/secrets/auth.json \ --path="/coreos/${iso_file}:${ISO_DIR_WRITABLE}/" \ --filter-by-os=linux/arm64 \ --confirm \ "${MACHINE_OS_IMAGES_IMAGE}"; then jq -r ".architectures.aarch64.artifacts.metal.formats.iso.disk.sha256" "${stream_json}" > "${ISO_DIR_WRITABLE}/${iso_file}.sha256" else + rm -f "${ISO_DIR_WRITABLE}/${iso_file}" "${ISO_DIR_WRITABLE}/${iso_file}.sha256" echo "failed to extract aarch64 ISO for ${prefix}" >&2 fi done @@ if [[ "${arch}" == "x86_64" ]]; then ln -f -s "${coreos_name}-aarch64-initrd.img" "${DEST_DIR}/ironic-python-agent_aarch64.initramfs" - ln -f -s "${coreos_name}-aarch64.iso" "${DEST_DIR}/ironic-python-agent_aarch64.iso" + if [ -f "${DEST_DIR}/${coreos_name}-aarch64.iso" ]; then + ln -f -s "${coreos_name}-aarch64.iso" "${DEST_DIR}/ironic-python-agent_aarch64.iso" + else + rm -f "${DEST_DIR}/ironic-python-agent_aarch64.iso" + echo "missing ${coreos_name}-aarch64.iso; ensure MACHINE_OS_IMAGES_IMAGE is set and extraction succeeded" >&2 + exit 1 + fi fiAlso applies to: 117-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/copy-metal` around lines 63 - 85, When MACHINE_OS_IMAGES_IMAGE is unset or the oc image extract for aarch64 fails, the script still leaves/creates stale aarch64 artifacts; update the logic in the loop that sets iso_file (e.g., "${prefix}-aarch64.iso"), ISO_DIR_WRITABLE output, and the later symlink creation (ironic-python-agent_aarch64.iso / coreos_name-aarch64.iso) so that on extract failure you remove any partial "${ISO_DIR_WRITABLE}/${iso_file}" and "${ISO_DIR_WRITABLE}/${iso_file}.sha256" and do not create the aarch64 symlink; instead only create the aarch64 symlink after verifying the actual "${coreos_name}-aarch64.iso" exists (i.e., confirm copy-iso produced it) and only then create the ironic-python-agent_aarch64.iso link.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/copy-metal`:
- Around line 63-85: When MACHINE_OS_IMAGES_IMAGE is unset or the oc image
extract for aarch64 fails, the script still leaves/creates stale aarch64
artifacts; update the logic in the loop that sets iso_file (e.g.,
"${prefix}-aarch64.iso"), ISO_DIR_WRITABLE output, and the later symlink
creation (ironic-python-agent_aarch64.iso / coreos_name-aarch64.iso) so that on
extract failure you remove any partial "${ISO_DIR_WRITABLE}/${iso_file}" and
"${ISO_DIR_WRITABLE}/${iso_file}.sha256" and do not create the aarch64 symlink;
instead only create the aarch64 symlink after verifying the actual
"${coreos_name}-aarch64.iso" exists (i.e., confirm copy-iso produced it) and
only then create the ironic-python-agent_aarch64.iso link.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c951785b-e442-42df-8f83-25d08fb72785
📒 Files selected for processing (4)
DockerfileMakefilefetch_image.shscripts/copy-metal
✅ Files skipped from review due to trivial changes (2)
- Dockerfile
- Makefile
🚧 Files skipped from review as they are similar to previous changes (1)
- fetch_image.sh
zaneb
left a comment
zaneb
left a comment
There was a problem hiding this comment.
What is the plan for changing the flags to actually bundle both versions?
@zaneb until we have both in the same payload we can't really test it, but after we have v10 too we can probably change the versions in the Dockerfile to include both, like "9 10" |
Support the --stream flag on openshift-install coreos print-stream-json (MCO-2133) to download ISOs for multiple CoreOS versions at build time. A COREOS_VERSIONS build arg controls which versions to bundle (default: 9). At runtime, a COREOS_VERSION env var selects which version the ironic-python-agent symlinks point to. The selected version is validated against what is actually bundled in the image. Cross-arch aarch64 extraction handles all bundled versions and only writes the checksum file on successful extract. Backward compatible: default behavior is unchanged.
|
@elfosardo: This pull request references METAL-1729 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. |
|
@elfosardo: 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. |
|
/lgtm
In a sense, changing this is the thing that would make us have both in one payload. Now that both stream files are available in the installer, there's no real obstacle. |
|
/verified bypass default behavior has not changed |
|
@elfosardo: The 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. |
openshift-ci-robot
added
the
verified
3 participants
Support the --stream flag on openshift-install coreos print-stream-json
(MCO-2133) to download ISOs for multiple CoreOS versions at build time.
A COREOS_VERSIONS build arg controls which versions to bundle (default:
9). At runtime, a COREOS_VERSION env var selects which version the
ironic-python-agent symlinks point to. The selected version is validated
against what is actually bundled in the image.
Cross-arch aarch64 extraction handles all bundled versions and only
writes the checksum file on successful extract.
Backward compatible: default behavior is unchanged.