[OTA-1545] Extend ClusterVersion for accepted risks #2360

hongkailiu · 2025-06-09T21:07:23Z

The API extensions is proposed in openshift/enhancements#1807

Add a new field 'clusterversion.spec.desiredUpdate.accept': It contains
the names of conditional update risks that are considered acceptable.
Move clusterversion.status.conditionalUpdates.risks two levels up as
clusterversion.status.conditionalUpdateRisks. It contains all the risks
for clusterversion.status.conditionalUpdates.
Add new field 'clusterversion.status.conditionalUpdates.riskNames': It
contains the names of risk for the conditional update. It deprecates
clusterversion.status.conditionalUpdates.risks.
Add a new field 'clusterversion.status.conditionalUpdateRisks.conditions':
It contains the observations of the conditional update risk's current
status.

openshift-ci · 2025-06-09T21:07:27Z

Hello @hongkailiu! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

openshift-ci · 2025-06-09T21:08:23Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hongkailiu
Once this PR has been reviewed and has the lgtm label, please assign joelspeed for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

hongkailiu · 2025-07-03T02:33:11Z

/retest-required

hongkailiu · 2025-07-03T13:44:07Z

/retest-required

petr-muller

I am aware I am late to the party, so feel free to dismiss everything and treat it as an unwanted drive-by. Mostly just voicing my thoughts as I read the structs, but I admit have not yet read the enhancement.

config/v1/types_cluster_version.go

petr-muller · 2025-07-03T16:59:29Z

config/v1/types_cluster_version.go

@@ -780,11 +806,23 @@ type ConditionalUpdate struct {
 	// +required
 	Release Release `json:"release"`

+	// riskNames represents the set of the names of conditionalUpdateRisks in the status that are exposed to the release in this conditional update.
+	// The cluster-version operator will evaluate these risks and only accept the update if there is at least one risk and for every risk it is either not applied to the cluster or considered acceptable by the cluster administrator.


nit

Suggested change

// The cluster-version operator will evaluate these risks and only accept the update if there is at least one risk and for every risk it is either not applied to the cluster or considered acceptable by the cluster administrator.

// The cluster version operator will evaluate these risks and only accept the update if there is at least one risk and for every risk it is either not applied to the cluster or considered acceptable by the cluster administrator.

But maybe the API documentation should not make assumptions about how the controller that manages it is named, and we should simply talk about the expected behavior given an API shape? "An update will only be 'accepted' (whatever that means?) if..."

Also the "if there is at least one risk" is confusing to me. Why would having zero risks be unacceptable?

Why would having zero risks be unacceptable?

This is a very valid question.
Is it possible that a ConditionalUpdate has no risk at all? I think No. It should be aligned with the next field Risks:

api/config/v1/types_cluster_version.go

Lines 821 to 833 in 9b52e32

// risks represents the range of issues associated with

// updating to the target release. The cluster-version

// operator will evaluate all entries, and only recommend the

// update if there is at least one entry and all entries

// recommend the update.

// Deprecated: the risks has been deprecated by riskNames.

// +kubebuilder:validation:MinItems=1

// +patchMergeKey=name

// +patchStrategy=merge

// +listType=map

// +listMapKey=name

// +required

Risks []ConditionalUpdateRisk `json:"risks" patchStrategy:"merge" patchMergeKey:"name"`

Made the following change as well:

+optional
--->
// +kubebuilder:validation:MinItems=1 and // +required

You cannot add a new required field. That is a breaking change to users/clients and invalidates all existing instances/expectations of the API. This field must be optional. You can still add the minimum items requirement but it should always be possible for this field to be unspecified.

Reverted back to +optional.

You can still add the minimum items requirement

+kubebuilder:validation:MinItems=1 is still left there.

Is this understanding correct?
+optional and +kubebuilder:validation:MinItems=1 mean:

riskNames is not specified in the object. Valid up to +optional.

riskNames is the empty set. Invalid up to +kubebuilder:validation:MinItems=1.

riskNames is not empty. Valid as the expected case for the API extension.

Also added the omitempty label because otherwise:

config/v1/types_cluster_version.go:820:2: optionalfields: field RiskNames is optional and does not allow the zero value. It must have the omitempty tag. (kubeapilinter)

Added 2 tests for the first 2 cases above.

See #2360 (comment)

config/v1/types_cluster_version.go

hongkailiu · 2025-07-03T20:03:28Z

Thanks for the feedback. @petr-muller

I will address your comments today or tomorrow.

config/v1/types_cluster_version.go

config/v1/tests/clusterversions.config.openshift.io/ClusterUpdateAcceptedRisks.yaml

- Add a new field 'clusterversion.spec.desiredUpdate.accept': It contains the names of conditional update risks that are considered acceptable. - Move `clusterversion.status.conditionalUpdates.risks` two levels up as `clusterversion.status.conditionalUpdateRisks`. It contains all the risks for `clusterversion.status.conditionalUpdates`. - Add new field 'clusterversion.status.conditionalUpdates.riskNames': It contains the names of risk for the conditional update. It deprecates `clusterversion.status.conditionalUpdates.risks`. - Add a new field 'clusterversion.status.conditionalUpdateRisks.conditions': It contains the observations of the conditional update risk's current status.

hongkailiu · 2025-07-07T19:13:09Z

At the moment, there are 9 cases added for the extended API.

git --no-pager log --pretty=oneline -1 
6dd18c02beebc16a21d0da35be372665df491371 (HEAD -> OTA-1545, hongkailiu/OTA-1545) make update

$ diff -Naupr config/v1/tests/clusterversions.config.openshift.io/AAA_ungated.yaml config/v1/tests/clusterversions.config.openshift.io/ClusterUpdateAcceptedRisks.yaml | rg '\+    - name:|\+      expected.*Error:'
+    - name: Should be able to set accepted risks
+    - name: A risk name greater than 256 characters is not allowed
+      expectedError: "Too long: may not be more than 256 bytes"
+    - name: Risk names from the accept field must be unique
+      expectedError: 'Duplicate value: "RiskA"'
+    - name: The riskNames field might be unspecified
+    - name: Should be able to update fields related to accepted risks
+    - name: More than a single condition on an update risk is not allowed
+      expectedStatusError: "conditions: Too many: 2: must have at most 1 item"
+    - name: Risk names of a conditional update must be unique
+      expectedStatusError: 'Duplicate value: "DualStackNeedsController"'
+    - name: The type of a risk condition must be Applies
+      expectedStatusError: "type must be 'Applies'"
+    - name: The value of riskNames cannot be the empty set
+      expectedStatusError: "should have at least 1 items"

openshift-ci · 2025-07-07T22:52:48Z

@hongkailiu: all tests passed!

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-sigs/prow repository. I understand the commands that are listed here.

everettraven

Noticed a small thing related to some of the description changes related to the feature showing up for the default CRDs that will get applied even when the feature isn't enabled.

This is probably OK, but it is a bit funky. Easiest workaround is wait until GA to add these description changes to the existing fields - especially the deprecation one.

Aside from that, I think this looks good and is ready for @JoelSpeed to take a pass. Joel, what are your thoughts on the description changes for pre-existing fields here?

everettraven · 2025-07-08T19:13:03Z

config/v1/types_cluster_version.go

 	// risks represents the range of issues associated with
 	// updating to the target release. The cluster-version
 	// operator will evaluate all entries, and only recommend the
 	// update if there is at least one entry and all entries
 	// recommend the update.
+	// Deprecated: the risks has been deprecated by riskNames.


Something worth noting - this shows up on the non-featuregated version of the API.

I'm not sure there is much of a workaround for this unless you wait to add this deprecation text until you actually go to promote the feature-gate that deprecates this to GA.

I would probably recommend removing this for now.

everettraven · 2025-07-08T19:15:52Z

config/v1/types_cluster_version.go

+	// update targets, or in the conditionUpdates set and all associated risks
+	// are specified in desiredUpdate.accept.


Similarly, this change also appears in the default CRD. You may want to include that this part of the description only applies when the particular feature gate is enabled or drop it from the description until you GA the feature.

JoelSpeed · 2025-07-10T13:02:56Z

Joel, what are your thoughts on the description changes for pre-existing fields here?

Not yet looked at the specific case, but in theory, we could have a marker that applies additional description only when a certain feature gate is enabled 🤔 I'm not sure how that would work if you had multiple gates though, you'd have to do what we do with the enum and specify the combinations. I don't this is likely a big enough issue for us to worry about right now

JoelSpeed

Just trying to understand the changes here, can someone confirm if this is correct?

At the moment, each update risk is listed within the conditional update that it relates to. With the change here, we move all of the risks together to a higher level, and instead, add a list of riskNames to the conditional update.

Centralising all of the risk information but meaning that when looking at a conditional update, I now have to go and find the information somewhere else?

It doesn't look like we are actually adding any new information here apart from adding a spec list of accepted risk names?

JoelSpeed · 2025-07-11T13:28:59Z

config/v1/types_cluster_version.go

+	// conditionalUpdateRisks contains the list of risks associated with conditionalUpdates.
+	// When performing a conditional update, all its associated risks will be compared with the set of accepted risks in the spec.desiredUpdate.accept field.
+	// If all risks for a conditional update are included in the spec.desiredUpdate.accept set, the conditional update will proceed, otherwise it is blocked.
+	// The risk's names in the list must be unique.


Double plural reads oddly to me, WDYT?

Suggested change

// The risk's names in the list must be unique.

// The risk names in the list must be unique.

JoelSpeed · 2025-07-11T13:29:20Z

config/v1/types_cluster_version.go

+	// +patchMergeKey=name
+	// +patchStrategy=merge


Not valid for CRDs, please remove

JoelSpeed · 2025-07-11T13:29:30Z

config/v1/types_cluster_version.go

+	// +listType=map
+	// +listMapKey=name
+	// +optional
+	ConditionalUpdateRisks []ConditionalUpdateRisk `json:"conditionalUpdateRisks,omitempty" patchStrategy:"merge" patchMergeKey:"name"`


Suggested change

ConditionalUpdateRisks []ConditionalUpdateRisk `json:"conditionalUpdateRisks,omitempty" patchStrategy:"merge" patchMergeKey:"name"`

ConditionalUpdateRisks []ConditionalUpdateRisk `json:"conditionalUpdateRisks,omitempty"`

JoelSpeed · 2025-07-11T13:31:34Z

config/v1/types_cluster_version.go

+	// +kubebuilder:validation:MaxItems=1000
+	// +listType=set
+	// +optional
+	Accept []string `json:"accept"`


Out of interest, why not acceptedRisks to match with the history version of this field?

JoelSpeed · 2025-07-11T13:33:29Z

config/v1/types_cluster_version.go

 	// risks represents the range of issues associated with
 	// updating to the target release. The cluster-version
 	// operator will evaluate all entries, and only recommend the
 	// update if there is at least one entry and all entries
 	// recommend the update.
+	// Deprecated: the risks has been deprecated by riskNames.


Suggested change

// Deprecated: the risks has been deprecated by riskNames.

// Deprecated: the risks field has been deprecated in favour of riskNames.

JoelSpeed · 2025-07-11T13:34:42Z

config/v1/types_cluster_version.go

+	// riskNames represents the set of the names of conditionalUpdateRisks in the status that are exposed to the release in this conditional update.
+	// A condition update is accepted only if each of its risk is either not applied to the cluster or considered acceptable by the cluster administrator.
+	// Entries must be unique and must not exceed 256 characters.
+	// riskNames must not contain more than 500 entries.
+	// +openshift:enable:FeatureGate=ClusterUpdateAcceptedRisks
+	// +kubebuilder:validation:MinItems=1
+	// +kubebuilder:validation:items:MaxLength=256
+	// +kubebuilder:validation:MaxItems=500
+	// +listType=set
+	// +optional
+	RiskNames []string `json:"riskNames,omitempty"`


So this is an odd direction for a change. Why are we moving from a list of structured objects to a list of strings? Normally, I see this the other way around?

JoelSpeed · 2025-07-11T13:34:58Z

config/v1/types_cluster_version.go

+	// riskNames represents the set of the names of conditionalUpdateRisks in the status that are exposed to the release in this conditional update.
+	// A condition update is accepted only if each of its risk is either not applied to the cluster or considered acceptable by the cluster administrator.
+	// Entries must be unique and must not exceed 256 characters.
+	// riskNames must not contain more than 500 entries.
+	// +openshift:enable:FeatureGate=ClusterUpdateAcceptedRisks
+	// +kubebuilder:validation:MinItems=1
+	// +kubebuilder:validation:items:MaxLength=256
+	// +kubebuilder:validation:MaxItems=500
+	// +listType=set
+	// +optional
+	RiskNames []string `json:"riskNames,omitempty"`


What does it mean to set both risks and riskNames?

JoelSpeed · 2025-07-11T13:35:53Z

config/v1/types_cluster_version.go

+	// conditions must not contain more than one entry.
+	// +openshift:enable:FeatureGate=ClusterUpdateAcceptedRisks
+	// +kubebuilder:validation:items:XValidation:rule="has(self.type) && self.type == 'Applies'",message="type must be 'Applies'"
+	// +kubebuilder:validation:MaxItems=1


MinItems too please

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 9, 2025

hongkailiu changed the title ~~[OTA-1545] Extend ClusterVersion for accepted risks~~ [wip][OTA-1545] Extend ClusterVersion for accepted risks Jun 9, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 9, 2025

openshift-ci bot requested review from deads2k and everettraven June 9, 2025 21:08

hongkailiu force-pushed the OTA-1545 branch from 0eed585 to 611d63d Compare June 10, 2025 14:30

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2025

hongkailiu force-pushed the OTA-1545 branch from 1398b1f to 92b713d Compare June 10, 2025 15:09

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 10, 2025

hongkailiu force-pushed the OTA-1545 branch from 92b713d to a3e4639 Compare June 10, 2025 15:12

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 10, 2025

hongkailiu force-pushed the OTA-1545 branch 2 times, most recently from 6dc1e06 to e780c3a Compare June 10, 2025 16:12

openshift-ci bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 10, 2025

hongkailiu force-pushed the OTA-1545 branch 4 times, most recently from 5218865 to 4b05550 Compare June 10, 2025 22:49

openshift-ci bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jun 10, 2025

hongkailiu mentioned this pull request Jun 10, 2025

[wip][OTA-1544]update: support accepted risks openshift/enhancements#1807

Open

hongkailiu force-pushed the OTA-1545 branch 3 times, most recently from 854ed1f to f03a0de Compare June 11, 2025 03:39

hongkailiu force-pushed the OTA-1545 branch 5 times, most recently from dcf9d68 to c239767 Compare July 3, 2025 01:01

petr-muller reviewed Jul 3, 2025

View reviewed changes

hongkailiu force-pushed the OTA-1545 branch 2 times, most recently from e77f91e to 3307276 Compare July 4, 2025 14:44

everettraven reviewed Jul 7, 2025

View reviewed changes

config/v1/types_cluster_version.go Outdated Show resolved Hide resolved

everettraven reviewed Jul 7, 2025

View reviewed changes

config/v1/tests/clusterversions.config.openshift.io/ClusterUpdateAcceptedRisks.yaml Outdated Show resolved Hide resolved

hongkailiu force-pushed the OTA-1545 branch 7 times, most recently from e1bda97 to 8991d06 Compare July 7, 2025 19:01

hongkailiu added 2 commits July 7, 2025 15:06

make update

6dd18c0

hongkailiu force-pushed the OTA-1545 branch from 8991d06 to 6dd18c0 Compare July 7, 2025 19:09

hongkailiu requested review from petr-muller and everettraven July 8, 2025 12:42

everettraven reviewed Jul 8, 2025

View reviewed changes

JoelSpeed reviewed Jul 11, 2025

View reviewed changes

	// The cluster-version operator will evaluate these risks and only accept the update if there is at least one risk and for every risk it is either not applied to the cluster or considered acceptable by the cluster administrator.
	// The cluster version operator will evaluate these risks and only accept the update if there is at least one risk and for every risk it is either not applied to the cluster or considered acceptable by the cluster administrator.

	// risks represents the range of issues associated with
	// updating to the target release. The cluster-version
	// operator will evaluate all entries, and only recommend the
	// update if there is at least one entry and all entries
	// recommend the update.
	// Deprecated: the risks has been deprecated by riskNames.
	// +kubebuilder:validation:MinItems=1
	// +patchMergeKey=name
	// +patchStrategy=merge
	// +listType=map
	// +listMapKey=name
	// +required
	Risks []ConditionalUpdateRisk `json:"risks" patchStrategy:"merge" patchMergeKey:"name"`

		// update targets, or in the conditionUpdates set and all associated risks
		// are specified in desiredUpdate.accept.

	// The risk's names in the list must be unique.
	// The risk names in the list must be unique.

	ConditionalUpdateRisks []ConditionalUpdateRisk `json:"conditionalUpdateRisks,omitempty" patchStrategy:"merge" patchMergeKey:"name"`
	ConditionalUpdateRisks []ConditionalUpdateRisk `json:"conditionalUpdateRisks,omitempty"`

	// Deprecated: the risks has been deprecated by riskNames.
	// Deprecated: the risks field has been deprecated in favour of riskNames.

[OTA-1545] Extend ClusterVersion for accepted risks #2360

Are you sure you want to change the base?

[OTA-1545] Extend ClusterVersion for accepted risks #2360

Uh oh!

Conversation

hongkailiu commented Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jun 9, 2025

Uh oh!

openshift-ci bot commented Jun 9, 2025

Uh oh!

hongkailiu commented Jul 3, 2025

Uh oh!

hongkailiu commented Jul 3, 2025

Uh oh!

petr-muller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everettraven Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hongkailiu Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hongkailiu commented Jul 3, 2025

Uh oh!

Uh oh!

Uh oh!

hongkailiu commented Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Jul 7, 2025

Uh oh!

everettraven left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Jul 10, 2025

Uh oh!

JoelSpeed left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hongkailiu commented Jun 9, 2025 •

edited

Loading

everettraven Jul 7, 2025 •

edited

Loading

hongkailiu Jul 7, 2025 •

edited

Loading

hongkailiu commented Jul 7, 2025 •

edited

Loading