MCO-2117: Allow default OSImageStream overrides#5714
MCO-2117: Allow default OSImageStream overrides#5714pablintino wants to merge 1 commit intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@pablintino: This pull request references MCO-2117 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
approved
pablintino
force-pushed
the
mco-2117
branch
3 times, most recently
from
1a8d2b6 to
6acb9de
Compare
| sysCtx, err := sysCtxBuilder.Build() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not prepare for OSImageStream inspection: %w", err) | ||
| } | ||
| defer func() { | ||
| if err := sysCtx.Cleanup(); err != nil { | ||
| klog.Warningf("Unable to clean resources after OSImageStream inspection: %s", err) | ||
| } | ||
| }() | ||
|
|
||
| factory := osimagestream.NewDefaultStreamSourceFactory(&osimagestream.DefaultImagesInspectorFactory{}) | ||
| osImageStream, err := factory.Create(ctx, sysCtx.SysContext, osimagestream.CreateOptions{ | ||
| ExistingOSImageStream: existingOSImageStream, | ||
| ReleaseImageStream: imageStream, | ||
| CliImages: &osimagestream.OSImageTuple{ |
There was a problem hiding this comment.
Note for reviewers: This change is "this big" mostly because the removal of the thin BuildOsImageStreamBootstrap wrappers as part of this PR. The important point, from the default handling point of view is that the ExistingOSImageStream is filled with whatever the installer passed.
pkg/controller/common/constants.go
Outdated
| ReleaseImageVersionAnnotationKey = "machineconfiguration.openshift.io/release-image-version" | ||
|
|
||
| // BuiltinDefaultStreamAnnotationKey is the MCO fallback default stream. | ||
| BuiltinDefaultStreamAnnotationKey = "machineconfiguration.openshift.io/builtin-default-stream" |
There was a problem hiding this comment.
Note for the reviewer: Used to store the fallback stream the MCO would use in case no default is provided by the installer/user.
Why do we need to store it? To avoid inspecting the payload over and over again during operator syncs.
There was a problem hiding this comment.
I think this is a good thing, since consulting the payload during an upgrade scenario might be problematic on a default change. How we have it setup right now should only cause a rebuild on MCO hash changes.
I wonder if we could upgrade test such a scenario to see if it behaves as we expect, though.
There was a problem hiding this comment.
Thinking about this more...as we're going to use an upgrade block to force all pools to transition to the new default stream before upgrading, the scenario described above should never happen? Sorry for the noise 😅
There was a problem hiding this comment.
@djoshy 100% the idea is to add the EoL in a z stream before we expect the stream to dissapear. The mechanism? A simple upgrade condition. I think the main agreement we made was delete streams in major releases only, that will help too.
| sysCtx, err := sysCtxBuilder.Build() | ||
| if err != nil { | ||
| return fmt.Errorf("could not prepare for OSImageStream inspection: %w", err) | ||
| } | ||
| defer func() { | ||
| if err := sysCtx.Cleanup(); err != nil { | ||
| klog.Warningf("Unable to clean resources after OSImageStream inspection: %s", err) | ||
| } | ||
| }() | ||
|
|
||
| factory := osimagestream.NewDefaultStreamSourceFactory(&osimagestream.DefaultImagesInspectorFactory{}) | ||
| osImageStream, err := factory.Create( | ||
| buildCtx, | ||
| sysCtx.SysContext, | ||
| osimagestream.CreateOptions{ | ||
| ReleaseImage: image, | ||
| ConfigMapLister: optr.mcoCmLister, | ||
| ExistingOSImageStream: existingOSImageStream, | ||
| }) |
There was a problem hiding this comment.
Note for the reviewer: Same as https://github.com/openshift/machine-config-operator/pull/5714/changes#r2871640129
pkg/operator/osimagestream_ocp.go
Outdated
| } | ||
| releaseVersion, ok := osImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] | ||
| return !ok || releaseVersion != version.Hash | ||
| return !ok || releaseVersion != version.Hash || osimagestream.GetBuiltinDefault(osImageStream) == "" |
There was a problem hiding this comment.
Note for the reviewer: If the annotation is removed ensure we rebuild the CR. Shouldn't happen, but just in case an admin user removes it.
| i.cacheLock.Lock() | ||
| defer i.cacheLock.Unlock() | ||
| if i.imageStream != nil { | ||
| return i.imageStream, nil | ||
| } | ||
| is, err := i.fetchImageStream(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| i.imageStream = is | ||
| return is, nil | ||
| } |
There was a problem hiding this comment.
Note for the reviewer: Now we have a look at the ImageStream twice, one to fetch the streams and another one to determine the default stream. To avoid that new extra call (or future calls if we need more) I've added this simple cache mechanism.
| // CreateRuntimeSources builds an OSImageStream for runtime operation. | ||
| CreateRuntimeSources(ctx context.Context, releaseImage string, sysCtx *types.SystemContext) (*v1alpha1.OSImageStream, error) | ||
| // CreateBootstrapSources builds an OSImageStream for bootstrap operation. | ||
| CreateBootstrapSources(ctx context.Context, imageStream *imagev1.ImageStream, cliImages *OSImageTuple, sysCtx *types.SystemContext) (*v1alpha1.OSImageStream, error) | ||
| // Create builds an OSImageStream from the configured sources and options. | ||
| Create(ctx context.Context, sysCtx *types.SystemContext, opts CreateOptions) (*v1alpha1.OSImageStream, error) |
There was a problem hiding this comment.
Note for the reviewer: With the inclusion of the default stream field the signature of these two functions started to be a mess I didn't like at all so I converged both and switch to a single arg with all the possible options as fields.
| cv, err := clientSet.ClusterVersions().Get(ctx, "version", metav1.GetOptions{}) | ||
| require.NoError(t, err, "Error retrieving ClusterVersion") | ||
|
|
||
| payloadVersion, err := semver.ParseTolerant(cv.Status.Desired.Version) | ||
| require.NoError(t, err, "Error parsing payload version") | ||
|
|
||
| featureGateStatus := features.FeatureSets(payloadVersion.Major, SelfManaged, currentFeatureSet) |
There was a problem hiding this comment.
Note for the reviewer: This is because of a breaking change in the FeatureGate's API.
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
|
@pablintino: This pull request references MCO-2117 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. |
pkg/osimagestream/osimagestream.go
Outdated
| } | ||
|
|
||
| func getDefaultImageStreamTag() string { | ||
| imageTag := "rhel-coreos" |
There was a problem hiding this comment.
Interesting, so when we eventually drop rhel9, would the rhel10 stream get this tag?
djoshy
left a comment
djoshy
left a comment
There was a problem hiding this comment.
Overall makes sense to me, thanks for the very helpful notes.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, pablintino 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughIntroduces a unified OSImageStream creation flow (CreateOptions-based factory), adds thread-safe ImageStream caching, ClusterVersion utilities (GetClusterVersion/GetInstallVersion), OSImageStream informer handlers and operator wiring, a builtin-default annotation constant, and comprehensive tests and refactors across controllers and osimagestream logic. Changes
Sequence DiagramsequenceDiagram
participant Client
participant BootstrapCtrl
participant Factory
participant Inspector
participant Operator
participant MCPController
Client->>BootstrapCtrl: start bootstrap (may include existing OSImageStream)
BootstrapCtrl->>BootstrapCtrl: determine existingOSImageStream
alt OSImageStream feature enabled
BootstrapCtrl->>Factory: Create(ctx, sysCtx, CreateOptions{ExistingOSImageStream, ReleaseImage/Stream, InstallVersion})
Factory->>Inspector: fetch/inspect release / ImageStream / CLI sources
Inspector-->>Factory: return streams & metadata
Factory->>Factory: compute builtin default, merge sources, assemble OSImageStream
Factory-->>BootstrapCtrl: return assembled OSImageStream
BootstrapCtrl->>Operator: apply/create OSImageStream resource
Operator->>MCPController: OSImageStream status update triggers informer
MCPController->>Operator: updateOSImageStream handler enqueues affected MCPs
else feature disabled
BootstrapCtrl->>BootstrapCtrl: set osImageStream = nil / skip create
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 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)
📝 Coding Plan
Comment Tip CodeRabbit can suggest fixes for GitHub Check annotations.Configure the |
|
@pablintino: This pull request references MCO-2117 which is a valid jira issue. 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: 3
🧹 Nitpick comments (2)
pkg/osimagestream/osimagestream.go (2)
201-219: Non-deterministic ordering of AvailableStreams.
maps.Valuesreturns values in arbitrary iteration order, causingStatus.AvailableStreamsto have inconsistent ordering across reconciliations. This could lead to unnecessary CR updates or flaky tests.♻️ Suggested fix: Sort streams by name for deterministic output
func collect(ctx context.Context, sources []StreamSource) []v1alpha1.OSImageStreamSet { result := make(map[string]v1alpha1.OSImageStreamSet) for _, source := range sources { streams, err := source.FetchStreams(ctx) if err != nil { // Do not return: Soft failure, best effort klog.Warningf("error fetching OSImageStreams from a source: %v", err) } for _, stream := range streams { _, exists := result[stream.Name] if exists { // Conflicts are allowed and we simply override the previous content klog.V(4).Infof("overriding OSImageStream %s with %s %s", stream.Name, stream.OSImage, stream.OSExtensionsImage) } result[stream.Name] = *stream } } - return slices.Collect(maps.Values(result)) + streams := slices.Collect(maps.Values(result)) + slices.SortFunc(streams, func(a, b v1alpha1.OSImageStreamSet) int { + if a.Name < b.Name { + return -1 + } + if a.Name > b.Name { + return 1 + } + return 0 + }) + return streams }Or more concisely with
cmp.Compare:slices.SortFunc(streams, func(a, b v1alpha1.OSImageStreamSet) int { return cmp.Compare(a.Name, b.Name) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/osimagestream/osimagestream.go` around lines 201 - 219, The collect function currently returns slices.Collect(maps.Values(result)) which yields non-deterministic ordering; update collect (after building result and before return) to convert maps.Values(result) into a slice and sort it deterministically by OSImageStreamSet.Name (e.g. using slices.SortFunc or cmp.Compare) so Status.AvailableStreams is stable across reconciliations; ensure you replace the direct return with the sorted slice and keep the existing behavior of overriding duplicates in result.
141-143: Consider adding a comment explaining the status fallback.This logic preserves the existing
Status.DefaultStreamduring reconciliation when the user hasn't explicitly setSpec.DefaultStream. While this is correct behavior to prevent unexpected changes to running clusters, a brief inline comment would clarify this intent for future maintainers.📝 Suggested documentation
requestedDefaultStream := GetOSImageStreamSpecDefault(existingOSImageStream) if requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "" { - // If the spec is empty but the CR is already populated pick whatever existed + // Preserve the existing default during reconciliation when no spec override is set. + // This prevents unexpected changes to running clusters on re-sync. requestedDefaultStream = existingOSImageStream.Status.DefaultStream🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/osimagestream/osimagestream.go` around lines 141 - 143, Add a brief inline comment above the conditional that assigns requestedDefaultStream from existingOSImageStream.Status.DefaultStream (the block that checks requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "") explaining this is an intentional "status fallback" used during reconciliation: when the user has not set Spec.DefaultStream, preserve the cluster's current DefaultStream recorded in Status.DefaultStream to avoid changing runtime behavior; mention that this prevents unexpected drift and is only applied when Spec is empty.
🤖 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/operator/operator.go`:
- Around line 375-376: buildOSImageStream() currently calls Update() and
UpdateStatus() unconditionally when rebuildRequired() (which checks
version.Hash) returns true, causing unnecessary writes; add a semantic diff
guard before invoking Update() and UpdateStatus() so you only perform API
updates when the constructed OSImageStream object actually differs from the
existing one. Locate buildOSImageStream(), compare the desired OSImageStream
(including annotations like version.Hash and status fields) against the
fetched/original OSImageStream, and only call Update() or UpdateStatus() when
that deep/semantic comparison indicates changes; follow the same pattern used in
updateOSImageStream() and sync.go (compare resource spec/annotations/status
first) to avoid redundant writes and extra event handler firings.
In `@pkg/operator/osimagestream_ocp.go`:
- Around line 296-304: Add a unit test for osImageStreamRequiresRebuild that
constructs a v1alpha1.OSImageStream with the ReleaseImageVersionAnnotationKey
set to version.Hash but without the builtin default annotation and asserts
osImageStreamRequiresRebuild(osImageStream) returns true; target the
osImageStreamRequiresRebuild function and use the v1alpha1.OSImageStream type
and version.Hash constant so the test specifically verifies the missing builtin
default path (osimagestream.GetBuiltinDefault(...) == "") triggers a rebuild.
In `@pkg/osimagestream/clusterversion.go`:
- Around line 34-55: GetInstallVersion dereferences clusterVersion without
checking for nil; add an initial nil check in GetInstallVersion (e.g., if
clusterVersion == nil) and return a descriptive error (matching the style used
by GetReleasePayloadImage) before accessing clusterVersion.Status.History so the
function fails gracefully instead of panicking.
---
Nitpick comments:
In `@pkg/osimagestream/osimagestream.go`:
- Around line 201-219: The collect function currently returns
slices.Collect(maps.Values(result)) which yields non-deterministic ordering;
update collect (after building result and before return) to convert
maps.Values(result) into a slice and sort it deterministically by
OSImageStreamSet.Name (e.g. using slices.SortFunc or cmp.Compare) so
Status.AvailableStreams is stable across reconciliations; ensure you replace the
direct return with the sorted slice and keep the existing behavior of overriding
duplicates in result.
- Around line 141-143: Add a brief inline comment above the conditional that
assigns requestedDefaultStream from existingOSImageStream.Status.DefaultStream
(the block that checks requestedDefaultStream == "" && existingOSImageStream !=
nil && existingOSImageStream.Status.DefaultStream != "") explaining this is an
intentional "status fallback" used during reconciliation: when the user has not
set Spec.DefaultStream, preserve the cluster's current DefaultStream recorded in
Status.DefaultStream to avoid changing runtime behavior; mention that this
prevents unexpected drift and is only applied when Spec is empty.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9687e1fa-dd50-4264-997f-94776a8d6666
⛔ Files ignored due to path filters (64)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/payload-command/render/render.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/payload-command/render/write_featureset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatoradmissionwebhookconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestreamspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (17)
go.modpkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/render/render_controller.gopkg/operator/operator.gopkg/operator/osimagestream_ocp.gopkg/osimagestream/clusterversion.gopkg/osimagestream/clusterversion_test.gopkg/osimagestream/helpers.gopkg/osimagestream/helpers_test.gopkg/osimagestream/imagestream_provider.gopkg/osimagestream/imagestream_provider_test.gopkg/osimagestream/mocks_test.gopkg/osimagestream/osimagestream.gopkg/osimagestream/osimagestream_test.gotest/e2e-bootstrap/bootstrap_test.go
pkg/operator/osimagestream_ocp.go
Outdated
| // osImageStreamRequiresRebuild checks if the OSImageStream needs to be created or updated. | ||
| // Returns true if osImageStream is nil, if its version annotation doesn't match the current version, | ||
| // or if the builtin default stream annotation is missing. | ||
| func osImageStreamRequiresRebuild(osImageStream *v1alpha1.OSImageStream) bool { | ||
| if osImageStream == nil { | ||
| return true | ||
| } | ||
| releaseVersion, ok := osImageStream.Annotations[ctrlcommon.ReleaseImageVersionAnnotationKey] | ||
| return !ok || releaseVersion != version.Hash | ||
| return !ok || releaseVersion != version.Hash || osimagestream.GetBuiltinDefault(osImageStream) == "" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for existing tests covering osImageStreamRequiresRebuild
# Test: Check for test coverage of the rebuild requirement logic
rg -n "osImageStreamRequiresRebuild|RequiresRebuild" --type goRepository: openshift/machine-config-operator
Length of output: 851
🏁 Script executed:
#!/bin/bash
# Find test files related to osimagestream
fd -e "*test.go" | xargs grep -l "osImageStreamRequiresRebuild\|osimagestream" | head -20Repository: openshift/machine-config-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check the actual implementation of osImageStreamRequiresRebuild and GetBuiltinDefault
sed -n '296,304p' pkg/operator/osimagestream_ocp.go
# Also look for more context around the function
sed -n '290,310p' pkg/operator/osimagestream_ocp.goRepository: openshift/machine-config-operator
Length of output: 1279
🏁 Script executed:
#!/bin/bash
# Find the GetBuiltinDefault function to understand what it does
rg -n "func GetBuiltinDefault" --type go -A 5Repository: openshift/machine-config-operator
Length of output: 459
🏁 Script executed:
#!/bin/bash
# Search for test files in the operator directory
fd -e "*test.go" pkg/operator/ | xargs grep -l "osImageStream" 2>/dev/null || echo "No matches found"Repository: openshift/machine-config-operator
Length of output: 468
🏁 Script executed:
#!/bin/bash
# Check if there's an osimagestream_ocp_test.go file
ls -la pkg/operator/*osimage* 2>/dev/null || echo "No osimage files found in pkg/operator"Repository: openshift/machine-config-operator
Length of output: 244
🏁 Script executed:
#!/bin/bash
# Search for any test files that might test the rebuild logic
rg -l "TestOsImageStream|TestRequiresRebuild" pkg/ --type goRepository: openshift/machine-config-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Search more broadly for test files containing osImageStream references
rg "osImageStreamRequiresRebuild" --type go -lRepository: openshift/machine-config-operator
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Check the actual context where osImageStreamRequiresRebuild is called
rg -B 10 -A 10 "osImageStreamRequiresRebuild" pkg/operator/osimagestream_ocp.go | head -50Repository: openshift/machine-config-operator
Length of output: 1725
🏁 Script executed:
#!/bin/bash
# Search for test files in pkg/operator directory properly
fd . pkg/operator -name "*test.go"Repository: openshift/machine-config-operator
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Search for all test files that might contain osImageStream references
find . -name "*_test.go" -type f | xargs grep -l "osImageStream" 2>/dev/null || echo "No test files found with osImageStream"Repository: openshift/machine-config-operator
Length of output: 112
🏁 Script executed:
#!/bin/bash
# Check if there are test files in the entire pkg directory that test this functionality
rg -l "osImageStream" --type go | grep -i testRepository: openshift/machine-config-operator
Length of output: 110
🏁 Script executed:
#!/bin/bash
# Look for any test files in pkg/operator
find pkg/operator -name "*test*" -type fRepository: openshift/machine-config-operator
Length of output: 158
🏁 Script executed:
#!/bin/bash
# Check sync_test.go for osImageStreamRequiresRebuild tests
rg "osImageStreamRequiresRebuild" pkg/operator/sync_test.go -B 5 -A 10Repository: openshift/machine-config-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Check if there are any tests for osImageStream in sync_test.go
rg "osImageStream" pkg/operator/sync_test.go -B 2 -A 5 | head -100Repository: openshift/machine-config-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Examine helpers_test.go to see GetBuiltinDefault tests
cat pkg/osimagestream/helpers_test.goRepository: openshift/machine-config-operator
Length of output: 5274
🏁 Script executed:
#!/bin/bash
# Read sync_test.go to see if it has any OSImageStream related tests
wc -l pkg/operator/sync_test.goRepository: openshift/machine-config-operator
Length of output: 106
🏁 Script executed:
#!/bin/bash
# Search for any mention of osImageStream in sync_test.go
grep -n "osImageStream\|OSImageStream" pkg/operator/sync_test.goRepository: openshift/machine-config-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Let's look at the structure of sync_test.go to understand what it tests
head -100 pkg/operator/sync_test.goRepository: openshift/machine-config-operator
Length of output: 4009
🏁 Script executed:
#!/bin/bash
# Check if there are any unit tests for osImageStreamRequiresRebuild function specifically
rg "TestOsImageStreamRequiresRebuild|TestRequiresRebuild" --type goRepository: openshift/machine-config-operator
Length of output: 59
🏁 Script executed:
#!/bin/bash
# Look at the entire sync_test.go to understand what sync functions are tested
grep -n "^func Test" pkg/operator/sync_test.goRepository: openshift/machine-config-operator
Length of output: 240
🏁 Script executed:
#!/bin/bash
# Check if there's any test that covers the syncOSImageStream or related flow
grep -n "syncOSImageStream\|OSImageStream" pkg/operator/sync_test.go | head -20Repository: openshift/machine-config-operator
Length of output: 59
Add test case for osImageStreamRequiresRebuild with missing builtin default annotation.
The implementation correctly triggers a rebuild when the builtin default annotation is missing (osimagestream.GetBuiltinDefault(osImageStream) == "" returns true). However, the osImageStreamRequiresRebuild function itself lacks unit test coverage. While GetBuiltinDefault is tested in pkg/osimagestream/helpers_test.go, the rebuild logic should have its own test verifying that an OSImageStream with the version annotation present but the builtin default annotation missing triggers a rebuild.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/operator/osimagestream_ocp.go` around lines 296 - 304, Add a unit test
for osImageStreamRequiresRebuild that constructs a v1alpha1.OSImageStream with
the ReleaseImageVersionAnnotationKey set to version.Hash but without the builtin
default annotation and asserts osImageStreamRequiresRebuild(osImageStream)
returns true; target the osImageStreamRequiresRebuild function and use the
v1alpha1.OSImageStream type and version.Hash constant so the test specifically
verifies the missing builtin default path (osimagestream.GetBuiltinDefault(...)
== "") triggers a rebuild.
|
@pablintino: This pull request references MCO-2117 which is a valid jira issue. 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: This pull request references MCO-2117 which is a valid jira issue. 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 (2)
pkg/osimagestream/osimagestream.go (2)
141-158:⚠️ Potential issue | 🟠 MajorDrop the sticky
Status.DefaultStreamfallback.When
Spec.DefaultStreamis empty or no longer valid, this still pins reconciliation to the oldStatus.DefaultStreaminstead of falling back to the builtin default. That contradicts the new override contract and can keep clusters stuck on a stale default.pkg/osimagestream/osimagestream_test.goLines 171-180 will need to follow the same fallback order once this is fixed.Suggested fix
func getDefaultStreamSet(streams []v1alpha1.OSImageStreamSet, builtinDefault string, existingOSImageStream *v1alpha1.OSImageStream) (string, error) { streamNames := GetStreamSetsNames(streams) requestedDefaultStream := GetOSImageStreamSpecDefault(existingOSImageStream) - if requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "" { - // If the spec is empty but the CR is already populated pick whatever existed - requestedDefaultStream = existingOSImageStream.Status.DefaultStream - } else if requestedDefaultStream == "" { - // No user preference: if there's only one stream just use it, - // otherwise fall back to the builtin default. - if len(streams) == 1 { - return streams[0].Name, nil - } - requestedDefaultStream = builtinDefault + if requestedDefaultStream != "" && slices.Contains(streamNames, requestedDefaultStream) { + return requestedDefaultStream, nil } - if slices.Contains(streamNames, requestedDefaultStream) { - return requestedDefaultStream, nil + if len(streams) == 1 { + return streams[0].Name, nil } - return "", fmt.Errorf("could not find the requested %s default stream in the list of OSImageStreams %s", requestedDefaultStream, streamNames) + if slices.Contains(streamNames, builtinDefault) { + return builtinDefault, nil + } + + return "", fmt.Errorf("could not find a usable default stream in %v (requested=%q, builtin=%q)", streamNames, requestedDefaultStream, builtinDefault) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/osimagestream/osimagestream.go` around lines 141 - 158, The code currently falls back to existingOSImageStream.Status.DefaultStream when GetOSImageStreamSpecDefault(existingOSImageStream) is empty; remove that “sticky” Status.DefaultStream fallback so an empty Spec.DefaultStream does not pin reconciliation to a prior status. In the GetOSImageStreamSpecDefault usage block (around requestedDefaultStream, existingOSImageStream, Status.DefaultStream, streams, streamNames, builtinDefault) delete the branch that assigns existingOSImageStream.Status.DefaultStream and instead treat an empty requestedDefaultStream as "no user preference" — if len(streams)==1 return streams[0].Name, else set requestedDefaultStream = builtinDefault; keep the subsequent slices.Contains check and error path, and update the related tests in pkg/osimagestream/osimagestream_test.go to expect the same fallback order.
161-188:⚠️ Potential issue | 🟠 MajorResolve the builtin default from payload defaults, not
releaseVersion.Major().This helper still hard-codes
4 -> rhel-9and>=5 -> rhel-10. The PR objective says the builtin default should come from the payloadImageStream’s default OS tags, then map that image back to the matchingOSImageStreamSet; otherwise the controller can annotate/select the wrong builtin default whenever payload defaults diverge from the major-version heuristic. As written, this function also cannot implement that contract because it never sees the collected stream sets.pkg/osimagestream/osimagestream_test.goLines 915-1010 currently lock in the same old heuristic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/osimagestream/osimagestream.go` around lines 161 - 188, getBuiltinDefaultStreamName currently determines the builtin default purely from releaseVersion.Major(), which ignores the payload ImageStream defaults and the available OSImageStreamSets; update the function to use the payload ImageStream's defaults (via ImageStreamProvider/getVersionFromImageStream or a new call) and map the default OS tag back to the matching OSImageStreamSet to return the appropriate StreamName (e.g., check the ImageStream's default tag or annotation, find the OSImageStreamSet whose images/tags include that image, and return its name); if no payload default-to-set mapping can be determined, fall back to the previous major-version heuristic (reuse releaseVersion and return StreamNameRHEL9 or StreamNameRHEL10).
🧹 Nitpick comments (1)
pkg/osimagestream/imagestream_provider_test.go (1)
143-203: Add one concurrent-read case for the new cache path.These tests cover sequential retry/caching, but the production change is specifically the shared mutex-protected cache. A goroutine-based subtest asserting
fetchCount == 1would protect against duplicate fetches or racey cache population.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/osimagestream/imagestream_provider_test.go` around lines 143 - 203, Add a concurrent-read subtest to exercise the mutex-protected cache: in TestImageStreamProviderNetwork_CachesOnSuccess (or a new subtest within it) spawn multiple goroutines that concurrently call provider.ReadImageStream(ctx) after creating provider via NewImageStreamProviderNetwork and the mockImagesInspector, wait for all to finish (e.g., sync.WaitGroup), then assert inspector.fetchCount == 1 and that all returned pointers are the same; this verifies the cache is populated only once under concurrency and prevents duplicate fetches/races when mockImagesInspector.fetchCount is incremented during fetch.
🤖 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/controller/bootstrap/bootstrap.go`:
- Around line 203-205: The code currently assigns any
*mcfgv1alpha1.OSImageStream to osImageStream, allowing a non-singleton manifest
to win; change the assignment so it only consumes the cluster singleton
OSImageStream used at runtime: in the switch arm for
*mcfgv1alpha1.OSImageStream, check the object's identity (e.g.,
obj.GetNamespace() and obj.GetName() or the specific singleton name/constant the
runtime reads) and only set osImageStream = obj when it matches the expected
cluster singleton; otherwise ignore (and optionally log) the extra OSImageStream
so the bootstrap path cannot be overridden by non-singleton manifests.
In `@pkg/controller/render/render_controller.go`:
- Around line 285-290: The current early-return only compares
oldOSImageStream.Status vs curOSImageStream.Status, so changes to
spec.defaultStream are ignored; update the guard in the reconcile path (where
oldOSImageStream and curOSImageStream are compared) to also detect changes to
curOSImageStream.Spec.DefaultStream (or compare the whole Spec.DefaultStream
field) and only return early when both Status and Spec.DefaultStream are
unchanged—i.e., include a check that requeues pools when
curOSImageStream.Spec.DefaultStream != oldOSImageStream.Spec.DefaultStream (or
use reflect.DeepEqual on a struct containing Status and the DefaultStream) so
spec.defaultStream updates trigger enqueuing.
---
Duplicate comments:
In `@pkg/osimagestream/osimagestream.go`:
- Around line 141-158: The code currently falls back to
existingOSImageStream.Status.DefaultStream when
GetOSImageStreamSpecDefault(existingOSImageStream) is empty; remove that
“sticky” Status.DefaultStream fallback so an empty Spec.DefaultStream does not
pin reconciliation to a prior status. In the GetOSImageStreamSpecDefault usage
block (around requestedDefaultStream, existingOSImageStream,
Status.DefaultStream, streams, streamNames, builtinDefault) delete the branch
that assigns existingOSImageStream.Status.DefaultStream and instead treat an
empty requestedDefaultStream as "no user preference" — if len(streams)==1 return
streams[0].Name, else set requestedDefaultStream = builtinDefault; keep the
subsequent slices.Contains check and error path, and update the related tests in
pkg/osimagestream/osimagestream_test.go to expect the same fallback order.
- Around line 161-188: getBuiltinDefaultStreamName currently determines the
builtin default purely from releaseVersion.Major(), which ignores the payload
ImageStream defaults and the available OSImageStreamSets; update the function to
use the payload ImageStream's defaults (via
ImageStreamProvider/getVersionFromImageStream or a new call) and map the default
OS tag back to the matching OSImageStreamSet to return the appropriate
StreamName (e.g., check the ImageStream's default tag or annotation, find the
OSImageStreamSet whose images/tags include that image, and return its name); if
no payload default-to-set mapping can be determined, fall back to the previous
major-version heuristic (reuse releaseVersion and return StreamNameRHEL9 or
StreamNameRHEL10).
---
Nitpick comments:
In `@pkg/osimagestream/imagestream_provider_test.go`:
- Around line 143-203: Add a concurrent-read subtest to exercise the
mutex-protected cache: in TestImageStreamProviderNetwork_CachesOnSuccess (or a
new subtest within it) spawn multiple goroutines that concurrently call
provider.ReadImageStream(ctx) after creating provider via
NewImageStreamProviderNetwork and the mockImagesInspector, wait for all to
finish (e.g., sync.WaitGroup), then assert inspector.fetchCount == 1 and that
all returned pointers are the same; this verifies the cache is populated only
once under concurrency and prevents duplicate fetches/races when
mockImagesInspector.fetchCount is incremented during fetch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7b0e807-be72-4dfb-8657-3b262c0cd526
⛔ Files ignored due to path filters (64)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/payload-command/render/render.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/payload-command/render/write_featureset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatoradmissionwebhookconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestreamspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (18)
go.modpkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/render/render_controller.gopkg/operator/operator.gopkg/operator/osimagestream_ocp.gopkg/osimagestream/clusterversion.gopkg/osimagestream/clusterversion_test.gopkg/osimagestream/helpers.gopkg/osimagestream/helpers_test.gopkg/osimagestream/imagestream_provider.gopkg/osimagestream/imagestream_provider_test.gopkg/osimagestream/mocks_test.gopkg/osimagestream/osimagestream.gopkg/osimagestream/osimagestream_test.gotest/e2e-2of2/osimagestream_test.gotest/e2e-bootstrap/bootstrap_test.go
🚧 Files skipped from review as they are similar to previous changes (6)
- pkg/osimagestream/helpers.go
- pkg/osimagestream/helpers_test.go
- go.mod
- pkg/osimagestream/mocks_test.go
- test/e2e-2of2/osimagestream_test.go
- pkg/operator/osimagestream_ocp.go
| case *mcfgv1alpha1.OSImageStream: | ||
| // If given, it's treated as user input with config such as the default stream | ||
| osImageStream = obj |
There was a problem hiding this comment.
Only consume the singleton cluster OSImageStream here.
Lines 203-205 accept any OSImageStream manifest and silently let the last one win. The runtime path later reads only the singleton cluster resource, so bootstrap can apply the wrong manifest's spec.defaultStream if an extra OSImageStream is present.
Proposed fix
case *mcfgv1alpha1.OSImageStream:
- // If given, it's treated as user input with config such as the default stream
- osImageStream = obj
+ if obj.GetName() != ctrlcommon.ClusterInstanceNameOSImageStream {
+ klog.V(4).Infof("skipping OSImageStream %q; only %q is consumed during bootstrap", obj.GetName(), ctrlcommon.ClusterInstanceNameOSImageStream)
+ continue
+ }
+ if osImageStream != nil {
+ return fmt.Errorf("multiple OSImageStream manifests found for %q", ctrlcommon.ClusterInstanceNameOSImageStream)
+ }
+ // If given, it's treated as user input with config such as the default stream.
+ osImageStream = obj📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| case *mcfgv1alpha1.OSImageStream: | |
| // If given, it's treated as user input with config such as the default stream | |
| osImageStream = obj | |
| case *mcfgv1alpha1.OSImageStream: | |
| if obj.GetName() != ctrlcommon.ClusterInstanceNameOSImageStream { | |
| klog.V(4).Infof("skipping OSImageStream %q; only %q is consumed during bootstrap", obj.GetName(), ctrlcommon.ClusterInstanceNameOSImageStream) | |
| continue | |
| } | |
| if osImageStream != nil { | |
| return fmt.Errorf("multiple OSImageStream manifests found for %q", ctrlcommon.ClusterInstanceNameOSImageStream) | |
| } | |
| // If given, it's treated as user input with config such as the default stream. | |
| osImageStream = obj |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/bootstrap/bootstrap.go` around lines 203 - 205, The code
currently assigns any *mcfgv1alpha1.OSImageStream to osImageStream, allowing a
non-singleton manifest to win; change the assignment so it only consumes the
cluster singleton OSImageStream used at runtime: in the switch arm for
*mcfgv1alpha1.OSImageStream, check the object's identity (e.g.,
obj.GetNamespace() and obj.GetName() or the specific singleton name/constant the
runtime reads) and only set osImageStream = obj when it matches the expected
cluster singleton; otherwise ignore (and optionally log) the extra OSImageStream
so the bootstrap path cannot be overridden by non-singleton manifests.
| // return if status hasn't changed | ||
| // The operator components only reacts to what the OSImageStream | ||
| // publish in its status subresource. | ||
| if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) { | ||
| return | ||
| } |
There was a problem hiding this comment.
Requeue pools when spec.defaultStream changes.
Line 288 only compares Status, so a patch to osimagestream/cluster.spec.defaultStream exits early and no pool gets enqueued. That breaks the running-cluster override flow this PR is adding, because render resolves the default stream from the OSImageStream object itself.
Proposed fix
- // return if status hasn't changed
- // The operator components only reacts to what the OSImageStream
- // publish in its status subresource.
- if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) {
+ // Return only when neither the published streams nor the selected default changed.
+ if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) &&
+ reflect.DeepEqual(oldOSImageStream.Spec, curOSImageStream.Spec) {
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // return if status hasn't changed | |
| // The operator components only reacts to what the OSImageStream | |
| // publish in its status subresource. | |
| if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) { | |
| return | |
| } | |
| // Return only when neither the published streams nor the selected default changed. | |
| if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) && | |
| reflect.DeepEqual(oldOSImageStream.Spec, curOSImageStream.Spec) { | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/controller/render/render_controller.go` around lines 285 - 290, The
current early-return only compares oldOSImageStream.Status vs
curOSImageStream.Status, so changes to spec.defaultStream are ignored; update
the guard in the reconcile path (where oldOSImageStream and curOSImageStream are
compared) to also detect changes to curOSImageStream.Spec.DefaultStream (or
compare the whole Spec.DefaultStream field) and only return early when both
Status and Spec.DefaultStream are unchanged—i.e., include a check that requeues
pools when curOSImageStream.Spec.DefaultStream !=
oldOSImageStream.Spec.DefaultStream (or use reflect.DeepEqual on a struct
containing Status and the DefaultStream) so spec.defaultStream updates trigger
enqueuing.
|
@pablintino: This pull request references MCO-2117 which is a valid jira issue. 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 (3)
pkg/controller/render/render_controller.go (1)
285-290:⚠️ Potential issue | 🟠 MajorSpec-only default changes are still ignored.
Line 288 only compares
Status, so a patch toosimagestream/cluster.spec.defaultStreamcan return early without re-enqueuing anyMachineConfigPool. That breaks the running-cluster override flow this PR is adding.Suggested fix
- // return if status hasn't changed - // The operator components only reacts to what the OSImageStream - // publish in its status subresource. - if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) { + // Return only when neither the published streams nor the selected default changed. + if reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) && + reflect.DeepEqual(oldOSImageStream.Spec, curOSImageStream.Spec) { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/controller/render/render_controller.go` around lines 285 - 290, The current early-return checks only reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) so spec-only changes (like osimagestream.spec.defaultStream) are ignored; update the condition to also compare the spec default stream (or the relevant spec field) before returning. Specifically, change the check to return only when both reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) and reflect.DeepEqual(oldOSImageStream.Spec.DefaultStream, curOSImageStream.Spec.DefaultStream) (or an equivalent comparison of the relevant Spec field) are true so a change to Spec.DefaultStream will not be skipped; reference oldOSImageStream, curOSImageStream, .Status and .Spec.DefaultStream in your fix.pkg/osimagestream/osimagestream.go (2)
161-188:⚠️ Potential issue | 🟠 MajorStop deriving the builtin default from the major version alone.
getBuiltinDefaultStreamNamestill returnsrhel-9/rhel-10purely from the release major version, andinstallVersionshort-circuits that path before the payloadImageStreamis even consulted. That does not implement the PR objective of resolving the builtin default from the payload's default OS tags, so this can still annotate/pick the wrong builtin default whenever payload defaults diverge from the hard-coded version map.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/osimagestream/osimagestream.go` around lines 161 - 188, The function getBuiltinDefaultStreamName currently short-circuits on installVersion and then derives the builtin default solely from releaseVersion.Major(), which ignores the payload ImageStream's default OS tags; change the logic in getBuiltinDefaultStreamName to first consult the payload ImageStream (via imageStreamProvider / existing helper getVersionFromImageStream or a new helper that reads the payload ImageStream default OS tags) to resolve the builtin default stream name (map the payload default OS tag to StreamNameRHEL9/StreamNameRHEL10), only falling back to using installVersion or the parsed build releaseVersion->major mapping if the payload does not provide a default OS tag; update references to installVersion, imageStreamProvider, getVersionFromImageStream, and the StreamNameRHEL9/StreamNameRHEL10 constants accordingly.
141-158:⚠️ Potential issue | 🟠 MajorHonor the new fallback order instead of pinning
Status.DefaultStream.This still treats
Status.DefaultStreamas sticky whenSpec.DefaultStreamis empty, and it still errors when the requested spec default is no longer present. Both cases contradict the newspec -> single stream -> builtincontract and can wedge reconciliation after the available stream set changes.Suggested fix
func getDefaultStreamSet(streams []v1alpha1.OSImageStreamSet, builtinDefault string, existingOSImageStream *v1alpha1.OSImageStream) (string, error) { streamNames := GetStreamSetsNames(streams) requestedDefaultStream := GetOSImageStreamSpecDefault(existingOSImageStream) - if requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "" { - // If the spec is empty but the CR is already populated pick whatever existed - requestedDefaultStream = existingOSImageStream.Status.DefaultStream - } else if requestedDefaultStream == "" { - // No user preference: if there's only one stream just use it, - // otherwise fall back to the builtin default. - if len(streams) == 1 { - return streams[0].Name, nil - } - requestedDefaultStream = builtinDefault - } - - if slices.Contains(streamNames, requestedDefaultStream) { + if requestedDefaultStream != "" && slices.Contains(streamNames, requestedDefaultStream) { return requestedDefaultStream, nil } + + if len(streams) == 1 { + return streams[0].Name, nil + } + + if slices.Contains(streamNames, builtinDefault) { + return builtinDefault, nil + } return "", fmt.Errorf("could not find the requested %s default stream in the list of OSImageStreams %s", requestedDefaultStream, streamNames) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/osimagestream/osimagestream.go` around lines 141 - 158, The code currently treats existingOSImageStream.Status.DefaultStream as a sticky fallback and returns an error if the requested default (from GetOSImageStreamSpecDefault) is missing; change the logic in the default-selection block so the fallback order is strictly: spec -> single available stream -> builtinDefault. Concretely, remove the branch that assigns existingOSImageStream.Status.DefaultStream when spec is empty, always set requestedDefaultStream = GetOSImageStreamSpecDefault(existingOSImageStream) and if it's empty choose the single stream when len(streams)==1 else builtinDefault; additionally, if a non-empty requestedDefaultStream is not present in streamNames (slices.Contains), do not return an error—fall back to the single-stream or builtinDefault using the same single-stream-first rule and then return that value instead of erroring.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/controller/render/render_controller.go`:
- Around line 285-290: The current early-return checks only
reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) so spec-only
changes (like osimagestream.spec.defaultStream) are ignored; update the
condition to also compare the spec default stream (or the relevant spec field)
before returning. Specifically, change the check to return only when both
reflect.DeepEqual(oldOSImageStream.Status, curOSImageStream.Status) and
reflect.DeepEqual(oldOSImageStream.Spec.DefaultStream,
curOSImageStream.Spec.DefaultStream) (or an equivalent comparison of the
relevant Spec field) are true so a change to Spec.DefaultStream will not be
skipped; reference oldOSImageStream, curOSImageStream, .Status and
.Spec.DefaultStream in your fix.
In `@pkg/osimagestream/osimagestream.go`:
- Around line 161-188: The function getBuiltinDefaultStreamName currently
short-circuits on installVersion and then derives the builtin default solely
from releaseVersion.Major(), which ignores the payload ImageStream's default OS
tags; change the logic in getBuiltinDefaultStreamName to first consult the
payload ImageStream (via imageStreamProvider / existing helper
getVersionFromImageStream or a new helper that reads the payload ImageStream
default OS tags) to resolve the builtin default stream name (map the payload
default OS tag to StreamNameRHEL9/StreamNameRHEL10), only falling back to using
installVersion or the parsed build releaseVersion->major mapping if the payload
does not provide a default OS tag; update references to installVersion,
imageStreamProvider, getVersionFromImageStream, and the
StreamNameRHEL9/StreamNameRHEL10 constants accordingly.
- Around line 141-158: The code currently treats
existingOSImageStream.Status.DefaultStream as a sticky fallback and returns an
error if the requested default (from GetOSImageStreamSpecDefault) is missing;
change the logic in the default-selection block so the fallback order is
strictly: spec -> single available stream -> builtinDefault. Concretely, remove
the branch that assigns existingOSImageStream.Status.DefaultStream when spec is
empty, always set requestedDefaultStream =
GetOSImageStreamSpecDefault(existingOSImageStream) and if it's empty choose the
single stream when len(streams)==1 else builtinDefault; additionally, if a
non-empty requestedDefaultStream is not present in streamNames
(slices.Contains), do not return an error—fall back to the single-stream or
builtinDefault using the same single-stream-first rule and then return that
value instead of erroring.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 62a86a7f-acfa-4f99-8069-1f01c3156c7a
⛔ Files ignored due to path filters (64)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/api/.golangci.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/types_compatibilityrequirement.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apiextensions/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/apps/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/apps/v1/zz_prerelease_lifecycle_generated.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/types_ingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_ingresses.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_cluster_monitoring.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/types_insights.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_backups.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_clustermonitorings.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_criocredentialproviderconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.crd-manifests/0000_10_config-operator_01_insightsdatagathers.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/config/v1alpha1/zz_generated.deepcopy.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/config/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/features.mdis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/features.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/features/util.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/types_controlplanemachineset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machine/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/machineconfiguration/v1/types.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_kubeletconfigs.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-Default.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-Hypershift-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1/zz_generated.crd-manifests/0000_80_machine-config_01_machineconfignodes-OKD.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/types_osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-Hypershift.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-SelfManagedHA.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_internalreleaseimages-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.crd-manifests/0000_80_machine-config_01_osimagestreams.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/machineconfiguration/v1alpha1/zz_generated.swagger_doc_generated.gois excluded by!**/vendor/**,!vendor/**,!**/zz_generated*vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_00_cluster-version-operator_01_clusterversionoperators.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-CustomNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_10_etcd_01_etcdbackups.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-DevPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis-TechPreviewNoUpgrade.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/operator/v1alpha1/zz_generated.crd-manifests/0000_30_cluster-api_01_clusterapis.crd.yamlis excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/payload-command/render/render.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/api/payload-command/render/write_featureset.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/clustermonitoringspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/config/v1alpha1/prometheusoperatoradmissionwebhookconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/config/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestream.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/machineconfiguration/v1alpha1/osimagestreamspec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/machineconfiguration/applyconfigurations/utils.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (18)
go.modpkg/controller/bootstrap/bootstrap.gopkg/controller/common/constants.gopkg/controller/internalreleaseimage/internalreleaseimage_controller.gopkg/controller/render/render_controller.gopkg/operator/operator.gopkg/operator/osimagestream_ocp.gopkg/osimagestream/clusterversion.gopkg/osimagestream/clusterversion_test.gopkg/osimagestream/helpers.gopkg/osimagestream/helpers_test.gopkg/osimagestream/imagestream_provider.gopkg/osimagestream/imagestream_provider_test.gopkg/osimagestream/mocks_test.gopkg/osimagestream/osimagestream.gopkg/osimagestream/osimagestream_test.gotest/e2e-2of2/osimagestream_test.gotest/e2e-bootstrap/bootstrap_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e-2of2/osimagestream_test.go
pablintino
force-pushed
the
mco-2117
branch
2 times, most recently
from
d50a73b to
6e53d54
Compare
|
/test unit |
|
@pablintino: This pull request references MCO-2117 which is a valid jira issue. 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. |
|
/retest-required |
|
/retest |
pablintino
force-pushed
the
mco-2117
branch
2 times, most recently
from
e0fa169 to
3d4c2c1
Compare
|
/test unit |
|
/test e2e-hypershift |
|
/retest-required |
pablintino
force-pushed
the
mco-2117
branch
2 times, most recently
from
4385edb to
7f3db86
Compare
|
/retest-required |
|
/test e2e-aws-ovn |
cheesesashimi
left a comment
cheesesashimi
left a comment
There was a problem hiding this comment.
Overall, this looks good! I do have a couple of thoughts to consider.
| }) | ||
| existingOSImageStream := createOptions.ExistingOSImageStream | ||
| requestedDefaultStream := GetOSImageStreamSpecDefault(existingOSImageStream) | ||
| if requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "" { |
There was a problem hiding this comment.
suggestion (non-blocking): We can make this logic a bit easier to follow and parse by dropping the else if:
if requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "" {
// If the spec is empty but the CR is already populated pick whatever existed
requestedDefaultStream = existingOSImageStream.Status.DefaultStream
}
if requestedDefaultStream == "" {
// No user preference: if there's only one stream just use it,
// otherwise fall back to the builtin default.
if len(streams) == 1 {
return streams[0].Name, nil
}
builtinDefault, err := getBuiltinDefaultStreamName(createOptions.InstallVersion)
if err != nil {
return "", fmt.Errorf("could not determine the builtin default stream: %w", err)
}
requestedDefaultStream = builtinDefault
}There was a problem hiding this comment.
Good point, changed.
pkg/osimagestream/osimagestream.go
Outdated
| for _, stream := range streamNames { | ||
| if (strings.Contains(stream, "coreos-9") || strings.Contains(stream, "9-coreos") || strings.Contains(stream, "9")) && !strings.Contains(stream, "10") { | ||
| return stream, nil | ||
| // getBuiltinDefaultStreamName returns the default stream name based on the OCP version. |
There was a problem hiding this comment.
issue: This will break OKD. In this particular case, it may be worth maintaining an OCP and OKD version of this function, segregated with build tags. The OKD version could be as simple as:
func getBuiltinDefaultStreamName(_ *k8sversion.Version) (string, error) {
return streamNameCentos10, nil
}The other way that I'm aware of to determine whether a cluster is OKD at runtime is by looking at .spec.upstream on the ClusterVersion object since that's what our e2e tests use. And I'm not sure how reliable that method actually is.
There was a problem hiding this comment.
I must agree the implementation may not be obvious and/or robust. I thought as a solution for OKD the condition in https://github.com/openshift/machine-config-operator/pull/5714/changes#diff-1353921332a3d771864ad7388ab828a62e717ed7dc8f02ee13b5be6361f9c769R144-R146.
As OKD payloads may have only a single stream that piece of code should avoid hitting this function and returning the wrong stream.
I'm going to take your comment and improve this, cause it's not obvious and not reliable.
pkg/osimagestream/osimagestream.go
Outdated
| // StreamNameRHEL9 is the stream name for RHEL 9 based CoreOS images. | ||
| StreamNameRHEL9 = "rhel-9" | ||
| // StreamNameRHEL10 is the stream name for RHEL 10 based CoreOS images. | ||
| StreamNameRHEL10 = "rhel-10" |
There was a problem hiding this comment.
issue: We also need streamNameCentos10 = "centos-10" to maintain compatibility with OKD.
Perhaps we could also colocate this constant that with the OKD version of the getBuiltinDefaultStreamName() described below while also colocating the OCP constants with the OCP version of that function?
pkg/osimagestream/osimagestream.go
Outdated
|
|
||
| const ( | ||
| // StreamNameRHEL9 is the stream name for RHEL 9 based CoreOS images. | ||
| StreamNameRHEL9 = "rhel-9" |
There was a problem hiding this comment.
suggestion (non-blocking): I know that we are trying to avoid hard-coding these values because we'll have to maintain them. For now, maybe we could avoid exporting them outside of the osimagestream package by making them streamNameRHEL9 and streamNameRHEL10? That way, there's less of a chance that someone would do conditional logic on their values later.
There was a problem hiding this comment.
Yeah, same thought here, but unfortunately, I think we may need to have explicit stream names :(, at least for the streams we will manage (and we will EoL at some point).
Here we have another example of why may need to have references to streams (despite I do not like the idea at all neither) https://github.com/openshift/machine-config-operator/pull/5752/changes#diff-4455c81495e83defc0ba998694bf0c729383bae72d218242f3d84671fb19229aR126
pablintino
left a comment
pablintino
left a comment
There was a problem hiding this comment.
@cheesesashimi I've pushed a change to try to address your comments.
For the OKD vs OCP ones I've leaned towards a runtime check of version.IsSCOS() cause that approach gives me a chance to test the function, something I couldn't do using plain build tags in dedicated files, as we do not have OKD tests iiuc.
| }) | ||
| existingOSImageStream := createOptions.ExistingOSImageStream | ||
| requestedDefaultStream := GetOSImageStreamSpecDefault(existingOSImageStream) | ||
| if requestedDefaultStream == "" && existingOSImageStream != nil && existingOSImageStream.Status.DefaultStream != "" { |
There was a problem hiding this comment.
Good point, changed.
pkg/osimagestream/osimagestream.go
Outdated
| for _, stream := range streamNames { | ||
| if (strings.Contains(stream, "coreos-9") || strings.Contains(stream, "9-coreos") || strings.Contains(stream, "9")) && !strings.Contains(stream, "10") { | ||
| return stream, nil | ||
| // getBuiltinDefaultStreamName returns the default stream name based on the OCP version. |
There was a problem hiding this comment.
I must agree the implementation may not be obvious and/or robust. I thought as a solution for OKD the condition in https://github.com/openshift/machine-config-operator/pull/5714/changes#diff-1353921332a3d771864ad7388ab828a62e717ed7dc8f02ee13b5be6361f9c769R144-R146.
As OKD payloads may have only a single stream that piece of code should avoid hitting this function and returning the wrong stream.
I'm going to take your comment and improve this, cause it's not obvious and not reliable.
pkg/osimagestream/osimagestream.go
Outdated
| // StreamNameRHEL9 is the stream name for RHEL 9 based CoreOS images. | ||
| StreamNameRHEL9 = "rhel-9" | ||
| // StreamNameRHEL10 is the stream name for RHEL 10 based CoreOS images. | ||
| StreamNameRHEL10 = "rhel-10" |
pkg/osimagestream/osimagestream.go
Outdated
|
|
||
| const ( | ||
| // StreamNameRHEL9 is the stream name for RHEL 9 based CoreOS images. | ||
| StreamNameRHEL9 = "rhel-9" |
There was a problem hiding this comment.
Yeah, same thought here, but unfortunately, I think we may need to have explicit stream names :(, at least for the streams we will manage (and we will EoL at some point).
Here we have another example of why may need to have references to streams (despite I do not like the idea at all neither) https://github.com/openshift/machine-config-operator/pull/5752/changes#diff-4455c81495e83defc0ba998694bf0c729383bae72d218242f3d84671fb19229aR126
Previously the default stream was determined by matching the OS version in the stream name, without the possibility of telling the MCO which default stream to use. This change replaces that logic, allowing users to input the defaultStream they want to use and falling back to the builtin if necessary. The builtin default stream determination logic has been improved to avoid hard-coding versions and it now picks the OSImageStream that points to the images of the default OS tags in the payload ImageStream. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
|
@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. |
|
@pablintino I agree that the
|
5 participants
- What I did
Previously the default stream was determined by matching the OS version in the stream name, without the possibility of telling the MCO which default stream to use. This change replaces that logic, allowing users to input the defaultStream they want to use and falling back to the builtin if necessary. The builtin default stream determination logic has been improved to avoid hard-coding versions and it now picks the OSImageStream that points to the images of the default OS tags in the payload ImageStream.
- How to verify it
There are two main scenarios to test.
Install a cluster with a different default set at day-zero
Change the global default of a cluster running default MCPs
- Description for the changelog
Allow users to override the default OSImageStream via Spec.DefaultStream, falling back to the builtin default resolved from the payload ImageStream tags.
Summary by CodeRabbit
New Features
Performance
Chores
Tests