MCO-2200: Simplify CoreOS fetching code#10534
MCO-2200: Simplify CoreOS fetching code#10534andfasano wants to merge 5 commits intoopenshift:mainfrom
Conversation
Refactored rhcos baseIso to use directly a stream
openshift-ci-robot
added
the
jira/valid-reference
|
@andfasano: 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. |
WalkthroughThis PR refactors how CoreOS streams are passed throughout the codebase, replacing a function-type fetcher pattern ( ChangesCoreOS Stream Parameter Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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.12.1)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
|
[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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/asset/rhcos/iso.go (1)
134-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer the supplied stream before extracting from the release payload.
When
i.stis non-nil, this branch still callsi.ocRelease.GetBaseIso(...)first. That means the AddNodes path can still return the ISO from the installer release payload instead of the target cluster stream passed frompkg/asset/agent/image/baseiso.goLine 75, which breaks the new nil/non-nil contract and can pick the wrong base ISO once the cluster stream diverges from the installer release.Suggested fix
func (i *BaseIso) retrieveBaseIso(ctx context.Context, archName string) (string, error) { // Default iso archName to x86_64. if archName == "" { archName = arch.RpmArch(types.ArchitectureAMD64) } + if i.st != nil { + logrus.Info("Downloading base ISO from provided stream") + if err := workflowreport.GetReport(ctx).SubStage(workflow.StageFetchBaseISODownload); err != nil { + return "", err + } + return i.downloadIso(ctx, archName) + } + if i.ocRelease != nil { // If we have the image registry location and 'oc' command is available then get from release payload logrus.Info("Extracting base ISO from release payload")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/asset/rhcos/iso.go` around lines 134 - 149, The code is calling i.ocRelease.GetBaseIso(...) even when a stream (i.st) was supplied; change the branch to prefer the supplied stream: if i.st != nil, use the provided stream to determine the base ISO (the stream passed from the agent/baseiso logic) and return that result without calling i.ocRelease.GetBaseIso; only call i.ocRelease.GetBaseIso and checkReleasePayloadBaseISOVersion when i.st is nil. Ensure you still invoke the workflowreport SubStage transitions (StageFetchBaseISOExtract and StageFetchBaseISOVerify) in the same places for both paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@pkg/asset/rhcos/iso.go`:
- Around line 134-149: The code is calling i.ocRelease.GetBaseIso(...) even when
a stream (i.st) was supplied; change the branch to prefer the supplied stream:
if i.st != nil, use the provided stream to determine the base ISO (the stream
passed from the agent/baseiso logic) and return that result without calling
i.ocRelease.GetBaseIso; only call i.ocRelease.GetBaseIso and
checkReleasePayloadBaseISOVersion when i.st is nil. Ensure you still invoke the
workflowreport SubStage transitions (StageFetchBaseISOExtract and
StageFetchBaseISOVerify) in the same places for both paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: aa494340-460e-4f96-9368-6db3ebf13406
📒 Files selected for processing (8)
pkg/asset/agent/image/agentimage.gopkg/asset/agent/image/baseiso.gopkg/asset/agent/image/ignition.gopkg/asset/imagebased/image/baseiso.gopkg/asset/imagebased/image/baseiso_test.gopkg/asset/rhcos/iso.gopkg/asset/rhcos/iso_test.gopkg/asset/rhcos/releaseextract.go
|
@andfasano: 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.
Previously a streamGetter func was introduced mainly to support a distinction between the ABI Install and AddNodes workflow
Given that in a production build the stream data in the cluster ConfigMap is guaranteed to be the same as the one embedded in the node-joiner/installer binaries, what if we just... didn't? We could use the same code for everything and remove heaps of complexity.
I'm struggling to think of a CI scenario in which this would prevent us from pre-merge testing something either.
If we're going to pass a whole Stream (rather than just a name like rhcos9 or rhcos10) then I'd prefer that we do it everywhere rather than have this nil vs. non-nil API. See the comment from CodeRabbit.
There was a problem hiding this comment.
Somebody should probably refactor IBI so that it uses the same pkg/asset/rhcos code instead of copying-and-pasting an old version of the ABI code from before it was moved out of the pk/asset/agent/image package.
🤔 I think for this specific use case such approach could work fine. Even though we usually considered the target cluster as the source of truth, the |
|
New refactor available in #10537 |
3 participants
This patch is meant to be a preliminary simplification to facilitate future
osImageStream(rhel-9/rhel-10) adoption (see #10481).Previously a
streamGetterfunc was introduced mainly to support a distinction between the ABIInstallandAddNodesworkflow: in the latter, the stream data are directly fetched from the target cluster - and not from the embedded installer metadata as usual. Anyhow this design didn't make it easier to introduce a new field (to select the source stream), and was used only for above case.Changes
CoreOSBuildFetcherfunction type and replaced it with a direct*stream.Streampointer throughout the base ISO fetching and CoreOS metadata resolution pathsnil(Installworkflow, baremetal bootstrap, unconfigured ignition), the code falls back to fetching from embedded metadata; when non-nil (AddNodesworkflow), the provided stream from the target cluster is used directlyGetMetalArtifacttoGetMetalArtifactWithStreamto make the nil-handling behavior explicitSummary by CodeRabbit