Enable OLM to update CRD when there is only one owner of that CRD #878

dinhxuanvu · 2019-05-31T13:41:39Z

Signed-off-by: Vu Dinh vdinh@redhat.com

pkg/controller/operators/catalog/operator.go

dinhxuanvu · 2019-06-05T15:02:46Z

/retest

pkg/controller/operators/catalog/operator.go

dinhxuanvu · 2019-06-07T06:13:57Z

/retest

dinhxuanvu · 2019-06-11T19:31:46Z

Ready for final review and approval for merging :)

njhale

Looking really good! Have a few comments that are mostly non-blocking.

pkg/controller/operators/catalog/operator.go

njhale · 2019-06-13T12:13:35Z

pkg/lib/index/api.go

+func ProvidedAPIsIndexFunc(obj interface{}) ([]string, error) {
+	indicies := []string{}
+
+	csv, ok := obj.(*v1alpha1.ClusterServiceVersion)


Non-blocking: this is probably a good place to use OLM's internal versions. If #881 merges first, that could look like:

import ( // ... "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators" "github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/install" // ... ) // ... var scheme = runtime.NewScheme() func init() { // Register internal types and conversion funcs install.Install(scheme) } // ... func ProvidedAPIsIndexFunc(obj interface{}) ([]string, error) { var indices []string csv := &operators.ClusterServiceVersion{} if err := scheme.Convert(obj, csv, nil); err != nil { return indices, fmt.Errorf("cannot generate indices for object of type %T", obj) } // ... } // ...

njhale · 2019-06-13T12:15:02Z

pkg/lib/index/api.go

@@ -0,0 +1,66 @@
+package indexer


Not-blocking: we should probably move all of the OLM specific stuff out of lib.

pkg/lib/index/api.go

njhale · 2019-06-13T13:26:02Z

pkg/lib/index/api.go

+		if len(parts) < 2 {
+			return indicies, fmt.Errorf("couldn't parse plural.group from crd name: %s", crd.Name)
+		}
+		indicies = append(indicies, fmt.Sprintf("provided=%s/%s/%s", parts[1], crd.Version, crd.Kind))


nit: I don't think "provided=" is necessary.

njhale · 2019-06-13T13:27:18Z

pkg/lib/index/api.go

+}
+
+// APIsIndexValues returns the names of CSVs that own the given CRD
+func APIsIndexValues(indexers map[string]cache.Indexer, crd v1beta1ext.CustomResourceDefinition) (map[string]struct{}, error) {


nit: CRDProviderNames(...)?

njhale · 2019-06-13T13:59:51Z

test/e2e/installplan_e2e_test.go

+			expectedCRDVersions[key] = struct{}{}
+		}
+
+		mainCSV := newCSV(mainPackageStable, testNamespace, "", semver.MustParse("0.1.0"), []apiextensions.CustomResourceDefinition{mainCRD}, nil, mainNamedStrategy)


Is it possible to build a catalog with both CRDs and have the new CSV replace the original instead of deleting/recreating the subscription?

njhale · 2019-06-13T14:01:03Z

test/e2e/installplan_e2e_test.go

@@ -641,6 +642,176 @@ func TestCreateInstallPlanWithPreExistingCRDOwners(t *testing.T) {
 	})
 }

+func TestUpdateInstallPlan(t *testing.T) {
+	defer cleaner.NotifyTestComplete(t, true)
+	t.Run("UpdateSingleExistingCRDOwner", func(t *testing.T) {


Nice! I think we also want a negative test-case; i.e. CRD upgrade is blocked from a pre-existing owner.

ecordell · 2019-06-17T13:50:45Z

test/e2e/installplan_e2e_test.go

+		fetchedCRD, err := c.ApiextensionsV1beta1Interface().ApiextensionsV1beta1().CustomResourceDefinitions().Get(crdName, metav1.GetOptions{})
+		require.NoError(t, err)
+
+		for _, version := range fetchedCRD.Spec.Versions {


This checks that every version in fectchedCRD is in expectedCRDVersions, but it doesn't check that every version in expectedCRDVersions is in fechedCRD.

(in particulur, both mainCRD and updatedCRD will pass this check since they both contain v1alpha1)

I think you could probably just do require.Equal(t, updatedCRD.Spec.Versions, fetchedCRD.Spec.Versions) to ensure the lists are equal

require.EqualElements ignores ordering if that's needed.

dinhxuanvu · 2019-06-18T11:58:49Z

/retest

dinhxuanvu · 2019-06-18T14:31:21Z

/retest

dinhxuanvu · 2019-06-18T18:35:33Z

/retest

Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu · 2019-06-19T12:24:33Z

/retest

jpeeler · 2019-06-20T19:20:47Z

/lgtm

openshift-ci-robot · 2019-06-20T19:20:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dinhxuanvu, jpeeler

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

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

senorhesles · 2021-10-21T14:27:50Z

Which release did this fix initially make it into?

openshift-ci-robot requested review from alecmerdler and njhale May 31, 2019 13:41

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label May 31, 2019

ecordell reviewed May 31, 2019

View reviewed changes

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

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 4, 2019

jpeeler reviewed Jun 6, 2019

View reviewed changes

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

jpeeler reviewed Jun 6, 2019

View reviewed changes

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

dinhxuanvu force-pushed the update-crd-one branch from b8951c6 to 014856a Compare June 6, 2019 22:16

dinhxuanvu force-pushed the update-crd-one branch 3 times, most recently from d6590c3 to dbe3059 Compare June 11, 2019 08:35

jpeeler mentioned this pull request Jun 11, 2019

Allow CRD updates with multiple owners #902

Merged

dinhxuanvu force-pushed the update-crd-one branch from dbe3059 to 944a940 Compare June 11, 2019 19:28

njhale reviewed Jun 13, 2019

View reviewed changes

dinhxuanvu force-pushed the update-crd-one branch 4 times, most recently from 8d37369 to dad2344 Compare June 17, 2019 10:50

ecordell reviewed Jun 17, 2019

View reviewed changes

dinhxuanvu force-pushed the update-crd-one branch from dad2344 to 56d199f Compare June 17, 2019 18:27

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2019

dinhxuanvu force-pushed the update-crd-one branch from 56d199f to 2c43d2e Compare June 17, 2019 19:01

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2019

dinhxuanvu force-pushed the update-crd-one branch 2 times, most recently from 595e920 to 61f9566 Compare June 17, 2019 20:55

dinhxuanvu added 3 commits June 18, 2019 16:44

Enable OLM to update CRD when there is only one owner of that CRD

8cec21a

Signed-off-by: Vu Dinh <vdinh@redhat.com>

Add IndexFunc using ProvidedAPIs as key to look up CSVs

924c565

Signed-off-by: Vu Dinh <vdinh@redhat.com>

Add e2e test case for single owner CRD update

24c59dc

Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu force-pushed the update-crd-one branch 3 times, most recently from 18703fc to fededd3 Compare June 18, 2019 22:00

Add a new test case to cover preexisting CRD case

930163c

Signed-off-by: Vu Dinh <vdinh@redhat.com>

dinhxuanvu force-pushed the update-crd-one branch from fededd3 to 930163c Compare June 19, 2019 05:15

openshift-ci-robot assigned jpeeler Jun 20, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 20, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2019

openshift-merge-robot merged commit b234fe7 into operator-framework:master Jun 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable OLM to update CRD when there is only one owner of that CRD #878

Enable OLM to update CRD when there is only one owner of that CRD #878

dinhxuanvu commented May 31, 2019

dinhxuanvu commented Jun 5, 2019

dinhxuanvu commented Jun 7, 2019

dinhxuanvu commented Jun 11, 2019

njhale left a comment

njhale Jun 13, 2019

njhale Jun 13, 2019

njhale Jun 13, 2019

njhale Jun 13, 2019

njhale Jun 13, 2019

njhale Jun 13, 2019

ecordell Jun 17, 2019

njhale Jun 18, 2019

dinhxuanvu commented Jun 18, 2019

dinhxuanvu commented Jun 18, 2019

dinhxuanvu commented Jun 18, 2019

dinhxuanvu commented Jun 19, 2019

jpeeler commented Jun 20, 2019

openshift-ci-robot commented Jun 20, 2019

senorhesles commented Oct 21, 2021

Enable OLM to update CRD when there is only one owner of that CRD #878

Enable OLM to update CRD when there is only one owner of that CRD #878

Conversation

dinhxuanvu commented May 31, 2019

dinhxuanvu commented Jun 5, 2019

dinhxuanvu commented Jun 7, 2019

dinhxuanvu commented Jun 11, 2019

njhale left a 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

dinhxuanvu commented Jun 18, 2019

dinhxuanvu commented Jun 18, 2019

dinhxuanvu commented Jun 18, 2019

dinhxuanvu commented Jun 19, 2019

jpeeler commented Jun 20, 2019

openshift-ci-robot commented Jun 20, 2019

senorhesles commented Oct 21, 2021