Bug 1825330: support creating v1beta CRDs to avoid data loss #1470

exdx · 2020-04-21T00:23:11Z

Description of the change:
Supporting v1 CRDs originally intended to create and update all CRDs at v1 - converting those provided at v1beta1. However, due to some issues around the validation, and the fact that CRDs are not fully backward-compatible, OLM will have to create v1beta1 CRDs with the v1beta1 client. This PR adds back support for creating v1beta1 CRDs.

Motivation for the change:

Reviewer Checklist

Implementation matches the proposed design, or proposal is updated to match implementation
Sufficient unit test coverage
Sufficient end-to-end test coverage
Docs updated or added to /docs
Commit messages sensible and descriptive

openshift-ci-robot · 2020-04-21T00:23:38Z

@exdx: This pull request references Bugzilla bug 1825330, 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 1825330: support creating v1beta CRDs to avoid data loss

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.

njhale · 2020-04-21T13:46:01Z

It would be nice to have a test that reproduces the issue so we can be certain of the cause and catch it in the future.

njhale · 2020-04-21T13:59:51Z

Putting this here for record keeping purposes: kubernetes/kubernetes#87231

sttts · 2020-04-22T13:16:55Z

/hold

Here is so much wrong about conversion.

pkg/lib/crd/serialize.go

exdx · 2020-04-22T13:44:02Z

Closing this in favor of adding a v1beta1 codepath (something we had originally done in prior versions of the v1 support PR). There will be no client-side conversion between v1beta1 and v1 CRD types in OLM.

openshift-ci-robot · 2020-04-22T14:37:16Z

@exdx: This pull request references Bugzilla bug 1825330, which is valid.

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 POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1825330: support creating v1beta CRDs to avoid data loss

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.

Replace hub and spoke CRD conversion with direct unmarshaling to v1. This follows advice to avoid the internal k8s types and conversions client-side (see operator-framework/operator-lifecycle-manager#1470 (comment)).

sttts · 2020-04-27T14:56:11Z

pkg/controller/operators/catalog/operator.go

+		return nil
+	}
+	convertedCRD := &apiextensions.CustomResourceDefinition{}
+	if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {


here is still a conversion.

Here we convert to the internal version to do static schema validation. Specifically, the NewSchemaValidator which creates an openapi schema validator for the given CRD validation, expects an internal CRD type to be provided to it, so OLM does the conversion here when validating the new CRD.

The internal CRD is never written to the cluster again. We can make a follow-up BZ to use dry-run APIs instead of using the internal validation package, if necessary.

Yes, would prefer dry-run (follow-up). The schema validator is also a component considered private.

How can we dry-run against an CRD API that has not been applied to the cluster yet?

The goal is to catch the mistake before the CRD has been created.

There seems to be little practical issue with using the internal types, correct? We could just as easily read the json schema from the CRD spec and use an off-the-shelf schema validator to accomplish the same goals.

pkg/controller/operators/catalog/step.go

pkg/lib/crd/version.go

test/e2e/util_test.go

…n to v1. Due to data loss during client side conversions OLM will support two different paths for v1 and v1beta1 CRDs.

pkg/controller/operators/catalog/operator.go

ecordell · 2020-04-27T20:51:39Z

pkg/controller/operators/catalog/operator.go

+		return nil
+	}
+	convertedCRD := &apiextensions.CustomResourceDefinition{}
+	if err := apiextensionsv1beta1.Convert_v1beta1_CustomResourceDefinition_To_apiextensions_CustomResourceDefinition(newCRD, convertedCRD, nil); err != nil {


How can we dry-run against an CRD API that has not been applied to the cluster yet?

The goal is to catch the mistake before the CRD has been created.

There seems to be little practical issue with using the internal types, correct? We could just as easily read the json schema from the CRD spec and use an off-the-shelf schema validator to accomplish the same goals.

exdx · 2020-04-28T02:28:56Z

/test e2e-gcp

exdx · 2020-04-28T11:27:31Z

/test e2e-gcp

exdx · 2020-04-28T13:12:04Z

/test e2e-gcp

ecordell

/lgtm

ecordell · 2020-04-28T13:23:53Z

pkg/lib/crd/unmarshal.go

+	return crd, nil
+}
+
+// UnmarshalV1 takes in a CRD manifest and returns a v1beta1 versioned CRD object.


Suggested change

// UnmarshalV1 takes in a CRD manifest and returns a v1beta1 versioned CRD object.

// UnmarshalV1Beta1 takes in a CRD manifest and returns a v1beta1 versioned CRD object.

benluddy · 2020-04-28T16:14:50Z

/approve

openshift-ci-robot · 2020-04-28T16:15:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: benluddy, exdx

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

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

openshift-ci-robot · 2020-04-28T16:18:43Z

@exdx: All pull requests linked via external trackers have merged: operator-framework/operator-lifecycle-manager#1470. Bugzilla bug 1825330 has been moved to the MODIFIED state.

In response to this:

Bug 1825330: support creating v1beta CRDs to avoid data loss

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.

Replace hub and spoke CRD conversion with direct unmarshaling to v1. This follows advice to avoid the internal k8s types and conversions client-side (see operator-framework/operator-lifecycle-manager#1470 (comment)).

Replace hub and spoke CRD conversion with direct unmarshaling to v1. This follows advice to avoid the internal k8s types and conversions client-side (see operator-framework/operator-lifecycle-manager#1470 (comment)). (cherry picked from commit 7d6ac12ec8bc5a864646bae56bf80d138c922b61)

Replace hub and spoke CRD conversion with direct unmarshaling to v1. This follows advice to avoid the internal k8s types and conversions client-side (see operator-framework/operator-lifecycle-manager#1470 (comment)). (upstream api commit: 7d6ac12ec8bc5a864646bae56bf80d138c922b61)

Replace hub and spoke CRD conversion with direct unmarshaling to v1. This follows advice to avoid the internal k8s types and conversions client-side (see operator-framework/operator-lifecycle-manager#1470 (comment)). Upstream-repository: api Upstream-commit: 7d6ac12ec8bc5a864646bae56bf80d138c922b61

exdx requested review from ecordell and njhale April 21, 2020 00:23

openshift-ci-robot requested review from anik120 and dinhxuanvu April 21, 2020 00:23

exdx changed the title ~~fix: support creating v1beta CRDs to avoid data loss~~ Bug 1825330: support creating v1beta CRDs to avoid data loss Apr 21, 2020

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

exdx force-pushed the fix/bug-1825330 branch from 13817b7 to 9b3ea60 Compare April 21, 2020 14:59

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 22, 2020

sttts reviewed Apr 22, 2020

View reviewed changes

pkg/lib/crd/serialize.go Outdated Show resolved Hide resolved

sttts reviewed Apr 22, 2020

View reviewed changes

pkg/lib/crd/serialize.go Outdated Show resolved Hide resolved

sttts reviewed Apr 22, 2020

View reviewed changes

pkg/lib/crd/serialize.go Outdated Show resolved Hide resolved

exdx closed this Apr 22, 2020

exdx reopened this Apr 22, 2020

exdx force-pushed the fix/bug-1825330 branch from 5722705 to 30f4a34 Compare April 22, 2020 14:39

tiraboschi mentioned this pull request Apr 22, 2020

Fix CI for klog/v2 kubevirt/hyperconverged-cluster-operator#544

Merged

exdx force-pushed the fix/bug-1825330 branch from 30f4a34 to 27317f6 Compare April 22, 2020 18:07

This was referenced Apr 22, 2020

OCP 4.5 related fixes kubevirt/hyperconverged-cluster-operator#536

Closed

Bump various go dependencies to consume recent fixes kubevirt/hyperconverged-cluster-operator#500

Merged

Bump NMO to 0.5.0 kubevirt/hyperconverged-cluster-operator#534

Merged

exdx force-pushed the fix/bug-1825330 branch from 27317f6 to b9896f0 Compare April 22, 2020 20:49

This was referenced Apr 23, 2020

Bump SSP to 1.0.30 kubevirt/hyperconverged-cluster-operator#535

Merged

disable kubemacpool kubevirt/hyperconverged-cluster-operator#539

Merged

sttts reviewed Apr 27, 2020

View reviewed changes

pkg/controller/operators/catalog/step.go Outdated Show resolved Hide resolved

sttts reviewed Apr 27, 2020

View reviewed changes

pkg/lib/crd/version.go Outdated Show resolved Hide resolved

sttts reviewed Apr 27, 2020

View reviewed changes

test/e2e/util_test.go Outdated Show resolved Hide resolved

fix: support creating v1beta CRDs to avoid data loss during conversio…

4e6ed58

…n to v1. Due to data loss during client side conversions OLM will support two different paths for v1 and v1beta1 CRDs.

exdx force-pushed the fix/bug-1825330 branch 2 times, most recently from ea2997d to 5f054ee Compare April 27, 2020 20:12

ecordell reviewed Apr 27, 2020

View reviewed changes

fix: address sttts feedback

695c67f

exdx force-pushed the fix/bug-1825330 branch from 5f054ee to 695c67f Compare April 27, 2020 21:14

ecordell mentioned this pull request Apr 27, 2020

How often releases cut? #1415

Closed

ecordell reviewed Apr 28, 2020

View reviewed changes

openshift-ci-robot assigned ecordell Apr 28, 2020

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

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

openshift-merge-robot merged commit 15f5ba3 into operator-framework:master Apr 28, 2020

tiraboschi mentioned this pull request Apr 29, 2020

use pre-prepared image for kubevirtci tests kubevirt/hyperconverged-cluster-operator#550

Merged

tiraboschi mentioned this pull request Apr 29, 2020

Introducing kustomize for HCO deployment kubevirt/hyperconverged-cluster-operator#463

Merged

	// UnmarshalV1 takes in a CRD manifest and returns a v1beta1 versioned CRD object.
	// UnmarshalV1Beta1 takes in a CRD manifest and returns a v1beta1 versioned CRD object.

Bug 1825330: support creating v1beta CRDs to avoid data loss #1470

Bug 1825330: support creating v1beta CRDs to avoid data loss #1470

Uh oh!

Conversation

exdx commented Apr 21, 2020

Uh oh!

openshift-ci-robot commented Apr 21, 2020

Uh oh!

njhale commented Apr 21, 2020

Uh oh!

njhale commented Apr 21, 2020

Uh oh!

sttts commented Apr 22, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

exdx commented Apr 22, 2020

Uh oh!

openshift-ci-robot commented Apr 22, 2020

Uh oh!

sttts Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

exdx Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

sttts Apr 27, 2020

Choose a reason for hiding this comment

Uh oh!

ecordell Apr 27, 2020 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ecordell Apr 27, 2020 • 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!

exdx commented Apr 28, 2020

Uh oh!

exdx commented Apr 28, 2020

Uh oh!

exdx commented Apr 28, 2020

Uh oh!

ecordell left a comment

Choose a reason for hiding this comment

Uh oh!

ecordell Apr 28, 2020

Choose a reason for hiding this comment

Uh oh!

benluddy commented Apr 28, 2020

Uh oh!

openshift-ci-robot commented Apr 28, 2020

Uh oh!

openshift-ci-robot commented Apr 28, 2020

Uh oh!

Uh oh!

ecordell Apr 27, 2020 •

edited

Loading

ecordell Apr 27, 2020 •

edited

Loading