Skip to content

config/v1/types_cluster_version: Add Architecture to DesiredUpdate #1339

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

Merged
openshift-merge-robot merged 1 commit into from
Dec 7, 2022
Merged

config/v1/types_cluster_version: Add Architecture to DesiredUpdate #1339

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 18 additions & 7 deletions config/v1/0000_00_cluster-version-operator_01_clusterversion.crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,18 +86,29 @@ spec:
description: clusterID uniquely identifies this cluster. This is expected to be an RFC4122 UUID value (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx in hexadecimal values). This is a required field.
type: string
desiredUpdate:
description: "desiredUpdate is an optional field that indicates the desired value of the cluster version. Setting this value will trigger an upgrade (if the current version does not match the desired version). The set of recommended update values is listed as part of available updates in status, and setting values outside that range may cause the upgrade to fail. You may specify the version field without setting image if an update exists with that version in the availableUpdates or history. \n If an upgrade fails the operator will halt and report status about the failing component. Setting the desired update value back to the previous version will cause a rollback to be attempted. Not all rollbacks will succeed."
description: "desiredUpdate is an optional field that indicates the desired value of the cluster version. Setting this value will trigger an upgrade (if the current version does not match the desired version). The set of recommended update values is listed as part of available updates in status, and setting values outside that range may cause the upgrade to fail. \n Some of the fields are inter-related with restrictions and meanings described here. 1. image is specified, version is specified, architecture is specified. API validation error. 2. image is specified, version is specified, architecture is not specified. You should not do this. version is silently ignored and image is used. 3. image is specified, version is not specified, architecture is specified. API validation error. 4. image is specified, version is not specified, architecture is not specified. image is used. 5. image is not specified, version is specified, architecture is specified. version and desired architecture are used to select an image. 6. image is not specified, version is specified, architecture is not specified. version and current architecture are used to select an image. 7. image is not specified, version is not specified, architecture is specified. API validation error. 8. image is not specified, version is not specified, architecture is not specified. API validation error. \n If an upgrade fails the operator will halt and report status about the failing component. Setting the desired update value back to the previous version will cause a rollback to be attempted. Not all rollbacks will succeed."
type: object
properties:
architecture:
description: architecture is an optional field that indicates the desired value of the cluster architecture. In this context cluster architecture means either a single architecture or a multi architecture. architecture can only be set to Multi thereby only allowing updates from single to multi architecture. If architecture is set, image cannot be set and version must be set. Valid values are 'Multi' and empty.
type: string
enum:
- Multi
- ""
force:
description: force allows an administrator to update to an image that has failed verification or upgradeable checks. This option should only be used when the authenticity of the provided image has been verified out of band because the provided image will run with full administrative access to the cluster. Do not use this flag with images that comes from unknown or potentially malicious sources.
type: boolean
image:
description: image is a container image location that contains the update. When this field is part of spec, image is optional if version is specified and the availableUpdates field contains a matching version.
description: image is a container image location that contains the update. image should be used when the desired version does not exist in availableUpdates or history. When image is set, version is ignored. When image is set, version should be empty. When image is set, architecture cannot be specified.
type: string
version:
description: version is a semantic versioning identifying the update version. When this field is part of spec, version is optional if image is specified.
description: version is a semantic version identifying the update version. version is ignored if image is specified and required if architecture is specified.
type: string
x-kubernetes-validations:
- rule: 'has(self.architecture) && has(self.image) ? (self.architecture == '''' || self.image == '''') : true'
message: cannot set both Architecture and Image
- rule: 'has(self.architecture) && self.architecture != '''' ? self.version != '''' : true'
message: Version must be set if Architecture is set
overrides:
description: overrides is list of overides for components that are managed by cluster version operator. Marking a component unmanaged will prevent the operator from creating or updating the object.
type: array
Expand Down Expand Up @@ -157,7 +168,7 @@ spec:
description: url contains information about this release. This URL is set by the 'url' metadata property on a release or the metadata returned by the update API and should be displayed as a link in user interfaces. The URL field may not be set for test or nightly releases.
type: string
version:
description: version is a semantic versioning identifying the update version. When this field is part of spec, version is optional if image is specified.
description: version is a semantic version identifying the update version. When this field is part of spec, version is optional if image is specified.
type: string
nullable: true
capabilities:
Expand Down Expand Up @@ -267,7 +278,7 @@ spec:
description: url contains information about this release. This URL is set by the 'url' metadata property on a release or the metadata returned by the update API and should be displayed as a link in user interfaces. The URL field may not be set for test or nightly releases.
type: string
version:
description: version is a semantic versioning identifying the update version. When this field is part of spec, version is optional if image is specified.
description: version is a semantic version identifying the update version. When this field is part of spec, version is optional if image is specified.
type: string
risks:
description: 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.
Expand Down Expand Up @@ -368,7 +379,7 @@ spec:
description: url contains information about this release. This URL is set by the 'url' metadata property on a release or the metadata returned by the update API and should be displayed as a link in user interfaces. The URL field may not be set for test or nightly releases.
type: string
version:
description: version is a semantic versioning identifying the update version. When this field is part of spec, version is optional if image is specified.
description: version is a semantic version identifying the update version. When this field is part of spec, version is optional if image is specified.
type: string
history:
description: history contains a list of the most recent versions applied to the cluster. This value may be empty during cluster startup, and then will be updated when a new update is being applied. The newest update is first in the list and it is ordered by recency. Updates in the history have state Completed if the rollout completed - if an update was failing or halfway applied the state will be Partial. Only a limited amount of update history is preserved.
Expand Down Expand Up @@ -405,7 +416,7 @@ spec:
description: verified indicates whether the provided update was properly verified before it was installed. If this is false the cluster may not be trusted. Verified does not cover upgradeable checks that depend on the cluster state at the time when the update target was accepted.
type: boolean
version:
description: version is a semantic versioning identifying the update version. If the requested image does not define a version, or if a failure occurs retrieving the image, this value may be empty.
description: version is a semantic version identifying the update version. If the requested image does not define a version, or if a failure occurs retrieving the image, this value may be empty.
type: string
observedGeneration:
description: observedGeneration reports which version of the spec is being synced. If this value is not equal to metadata.generation, then the desired and conditions fields may represent a previous version.
Expand Down
122 changes: 122 additions & 0 deletions config/v1/stable.clusterversion.testsuite.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,3 +14,125 @@ tests:
kind: ClusterVersion
spec:
clusterID: foo
- name: Should allow image to be set
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
image: bar
expected: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
image: bar
- name: Should allow version to be set
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
version: 4.11.1
expected: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
version: 4.11.1
- name: Should allow architecture to be empty
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: ""
version: 4.11.1
expected: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: ""
version: 4.11.1
- name: Should allow architecture and version to be set
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: Multi
version: 4.11.1
expected: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: Multi
version: 4.11.1
- name: Version must be set if architecture is set
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: Multi
expectedError: "Version must be set if Architecture is set"
- name: Should not allow image and architecture to be set
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: Multi
version: 4.11.1
image: bar
expectedError: "cannot set both Architecture and Image"
onUpdate:
Copy link
Contributor

@JoelSpeed JoelSpeed Dec 5, 2022

Choose a reason for hiding this comment

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

Do any of the validations rely on a previously persisted state? If they don't, then we don't need separate update tests and the create validation should cover both cases

Copy link
Member

@LalatenduMohanty LalatenduMohanty Dec 5, 2022

Choose a reason for hiding this comment

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

Ok, I am going to remove this test.

Copy link
Contributor

@JoelSpeed JoelSpeed Dec 6, 2022

Choose a reason for hiding this comment

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

I had meant the entire set of onUpdate, none of them seem necessary since they all duplicate previous test cases IIUC. But I see now why you've got two different ones, it's the transitions from image to both and from architecture to both, makes sense, we should add an onCreate version though

Copy link
Member

@LalatenduMohanty LalatenduMohanty Dec 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Both onUpdate tests are covered by the following onCreate test, so unless I am missing something we do not need to add any new onCreate tests.

- name: Should not allow image and architecture to be set
    initial: |
      apiVersion: config.openshift.io/v1
      kind: ClusterVersion
      spec:
        clusterID: foo
        desiredUpdate:
          architecture: Multi
          version: 4.11.1
          image: bar

Copy link
Contributor Author

@jottofar jottofar Dec 7, 2022

Choose a reason for hiding this comment

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

We have onCreate Should not allow image and architecture to be set which checks that both cannot be set so I'm not sure what can be added to onCreate to replicate the two onUpdates since each of them can start with only one of the values but the onCreate cannot (AFAIK).

Copy link
Contributor Author

@jottofar jottofar Dec 7, 2022

Choose a reason for hiding this comment

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

And I originally added the onUpdate tests to cover a user doing an oc patch for example.

Copy link
Member

@LalatenduMohanty LalatenduMohanty Dec 7, 2022

Choose a reason for hiding this comment

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

Also I can work on this even after the PR is merged. So lets not block the PR to get merged because of this.

- name: Should not allow image to be set if architecture set
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: Multi
version: 4.11.1
updated: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: Multi
version: 4.11.1
image: bar
expectedError: "cannot set both Architecture and Image"
- name: Should not allow architecture to be set if image set
initial: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
image: bar
updated: |
apiVersion: config.openshift.io/v1
kind: ClusterVersion
spec:
clusterID: foo
desiredUpdate:
architecture: Multi
version: 4.11.1
image: bar
expectedError: "cannot set both Architecture and Image"
53 changes: 44 additions & 9 deletions config/v1/types_cluster_version.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,17 @@ type ClusterVersionSpec struct {
// the current version does not match the desired version). The set of
// recommended update values is listed as part of available updates in
// status, and setting values outside that range may cause the upgrade
// to fail. You may specify the version field without setting image if
// an update exists with that version in the availableUpdates or history.
// to fail.
//
// Some of the fields are inter-related with restrictions and meanings described here.
// 1. image is specified, version is specified, architecture is specified. API validation error.
// 2. image is specified, version is specified, architecture is not specified. You should not do this. version is silently ignored and image is used.
// 3. image is specified, version is not specified, architecture is specified. API validation error.
// 4. image is specified, version is not specified, architecture is not specified. image is used.
// 5. image is not specified, version is specified, architecture is specified. version and desired architecture are used to select an image.
// 6. image is not specified, version is specified, architecture is not specified. version and current architecture are used to select an image.
// 7. image is not specified, version is not specified, architecture is specified. API validation error.
// 8. image is not specified, version is not specified, architecture is not specified. API validation error.
//
// If an upgrade fails the operator will halt and report status
// about the failing component. Setting the desired update value back to
Expand Down Expand Up @@ -191,7 +200,7 @@ type UpdateHistory struct {
// +nullable
CompletionTime *metav1.Time `json:"completionTime"`

// version is a semantic versioning identifying the update version. If the
// version is a semantic version identifying the update version. If the
// requested image does not define a version, or if a failure occurs
// retrieving the image, this value may be empty.
//
Expand Down Expand Up @@ -224,6 +233,16 @@ type UpdateHistory struct {
// ClusterID is string RFC4122 uuid.
type ClusterID string

// ClusterVersionArchitecture enumerates valid cluster architectures.
// +kubebuilder:validation:Enum="Multi";""
type ClusterVersionArchitecture string

const (
// ClusterVersionArchitectureMulti identifies a multi architecture. A multi
// architecture cluster is capable of running nodes with multiple architectures.
ClusterVersionArchitectureMulti ClusterVersionArchitecture = "Multi"
)

// ClusterVersionCapability enumerates optional, core cluster components.
// +kubebuilder:validation:Enum=openshift-samples;baremetal;marketplace;Console;Insights;Storage;CSISnapshot
type ClusterVersionCapability string
Expand Down Expand Up @@ -406,17 +425,33 @@ type ComponentOverride struct {
type URL string

// Update represents an administrator update request.
// +kubebuilder:validation:XValidation:rule="has(self.architecture) && has(self.image) ? (self.architecture == '' || self.image == '') : true",message="cannot set both Architecture and Image"
// +kubebuilder:validation:XValidation:rule="has(self.architecture) && self.architecture != '' ? self.version != '' : true",message="Version must be set if Architecture is set"
// +k8s:deepcopy-gen=true
type Update struct {
// version is a semantic versioning identifying the update version. When this
// field is part of spec, version is optional if image is specified.
// architecture is an optional field that indicates the desired
// value of the cluster architecture. In this context cluster
// architecture means either a single architecture or a multi
// architecture. architecture can only be set to Multi thereby
Copy link
Contributor

@JoelSpeed JoelSpeed Dec 6, 2022

Choose a reason for hiding this comment

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

So there's no way to transition back to the a single arch cluster?

I wonder if we need to explicitly say, "on future updates, if the existing cluster image is multi architecture, then the downloaded image will also be multi architecture. There is no way to convert a cluster from multi to single architecture."

I think I missed this while reviewing before but it seems odd that on future updates, having the field omitted/empty relies on the existing architecture

There is no way to convert a cluster from multi to single architecture

Thinking about this, which I got from trying to understand this API, that's not actually true is it, if I forced an image into the update that was single arch I could force the cluster back to single arch, is that ok?

Copy link
Member

@LalatenduMohanty LalatenduMohanty Dec 6, 2022

Choose a reason for hiding this comment

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

Thinking about this, which I got from trying to understand this API, that's not actually true is it, if I forced an image into the update that was single arch I could force the cluster back to single arch, is that ok?

That is right. Even if the current ask is only one time migration from single arch to multi arch , we do not want to add any more code to restrict the opposite. Officially we will only document that single to multi is supported. But there will be room for future change of scope.

// only allowing updates from single to multi architecture. If
// architecture is set, image cannot be set and version must be
// set.
Copy link
Contributor

@JoelSpeed JoelSpeed Dec 6, 2022

Choose a reason for hiding this comment

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

Nit: In other enums, we typically have a statement that says valid values are x,y,z, you have this in the blurb but it would be good to have that as an explicit statement as it gets a bit muddled in here IMO

Suggested change
// set.
// set.
// Valid values are 'Multi' and empty.

Copy link
Member

@LalatenduMohanty LalatenduMohanty Dec 6, 2022

Choose a reason for hiding this comment

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

Agree, will fix it.

Copy link
Member

@LalatenduMohanty LalatenduMohanty Dec 6, 2022

Choose a reason for hiding this comment

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

Fixed in 08f0ef0

// Valid values are 'Multi' and empty.
//
// +optional
Architecture ClusterVersionArchitecture `json:"architecture"`

// version is a semantic version identifying the update version.
// version is ignored if image is specified and required if
// architecture is specified.
//
// +optional
Version string `json:"version"`

// image is a container image location that contains the update. When this
// field is part of spec, image is optional if version is specified and the
// availableUpdates field contains a matching version.
// image is a container image location that contains the update.
// image should be used when the desired version does not exist in availableUpdates or history.
// When image is set, version is ignored. When image is set, version should be empty.
// When image is set, architecture cannot be specified.
//
// +optional
Image string `json:"image"`
Expand All @@ -435,7 +470,7 @@ type Update struct {
// Release represents an OpenShift release image and associated metadata.
// +k8s:deepcopy-gen=true
type Release struct {
// version is a semantic versioning identifying the update version. When this
// version is a semantic version identifying the update version. When this
// field is part of spec, version is optional if image is specified.
// +required
Version string `json:"version"`
Expand Down
Loading