Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2dc574
[OTA-1545] Extend ClusterVersion for accepted risks
hongkailiu Jun 9, 2025
2c8523b
Remove the docs changes on the existing fields
hongkailiu Jul 17, 2025
0d84d57
make update
hongkailiu Jul 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
506 changes: 506 additions & 0 deletions config/v1/tests/clusterversions.config.openshift.io/ClusterUpdateAcceptRisks.yaml
View file
Open in desktop

Large diffs are not rendered by default.

69 changes: 67 additions & 2 deletions config/v1/types_cluster_version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,19 @@ type ClusterVersionStatus struct {
// +listType=atomic
// +optional
ConditionalUpdates []ConditionalUpdate `json:"conditionalUpdates,omitempty"`

// 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.acceptRisks field.
// If all risks for a conditional update are included in the spec.desiredUpdate.acceptRisks set, the conditional update can proceed, otherwise it is blocked.
// The risk names in the list must be unique.
// conditionalUpdateRisks must not contain more than 500 entries.
// +openshift:enable:FeatureGate=ClusterUpdateAcceptRisks
// +kubebuilder:validation:MaxItems=500
// +kubebuilder:validation:MinItems=1
// +listType=map
// +listMapKey=name
// +optional
ConditionalUpdateRisks []ConditionalUpdateRisk `json:"conditionalUpdateRisks,omitempty"`
}

// UpdateState is a constant representing whether an update was successfully
Expand Down Expand Up @@ -255,8 +268,8 @@ type UpdateHistory struct {
Verified bool `json:"verified"`

// acceptedRisks records risks which were accepted to initiate the update.
// For example, it may menition an Upgradeable=False or missing signature
// that was overriden via desiredUpdate.force, or an update that was
// For example, it may mention an Upgradeable=False or missing signature
// that was overridden via desiredUpdate.force, or an update that was
// initiated despite not being in the availableUpdates set of recommended
// update targets.
// +optional
Expand Down Expand Up @@ -725,6 +738,30 @@ type Update struct {
//
// +optional
Force bool `json:"force"`

// acceptRisks is an optional set of names of conditional update risks that are considered acceptable.
// A conditional update is performed only if all of its risks are acceptable.
// This list may contain entries that apply to current, previous or future updates.
// The entries therefore may not map directly to a risk in .status.conditionalUpdateRisks.
// acceptRisks must not contain more than 1000 entries.
// Only one accept risk is allowed per risk name.
// +openshift:enable:FeatureGate=ClusterUpdateAcceptRisks
// +kubebuilder:validation:MaxItems=1000
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can this list be longer than the status.conditionalUpdateRisks? Does that make sense?

Please also add MinItems=1

Copy link
Member

@wking wking Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can this list be longer than the status.conditionalUpdateRisks? Does that make sense?

See this and this where I'm trying to explain why I think allowing additional acceptRisks entries makes life easier for cluster/fleet admins without much downside for in-cluster components. So yes, I think that does make sense. And as I said there, I'm fine with the 1k here, although there's nothing magical about 1k itself if you wanted to nudge it a bit in either direction.

Please also add MinItems=1

Is that "if you set it, you cannot set it to an empty list?". Do we care? It seems pretty easy to have the CVO treat "property unset" and "property set to an empty list" identically (and actually, likely hard for the CVO to distinguish between those two cases) , and I don't see much benefit in making empty-list illegal. Although just to save some network traffic, I guess I could stomach a MinItems=1 if adding that meant that API-approvers would allow us to set omitempty on this property 😉

Copy link
Member Author

@hongkailiu hongkailiu Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also add MinItems=1

Done.

Update:
Reverted back.

Otherwise, the lint failed with

config/v1/types_cluster_version.go:753:2: optionalfields: field AcceptRisks is optional and does not allow the zero value. It must have the omitempty tag. (kubeapilinter)
	AcceptRisks []string `json:"acceptRisks"`
	^
1 issues:
* kubeapilinter: 1

And that conflict to the previous choice here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, I'm in the process of mulling over making omitempty required on all fields, and backtracking on our previous decision about discoverability, as you've seen, it's causing lots of bugs and confusion.

You are correct, that if we don't have omitempty, then MinLength=1 conflicts (and woo linter picking that up)

The reason we care is that if you don't have MinLength=1 and do have omitempty, then structured clients cannot set [] as a value, but unstructured clients can. And that means a structured client cannot reliably round-trip a value that an unstructured client has set.

If you prefer to stay with the choice for discoverability/consistency, I'm ok with that, but I'm also ok with omitempty+MinLength, your call on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason we care is that if you don't have MinLength=1 and do have omitempty, then structured clients cannot set [] as a value, but unstructured clients can. And that means a structured client cannot reliably round-trip a value that an unstructured client has set.

Can unstructured clients actually set [] as a value? I'd expect that in that situation, the Kube API server would accept a request to set [], but because of the omitempty, it would get serialized for etcd storage with the property unset. And "the client is surprised by the lack of property in Kube API server responses" is a maybe, while "the client is surprised by the Kube API server rejecting [] due to MinLength=1" seems like it would be an always. And in the no-min-length, no-omitempty world, there's room for clients to wonder whether there's a distinction between "unset" and [], when in this case there is no semantic distinction between those two values.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can unstructured clients actually set [] as a value? I'd expect that in that situation, the Kube API server would accept a request to set [], but because of the omitempty, it would get serialized for etcd storage with the property unset.

For custom resources, the API server has no idea what the Go types look like and treats the data as unstructured all the way through into etcd, so, yes, it's absolutely possible for an unstructured client, writing a CRD to write foo: [] even if foo has omitempty in the Go type.

And "the client is surprised by the lack of property in Kube API server responses" is a maybe, while "the client is surprised by the Kube API server rejecting [] due to MinLength=1" seems like it would be an always

The issue presents as, unstructured client writes value, value gets accepted and persisted, then a structured client writes to the object and it gets removed (resource version + generation creeping up), unstructured client (lets call it a GitOps apply) notices the difference between there manifest, and writes the object again to put foo: [] back, request gets accepted... you can see where this is going

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After like 5 times of reading, I finally understood the options we have.
Up to my very limited memory space, I will keep this one unresolved as a reference for myself in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the patch 0e60cc1

the choice goes to MinItems=1 + omitempty.
(discoverability/consistency is dropped)

// +kubebuilder:validation:MinItems=1
// +listType=map
// +listMapKey=name
// +optional
AcceptRisks []AcceptRisk `json:"acceptRisks,omitempty"`
}

// AcceptRisk represents a risk that is considered acceptable.
type AcceptRisk struct {
// name is the name of the acceptable risk.
// It must be a non-empty string and must not exceed 256 characters.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=256
// +required
Name string `json:"name,omitempty"`
}

// Release represents an OpenShift release image and associated metadata.
Expand Down Expand Up @@ -780,6 +817,20 @@ type ConditionalUpdate struct {
// +required
Release Release `json:"release"`

// riskNames represents the set of the names of conditionalUpdateRisks that are relevant to this update for some clusters.
// The Applies condition of each conditionalUpdateRisks entry declares if that risk applies to this cluster.
// A conditional update is accepted only if each of its risks either does not apply to the cluster or is considered acceptable by the cluster administrator.
// The latter means that the risk names are included in value of the spec.desiredUpdate.acceptRisks field.
// Entries must be unique and must not exceed 256 characters.
// riskNames must not contain more than 500 entries.
// +openshift:enable:FeatureGate=ClusterUpdateAcceptRisks
// +kubebuilder:validation:MinItems=1
// +kubebuilder:validation:items:MaxLength=256
// +kubebuilder:validation:MaxItems=500
// +listType=set
// +optional
RiskNames []string `json:"riskNames,omitempty"`

// 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
Expand All @@ -806,6 +857,20 @@ type ConditionalUpdate struct {
// for not recommending a conditional update.
// +k8s:deepcopy-gen=true
type ConditionalUpdateRisk struct {
// conditions represents the observations of the conditional update
// risk's current status. Known types are:
// * Applies, for whether the risk applies to the current cluster.
// The condition's types in the list must be unique.
// conditions must not contain more than one entry.
// +openshift:enable:FeatureGate=ClusterUpdateAcceptRisks
// +kubebuilder:validation:items:XValidation:rule="has(self.type) && self.type == 'Applies'",message="type must be 'Applies'"
// +kubebuilder:validation:MaxItems=1
// +kubebuilder:validation:MinItems=1
// +listType=map
// +listMapKey=type
// +optional
Conditions []metav1.Condition `json:"conditions,omitempty"`

// url contains information about this risk.
// +kubebuilder:validation:Format=uri
// +kubebuilder:validation:MinLength=1
Expand Down
Loading