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
|
/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
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
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 |
|
The |
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
7413a6f to
3fa36ec
Compare
| config.PullSecret, | ||
| config.MirrorConfig, | ||
| )) | ||
| ), rhcos.DefaultOSImageStream) |
There was a problem hiding this comment.
In baremetal IPI, the ISO that is used to provision the baremetal bootstrap VM is also the one that is used to boot the control plane hosts with ironic - we don't download an ISO but literally use /dev/cdrom or whatever it is. So ultimately the stream to use here needs to be obtained from the install-config as well.
This PR is about ABI so we shouldn't try to fix that here, just noting that we need to make sure this is tracked by another ticket.
/cc @honza
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
3fa36ec to
902e870
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 - Update getHashFromInstaller() to accept and use osImageStream parameter - Fix test to use explicit OSImageStream value instead of empty string - Rely on SetInstallConfigDefaults() to provide default osImageStream value 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. Depends on openshift#10570 which sets the default OSImageStream centrally.
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
902e870 to
6c9972c
Compare
|
@zaneb PTAL - got unit test to pass |
|
/test e2e-agent-compact-ipv4-rhel10-techpreview |
| icOverrides.AdditionalTrustBundlePolicy = installConfig.Config.AdditionalTrustBundlePolicy | ||
| } | ||
|
|
||
| if installConfig.Config.OSImageStream != rhcos.DefaultOSImageStream && installConfig.Config.OSImageStream != "" { |
There was a problem hiding this comment.
nit: I don't think it can ever be "" because we set it by default now.
|
/test e2e-aws-ovn |
|
@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. |
8 participants
Note that much of this PR was created with Claude.
Summary by CodeRabbit
New Features
Tests