BUG 1769004: Make Machine/MachineSets structural #552

JoelSpeed · 2020-04-06T12:55:42Z

This PR:

Upgrades CRDs to extensions/v1
Sets XPreserveUnknownFields: true on runtime.RawExtensions within Machine and MachineSet types
Sets preserveUnknownFields: false on CRDs
Adds roundtrip fuzzed tests of Machine/MachineSet to ensure no data is lost by these changes

The user facing effect of this is that the CRDs are now structural and oc explain will work as expected

Note: This PR won't pass tests until rebased onto #550

openshift-ci-robot · 2020-04-06T12:55:48Z

@JoelSpeed: This pull request references Bugzilla bug 1769004, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.5.0) matches configured target release for branch (4.5.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

BUG 1769004: Make Machine/MachineSets structural

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/test-infra repository.

enxebre · 2020-04-07T09:31:41Z

install/0000_30_machine-api-operator_07_machinehealthcheck.crd.yaml

 kind: CustomResourceDefinition
 metadata:
  annotations:
    exclude.release.openshift.io/internal-openshift-hosted: "true"
+    controller-gen.kubebuilder.io/version: (devel)


can we drop this?

enxebre · 2020-04-07T09:34:11Z

install/0000_30_machine-api-operator_07_machinehealthcheck.crd.yaml

@@ -32,136 +20,150 @@ spec:
    singular: machinehealthcheck
  scope: Namespaced
  preserveUnknownFields: false


Can we drop preserveUnknownFields: false? it's default in apiextensions.k8s.io/v1. "This can either be true or undefined. False is forbidden." https://book.kubebuilder.io/reference/markers/crd-processing.html

I believe that is for the values within the OpenAPIV3Schema block.

As I understand it, this value is required to enable upgrades to work smoothly

For CustomResourceDefinitions created in apiextensions.k8s.io/v1, structural OpenAPI v3 validation schemas are required and pruning is enabled and cannot be disabled (note that CRDs converted from apiextensions.k8s.io/v1beta1 to apiextensions.k8s.io/v1 might lack structural schemas, and spec.preserveUnknownFields might be true).

From: https://kubernetes.io/docs/tasks/access-kubernetes-api/custom-resources/custom-resource-definitions/#pruning-versus-preserving-unknown-fields

Because the definitions have been created using the extensions/v1beta1 API up until now and didn't have structural schemas, the spec.PreserveUnknownFields value has been set to true, even when stored as extensions/v1.
So we need to force the value to false to ensure the CRD starts being pruned and restores the behaviour to the default behaviour for v1

cc @sttts to confirm

Posted a message to sig-api-machinery (https://kubernetes.slack.com/archives/C0EG7JC6T/p1586869328387600) and got a confirmation there that the behaviour is as I've described

false is the default. If I see this correctly it only depends only on your apply mechanism. These CRDs are applied through CVO?

Correct, these are applied by CVO, do we not need the field then? It was my understanding that if the CRD was created as v1beta1, it would be stuck with preserveUnknownFields: true if read/updated as v1, unless explicitly overwritten, is that not the case?

As I wrote: it depends on CVO's apply mechanism. If it reads your former v1beta1 CRD as v1, it won't know that it was originally a v1beta1 CRD. I assume it will compare the v1 in cluster to the v1 from your manifests. Read the CVO code to find out whether it takes preserveUnknownFields into consideration.

Ok, having a look through, it seems that applying a CRD via CVO "stomps" the spec, and then sends an update rather than doing a merge as I had expected (like kubectl apply), so I think we can drop preserveUnknownFields, thanks @sttts

https://github.com/openshift/cluster-version-operator/blob/ee4cd7ea187de8f65eed1a57937254fbe8810308/lib/resourcemerge/apiext.go#L61-L65

https://github.com/openshift/cluster-version-operator/blob/ee4cd7ea187de8f65eed1a57937254fbe8810308/lib/resourceapply/apiext.go#L56-L63

enxebre · 2020-04-07T09:35:15Z

hack/gen-crd.sh

@@ -37,4 +44,8 @@ annotate_crd $dir/src/github.com/openshift/machine-api-operator/config/crds/mach
 annotate_crd $dir/src/github.com/openshift/machine-api-operator/config/crds/machine.openshift.io_machinesets.yaml install/0000_30_machine-api-operator_03_machineset.crd.yaml
 annotate_crd $dir/src/github.com/openshift/machine-api-operator/config/crds/machine.openshift.io_machines.yaml install/0000_30_machine-api-operator_02_machine.crd.yaml

+set_preserve_unknown_fields install/0000_30_machine-api-operator_03_machineset.crd.yaml
+set_preserve_unknown_fields install/0000_30_machine-api-operator_02_machine.crd.yaml


https://github.com/openshift/machine-api-operator/pull/552/files#r404671566

JoelSpeed · 2020-04-20T11:21:04Z

/retest

sttts · 2020-04-21T07:55:28Z

install/0000_30_machine-api-operator_02_machine.crd.yaml

-              properties:
-                annotations:
-                  additionalProperties:
+  preserveUnknownFields: false


fyi, this is the default in v1. No need to specify.

enxebre · 2020-04-22T12:42:34Z

/retest
/approve

openshift-ci-robot · 2020-04-22T12:44:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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~~ [enxebre]

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

JoelSpeed · 2020-04-22T14:52:13Z

/retest

JoelSpeed · 2020-04-23T09:55:16Z

/retest

JoelSpeed · 2020-04-23T16:45:19Z

/retest

elmiko · 2020-04-24T13:35:38Z

this is a great addition, love the fuzzing tests too. nicely done Joel!

/lgtm

openshift-bot · 2020-04-24T14:03:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2020-04-24T14:42:41Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-04-24T15:47:34Z

@JoelSpeed: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-azure	`7a78987`	link	`/test e2e-azure`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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

openshift-bot · 2020-04-24T17:31:39Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2020-04-24T18:54:51Z

@JoelSpeed: All pull requests linked via external trackers have merged: openshift/machine-api-operator#552. Bugzilla bug 1769004 has been moved to the MODIFIED state.

In response to this:

BUG 1769004: Make Machine/MachineSets structural

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/test-infra repository.

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Apr 6, 2020

openshift-ci-robot requested review from elmiko and michaelgugino April 6, 2020 12:56

enxebre reviewed Apr 7, 2020

View reviewed changes

JoelSpeed force-pushed the structural-machines branch 2 times, most recently from b3af466 to b13c082 Compare April 7, 2020 11:15

sttts reviewed Apr 21, 2020

View reviewed changes

JoelSpeed added 3 commits April 22, 2020 10:57

Update CRDs to extensions/v1

f77312b

Run Fuzzed RoundTrip test for Machine and MachineSets

c0a96f8

Update vendor

7a78987

JoelSpeed force-pushed the structural-machines branch 2 times, most recently from 5cb8e01 to 7a78987 Compare April 22, 2020 10:09

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 22, 2020

openshift-ci-robot assigned elmiko Apr 24, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 24, 2020

openshift-merge-robot merged commit b9f8d29 into openshift:master Apr 24, 2020

JoelSpeed deleted the structural-machines branch May 22, 2020 12:56

rainest mentioned this pull request Dec 2, 2021

Add KongIngress types for generated CRDs Kong/kubernetes-ingress-controller#1971

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BUG 1769004: Make Machine/MachineSets structural #552

BUG 1769004: Make Machine/MachineSets structural #552

JoelSpeed commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 2020

enxebre Apr 7, 2020

enxebre Apr 7, 2020

JoelSpeed Apr 7, 2020

enxebre Apr 7, 2020

JoelSpeed Apr 14, 2020

sttts Apr 21, 2020

JoelSpeed Apr 21, 2020

sttts Apr 22, 2020

JoelSpeed Apr 22, 2020

enxebre Apr 7, 2020

JoelSpeed commented Apr 20, 2020

sttts Apr 21, 2020

enxebre commented Apr 22, 2020

openshift-ci-robot commented Apr 22, 2020

JoelSpeed commented Apr 22, 2020

JoelSpeed commented Apr 23, 2020

JoelSpeed commented Apr 23, 2020

elmiko commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-ci-robot commented Apr 24, 2020 •

edited

openshift-bot commented Apr 24, 2020

openshift-ci-robot commented Apr 24, 2020

BUG 1769004: Make Machine/MachineSets structural #552

BUG 1769004: Make Machine/MachineSets structural #552

Conversation

JoelSpeed commented Apr 6, 2020

openshift-ci-robot commented Apr 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JoelSpeed commented Apr 20, 2020

Choose a reason for hiding this comment

enxebre commented Apr 22, 2020

openshift-ci-robot commented Apr 22, 2020

JoelSpeed commented Apr 22, 2020

JoelSpeed commented Apr 23, 2020

JoelSpeed commented Apr 23, 2020

elmiko commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-bot commented Apr 24, 2020

openshift-ci-robot commented Apr 24, 2020 • edited

openshift-bot commented Apr 24, 2020

openshift-ci-robot commented Apr 24, 2020

openshift-ci-robot commented Apr 24, 2020 •

edited