-
Notifications
You must be signed in to change notification settings - Fork 315
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HOSTEDCP-632: hypershift-operator/controllers/hostedcluster: isUpgradeable minor-bump scoping #2336
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
285c632
to
fd519c4
Compare
…mp scoping Godocs for Upgradeable [1]: Upgradeable indicates whether the component (operator and all configured operands) is safe to upgrade based on the current cluster state. When Upgradeable is False, the cluster-version operator will prevent the cluster from performing impacted updates unless forced. When set on ClusterVersion, the message will explain which updates (minor or patch) are impacted. When set on ClusterOperator, False will block minor OpenShift updates. The message field should contain a human readable description of what the administrator should do to allow the cluster or component to successfully update. The cluster-version operator will allow updates when this condition is not False, including when it is missing, True, or Unknown. So we specifically doc it as only about 4.y -> 4.(y+1) minor updates when seen on ClusterOperator. But we leave it unclear on ClusterVersion because when you set some ClusterVersion overrides, it can break patch updates, so QE asked us to also block patch updates in that case [2,3]. With this patch, I'm using availableUpdates and conditionalUpdates to look up a version associated with the proposed target release pullspec. That's a bit less reliable than the current cluster-version operator behavior, which is extracting the proposed target version from the proposed release image itself (e.g. see [4]). But it's probably sufficient for now, with the odds that the OpenShift Update Service serves bad data low. And we can refine further in the future if we want. [1]: https://github.com/openshift/api/blob/cce310ad2932f6de24491052d506926e484c082c/config/v1/types_cluster_operator.go#L179-L190 : [2]: openshift/cluster-version-operator#364 [3]: https://bugzilla.redhat.com/show_bug.cgi?id=1822844 [4]: openshift/cluster-version-operator#431
fd519c4
to
0c04d51
Compare
@wking: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @wking !
One comment. It also looks like you need to fix the unit test.
@@ -4214,10 +4214,40 @@ func (r *HostedClusterReconciler) reconcileAWSSubnets(ctx context.Context, creat | |||
return nil | |||
} | |||
|
|||
func releaseImageToVersion(hcluster *hyperv1.HostedCluster, image string) (semver.Version, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference would be to just look up the version from the release image. For example, see:
hypershift/hypershift-operator/controllers/hostedcluster/hostedcluster_controller.go
Lines 1398 to 1405 in 04a7434
releaseInfo, err := r.ReleaseProvider.Lookup(ctx, hcluster.Spec.Release.Image, pullSecretBytes) | |
if err != nil { | |
return ctrl.Result{}, fmt.Errorf("failed to lookup release image: %w", err) | |
} | |
releaseImageVersion, err = semver.Parse(releaseInfo.Version()) | |
if err != nil { | |
return ctrl.Result{}, fmt.Errorf("failed to parse release image version: %w", err) | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
releaseInfo, err := r.ReleaseProvider.Lookup(ctx, hcluster.Spec.Release.Image, pullSecretBytes)
Tunneling pullSecretBytes
around down from HostedClusterReconciler.reconcile
to isProgressing
to isUpgradeable
feels awkward. Thoughts about adjusting the ProviderWithRegistryOverrides
interface to take WithPullSecret
to push that information down into the structures once we have it? Or alternatively, adjusting the release providers that need the pull secret (just PodProvider
and RegistryClientProvider
?) to take a callback when initializing them here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the thing is that the provider is a controller-level variable while the pull secret we use is hostedcluster-specific. We do want to use the pull secret that is provided with the HostedCluster if at some point you are trying to use a release that only your pull secret has access to.
@wking: This pull request references HOSTEDCP-632 which is a valid jira issue. In 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 kubernetes/test-infra repository. |
@wking thanks! This has been on our list for a while. Unit test need fixing up. |
} | ||
|
||
if requestedVersion.Major == currentTargetVersion.Major && requestedVersion.Minor == currentTargetVersion.Minor { | ||
// ClusterVersion's Upgradeable condition is mostly about minor bumps from x.y to x.(y+1) and larger. It is not intended to block patch updates from x.y.z to x.y.z' except under very limited circumstances which we can ignore for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wking Not sure why did you mention mostly
in ClusterVersion's Upgradeable condition is mostly about minor bumps from x.y to x.(y+1) and larger
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think CVO override is applicable to managed OCP clusters. So we just mention that the upgradeable condition is only about minors.
PR needs rebase. 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 kubernetes/test-infra repository. |
superseded by #2381 |
@sjenning: Closed this PR. In 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 kubernetes/test-infra repository. |
Godocs for
Upgradeable
:So we specifically doc it as only about 4.y -> 4.(y+1) minor updates when seen on ClusterOperator. But we leave it unclear on ClusterVersion because when you set some ClusterVersion overrides, it can break patch updates, so QE asked us to also block patch updates in that case.
With this patch, I'm using
availableUpdates
andconditionalUpdates
to look up a version associated with the proposed target release pullspec. That's a bit less reliable than the current cluster-version operator behavior, which is extracting the proposed target version from the proposed release image itself (e.g. see here). But it's probably sufficient for now, with the odds that the OpenShift Update Service serves bad data low. And we can refine further in the future if we want.