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 4 commits intoopenshift:mainfrom
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. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
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
from
deafe5d to
7b76730
Compare
isabella-janssen
force-pushed
the
mco-2200-claude
branch
from
7b76730 to
e2aedd3
Compare
|
/test all |
|
/retest-required |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
@pablintino this change should be there now, thank you for the reminder! |
| &mirror.CaBundle{}, | ||
| &gencrypto.AuthConfig{}, | ||
| &common.InfraEnvID{}, | ||
| &agentcommon.OptionalInstallConfig{}, |
There was a problem hiding this comment.
Adding a comment here since it's one of the latest asset in the agent graph. ABI must support installing a cluster either by using install-config.yaml + agent-config.yaml or by ZTP manifests.
Internally we transform the install-config + agent-config in a set of ZTP manifests (grouped in AgentManifests), and then they are used to generate the subsequent agent Ignition and ISO artifacts.
I think it would be a lot easier if at least the field was available in a ZTP manifest, but I think we agreed to add it later.
For such reasons, and also to make it easier update in future the code, I think it'd be better to create a simple asset (ie OSImageStreamField or any other reasonable name) that will simply extract the install-config.yaml osimagestream and expose it as a value so just with the OptionalInstallConfig as a dependency). This asset then could be added as a dependency whenever the osimagestream field is required, decoupling thus the dependency on OptionalInstallConfig for the other assets in the agent graph.
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
bda8fd0 to
ee65438
Compare
|
|
||
| if installConfig.Config.OSImageStream != "" { | ||
| icOverridden = true | ||
| icOverrides.OSImageStream = installConfig.Config.OSImageStream |
| installConfig := &installconfig.InstallConfig{} | ||
| dependencies.Get(installConfig) | ||
|
|
||
| // Then try agent optional install config |
There was a problem hiding this comment.
Unfortunately, I don't think this works. Unlike OptionalInstallConfig, InstallConfig is not optional, so adding it to the agent dependency graph will break things that are supposed to work even if you don't have an install-config.
I think you will need to create a separate AgentOSImageStream asset.
Creates separate AgentOSImageStream asset for Agent workflows to avoid breaking ZTP-only deployments. IPI OSImageStream depends on required InstallConfig, while AgentOSImageStream depends on OptionalInstallConfig and can work without install-config.yaml.
dkhater-redhat
force-pushed
the
mco-2200-claude
branch
from
ee65438 to
c1a889a
Compare
|
@zaneb PTAL |
| func (i *BaseIso) getRootFSURL(ctx context.Context, archName string, agentWorkflow *workflow.AgentWorkflow, clusterInfo *joiner.ClusterInfo) (string, error) { | ||
| metal, err := rhcos.GetMetalArtifact( | ||
| ctx, archName, customStreamGetter(agentWorkflow, clusterInfo)) | ||
| func (i *BaseIso) getRootFSURL(ctx context.Context, archName string, agentWorkflow *workflow.AgentWorkflow, clusterInfo *joiner.ClusterInfo, osImageStream *manifests.AgentOSImageStream) (string, error) { |
There was a problem hiding this comment.
nit: could pass just the .Stream itself rather than the whole asset here.
| &AgentClusterInstall{}, | ||
| &ClusterDeployment{}, | ||
| &ClusterImageSet{}, | ||
| &manifests.OSImageStream{}, |
There was a problem hiding this comment.
Not sure what this dependency is doing at the moment tbh.
| } | ||
|
|
||
| // TODO: Add fallback to AgentClusterInstall.Spec.OSImageStream once | ||
| // the field is available in the assisted-service API |
There was a problem hiding this comment.
If we're storing this in the installConfigOverrides of AgentClusterInstall, then we should really be reading it from there and not from the install-config directly. (User could do agent create cluster-manifests and then manually edit AgentClusterInstall, and we should abide by that.)
|
|
||
| // Only generate for install workflow, not add-nodes | ||
| if agentWorkflow.Workflow != workflow.AgentWorkflowTypeInstall { | ||
| return nil |
There was a problem hiding this comment.
For add-nodes we really need to ask the cluster what the default install stream is, since the user can configure this at runtime IIUC.
- Pass stream value instead of whole asset to getRootFSURL - Remove IPI OSImageStream dependency from AgentManifests - Read from AgentClusterInstall.installConfigOverrides (Layer 2) - Query cluster OSImageStream CR for add-nodes workflow
RHEL 10 uses different file naming in machine-os-images container:
- rhel-9: /coreos/coreos-{arch}.iso
- rhel-10: /coreos/coreos10-{arch}.iso
Updated releaseextract to use stream-aware file path helpers:
- getCoreOsFileName()
- getCoreOsSha256FileName()
- getCoreOsStreamFileName()
Without this fix, rhel-10 deployments fail when extracting the boot
ISO from the release payload.
| // getCoreOsFileName returns the ISO file path in machine-os-images based on stream. | ||
| func getCoreOsFileName(stream types.OSImageStream, architecture string) string { | ||
| // rhel-10 uses coreos10- prefix, rhel-9 uses coreos- prefix | ||
| if stream == types.OSImageStreamRHCOS10 { | ||
| return fmt.Sprintf("/coreos/coreos10-%s.iso", architecture) | ||
| } | ||
| return fmt.Sprintf("/coreos/coreos-%s.iso", architecture) | ||
| } | ||
|
|
||
| // getCoreOsSha256FileName returns the ISO checksum file path based on stream. | ||
| func getCoreOsSha256FileName(stream types.OSImageStream, architecture string) string { | ||
| if stream == types.OSImageStreamRHCOS10 { | ||
| return fmt.Sprintf("/coreos/coreos10-%s.iso.sha256", architecture) | ||
| } | ||
| return fmt.Sprintf("/coreos/coreos-%s.iso.sha256", architecture) | ||
| } | ||
|
|
||
| // getCoreOsStreamFileName returns the stream metadata file path based on stream. | ||
| func getCoreOsStreamFileName(stream types.OSImageStream) string { | ||
| if stream == types.OSImageStreamRHCOS10 { | ||
| return "/coreos/coreos10-stream.json" | ||
| } | ||
| return "/coreos/coreos-stream.json" | ||
| } | ||
|
|
There was a problem hiding this comment.
Terrible (not your fault). The naming in the openshift-os-images is a bit of a nightmare to align with our stream names, but, why not somehing like this:
func getCoreOSFilePrefix(stream types.OSImageStream) string {
prefix := "coreos"
if stream != types.OSImageStreamRHCOS9 {
stremParts := strings.SplitN(string(stream), "-", 2)
if len(stremParts) > 1 {
prefix += stremParts[1]
}
}
return prefix
}
// getCoreOsFileName returns the ISO file path in machine-os-images based on stream.
func getCoreOsFileName(stream types.OSImageStream, architecture string) string {
return fmt.Sprintf("/coreos/%s-%s.iso", getCoreOSFilePrefix(stream), architecture)
}
// getCoreOsSha256FileName returns the ISO checksum file path based on stream.
func getCoreOsSha256FileName(stream types.OSImageStream, architecture string) string {
return fmt.Sprintf("/coreos/%s-%s.iso.sha256", getCoreOSFilePrefix(stream), architecture)
}
// getCoreOsStreamFileName returns the stream metadata file path based on stream.
func getCoreOsStreamFileName(stream types.OSImageStream) string {
return fmt.Sprintf("/coreos/%s-stream.json", getCoreOSFilePrefix(stream))
}We can try to agree with them the next ISO additions to follow the same pattern, eg. for the RHEL4NV work.
|
@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. |
There was a problem hiding this comment.
The name of this asset file should be agentosimagestream.go
There was a problem hiding this comment.
It'd be nice also to have a dedicated test file for this new asset
| return []asset.Asset{ | ||
| &workflow.AgentWorkflow{}, | ||
| &joiner.ClusterInfo{}, | ||
| &AgentManifests{}, |
There was a problem hiding this comment.
This works, but since the current asset requires only AgentClusterInstall it should be sufficient to have, as a dependency, just &manifests.AgentClusterInstall{} (useful to minimize the dependency requirements)
| // getStreamFromCluster queries the cluster's OSImageStream CR to determine | ||
| // the default stream. | ||
| func (a *AgentOSImageStream) getStreamFromCluster(ctx context.Context, client mcfgclient.Interface) (types.OSImageStream, error) { | ||
| osImageStream, err := client.MachineconfigurationV1alpha1().OSImageStreams().Get(ctx, "cluster", metav1.GetOptions{}) |
There was a problem hiding this comment.
This does not look correct, since it's a ClusterInfo responsibility to retrieve the osImageStream. The current asset should just fetch this information from the ClusterInfo dependency (through a field), and leave to ClusterInfo the decision on how to retrieve it. So the extraction logic should be refactored inside ClusterInfo
There was a problem hiding this comment.
Given the new specific agen asset AgenOSImageStream, all the changes here don't seem to still needed, isn't it?
|
|
||
| var stream types.OSImageStream | ||
|
|
||
| switch agentWorkflow.Workflow { |
There was a problem hiding this comment.
This is a slightly unusual pattern, since usually this kind of logic (discriminating between the Install and AddNodes workflow) is applied directly into the assets that need to consume the dependencies values.
In this case the asset does not consume the field but hides the implementation details about osImageStream retrieval, and exposes directly the value as a field meant to be consumed by other assets.
The good point is that, since this field is required by many different consumer, the retrieval logic is not duplicated across them, but remains centralized here (so fewer risks of different usages in different places). The drawback is that such logic remains a little bit less visible in the asset consumer code (as it keeps having its own workflow discriminating logic).
Even though I'm a little bit more inclined to improved readability, I'm not opposed to such approach
| &AgentClusterInstall{Config: fakeAgentClusterInstall}, | ||
| &ClusterDeployment{Config: fakeClusterDeployment}, | ||
| &ClusterImageSet{Config: fakeClusterImageSet}, | ||
| &manifests.OSImageStream{}, |
There was a problem hiding this comment.
As mentioned before, very likely this will not be required
| return err | ||
| } | ||
| osImage, err := getOSImagesInfo(ctx, archName, openshiftVersion, nil) | ||
| osImage, err := getOSImagesInfo(ctx, archName, openshiftVersion, nil, "") |
There was a problem hiding this comment.
nit: for readability, rather than an empty string a more speaking constant could be better
| func NewBaseISOFetcher(ocRelease ReleasePayload, streamGetter CoreOSBuildFetcher, osImageStream types.OSImageStream) *BaseIso { | ||
| if streamGetter == nil { | ||
| streamGetter = defaultCoreOSStreamGetter | ||
| if osImageStream != "" { |
There was a problem hiding this comment.
Note: a little bit more of "happy path" could help in making the code more readable here.
Anyhow it looks like here the code it's getting more complicated, and I'd like to have another pass to see if we could simplify a little bit.
|
/hold for #10534 |
openshift-ci
Bot
added
the
do-not-merge/hold
|
updated refactor PR- #10537 |
7 participants
Note that much of this PR was created with Claude.
Summary by CodeRabbit
New Features
Tests