api: Move ClusterVersion/ClusterOperator into config.openshift.io #127

smarterclayton · 2018-11-09T22:34:48Z

These types are used by the cluster version operator to update a
cluster and to report on the status of the cluster as the admin
would see it.

The ClusterVersion and ClusterOperator types were created in the
cluster version operator and are now stable enough to be moved here.

smarterclayton · 2018-11-09T22:35:01Z

/assign @deads2k

bparees · 2018-11-09T22:44:07Z

/cc @gabemontero

smarterclayton · 2018-11-09T23:33:38Z

Note that both dependency updating and go tests in the config pkg seem broken

deads2k · 2018-11-12T19:32:06Z

config/v1/json.go

+	"regexp"
+)
+
+// reUUID4 uses a regexp to avoid taking a dependency on a UUID library for now


apimachinery already has one: https://github.com/kubernetes/apimachinery/blob/master/pkg/util/uuid/uuid.go

We don't actually depend on that, so that brings in a whole mess of new dependencies

deads2k · 2018-11-12T19:32:47Z

config/v1/json.go

+var reUUID4 = regexp.MustCompile(`^[a-fA-F\d]{8}-[a-fA-F\d]{4}-4[a-fA-F\d]{3}-[89abAB][a-fA-F\d]{3}-[a-fA-F\d]{12}$`)
+
+// UnmarshalJSON unmarshals RFC4122 uuid from string.
+func (cid *ClusterID) UnmarshalJSON(b []byte) error {


are you sure you want to do this? it will prevent deserialization client-side. I would stay away from this and enforce it as a CRD validator.

This was being enforced in CVO right now. We won't be able to enforce this with a naive validator.

I agree with the point

config/v1/types_cluster_operator.go

deads2k · 2018-11-12T19:34:14Z

config/v1/types_cluster_operator.go

+type ClusterOperatorStatus struct {
+	// conditions describes the state of the operator's reconciliation functionality.
+	// +patchMergeKey=type
+	// +patchStrategy=merge


these don't work for CRDs. Prefer removing them.

They would actually work locally if you ran the apply patch code in client-go against these types.

deads2k · 2018-11-12T19:35:09Z

config/v1/types_cluster_operator.go

+type ClusterStatusConditionType string
+
+const (
+	// OperatorAvailable indicates that the binary maintained by the operator (eg: openshift-apiserver for the


@derekwaynecarr suggests "operand" for "binary maintained by the operator"

Ugh, I hate that word.

deads2k · 2018-11-12T19:36:25Z

config/v1/types_cluster_version.go

+	// it will use the appropriate update server for the cluster and region.
+	//
+	// +optional
+	Upstream *URL `json:"upstream"`


Why would we distinguish between "" and nil?

No need to, but will have to refactor the CVO for it.

IMO nil is valid option, saying i do not care use your default. But "" should be an error.

IMO nil is valid option, saying i do not care use your default. But "" should be an error.

Why not make "" mean "I don't care, use the default" and eliminate the invalid case for users to stumble on?

Actually, do we want to be able to disable upstream? If so, we should explicitly model that I think.

That would probably usually be

UpdateServer *UpdateServer ... type UpdateServer struct { // defaults to the appropriate location URL string }

or

DisableUpdates bool Upstream URL

We want the unusual conditions to be opted in - bools that default to true are too confusing for users.

bools that default to true are too confusing for users.

bools that default to true are too confusing for everyone

yes.

How about I take the follow up to add disablement as a net new field, with clear semantics that fit into "auto update" as a multi level thing:

I want nothing (don't even talk to anyone about updates)

I want to get versions from redhat [default]

I want to get versions from my custom server

I want to automatically update to those versions

deads2k · 2018-11-12T19:36:49Z

config/v1/types_cluster_version.go

+}
+
+// URL is a thin wrapper around string that ensures the string is a valid URL.
+type URL string


we haven't be assiduous about this. Would you like to be? I have no strong feelings.

Carry over from the CVO. I think used judiciously it is good, but can be taken too far.

config/v1/types_cluster_version.go

abhinavdahiya · 2018-11-12T22:52:23Z

config/v1/json.go

+)
+
+// reUUID4 uses a regexp to avoid taking a dependency on a UUID library for now
+var reUUID4 = regexp.MustCompile(`^[a-fA-F\d]{8}-[a-fA-F\d]{4}-4[a-fA-F\d]{3}-[89abAB][a-fA-F\d]{3}-[a-fA-F\d]{12}$`)


https://github.com/openshift/cluster-version-operator/blob/ffcd66a9b1e8ba7364f7ed46d0cac577e5b49e82/vendor/github.com/google/uuid/uuid.go#L138-L149 is the actual check we want to make.

I'm going to move this into the CVO, David is right that client side validation doesn't work in the long run. We already need to reflect validation errors back to the ClusterVersion status, so we might as well put that there.

smarterclayton · 2018-11-13T16:41:37Z

Comments addressed.

These types are used by the cluster version operator to update a cluster and to report on the status of the cluster as the admin would see it. The ClusterVersion and ClusterOperator types were created in the cluster version operator and are now stable enough to be moved here.

deads2k · 2018-11-13T21:31:02Z

Just the namespace vs cluster scope that we spoke about in person.

lgtm otherwise

deads2k · 2018-11-14T16:06:06Z

/lgtm

openshift-ci-robot · 2018-11-14T16:10:27Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, smarterclayton

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k,smarterclayton]

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

The typo was originally from 898d7e3 (api: Move ClusterVersion/ClusterOperator into config.openshift.io, 2018-11-09, openshift#127). The "any operator" bit explains why we don't want, for example, ingress-operator specific config in here. The later ClusterOperatorSpec comment goes into more detail on that with its 'pause' hypothetical, but a bit of extra hinting can't hurt ;).

…etes ecosystem" The abbreviated wording was originally from 898d7e3 (api: Move ClusterVersion/ClusterOperator into config.openshift.io, 2018-11-09, openshift#127). But using the full name in these docs gives readers one less abbreviation to recognize.

The typo was originally from 898d7e3 (api: Move ClusterVersion/ClusterOperator into config.openshift.io, 2018-11-09, openshift#127). The "any operator" bit explains why we don't want, for example, ingress-operator specific config in here. The later ClusterOperatorSpec comment goes into more detail on that with its 'pause' hypothetical, but a bit of extra hinting can't hurt ;).

This type used to be used for both the admin-provided spec.desiredUpdate and the operator-provided status.desired. That changed in 575f8d2 (config/v1: New Release type for ClusterVersionStatus, 2019-01-19, openshift#521, 4.6), when status.desired moved to a new Release type, so now Update is only used for spec.desiredUpdate. The properties have been '+optional' since they landed in 898d7e3 (api: Move ClusterVersion/ClusterOperator into config.openshift.io, 2018-11-09, openshift#127) and ab4ff93 (Update ClusterVersion to have a 'force' update flag and track verified, 2019-04-22, openshift#293). I'm adding 'omitempty', because if the admin doesn't have a particular version or image in mind, returning explicit empty strings is distracting noise. With this commit, it will be easier to focus on the version or image property that did get set, and you need at least one of them to be set to be a usable update request. The force property is a bit more wiggly, since there may be some benefit to explicitly pointing out that the admin is not forcing the update. But forcing is supposed to be an exceptional-situation safety valve, so I'm adding omitempty there too, because the benefit of de-emphasizing the property's presence (and reducing the chance that an admin says "hey, let's see what 'force: true' does" without reading associated docs) outweighs the benefit of explicitly pointing out 'force: false' cases.

openshift-ci-robot requested review from bparees and enj November 9, 2018 22:34

openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Nov 9, 2018

openshift-ci-robot assigned deads2k Nov 9, 2018

openshift-ci-robot requested a review from gabemontero November 9, 2018 22:44

smarterclayton force-pushed the add_cluster_op_version branch 4 times, most recently from 31d6063 to 570e1fa Compare November 9, 2018 23:18

deads2k reviewed Nov 12, 2018

View reviewed changes

config/v1/types_cluster_operator.go Show resolved Hide resolved

deads2k reviewed Nov 12, 2018

View reviewed changes

config/v1/types_cluster_version.go Show resolved Hide resolved

smarterclayton mentioned this pull request Nov 12, 2018

Move ClusterOperator to config.openshift.io openshift/cluster-version-operator#48

Closed

abhinavdahiya reviewed Nov 12, 2018

View reviewed changes

smarterclayton force-pushed the add_cluster_op_version branch from 570e1fa to 8ce86d9 Compare November 13, 2018 16:40

smarterclayton force-pushed the add_cluster_op_version branch from 8ce86d9 to 898d7e3 Compare November 13, 2018 21:03

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 14, 2018

openshift-merge-robot merged commit 539360d into openshift:master Nov 14, 2018

wking mentioned this pull request Oct 28, 2019

config/v1/types_cluster_operator: Fix "spec hold the" typo #499

Merged

wking mentioned this pull request Oct 28, 2019

config/v1/types_cluster_operator: "the kube ecosystem" -> "the Kubernetes ecosystem" #500

Merged

wking mentioned this pull request Aug 17, 2021

config/v1/types_cluster_version: Make Update properties omitempty #992

Closed

api: Move ClusterVersion/ClusterOperator into config.openshift.io #127

api: Move ClusterVersion/ClusterOperator into config.openshift.io #127

Uh oh!

Conversation

smarterclayton commented Nov 9, 2018

Uh oh!

smarterclayton commented Nov 9, 2018

Uh oh!

bparees commented Nov 9, 2018

Uh oh!

smarterclayton commented Nov 9, 2018

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!

Choose a reason for hiding this comment

Uh oh!

smarterclayton Nov 13, 2018 • 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!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

abhinavdahiya Nov 12, 2018 • 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!

smarterclayton commented Nov 13, 2018

Uh oh!

deads2k commented Nov 13, 2018

Uh oh!

deads2k commented Nov 14, 2018

Uh oh!

openshift-ci-robot commented Nov 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

smarterclayton Nov 13, 2018 •

edited

Loading

abhinavdahiya Nov 12, 2018 •

edited

Loading