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.
| 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 😅
| 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
| } | ||
| 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. |
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.com>
|
@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: 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. |
| } | ||
|
|
||
| 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 |
3 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.