MCO-2163: use machine-config-osimagestream to avoid hard-coding image tag names#10416
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
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 (1)
WalkthroughChanges modify CoreOS image resolution in the bootstrap process. The shell script now extracts and executes a binary from the machine-config-operator image to compute the pullspec and adds explicit cleanup/trapping. Supporting Go code updates the template data structure from Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
needs-rebase
cheesesashimi
force-pushed
the
zzlotnik/add-machine-config-osimagestream-extract-binary
branch
from
ab08bb0 to
5f7f1bc
Compare
openshift-ci
bot
removed
the
needs-rebase
| # Note: Proxy environment variables (HTTP_PROXY, HTTPS_PROXY, NO_PROXY) are | ||
| # automatically inherited from the environment if set via /etc/profile.d/proxy.sh | ||
| until COREOS_IMAGE=$("${TEMP_DIR}/machine-config-osimagestream" \ | ||
| get default-node-image \ |
There was a problem hiding this comment.
Well, we don't want the default OS image, we need to return the OS image install-config points too.
We need to pass it by using
We can use this PR to remove the
StreamTag field we won't use anymore.
There was a problem hiding this comment.
I removed the StreamTag field and tried to just use the OSImageStream field. Unless this field is explicitly set in the install-config, it will be empty. I pushed up a change that detects this case and sets it to rhcos.DefaultOSImageStream.
This made me think about what should be the source of truth for what the default OSImageStream is: Should it be the OSImageStream code in the MCO repo? Or should it be the installer code?
We can likely defer that decision for now because the way that both the installer and MCO code is written, it shouldn't matter much. Nevertheless, it is something we should think about.
| --authfile /root/.docker/config.json \ | ||
| --per-host-certs /etc/containers/certs.d \ | ||
| --registry-config /etc/containers/registries.conf); do |
There was a problem hiding this comment.
Can't we rely on the defaults? (Just asking not a blocking question or ask for change)
There was a problem hiding this comment.
In #10406, I don't think we can rely on the defaults since machine-config-osimagestream will be running containerized. However, I think we can rely on the defaults here. I need to look at https://github.com/containers/image to see if there is a SetDefaultValues() function someplace for types.SystemContext. If not, then I'll put these values into the machine-config-osimagestream entrypoint as the default values.
cheesesashimi
force-pushed
the
zzlotnik/add-machine-config-osimagestream-extract-binary
branch
from
5f7f1bc to
dddbd35
Compare
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
What's bad about hardcoding the images? Is it because there are potentially more streams than rhcos9 & rhcos10, and we don't want to teach the installer about all of those?
cheesesashimi
force-pushed
the
zzlotnik/add-machine-config-osimagestream-extract-binary
branch
2 times, most recently
from
43dd650 to
fc89859
Compare
cheesesashimi
changed the title
|
@cheesesashimi: This pull request references MCO-2163 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. |
openshift-ci-robot
added
the
jira/valid-reference
Assisted-By: Claude Sonnet 4.5
cheesesashimi
force-pushed
the
zzlotnik/add-machine-config-osimagestream-extract-binary
branch
from
fc89859 to
39538df
Compare
|
@cheesesashimi: This pull request references MCO-2163 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. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/jira-refresh |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/rhcos/stream_scos.go (1)
7-8: Don't widentypes.OSImageStreamwith a SCOS-only literal
types.OSImageStreamis documented and validated asrhel-9/rhel-10, but this default introduces"centos-10"under the same type.pkg/asset/ignition/bootstrap/common.go:402-430now forwards that value into template data, so future code can no longer rely on the type’s declared value set. Prefer a separate internal string/enum for the MCO helper stream name instead of reusing the install-config type.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rhcos/stream_scos.go` around lines 7 - 8, DefaultOSImageStream currently broadens the allowed values of types.OSImageStream by assigning a SCOS-only literal ("centos-10"); instead create a separate internal constant (e.g., scosMCOStreamName string or a dedicated type) for the SCOS/MCO helper stream and revert DefaultOSImageStream to only valid types.OSImageStream values, then update callers that currently consume DefaultOSImageStream (the code path that forwards the stream into template data) to use the new internal constant for SCOS-specific logic instead of the types.OSImageStream-typed constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@data/data/bootstrap/files/usr/local/bin/node-image-pull.sh.template`:
- Around line 13-20: The extraction can leave a dangling container named
mco-osimagestream if podman cp fails, so make the process idempotent: generate a
unique container name (e.g., CONTAINER_NAME variable instead of the fixed
mco-osimagestream), register cleanup with trap to remove the container and
TEMP_DIR on exit (successful or error) and run podman rm in the trap so cleanup
always happens even when set -e aborts; update the extraction block that uses
podman create, podman cp and podman rm (and the similar block at lines 37-38) to
use the new CONTAINER_NAME and the trap-based cleanup.
---
Nitpick comments:
In `@pkg/rhcos/stream_scos.go`:
- Around line 7-8: DefaultOSImageStream currently broadens the allowed values of
types.OSImageStream by assigning a SCOS-only literal ("centos-10"); instead
create a separate internal constant (e.g., scosMCOStreamName string or a
dedicated type) for the SCOS/MCO helper stream and revert DefaultOSImageStream
to only valid types.OSImageStream values, then update callers that currently
consume DefaultOSImageStream (the code path that forwards the stream into
template data) to use the new internal constant for SCOS-specific logic instead
of the types.OSImageStream-typed constant.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3c472bbc-f9be-4878-b286-c618bb3ef4a9
📒 Files selected for processing (3)
data/data/bootstrap/files/usr/local/bin/node-image-pull.sh.templatepkg/asset/ignition/bootstrap/common.gopkg/rhcos/stream_scos.go
| until COREOS_IMAGE=$(image_for ${coreos_img}); do | ||
| echo 'Failed to query release image; retrying...' | ||
| # Get the Machine Config Operator image from the release payload | ||
| until MCO_IMAGE=$(image_for 'machine-config-operator'); do |
There was a problem hiding this comment.
Hmm, but note this adds another image we have to pull down here before we get to pivot. So I expect this to slightly affect bootstrapping time.
I guess this is pretty far along now, but wouldn't a cleaner design here be to put the machine-config-osimagestream helper stuff in the CVO? It already has an image command (which is what image_for uses) that's very similar to what we need here. E.g. a new command like osimage --stream {{ .OSImageStream }} which returns the pullspec of the right OS image.
There was a problem hiding this comment.
Yeah, I agree that the CVO would be a better long-term place for this to live. The bigger problem that we'll need to solve is that there is some common code for working with OSImageStreams that would need to live in both the CVO and the MCO. And ideally, we'd be able to find a home for that code.
There was a problem hiding this comment.
Nothing a little golang package import can't solve I would think? :)
| # Extract the machine-config-osimagestream binary from the MCO image. This avoids having to munge podman args, paths, env vars, etc. | ||
| podman create --name mco-osimagestream "${MCO_IMAGE}" | ||
| podman cp mco-osimagestream:/usr/bin/machine-config-osimagestream "${TEMP_DIR}/machine-config-osimagestream" | ||
| podman rm mco-osimagestream |
There was a problem hiding this comment.
We need to be memory-conscious here because we could also be running live, which means that all that content is living in RAM. E.g. SNO has strict memory requirements.
So probably we want to just podman rmi the MCO image entirely here once we've extracted out the binary.
There was a problem hiding this comment.
This is a really good point, thanks!
| until COREOS_IMAGE=$(image_for ${coreos_img}); do | ||
| echo 'Failed to query release image; retrying...' | ||
| # Get the Machine Config Operator image from the release payload | ||
| until MCO_IMAGE=$(image_for 'machine-config-operator'); do |
There was a problem hiding this comment.
Nothing a little golang package import can't solve I would think? :)
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: jlebon 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 |
|
/retest-required |
|
@cheesesashimi: 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. |
5 participants
This makes use of the new
machine-config-osimagestreambinary helper that was added to the MCO. The intent is to use this binary to look up the OS image pullspec for the default OS ImageStream.