config/v1/types_cluster_version: Add conditionalUpdates and friends #1011

wking · 2021-09-15T18:59:57Z

Implementing targeted edge blocking as described in openshift/enhancements#821

wking · 2021-09-15T20:08:50Z

verify job is too shy. I don't have these tools installed locally; it should just give me a diff I can apply instead...

$ curl -L https://github.com/openshift/kubernetes-sigs-controller-tools/releases/download/v0.6.0/controller-gen-linux-arm64
Not Found

wking · 2021-09-16T17:17:01Z

All green :)

/assign @deads2k

config/v1/types_cluster_version.go

deads2k · 2021-09-20T18:46:45Z

config/v1/types_cluster_version.go

+	// current status. Known types are:
+	// * Evaluating, for whether the cluster-version operator will attempt to evaluate any risks[].matchingRules.
+	// * Recommended, for whether the update is recommended for the current cluster.
+	// +patchMergeKey=type


@sttts is better with the particulars of which one these you want. i think you want one where you have a key.

This is a blind copy from the docs.

config/v1/types_cluster_version.go

Implementing targeted edge blocking as described in [1]. Except that some properties which are optional in the enhancement are required here. I think this is strictly worse handling for the cluster-version operator, because we are just getting this data from the upstream update service's Cincinnati JSON, and if the upstream says "here's a risk, but I am not linking a URI", and the CVO subsequently rejects the entire update payload as invalid, then downstream consumers are not hearing about the reason/message/matchingRules that the upstream may have been passing along. But I'm not an API approver, and David is asking for the +required in [2,3,4,5]. I dunno why he isn't finding my argument's convincing, but I'm out of time to continue arguing my position. Swagger, deepcopy, and CRD changes were generated with: $ make update [1]: openshift/enhancements#821 [2]: openshift#1011 (comment) [3]: openshift#1011 (comment) [4]: openshift#1011 (comment) [5]: openshift#1011 (comment)

Implementing targeted edge blocking as described in [1]. Except: * Some properties which are optional in the enhancement are required here. I think this is strictly worse handling for the cluster-version operator, because we are just getting this data from the upstream update service's Cincinnati JSON, and if the upstream says "here's a risk, but I am not linking a URI", and the CVO subsequently rejects the entire update payload as invalid, then downstream consumers are not hearing about the reason/message/matchingRules that the upstream may have been passing along. But I'm not an API approver, and David is asking for the +required in [2,3,4,5]. I dunno why he isn't finding my argument's convincing, but I'm out of time to continue arguing my position. * Similarly, the ClusterCondition.Type property has an enum, because David requires it [6]. This means that the CVO will need to self-censor if there are unrecognized types (because we don't want our ClusterVersion writes rejected on API-server validation failures). That means that instead of saying "... and there was another rule $UNRECOGNIZED_TYPE that I don't know how to evaluate", the CVO will have to silently drop that rule. And if that's the only rule, the CVO will have to silently drop that risk and conditional update. David is fine with that, even if I'm not on board, and he's the API approver, so that's what we'll do. I've also upcased the types to Always and PromQL; from David: Always and PromQL. We use CamelCase for enumerated values * history[].overrides became history[].acceptedRisks to satisfy David's concerns about misinterpretation [7]. Swagger, deepcopy, and CRD changes were generated with: $ make update [1]: openshift/enhancements#821 [2]: openshift#1011 (comment) [3]: openshift#1011 (comment) [4]: openshift#1011 (comment) [5]: openshift#1011 (comment) [6]: openshift#1011 (comment) [7]: openshift#1011 (comment)

wking · 2021-09-22T23:44:54Z

verify hates me, but I think this most recent run has failed on:

run `controller-gen schemapatch:manifests=./samples/v1 paths=./samples/v1 output:dir=/tmp/tmp.0Ou081N0wo -w` to see all available markers, or `controller-gen schemapatch:manifests=./samples/v1 paths=./samples/v1 output:dir=/tmp/tmp.0Ou081N0wo -h` for usage
make: *** [verify-codegen-crds-samples] Error 1

/retest

config/v1/types_cluster_version.go

Implementing targeted edge blocking as described in [1]. Except: * Some properties which are optional in the enhancement are required here. I think this is strictly worse handling for the cluster-version operator, because we are just getting this data from the upstream update service's Cincinnati JSON, and if the upstream says "here's a risk, but I am not linking a URI", and the CVO subsequently rejects the entire update payload as invalid, then downstream consumers are not hearing about the reason/message/matchingRules that the upstream may have been passing along. But I'm not an API approver, and David is asking for the +required in [2,3,4,5]. I dunno why he isn't finding my argument's convincing, but I'm out of time to continue arguing my position. * Similarly, the ClusterCondition.Type property has an enum, because David requires it [6]. This means that the CVO will need to self-censor if there are unrecognized types (because we don't want our ClusterVersion writes rejected on API-server validation failures). That means that instead of saying "... and there was another rule $UNRECOGNIZED_TYPE that I don't know how to evaluate", the CVO will have to silently drop that rule. And if that's the only rule, the CVO will have to silently drop that risk and conditional update. David is fine with that, even if I'm not on board, and he's the API approver, so that's what we'll do. I've also upcased the types to Always and PromQL; from David: Always and PromQL. We use CamelCase for enumerated values * history[].overrides became history[].acceptedRisks to satisfy David's concerns about misinterpretation [7]. Swagger, deepcopy, and CRD changes were generated with: $ make update [1]: openshift/enhancements#821 [2]: openshift#1011 (comment) [3]: openshift#1011 (comment) [4]: openshift#1011 (comment) [5]: openshift#1011 (comment) [6]: openshift#1011 (comment) [7]: openshift#1011 (comment)

Implementing targeted edge blocking as described in [1]. Except: * Some properties which are optional in the enhancement are required here. I think this is strictly worse handling for the cluster-version operator, because we are just getting this data from the upstream update service's Cincinnati JSON, and if the upstream says "here's a risk, but I am not linking a URI", and the CVO subsequently rejects the entire update payload as invalid, then downstream consumers are not hearing about the reason/message/matchingRules that the upstream may have been passing along. But I'm not an API approver, and David is asking for the +required in [2,3,4,5]. I dunno why he isn't finding my argument's convincing, but I'm out of time to continue arguing my position. * Similarly, the ClusterCondition.Type property has an enum, because David requires it [6]. This means that the CVO will need to self-censor if there are unrecognized types (because we don't want our ClusterVersion writes rejected on API-server validation failures). That means that instead of saying "... and there was another rule $UNRECOGNIZED_TYPE that I don't know how to evaluate", the CVO will have to silently drop that rule. And if that's the only rule, the CVO will have to silently drop that risk and conditional update. David is fine with that, even if I'm not on board, and he's the API approver, so that's what we'll do. I've also upcased the types to Always and PromQL; from David: Always and PromQL. We use CamelCase for enumerated values * history[].overrides became history[].acceptedRisks to satisfy David's concerns about misinterpretation [7]. * The server-side apply annotations documented in [8]: listType, listMapKey, patchMergeStrategy, and patchMergeKey. Swagger, deepcopy, and CRD changes were generated with: $ make update [1]: openshift/enhancements#821 [2]: openshift#1011 (comment) [3]: openshift#1011 (comment) [4]: openshift#1011 (comment) [5]: openshift#1011 (comment) [6]: openshift#1011 (comment) [7]: openshift#1011 (comment) [8]: https://kubernetes.io/docs/reference/using-api/server-side-apply/

deads2k · 2021-09-23T17:23:50Z

/approve
/lgtm

openshift-ci · 2021-09-23T17:24:23Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [deads2k]

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

…anded Folding in changes made in order to land [1]. [1]: openshift/api#1011

…anded Following up on openshift#821 with the changes made in order to land openshift/api#1011.

…anded Following up on openshift#821 with the changes made in order to land openshift/api#1011. The graph-data properties remain optional, because graph-data has been used for unconditional blocks that do not declare risks today. The update service will continue to read those blocks, but will implement them by pruning 'edges'. So things that end up in 'conditionalEdges' will still need all the metadata populated to keep David happy ;).

Pulling in [1]. Generated with: $ go get github.com/openshift/api@00988ef88ee072cf39fda7558c44737e8ff2b71d ... go get: upgraded github.com/google/go-cmp v0.5.2 => v0.5.5 go get: upgraded github.com/openshift/api v0.0.0-20210517065120-b325f58df679 => v0.0.0-20210923172539-00988ef88ee0 go get: upgraded golang.org/x/net v0.0.0-20210224082022-3d97a244fca7 => v0.0.0-20210520170846-37e1c6afe023 go get: upgraded k8s.io/api v0.21.1 => v0.22.1 go get: upgraded k8s.io/apimachinery v0.21.1 => v0.22.1 go get: upgraded k8s.io/klog/v2 v2.8.0 => v2.9.0 $ go mod vendor $ go mod tidy $ git add -A go.* vendor [1]: openshift/api#1011

…anded Following up on openshift#821 with the changes made in order to land openshift/api#1011. The graph-data properties remain optional, because graph-data has been used for unconditional blocks that do not declare risks today. The update service will continue to read those blocks, but will implement them by pruning 'edges'. So things that end up in 'conditionalEdges' will still need all the metadata populated to keep David happy ;).

Pulling in [1]. Generated with: $ go get github.com/openshift/api@00988ef88ee072cf39fda7558c44737e8ff2b71d ... go get: upgraded github.com/google/go-cmp v0.5.2 => v0.5.5 go get: upgraded github.com/openshift/api v0.0.0-20210517065120-b325f58df679 => v0.0.0-20210923172539-00988ef88ee0 go get: upgraded golang.org/x/net v0.0.0-20210224082022-3d97a244fca7 => v0.0.0-20210520170846-37e1c6afe023 go get: upgraded k8s.io/api v0.21.1 => v0.22.1 go get: upgraded k8s.io/apimachinery v0.21.1 => v0.22.1 go get: upgraded k8s.io/klog/v2 v2.8.0 => v2.9.0 And because splitting Kube components between 0.21 and 0.22 leads to function-signature mismatches, move everything else to 0.22 too: $ sed -i 's/v0.21.1/v0.22.1/' go.mod Then update the vendor directory and stage for commit: $ go mod vendor $ go mod tidy $ git add -A go.* vendor [1]: openshift/api#1011

Implementing targeted edge blocking as described in [1]. Except: * Some properties which are optional in the enhancement are required here. I think this is strictly worse handling for the cluster-version operator, because we are just getting this data from the upstream update service's Cincinnati JSON, and if the upstream says "here's a risk, but I am not linking a URI", and the CVO subsequently rejects the entire update payload as invalid, then downstream consumers are not hearing about the reason/message/matchingRules that the upstream may have been passing along. But I'm not an API approver, and David is asking for the +required in [2,3,4,5]. I dunno why he isn't finding my argument's convincing, but I'm out of time to continue arguing my position. * Similarly, the ClusterCondition.Type property has an enum, because David requires it [6]. This means that the CVO will need to self-censor if there are unrecognized types (because we don't want our ClusterVersion writes rejected on API-server validation failures). That means that instead of saying "... and there was another rule $UNRECOGNIZED_TYPE that I don't know how to evaluate", the CVO will have to silently drop that rule. And if that's the only rule, the CVO will have to silently drop that risk and conditional update. David is fine with that, even if I'm not on board, and he's the API approver, so that's what we'll do. I've also upcased the types to Always and PromQL; from David: Always and PromQL. We use CamelCase for enumerated values * history[].overrides became history[].acceptedRisks to satisfy David's concerns about misinterpretation [7]. * The server-side apply annotations documented in [8]: listType, listMapKey, patchMergeStrategy, and patchMergeKey. Swagger, deepcopy, and CRD changes were generated with: $ make update [1]: openshift/enhancements#821 [2]: openshift#1011 (comment) [3]: openshift#1011 (comment) [4]: openshift#1011 (comment) [5]: openshift#1011 (comment) [6]: openshift#1011 (comment) [7]: openshift#1011 (comment) [8]: https://kubernetes.io/docs/reference/using-api/server-side-apply/

…onal update risk We've had Upgradeable since 2019 [1], but it is a confusing condition, because the "between minor versions" wording [2] in the message isn't obvious to users who have not yet internalized SemVer's MAJOR.MINOR.PATCH terminology [3]. Conditional update risks landed in 2021 [4] and give us a clear API for declaring exactly which update targets have the exposure. This commit adds client-side code to the tech-preview 'recommend' subcommand, so folks can get a feel for that user experience. If it goes over well, we can shift the logic to the cluster-version operator so all clients can benefit. The alreadyHasUpgradeableRisk check ensures we don't double up if the CVO eventually picks up this pivot. [1]: openshift/api#206 [2]: https://github.com/openshift/cluster-version-operator/blob/c4b8362d8acd08d63a600b4d53c33e8737ed7a53/pkg/cvo/upgradeable.go#L218-L228 [3]: https://semver.org/spec/v2.0.0.html#summary [4]: openshift/api#1011

openshift-ci bot requested review from sjenning and sttts September 15, 2021 19:00

wking force-pushed the targeted-edge-blocking branch from 2067324 to d8d916c Compare September 15, 2021 19:15

wking force-pushed the targeted-edge-blocking branch 7 times, most recently from 89b2c76 to f95ef15 Compare September 16, 2021 04:55

openshift-ci bot assigned deads2k Sep 16, 2021