MCO-2200: Add day-0 dual streams support for ABI install flow#10481
MCO-2200: Add day-0 dual streams support for ABI install flow#10481isabella-janssen wants to merge 1 commit into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughAssets for agent-driven installs now accept an optional install configuration and agent workflow info; this is threaded into OS image stream selection and rootFS URL resolution, and OSImageStream manifest generation is conditionally suppressed for non-install agent workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant AgentManifests
participant OSImageStream
participant InstallConfig
participant AgentWorkflow
participant RHCOSFetcher
AgentManifests->>OSImageStream: Generate()
OSImageStream->>InstallConfig: Ask for IPI InstallConfig
alt IPI InstallConfig present
InstallConfig-->>OSImageStream: Provide IPI config (OSImageStream)
OSImageStream->>RHCOSFetcher: Use OSImageStream to fetch metadata
RHCOSFetcher-->>OSImageStream: Return image stream / metadata
OSImageStream-->>AgentManifests: Return manifest
else No IPI config, agent OptionalInstallConfig present
OSImageStream->>InstallConfig: Check agent OptionalInstallConfig
alt AgentWorkflow == Install
AgentWorkflow-->>OSImageStream: Confirm Install workflow
OSImageStream->>RHCOSFetcher: Use agent OSImageStream to fetch metadata
RHCOSFetcher-->>OSImageStream: Return image stream / metadata
OSImageStream-->>AgentManifests: Return manifest
else Other agent workflow
OSImageStream-->>AgentManifests: Return nil (no manifest)
end
else No config available
OSImageStream-->>AgentManifests: Return nil (no manifest)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
openshift-ci-robot
added
the
jira/valid-reference
|
@isabella-janssen: This pull request references MCO-2200 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
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
isabella-janssen
force-pushed
the
mco-2200-claude
branch
3 times, most recently
from
82f8861 to
5c23add
Compare
|
@isabella-janssen: This pull request references MCO-2200 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.
Actionable comments posted: 1
🤖 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/types/validation/installconfig_test.go`:
- Around line 3038-3043: The restoreFnFactory in the test sets types.SCOS =
false unconditionally and must instead capture the original value and restore it
after the test; update the restoreFnFactory closure to save orig := types.SCOS,
then return a func() that sets types.SCOS = orig so the test (and parallel
tests) do not mutate global state permanently—apply this change to the
restoreFnFactory used alongside expectedError in the InstallConfig tests.
🪄 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: 7510ad2f-b046-42cd-9a85-96a75689d6f4
📒 Files selected for processing (7)
pkg/asset/agent/image/agentimage.gopkg/asset/agent/image/baseiso.gopkg/asset/agent/image/ignition.gopkg/asset/agent/manifests/agent.gopkg/asset/agent/manifests/agent_test.gopkg/asset/manifests/osimagestream.gopkg/types/validation/installconfig_test.go
isabella-janssen
force-pushed
the
mco-2200-claude
branch
from
5c23add to
73ee43d
Compare
|
@isabella-janssen: This pull request references MCO-2200 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. |
openshift-ci
Bot
added
the
needs-rebase
| } | ||
|
|
||
| // For install workflow, use osImageStream from install-config if supplied | ||
| if installConfig.Supplied && installConfig.Config != nil { |
There was a problem hiding this comment.
The original motivation of the customStreamGetter was to provide an alternative (custom) way to retrieve the bootimages data - because in the day2 add scenario we wanted to reuse the values found in the target cluster (from the coreos-bootimages configmap) rather than the "default" values embedded in the installer.
This approach technically works, but I was expecting an implementation more focused on the defaultCoreOSStreamGetter (or at least part of GetMetalArtifact). I understand that it may be more complex from an implementation point of view, but I was wondering if it could be worth rethinking that part so that the OSImageStream field could be considered a part of the default implementation, rather than a special custom one (based on writing a dedicated asset for that)
There was a problem hiding this comment.
I tried integrating the suggestion here to have the implementation more focused in GetMetalArtifact, so hopefully the current state of this PR is closer to what you were expecting.
isabella-janssen
force-pushed
the
mco-2200-claude
branch
from
73ee43d to
19fe75f
Compare
openshift-ci
Bot
removed
the
needs-rebase
isabella-janssen
force-pushed
the
mco-2200-claude
branch
from
19fe75f to
a6b4cbc
Compare
|
Hey @isabella-janssen! Just one thing based on the discussions we have had with the AS folks. |
|
/test ? |
|
/test verify-codegen |
|
/test gofmt |
|
/test golint |
|
/test golint |
isabella-janssen
force-pushed
the
mco-2200-claude
branch
2 times, most recently
from
7b76730 to
e2aedd3
Compare
|
/test all |
|
/retest-required |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
openshift-ci
Bot
added
the
do-not-merge/hold
|
updated refactor PR- #10537 |
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
2 times, most recently
from
aa17117 to
8ee3939
Compare
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
8ee3939 to
f2e321f
Compare
|
|
||
| // getOSImageStreamFromManifests extracts the osImageStream from the AgentClusterInstall | ||
| // installConfigOverrides annotation, or returns default if not present. | ||
| func getOSImageStreamFromManifests(agentManifests *manifests.AgentManifests) types.OSImageStream { |
There was a problem hiding this comment.
I would find more natural to have the GetOSImageStream method (so the logic) directly defined in AgentClusterInstall (the method could be defined as a pointer receiver checking also for nil, same as here). BaseISO simply looks just yet another consumer of that field.
In this way any consumer could do something like:
osImageStream := agentManifests.AgentClusterInstall.GetOSImageStream()
bf6ff22 to
0b358ed
Compare
| } else { | ||
| // Default to the URL from the RHCOS streams file | ||
| defaultRootFSURL, err := baseIso.getRootFSURL(ctx, a.cpuArch) | ||
| osImageStream := baseIso.getOSImageStream(agentManifests) |
There was a problem hiding this comment.
| osImageStream := baseIso.getOSImageStream(agentManifests) | |
| osImageStream := agentManifests.GetOSImageStream() |
| dependencies.Get(agentManifests, registriesConf, agentWorkflow, clusterInfo) | ||
|
|
||
| // Extract osImageStream from AgentClusterInstall annotation | ||
| osImageStream := i.getOSImageStream(agentManifests) |
There was a problem hiding this comment.
| osImageStream := i.getOSImageStream(agentManifests) | |
| osImageStream := agentManifests.GetOSImageStream() |
|
|
||
| // getOSImageStream extracts the osImageStream from the AgentClusterInstall | ||
| // installConfigOverrides annotation, or returns default if not present. | ||
| func (i *BaseIso) getOSImageStream(agentManifests *manifests.AgentManifests) types.OSImageStream { |
There was a problem hiding this comment.
This method could be safely removed, since it's just a wrapper around agentManifests.GetOSImageStream()
|
|
||
| osImage, err := getOSImagesInfo(ctx, archName, openshiftVersion) | ||
| osImageStream := agentManifests.GetOSImageStream() | ||
| if osImageStream == "" { |
There was a problem hiding this comment.
See my comments below, this could be simplified
| config.PullSecret, | ||
| config.MirrorConfig, | ||
| )) | ||
| ), "") |
There was a problem hiding this comment.
My suggestion would be to avoid using an "" for types.OSImageStream, but always using a well defined value. That will make the code a lot easier to maintain and to read IMHO
| func GetMetalArtifact(ctx context.Context, archName string) (stream.PlatformArtifacts, error) { | ||
| // using the embedded stream metadata with the specified stream. | ||
| func GetMetalArtifact(ctx context.Context, archName string, osImageStream types.OSImageStream) (stream.PlatformArtifacts, error) { | ||
| if osImageStream == "" { |
There was a problem hiding this comment.
See my comment below, I'd be inclined to avoid at all using an empty string as a valid value for types.OSImageStream
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
0b358ed to
f31f3c8
Compare
|
/retest |
|
/test e2e-agent-compact-ipv4-rhel10-techpreview |
|
/retest-required |
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
f31f3c8 to
5446712
Compare
This change enables support for RHEL 10 (coreos10-) and RHEL 9 (coreos-) file path differentiation in machine-os-images container and throughout the agent installer workflow. Changes: - Add OSImageStream field to installConfigOverrides in AgentClusterInstall - Add stream-aware helper functions for file paths in releaseextract.go - Update ReleasePayload interface and BaseIso to accept osImageStream parameter - Update GetMetalArtifact to use osImageStream for FetchCoreOSBuild calls - Add GetOSImageStream() method to AgentManifests following existing patterns - Eliminate empty string as valid OSImageStream value throughout codebase - Remove defensive empty string checks, enforce explicit stream values - Update all call sites to use rhcos.DefaultOSImageStream when appropriate This implements the foundation for RHEL 10 support in the agent installer by allowing the installer to distinguish between RHEL 9 and RHEL 10 boot artifacts based on the osImageStream annotation.
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
5446712 to
7413a6f
Compare
| // validate PreloadedOSImageName if configured | ||
| if p.PreloadedOSImageName != "" { | ||
| err = validatePreloadedImage(ctx, nc, p, ic.OSImageStream) | ||
| osImageStream := ic.OSImageStream |
There was a problem hiding this comment.
nit: this could become a smart getter in installConfig asset
There was a problem hiding this comment.
This comment somehow relates to this one: https://github.com/openshift/installer/pull/10481/changes#r3280912958
|
/approve from the (ABI) integration point of view the current changes looks fine to me. Holding the PR for any other feedback from @zaneb , and for some green TP 10 runs |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/test ? |
|
/test e2e-agent-compact-ipv4-rhel10-techpreview |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andfasano 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
|
@isabella-janssen: 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. |
zaneb
left a comment
zaneb
left a comment
There was a problem hiding this comment.
This looks basically good. I think we just want to set the default in the defaults rather than at all the places it is used... if that is in fact the thing we want for all install types (which does seem plausible! the most surprising thing is that this wasn't already the case).
/hold cancel
|
|
||
| func validateAMI(ctx context.Context, meta *Metadata, config *types.InstallConfig) field.ErrorList { | ||
| osImageStream := config.OSImageStream | ||
| if osImageStream == "" { |
There was a problem hiding this comment.
Surprising to see this AWS change in a PR about ABI.
I think this should be set in https://github.com/openshift/installer/blob/main/pkg/types/defaults/installconfig.go rather than here.
That would eliminate a lot of places where this same logic is repeated.
There was a problem hiding this comment.
@zaneb agree. In fact, the switchover PR has changed this interface to avoid doing this if play everywhere.
I'd prefer, if you agree, to continue with the change as it is as the switchover will probably take precedence and will completly remove this piece of code if the PR requires a rebase.
| baseIsoFileName: ocBaseIsoFilename, | ||
| baseIsoError: tc.getIsoError, | ||
| }) | ||
| }, "") |
There was a problem hiding this comment.
We should never pass "" to this now, right?
openshift-ci
Bot
removed
the
do-not-merge/hold
|
Looking at the It looks like its pulling I assume that this is being worked on in mco and as such shouldn't hold up this PR. It also looks like Zane's comment here will be addressed in #10533. /lgtm |
8 participants
Note that much of this PR was created with Claude.
Summary by CodeRabbit
New Features
Tests