MCO-2133: Select bootimages based on OSImageStream#10321
MCO-2133: Select bootimages based on OSImageStream#10321pablintino wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-2133 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. |
pablintino
force-pushed
the
mco-2133
branch
2 times, most recently
from
8f70a1a to
15030dd
Compare
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
pkg/asset/agent/image/baseiso.go
Outdated
| type CoreOSBuildFetcher func(ctx context.Context) (*stream.Stream, error) | ||
|
|
||
| func DefaultCoreOSStreamGetter(ctx context.Context) (*stream.Stream, error) { | ||
| return rhcos.FetchCoreOSBuild(ctx, rhcos.DefaultOSImageStream) |
There was a problem hiding this comment.
Not sure if I'm missing something, but my expectations for this asset weres to modify setStreamGetter method to take into account the OSImageStream (by reading it from the agent.OptionalInstallConfig{}, already listed as a dependency). Otherwise the live ISO will remain pinned to rhcos9.
There was a problem hiding this comment.
I haven't covered the agent cause I saw that I'd need to cover the case where the install-config is not provided but the AgentConfig is given, that means I need to add a field to it and so on. To avoid making the change bigger and bigger I thought about leaving the agent side consistent and ready for a follow up.
Anyways, if you think that for now supporting the intall-config.yaml only is enought, cool, I can incorporate this simple change to this PR and do the rest as a follow up.
There was a problem hiding this comment.
I see that as an incremental step. The first step usually is to support a new install-config field in the connected environment (ABI may be used also for that). With a simple change that part could be covered - otherwise with the current code a user may be confused by the fact that a config field was specified but "ignored".
The alternative (still valid) would be to put an explicit validation rule in agent.OptionalInstallConfig{} to fail if a value different from the default is specified as "unsupported". At least there will not be any confusing behavior for the end user
There was a problem hiding this comment.
@pablintino please disregard my previous comment. I understood (also from @djoshy comment) that for the 4.22 scope the current approach is correct (just default to rhcos.DefaultOSImageStream)
pablintino
force-pushed
the
mco-2133
branch
2 times, most recently
from
8cac45c to
1812d64
Compare
There was a problem hiding this comment.
Simplest ever approach, should this be ok for now? I'm not too familiar with the concept of marketplace, so not really sure.
There was a problem hiding this comment.
Reviewers: Please take a look at this change. It's the safest approach I could come up with. Basically, the structure of the ConfigMap is preserved and the stream field will continue to be reported, but from now on, it will use the default stream. A new streams field with a JSON map<streamName, streamJson> is now present. The format for SCOS remains unchanged, and the marketplace file is merged only if the file exists, which is only true for rhcos-9 for now.
cc:@djoshy
|
At a high-level this looks good, but I'm trying to find as much time as I can to go through the details. One thing that's missing is that the installer has a command to output the rhcos stream, https://github.com/openshift/installer/blob/main/pkg/coreoscli/cmd.go#L23 The underlying code is here: Line 27 in 435db5e I will continue to review this. |
I saw it, and I tried to address it alongside the agent references cause I thought it was used by the agent installer, but the amound of changes grew to the point the PR was bigger than I would like, so I decided to limit this PR to the bare minimum and acknoledge we need to continue working on this to cover the agent and that command. |
|
@patrickdillon I've thought twice about the command and true that may feel weird to keep it unaware of the streams. I've pushed an update to address it. What's missing? Basically everything related to the agent. |
|
/retest |
|
/test e2e-aws-ovn-rhcos10-devpreview I might have chosen a bad name for the presubmit, should probably be rhel10... |
|
/approve Approach looks good to me. I'm doing some final reviewing before tagging. Machine AMIs are being set correctly. Not sure why the previous run of /test e2e-aws-ovn-rhcos10-devpreview failed. The SSH gathering has been broken but we have a workaround in place now, so perhaps another run will provide more information. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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
|
There are some linter errors that need to be addressed: Sorry about hat last one, as that is existing code. Doesn't look too painful though... |
|
Before we merge this - do we want to add any labels/annotations on the rhel10 machinesets/machines that the MCO's boot image controller can then use for exclusion? In 4.22, boot image updates are enabled by default in gcp, aws, azure and vsphere and I think they would get automatically updated to the rhel9 stream(perhaps not for AWS, since the RHEL10 images won't be in the whitelist) Update: I opened openshift/machine-config-operator#5752 to a stab at the boot image side of this, reviews welcome! For now, I've kept old behavior of using |
WalkthroughParameterizes OS image stream selection across the codebase, adds a RHCOS RHEL‑10 manifest, and implements discovery and marketplace-overlay merging of multiple boot image streams for ConfigMap population and per-stream image/region lookups. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as "hack/build-coreos-manifest"
participant FS as "data/data/coreos/*"
participant Marketplace as "data/data/coreos/marketplace/*"
participant ConfigMap as "generated ConfigMap"
Builder->>FS: discover coreos-<stream>.json files
Builder->>Marketplace: locate marketplace overlay for each stream
Builder->>FS: read stream bytes for each discovered stream
Builder->>Marketplace: read overlay bytes (if present)
Builder->>Builder: merge marketplace overlay into stream bytes
Builder->>ConfigMap: embed default "stream" bytes and aggregated "streams" JSON
ConfigMap-->>Builder: persisted ConfigMap
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
pkg/coreoscli/cmd.go (1)
54-61: Unusedstreamvariable - flag binding and retrieval are inconsistent.The
streamvariable declared on line 54 is bound to the flag viaStringVar, butprintStreamJSONretrieves the flag value usingcmd.Flags().GetString("stream")instead of using the bound variable. Thestreamvariable is effectively dead code.Either use the bound variable (requires restructuring to make
streamaccessible) or remove the binding and useStringPinstead.♻️ Proposed fix - use simple String flag without binding
- var stream string printStreamCmd := &cobra.Command{ Use: "print-stream-json", Short: "Outputs the CoreOS stream metadata for the bootimages", Args: cobra.ExactArgs(0), RunE: printStreamJSON, } - printStreamCmd.Flags().StringVar(&stream, "stream", "", fmt.Sprintf("OS image stream to use (one of %v)", types.OSImageStreamValues)) + printStreamCmd.Flags().String("stream", "", fmt.Sprintf("OS image stream to use (one of %v)", types.OSImageStreamValues)) cmd.AddCommand(printStreamCmd)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/coreoscli/cmd.go` around lines 54 - 61, The local variable "stream" is declared and bound via printStreamCmd.Flags().StringVar(&stream, ...), but printStreamJSON reads the flag with cmd.Flags().GetString("stream"), making the bound variable unused; fix by removing the dead "stream" variable and replace the StringVar binding with a simple unbound flag (use Flags().StringP or Flags().String) on printStreamCmd so printStreamJSON continues to retrieve the value with cmd.Flags().GetString("stream"), or alternatively keep the StringVar binding and modify printStreamJSON to use the bound variable instead (update printStreamJSON signature/closure to reference the stream var); choose one approach and update usages of StringVar/GetString accordingly (refer to printStreamCmd, stream, StringVar, StringP, and printStreamJSON).hack/build-coreos-manifest.go (1)
41-43: Consider simplifying single-key map access.The current approach using
slices.Collect(maps.Keys(streamsFiles))[0]works but is verbose. A simpler approach exists.♻️ Suggested simplification
} else if len(streamsFiles) == 1 { - defaultFile := streamsFiles[slices.Collect(maps.Keys(streamsFiles))[0]] - defaultStreamFiles = &defaultFile + for _, file := range streamsFiles { + defaultStreamFiles = &file + break + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/build-coreos-manifest.go` around lines 41 - 43, The code pulls the sole value from streamsFiles using slices.Collect(maps.Keys(...))[0], which is verbose; simplify by iterating the map once to get the single value or key. Replace the verbose expression in the else-if branch (where streamsFiles, defaultFile, defaultStreamFiles are used) with a short loop such as: for _, v := range streamsFiles { defaultFile := v; defaultStreamFiles = &defaultFile; break } (or iterate keys if you need the key) to make the intent clearer and avoid slices.Collect/maps.Keys.pkg/rhcos/ami_regions.go (1)
14-32: Use genericsets.Set[string]instead of deprecatedsets.String.The
sets.Stringtype andsets.NewStringfunction are deprecated in favor of the genericsets.Set[string]andsets.New[string](). This was flagged by the linter (staticcheck).♻️ Proposed fix
-func AMIRegions(architecture types.Architecture, osImageStream types.OSImageStream) sets.String { +func AMIRegions(architecture types.Architecture, osImageStream types.OSImageStream) sets.Set[string] { stream, err := FetchCoreOSBuild(context.Background(), osImageStream) if err != nil { logrus.Errorf("could not fetch the rhcos stream data: %v", err) return nil } rpmArch := arch.RpmArch(string(architecture)) awsImages := stream.Architectures[rpmArch].Images.Aws if awsImages == nil { return nil } regions := make([]string, 0, len(awsImages.Regions)) for name, r := range awsImages.Regions { if r.Image == "" { continue } regions = append(regions, name) } - return sets.NewString(regions...) + return sets.New[string](regions...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rhcos/ami_regions.go` around lines 14 - 32, Update the AMIRegions function to use the generic sets type: change its return type from sets.String to sets.Set[string], replace calls to sets.NewString(regions...) with sets.New[string](regions...), and update any local type references from sets.String to sets.Set[string]; ensure the function signature (AMIRegions), the final return uses sets.New[string](regions...), and that any nil returns remain type-compatible or are replaced with nil of type sets.Set[string] as needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/build-coreos-manifest.go`:
- Around line 135-142: The loop over bootImageStream.Architectures is modifying
a copy (arch) so changes to arch.RHELCoreOSExtensions are lost; update the map
entry itself instead of the copy—e.g., fetch the entry by index (arch :=
bootImageStream.Architectures[name]), modify arch.RHELCoreOSExtensions and then
write it back to bootImageStream.Architectures[name], or change the map to hold
pointer values and modify the pointed value in place; ensure the assignment that
sets Marketplace = mkt updates the map entry rather than only the local copy.
In `@hack/rhcos/populate-marketplace-imagestream.go`:
- Around line 17-19: The marketplace output path constant
streamMarketplaceRHCOSJSON is missing the "marketplace/" subdirectory so files
are written to data/data/coreos/marketplace-coreos-rhel-9.json instead of
data/data/coreos/marketplace/coreos-rhel-9.json; update the constant value to
include the "marketplace/" segment (matching the expectation in
getMarketplaceStreamFileName in pkg/rhcos/stream.go) so the script writes to
data/data/coreos/marketplace/coreos-rhel-9.json.
---
Nitpick comments:
In `@hack/build-coreos-manifest.go`:
- Around line 41-43: The code pulls the sole value from streamsFiles using
slices.Collect(maps.Keys(...))[0], which is verbose; simplify by iterating the
map once to get the single value or key. Replace the verbose expression in the
else-if branch (where streamsFiles, defaultFile, defaultStreamFiles are used)
with a short loop such as: for _, v := range streamsFiles { defaultFile := v;
defaultStreamFiles = &defaultFile; break } (or iterate keys if you need the key)
to make the intent clearer and avoid slices.Collect/maps.Keys.
In `@pkg/coreoscli/cmd.go`:
- Around line 54-61: The local variable "stream" is declared and bound via
printStreamCmd.Flags().StringVar(&stream, ...), but printStreamJSON reads the
flag with cmd.Flags().GetString("stream"), making the bound variable unused; fix
by removing the dead "stream" variable and replace the StringVar binding with a
simple unbound flag (use Flags().StringP or Flags().String) on printStreamCmd so
printStreamJSON continues to retrieve the value with
cmd.Flags().GetString("stream"), or alternatively keep the StringVar binding and
modify printStreamJSON to use the bound variable instead (update printStreamJSON
signature/closure to reference the stream var); choose one approach and update
usages of StringVar/GetString accordingly (refer to printStreamCmd, stream,
StringVar, StringP, and printStreamJSON).
In `@pkg/rhcos/ami_regions.go`:
- Around line 14-32: Update the AMIRegions function to use the generic sets
type: change its return type from sets.String to sets.Set[string], replace calls
to sets.NewString(regions...) with sets.New[string](regions...), and update any
local type references from sets.String to sets.Set[string]; ensure the function
signature (AMIRegions), the final return uses sets.New[string](regions...), and
that any nil returns remain type-compatible or are replaced with nil of type
sets.Set[string] as needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f94697af-176c-4f12-b43d-9c3baeec92f1
📒 Files selected for processing (21)
data/data/coreos/coreos-rhel-10.jsondata/data/coreos/coreos-rhel-9.jsondata/data/coreos/marketplace/coreos-rhel-9.jsonhack/build-coreos-manifest.gohack/rhcos/populate-marketplace-imagestream.gopkg/asset/imagebased/image/baseiso.gopkg/asset/installconfig/aws/platform.gopkg/asset/installconfig/aws/regions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/nutanix/validation.gopkg/asset/installconfig/vsphere/validation.gopkg/asset/rhcos/image.gopkg/asset/rhcos/iso.gopkg/asset/rhcos/release.gopkg/coreoscli/cmd.gopkg/infrastructure/azure/azure.gopkg/rhcos/ami_regions.gopkg/rhcos/builds.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.gopkg/types/installconfig.go
pablintino
force-pushed
the
mco-2133
branch
2 times, most recently
from
e2cd71f to
c0c4b6a
Compare
|
@pablintino: This pull request references MCO-2133 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. |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
hack/build-coreos-manifest.go (1)
137-144:⚠️ Potential issue | 🔴 CriticalMap iteration returns a copy; marketplace data is not merged into the stream.
When iterating over
bootImageStream.Architectures, thearchvariable is a copy of the struct value. Modifications toarch.RHELCoreOSExtensionsdo not update the original map entry, causing the marketplace data to be silently discarded before marshaling.🐛 Proposed fix: Write the modified arch back to the map
for name, arch := range bootImageStream.Architectures { if mkt, ok := mktStream[name]; ok { if arch.RHELCoreOSExtensions == nil { arch.RHELCoreOSExtensions = &rhcos.Extensions{} } arch.RHELCoreOSExtensions.Marketplace = mkt + bootImageStream.Architectures[name] = arch } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/build-coreos-manifest.go` around lines 137 - 144, The loop over bootImageStream.Architectures uses a range variable arch which is a copy, so changes to arch.RHELCoreOSExtensions are not persisted; after creating or mutating arch.RHELCoreOSExtensions and setting arch.RHELCoreOSExtensions.Marketplace = mkt (where mkt comes from mktStream[name]), assign the modified arch back into the map (bootImageStream.Architectures[name] = arch) so the updated struct is stored; reference the loop variables name, arch, the map bootImageStream.Architectures, mktStream, and rhcos.Extensions when making this change.hack/rhcos/populate-marketplace-imagestream.go (1)
17-19:⚠️ Potential issue | 🔴 CriticalFix marketplace path to use subdirectory structure.
The populate script writes to
data/data/coreos/marketplace-coreos-rhel-9.jsonbutpkg/rhcos/stream.goreads fromdata/data/coreos/marketplace/coreos-rhel-9.json. The actual file already exists in the correct location with the subdirectory. Update the constant to match the reader's expectation.Proposed fix
const ( streamRHCOSJSON = "data/data/coreos/coreos-rhel-9.json" - streamMarketplaceRHCOSJSON = "data/data/coreos/marketplace-coreos-rhel-9.json" + streamMarketplaceRHCOSJSON = "data/data/coreos/marketplace/coreos-rhel-9.json" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/rhcos/populate-marketplace-imagestream.go` around lines 17 - 19, The constant streamMarketplaceRHCOSJSON in populate-marketplace-imagestream.go points to data/data/coreos/marketplace-coreos-rhel-9.json but pkg/rhcos/stream.go expects data/data/coreos/marketplace/coreos-rhel-9.json; update the value of streamMarketplaceRHCOSJSON to use the marketplace subdirectory (matching the existing file and the reader), keeping streamRHCOSJSON unchanged.
🧹 Nitpick comments (1)
pkg/coreoscli/cmd.go (1)
54-61: Unused variablestreambound to flag.The variable
streamdeclared at line 54 is bound to the--streamflag but never read. Instead,printStreamJSONretrieves the flag value viacmd.Flags().GetString("stream"). While this works correctly, the bound variable is dead code.Consider either:
- Removing the local variable and using
Flags().String()directly, or- Passing
streamtoprintStreamJSONvia closure or a different pattern.♻️ Option 1: Use Flags().String() without local binding
- var stream string printStreamCmd := &cobra.Command{ Use: "print-stream-json", Short: "Outputs the CoreOS stream metadata for the bootimages", Args: cobra.ExactArgs(0), RunE: printStreamJSON, } - printStreamCmd.Flags().StringVar(&stream, "stream", "", fmt.Sprintf("OS image stream to use (one of %v)", types.OSImageStreamValues)) + printStreamCmd.Flags().String("stream", "", fmt.Sprintf("OS image stream to use (one of %v)", types.OSImageStreamValues))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/coreoscli/cmd.go` around lines 54 - 61, The local variable "stream" is declared and bound via StringVar but never used; remove the dead variable and change the flag binding to use Flags().String (e.g., use printStreamCmd.Flags().String("stream", "", ...) so printStreamJSON continues to read the value via cmd.Flags().GetString("stream")), or alternatively capture the variable by creating the command with RunE as a closure that calls printStreamJSONWithStream(stream) (or update printStreamJSON to accept a stream param) so the bound "stream" is actually consumed; choose one approach and update the flag binding and call site (printStreamJSON/command creation) consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@hack/build-coreos-manifest.go`:
- Around line 137-144: The loop over bootImageStream.Architectures uses a range
variable arch which is a copy, so changes to arch.RHELCoreOSExtensions are not
persisted; after creating or mutating arch.RHELCoreOSExtensions and setting
arch.RHELCoreOSExtensions.Marketplace = mkt (where mkt comes from
mktStream[name]), assign the modified arch back into the map
(bootImageStream.Architectures[name] = arch) so the updated struct is stored;
reference the loop variables name, arch, the map bootImageStream.Architectures,
mktStream, and rhcos.Extensions when making this change.
In `@hack/rhcos/populate-marketplace-imagestream.go`:
- Around line 17-19: The constant streamMarketplaceRHCOSJSON in
populate-marketplace-imagestream.go points to
data/data/coreos/marketplace-coreos-rhel-9.json but pkg/rhcos/stream.go expects
data/data/coreos/marketplace/coreos-rhel-9.json; update the value of
streamMarketplaceRHCOSJSON to use the marketplace subdirectory (matching the
existing file and the reader), keeping streamRHCOSJSON unchanged.
---
Nitpick comments:
In `@pkg/coreoscli/cmd.go`:
- Around line 54-61: The local variable "stream" is declared and bound via
StringVar but never used; remove the dead variable and change the flag binding
to use Flags().String (e.g., use printStreamCmd.Flags().String("stream", "",
...) so printStreamJSON continues to read the value via
cmd.Flags().GetString("stream")), or alternatively capture the variable by
creating the command with RunE as a closure that calls
printStreamJSONWithStream(stream) (or update printStreamJSON to accept a stream
param) so the bound "stream" is actually consumed; choose one approach and
update the flag binding and call site (printStreamJSON/command creation)
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6556226e-9f4a-4edc-a867-979995b9f51c
📒 Files selected for processing (21)
data/data/coreos/coreos-rhel-10.jsondata/data/coreos/coreos-rhel-9.jsondata/data/coreos/marketplace/coreos-rhel-9.jsonhack/build-coreos-manifest.gohack/rhcos/populate-marketplace-imagestream.gopkg/asset/imagebased/image/baseiso.gopkg/asset/installconfig/aws/platform.gopkg/asset/installconfig/aws/regions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/nutanix/validation.gopkg/asset/installconfig/vsphere/validation.gopkg/asset/rhcos/image.gopkg/asset/rhcos/iso.gopkg/asset/rhcos/release.gopkg/coreoscli/cmd.gopkg/infrastructure/azure/azure.gopkg/rhcos/ami_regions.gopkg/rhcos/builds.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.gopkg/types/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/asset/rhcos/iso.go
- pkg/types/installconfig.go
- pkg/infrastructure/azure/azure.go
- pkg/asset/installconfig/aws/regions.go
- pkg/asset/installconfig/vsphere/validation.go
- pkg/asset/installconfig/aws/validation.go
- data/data/coreos/coreos-rhel-10.json
@djoshy Can this be addresed by https://issues.redhat.com/browse/MCO-2135? If so, do we need to address it in this or can we first introduce this one? I don't mind doing it here in a separate commit if it's required to have them since day-zero of the feature. Keep in mind no one is passing the osImageStream to the installer yet, neither CI, as CI is injecting the stream to the manifests directly for now. @patrickdillon I've fixed the linter issues and I made the |
I think this is fine, but once anyone (CI or even a dev testing this) starts using the InstallConfig path for streams, the boot images will get backupdated; potentially towards the end of installation(and definitely during steady state). They are unlikely to notice this due to the pivot. Unless of course they are specifically testing something that needs a RHEL10 bootimage. It is likely to go unnoticed in CI as well. We'll need to update existing boot image tests alongside openshift/machine-config-operator#5752 so the exclusion mechanism doesn't break on RHEL10 clusters. |
|
/retest |
|
/test e2e-aws-ovn-rhel10-devpreview |
|
@pablintino: This pull request references MCO-2133 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. |
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/rhcos/builds.go (1)
67-74:⚠️ Potential issue | 🔴 CriticalMarketplace modifications won't persist due to map value type.
The
Architecturesfield ismap[string]Arch(value type, not pointers). When iterating withfor name, arch := range st.Architectures, the variablearchis a copy of the struct value. Modifications toarch.RHELCoreOSExtensionsare applied to this copy and discarded after each iteration—they never update the original map.To fix this, either:
- Change the map type to
map[string]*Archin stream-metadata-go, or- Use index access:
st.Architectures[name].RHELCoreOSExtensions = ...(if the underlying library supports this pattern)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/rhcos/builds.go` around lines 67 - 74, The loop mutates a copy because Architectures is a map[string]Arch (value type) so setting fields on the loop variable `arch` won't persist; update the real map entry instead or change the map to hold pointers. Modify the loop that references `st.Architectures`, `arch`, `RHELCoreOSExtensions`, and `mktSt` so you assign into the map slot (e.g., load the entry, modify it, then write it back to st.Architectures[name]) or change the map type to map[string]*Arch and update code to use pointer entries so the Marketplace assignment persists.
🧹 Nitpick comments (2)
pkg/asset/installconfig/nutanix/validation.go (1)
162-170: Consider adding explicit stream compatibility detection in preloaded image validation.The preloaded image version comparison currently assumes versions are from the same RHCOS stream. When different streams are involved (RHCOS9 versions ~4xx vs. RHCOS10 versions ~10), integer comparison produces misleading error messages. If a user selects an RHCOS10 stream but has a preloaded RHCOS9 image, the error would incorrectly state the image is "too many revisions ahead" rather than identifying the stream mismatch.
Add validation to detect when the preloaded image's stream differs from the configured osImageStream and provide a clearer error message indicating the incompatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/asset/installconfig/nutanix/validation.go` around lines 162 - 170, The current version comparison using rhcosVerNum, imgVerNum and versionDiff can misreport when the image and configured osImageStream belong to different RHCOS streams; update the validation in the function containing this logic to first detect stream mismatch (e.g., compare parsed stream identifiers or major/version prefix from imgVerNum vs. rhcosVerNum or check against the configured osImageStream variable), and if streams differ return a clear error indicating the preloaded image's stream is incompatible with the configured osImageStream; only perform the numeric revision checks (versionDiff < 0, >=2, ==1) when the streams match.hack/build-coreos-manifest.go (1)
47-50: Minor: Error message could be clearer.The error message states "no %v image streams found" but the condition is actually that the default stream wasn't found among multiple discovered streams. Consider clarifying:
💡 Suggested improvement
defaultPaths, ok := streamsFiles[string(defaultStreamName)] if !ok { - return fmt.Errorf("no %v image streams found. %v is considered the default stream", defaultStreamName, defaultStreamName) + return fmt.Errorf("default stream %q not found among discovered streams; %v is required as the default", defaultStreamName, defaultStreamName) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/build-coreos-manifest.go` around lines 47 - 50, The error message when the default stream lookup fails (at the check using defaultPaths, ok := streamsFiles[string(defaultStreamName)]) is misleading; update the fmt.Errorf call to clearly state that the specified default stream (defaultStreamName) was not found among the discovered image streams, e.g. "default stream %v not found among discovered image streams; %v is considered the default" (or similar wording) so callers understand the condition; locate the fmt.Errorf call near the defaultPaths/streamsFiles check and replace the message accordingly, referencing defaultStreamName and optionally listing available keys from streamsFiles for extra clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/rhcos/builds.go`:
- Around line 67-74: The loop mutates a copy because Architectures is a
map[string]Arch (value type) so setting fields on the loop variable `arch` won't
persist; update the real map entry instead or change the map to hold pointers.
Modify the loop that references `st.Architectures`, `arch`,
`RHELCoreOSExtensions`, and `mktSt` so you assign into the map slot (e.g., load
the entry, modify it, then write it back to st.Architectures[name]) or change
the map type to map[string]*Arch and update code to use pointer entries so the
Marketplace assignment persists.
---
Nitpick comments:
In `@hack/build-coreos-manifest.go`:
- Around line 47-50: The error message when the default stream lookup fails (at
the check using defaultPaths, ok := streamsFiles[string(defaultStreamName)]) is
misleading; update the fmt.Errorf call to clearly state that the specified
default stream (defaultStreamName) was not found among the discovered image
streams, e.g. "default stream %v not found among discovered image streams; %v is
considered the default" (or similar wording) so callers understand the
condition; locate the fmt.Errorf call near the defaultPaths/streamsFiles check
and replace the message accordingly, referencing defaultStreamName and
optionally listing available keys from streamsFiles for extra clarity.
In `@pkg/asset/installconfig/nutanix/validation.go`:
- Around line 162-170: The current version comparison using rhcosVerNum,
imgVerNum and versionDiff can misreport when the image and configured
osImageStream belong to different RHCOS streams; update the validation in the
function containing this logic to first detect stream mismatch (e.g., compare
parsed stream identifiers or major/version prefix from imgVerNum vs. rhcosVerNum
or check against the configured osImageStream variable), and if streams differ
return a clear error indicating the preloaded image's stream is incompatible
with the configured osImageStream; only perform the numeric revision checks
(versionDiff < 0, >=2, ==1) when the streams match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25530b81-6f72-49b0-86ff-1459740b2ec4
📒 Files selected for processing (21)
data/data/coreos/coreos-rhel-10.jsondata/data/coreos/coreos-rhel-9.jsondata/data/coreos/marketplace/coreos-rhel-9.jsonhack/build-coreos-manifest.gohack/rhcos/populate-marketplace-imagestream.gopkg/asset/imagebased/image/baseiso.gopkg/asset/installconfig/aws/platform.gopkg/asset/installconfig/aws/regions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/nutanix/validation.gopkg/asset/installconfig/vsphere/validation.gopkg/asset/rhcos/image.gopkg/asset/rhcos/iso.gopkg/asset/rhcos/release.gopkg/coreoscli/cmd.gopkg/infrastructure/azure/azure.gopkg/rhcos/ami_regions.gopkg/rhcos/builds.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.gopkg/types/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/infrastructure/azure/azure.go
- pkg/asset/installconfig/aws/validation.go
- hack/rhcos/populate-marketplace-imagestream.go
- pkg/asset/installconfig/aws/regions.go
- pkg/rhcos/stream.go
- pkg/asset/rhcos/iso.go
- pkg/types/installconfig.go
- pkg/asset/imagebased/image/baseiso.go
|
/test e2e-aws-ovn-rhel10-devpreview |
|
Coderabbit still complains about the same issue & it's not obvious to me how you fixed it, but on the other hand it does not seem like a legit issue? The problem as I understand (maybe I'm wrong here) is that the marketplace images would get lost when producing the manifest, but I see the marketplace images populated for both ARM and x86 architectures when I check locally. So I am ok with this, but not sure if I'm missing something? |
patrickdillon
left a comment
patrickdillon
left a comment
There was a problem hiding this comment.
When testing I noticed that the output of
cat bin/manifests/coreos-bootimages.yaml | yq '.data.streams' | jq '."rhel-9"' | jq
Was not human readable. Claude helped me identify that this was double nested json, and these suggestions fixed it
hack/build-coreos-manifest.go
Outdated
| }, | ||
| } | ||
|
|
||
| streamsData := make(map[string]string) |
There was a problem hiding this comment.
| streamsData := make(map[string]string) | |
| streamsData := make(map[string]json.RawMessage) |
hack/build-coreos-manifest.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("could not read %s stream bytes: %w", name, err) | ||
| } | ||
| streamsData[name] = string(streamBytes) |
There was a problem hiding this comment.
| streamsData[name] = string(streamBytes) | |
| streamsData[name] = json.RawMessage(streamBytes) |
Consume the osImageStream install-config field through all code paths that consume embedded bootimage metadata, allowing the installer to load per-stream files (rhel-9.json, rhel-10.json) instead of a single rhcos.json. Defaults to RHEL 9 when the field is not set. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
|
@patrickdillon I completely missed the note that said: "Some comments are outside the diff and can’t be posted inline due to platform limitations." I took a look, and there was a similar piece of code outside of my original changes that had the exact same issue. I've fixed that now too.
There's a subtle Go detail here that Claude caught and I missed. In the Stream struct, almost all nested fields are embedded value structs, which is why we need to copy and re-assign them. The exception is the RHELCoreOSExtensions field, which is a pointer. When we run for name, arch := range bootImageStream.Architectures, the arch variable is a value copy. Modifying non-pointer fields on arch only affects the loop's local copy. However, because RHELCoreOSExtensions is a pointer, the local copy shares the exact same memory address as the original. Writing to the contents of that pointer modifies the original object too. Why weren't we seeing the issue locally? Because this initialization block never actually runs for our current data: if arch.RHELCoreOSExtensions == nil {
arch.RHELCoreOSExtensions = &rhcos.Extensions{}
}The architectures inside the marketplace file merge into the architectures of the regular stream file, which already have RHELCoreOSExtensions populated (not nil). The bug would only materialize if the marketplace pointed to an architecture that didn't have RHELCoreOSExtensions populated in the regular file. In that scenario, we would initialize a new pointer and assign it to our local arch copy, and that assignment would be lost as soon as the loop iteration ended. |
What do you think if we indent the content to make it even more human readable? Check what I've pushed. |
|
@pablintino: This pull request references MCO-2133 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. |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
hack/rhcos/populate-marketplace-imagestream.go (1)
18-19:⚠️ Potential issue | 🟠 MajorPlease keep the marketplace writer and reader on the same embedded path.
This still writes
data/data/coreos/marketplace-coreos-rhel-9.json. IfgetMarketplaceStreamFileName(...)is reading fromcoreos/marketplace/..., the generated marketplace overlay will never be loaded at runtime.#!/bin/bash set -euo pipefail echo "=== Writer path in hack/rhcos/populate-marketplace-imagestream.go ===" sed -n '17,21p' hack/rhcos/populate-marketplace-imagestream.go echo echo "=== Runtime marketplace path helpers ===" rg -n "getMarketplaceStreamFileName|marketplace" pkg/rhcos echo echo "=== Marketplace files currently checked in ===" fd . data/data/coreos | grep 'marketplace' || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/rhcos/populate-marketplace-imagestream.go` around lines 18 - 19, The marketplace writer constant streamMarketplaceRHCOSJSON is pointing to data/data/coreos/marketplace-coreos-rhel-9.json while the runtime reader (getMarketplaceStreamFileName) expects files under data/data/coreos/marketplace/; update the constant streamMarketplaceRHCOSJSON to use the embedded marketplace subdirectory (e.g., change to "data/data/coreos/marketplace/marketplace-coreos-rhel-9.json") so the writer and reader use the same embedded path, and verify any code that references streamMarketplaceRHCOSJSON still uses that symbol.
🧹 Nitpick comments (1)
pkg/types/installconfig.go (1)
679-683: Avoid exporting a mutable slice for the valid stream set.Any importer can modify
OSImageStreamValuesin place, which changes validation/help text process-wide. Prefer an unexported backing slice/array plus a helper that returns a copy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/types/installconfig.go` around lines 679 - 683, OSImageStreamValues is exported as a mutable slice which allows callers to mutate global validation data; replace it with an unexported backing slice (e.g. osImageStreamValuesBacking) and add an exported accessor function (e.g. GetOSImageStreamValues() []OSImageStream) that returns a copy of the backing slice; update any code that referenced OSImageStreamValues to call the accessor so callers only receive a copy and cannot mutate the shared data.
🤖 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/asset/installconfig/aws/validation.go`:
- Line 124: The validation currently calls
rhcos.AMIRegions(config.ControlPlane.Architecture, config.OSImageStream) and
only checks whether the requested region is present, which lets a
stream/architecture mismatch slip through; update the validation in the same
function to first detect when rhcos.AMIRegions(...) returns empty/nil for the
requested Architecture/OSImageStream and return a clear validation error (e.g.,
"selected osImageStream does not publish requested architecture") before
proceeding to the generic fallback path used later (the standard-partition copy
logic that currently runs when no AMIs are found); reference the
rhcos.AMIRegions call and the
config.ControlPlane.Architecture/config.OSImageStream fields when adding this
early rejection so the code fails-fast on unsupported stream/architecture
combos.
In `@pkg/asset/rhcos/iso.go`:
- Around line 33-35: The defaultCoreOSStreamGetter is hard-coded to
rhcos.DefaultOSImageStream causing Install workflows and baremetal bootstrap to
ignore a cluster's configured OSImageStream; update the flow so the chosen
stream from clusterInfo.OSImage (or InstallConfig.OSImageStream) is propagated
instead of falling back to nil/default: change the API so
defaultCoreOSStreamGetter (or a new variant) accepts a stream identifier (or
expose a helper like getCoreOSStream(ctx, streamName)) and update the two call
sites—pkg/infrastructure/baremetal/bootstrap.go (where nil is passed) and
pkg/asset/agent/image/baseiso.go (where customStreamGetter() returns nil for
AgentWorkflowTypeInstall)—to pass clusterInfo.OSImage (or the
InstallConfig.OSImageStream) when present; also adjust customStreamGetter to
return that stream for Install workflows rather than nil so
rhcos.FetchCoreOSBuild receives the proper stream name.
---
Duplicate comments:
In `@hack/rhcos/populate-marketplace-imagestream.go`:
- Around line 18-19: The marketplace writer constant streamMarketplaceRHCOSJSON
is pointing to data/data/coreos/marketplace-coreos-rhel-9.json while the runtime
reader (getMarketplaceStreamFileName) expects files under
data/data/coreos/marketplace/; update the constant streamMarketplaceRHCOSJSON to
use the embedded marketplace subdirectory (e.g., change to
"data/data/coreos/marketplace/marketplace-coreos-rhel-9.json") so the writer and
reader use the same embedded path, and verify any code that references
streamMarketplaceRHCOSJSON still uses that symbol.
---
Nitpick comments:
In `@pkg/types/installconfig.go`:
- Around line 679-683: OSImageStreamValues is exported as a mutable slice which
allows callers to mutate global validation data; replace it with an unexported
backing slice (e.g. osImageStreamValuesBacking) and add an exported accessor
function (e.g. GetOSImageStreamValues() []OSImageStream) that returns a copy of
the backing slice; update any code that referenced OSImageStreamValues to call
the accessor so callers only receive a copy and cannot mutate the shared data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 354e0215-0f60-432c-8ebd-612d2a8ece51
📒 Files selected for processing (21)
data/data/coreos/coreos-rhel-10.jsondata/data/coreos/coreos-rhel-9.jsondata/data/coreos/marketplace/coreos-rhel-9.jsonhack/build-coreos-manifest.gohack/rhcos/populate-marketplace-imagestream.gopkg/asset/imagebased/image/baseiso.gopkg/asset/installconfig/aws/platform.gopkg/asset/installconfig/aws/regions.gopkg/asset/installconfig/aws/validation.gopkg/asset/installconfig/nutanix/validation.gopkg/asset/installconfig/vsphere/validation.gopkg/asset/rhcos/image.gopkg/asset/rhcos/iso.gopkg/asset/rhcos/release.gopkg/coreoscli/cmd.gopkg/infrastructure/azure/azure.gopkg/rhcos/ami_regions.gopkg/rhcos/builds.gopkg/rhcos/stream.gopkg/rhcos/stream_scos.gopkg/types/installconfig.go
🚧 Files skipped from review as they are similar to previous changes (7)
- pkg/asset/rhcos/release.go
- pkg/infrastructure/azure/azure.go
- pkg/rhcos/stream.go
- pkg/asset/installconfig/aws/platform.go
- pkg/asset/imagebased/image/baseiso.go
- pkg/asset/installconfig/aws/regions.go
- pkg/asset/rhcos/image.go
|
/test artifacts-images |
|
/lgtm |
|
/verified by rhel10 presubmit |
openshift-ci-robot
added
the
verified
|
@patrickdillon: This PR has been marked as verified by 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. |
|
@pablintino: 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. |
|
/retest-required |
5 participants
Consume the osImageStream install-config field through all code paths that consume embedded bootimage metadata, allowing the installer to load per-stream files (rhel-9.json, rhel-10.json) instead of a single rhcos.json. Defaults to RHEL 9 when the field is not set.
Summary by CodeRabbit
New Features
Bug Fixes / Validation