From 422b134b8c38a54faf553a20a2620a92afc8b37f Mon Sep 17 00:00:00 2001 From: njhale Date: Sun, 27 Jan 2019 13:41:29 -0500 Subject: [PATCH 1/3] fix(subscriptions): fix race between subscription sync and cache Adds a "generated-by" annotation to subscriptions generated for requiredAPIs that contains the name of the generating installplan. If the "generated-by" installplan is not present in the cache at the generated subscription's sync time, the subscription is resynced until it is. --- go.mod | 2 +- go.sum | 6 +-- pkg/controller/operators/catalog/operator.go | 47 ++++++++++--------- .../registry/reconciler/configmap.go | 4 +- test/e2e/installplan_e2e_test.go | 1 + test/e2e/util_test.go | 4 +- 6 files changed, 35 insertions(+), 29 deletions(-) diff --git a/go.mod b/go.mod index f0671512df..0d49224916 100644 --- a/go.mod +++ b/go.mod @@ -17,7 +17,7 @@ require ( github.com/google/gofuzz v0.0.0-20170612174753-24818f796faf // indirect github.com/googleapis/gnostic v0.2.0 // indirect github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 // indirect - github.com/grpc-ecosystem/grpc-gateway v1.6.3 // indirect + github.com/grpc-ecosystem/grpc-gateway v1.6.3 github.com/imdario/mergo v0.3.6 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect github.com/json-iterator/go v1.1.5 // indirect diff --git a/go.sum b/go.sum index 93091fd130..00d3f82bfc 100644 --- a/go.sum +++ b/go.sum @@ -21,7 +21,7 @@ github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDk github.com/coreos/bbolt v1.3.0 h1:HIgH5xUWXT914HCI671AxuTTqjj64UOFr7pHn48LUTI= github.com/coreos/bbolt v1.3.0/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/etcd v3.3.9+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= -github.com/coreos/etcd v3.3.10+incompatible h1:jFneRYjIvLMLhDLCzuTuU4rSJUjRplcJQ7pD7MnhC04= +github.com/coreos/etcd v3.3.10+incompatible h1:KjVWqrZ5U0wa3CxY2AxlH6/UcB+PK2td1DcsYhA+HRs= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/go-semver v0.2.0 h1:3Jm3tLmsgAYcjC+4Up7hJrFBPr+n7rAqYeSw/SZazuY= github.com/coreos/go-semver v0.2.0/go.mod h1:nnelYz7RCh+5ahJtPPxZlU+153eP4D4r3EedlOD2RNk= @@ -107,7 +107,7 @@ github.com/gorilla/websocket v1.4.0/go.mod h1:E7qHFY5m1UJ88s3WnNqhKjPHQ0heANvMoA github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA= github.com/gregjones/httpcache v0.0.0-20181110185634-c63ab54fda8f h1:ShTPMJQes6tubcjzGMODIVG5hlrCeImaBnZzKF2N8SM= github.com/gregjones/httpcache v0.0.0-20181110185634-c63ab54fda8f/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA= -github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:Iju5GlWwrvL6UBg4zJJt3btmonfrMlCDdsejg4CZE7c= +github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:BWIsLfhgKhV5g/oF34aRjniBHLTZe5DNekSjbAjIS6c= github.com/grpc-ecosystem/go-grpc-middleware v1.0.0/go.mod h1:FiyG127CGDf3tlThmgyCl78X/SZQqEOJBCDaAfeWzPs= github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 h1:Ovs26xHkKqVztRpIrF/92BcuyuQ/YW4NSIpoGtfXNho= github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0/go.mod h1:8NvIoxWQoOIhqOTXgfV/d3M/q6VIi02HzZEHgUlZvzk= @@ -294,5 +294,5 @@ k8s.io/kube-aggregator v0.0.0-20181204002017-122bac39d429/go.mod h1:8sbzT4QQKDEm k8s.io/kube-openapi v0.0.0-20181031203759-72693cb1fadd h1:ggv/Vfza0i5xuhUZyYyxcc25AmQvHY8Zi1C2m8WgBvA= k8s.io/kube-openapi v0.0.0-20181031203759-72693cb1fadd/go.mod h1:BXM9ceUBTj2QnfH2MK1odQs778ajze1RxcmP6S8RVVc= k8s.io/kubernetes v1.11.7-beta.0.0.20181219023948-b875d52ea96d/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk= -k8s.io/kubernetes v1.11.8-beta.0.0.20190124204751-3a10094374f2 h1:CzIOMOEjH+sQw35LY1Gl0jwthkyOojzaq2HIeYZYOrM= +k8s.io/kubernetes v1.11.8-beta.0.0.20190124204751-3a10094374f2 h1:Q4hIsjqTbRprTaPk+gVDUuVipXpGJtTz7Lg2FS3xpmw= k8s.io/kubernetes v1.11.8-beta.0.0.20190124204751-3a10094374f2/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk= diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 46e8f87d9d..0acf0cb11b 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -47,6 +47,8 @@ const ( serviceKind = "Service" roleKind = "Role" roleBindingKind = "RoleBinding" + + generatedByKey = "olm/generated-by" ) // for test stubbing and for ensuring standardization of timezones to UTC @@ -675,33 +677,29 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub // check if there's an installplan that created this subscription (only if it doesn't have a reference yet) // this indicates it was newly resolved by another operator, and we should reference that installplan in the status - ips, err := o.lister.OperatorsV1alpha1().InstallPlanLister().InstallPlans(sub.GetNamespace()).List(labels.Everything()) - if err != nil { - logger.WithError(err).Debug("couldn't get installplans") - // if we can't list, just continue processing + ipName, ok := sub.GetAnnotations()[generatedByKey] + if !ok { + // err := fmt.Errorf("no installplan reference or %s annotation found", generatedByKey) + // logger.WithField("err", err.Error()).Error("an error occurred while associating a subscription with an installplan") return sub, nil } - out := sub.DeepCopy() + ip, err := o.lister.OperatorsV1alpha1().InstallPlanLister().InstallPlans(sub.GetNamespace()).Get(ipName) + if err != nil { + logger.WithField("installplan", ipName).Warn("unable to get installplan from cache") + return nil, err + } + logger.WithField("installplan", ipName).Debug("found installplan that generated subscription") - for _, ip := range ips { - for _, step := range ip.Status.Plan { - // TODO: is this enough? should we check equality of pkg/channel? - if step != nil && step.Resource.Kind == v1alpha1.SubscriptionKind && step.Resource.Name == sub.GetName() { - logger.WithField("installplan", ip.GetName()).Debug("found subscription in steps of existing installplan") - out.Status.Install = o.referenceForInstallPlan(ip) - out.Status.State = v1alpha1.SubscriptionStateUpgradePending - if updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(out); err != nil { - return nil, err - } else { - return updated, nil - } - } - } + out := sub.DeepCopy() + out.Status.Install = o.referenceForInstallPlan(ip) + out.Status.State = v1alpha1.SubscriptionStateUpgradePending + updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(out) + if err != nil { + return nil, err } - logger.Debug("did not find subscription in steps of existing installplan") - return sub, nil + return updated, nil } func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, error) { @@ -1001,6 +999,13 @@ func (o *Operator) ExecutePlan(plan *v1alpha1.InstallPlan) error { return errorwrap.Wrapf(err, "error parsing step manifest: %s", step.Resource.Name) } + // Add the InstallPlan's name as an annotation + if annotations := sub.GetAnnotations(); annotations != nil { + annotations[generatedByKey] = plan.GetName() + } else { + sub.SetAnnotations(map[string]string{generatedByKey: plan.GetName()}) + } + // Attempt to create the Subscription sub.SetNamespace(namespace) _, err = o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).Create(&sub) diff --git a/pkg/controller/registry/reconciler/configmap.go b/pkg/controller/registry/reconciler/configmap.go index f4708bd6dd..07cdaaf306 100644 --- a/pkg/controller/registry/reconciler/configmap.go +++ b/pkg/controller/registry/reconciler/configmap.go @@ -106,7 +106,7 @@ func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod { Command: []string{"grpc_health_probe", "-addr=localhost:50051"}, }, }, - InitialDelaySeconds: 5, + InitialDelaySeconds: 1, }, LivenessProbe: &v1.Probe{ Handler: v1.Handler{ @@ -114,7 +114,7 @@ func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod { Command: []string{"grpc_health_probe", "-addr=localhost:50051"}, }, }, - InitialDelaySeconds: 10, + InitialDelaySeconds: 2, }, }, }, diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index 55aaaa145f..91a183f6ca 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -567,6 +567,7 @@ func TestCreateInstallPlanWithPreExistingCRDOwners(t *testing.T) { // existing cleanup should remove this createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, mainCatalogSourceName, mainPackageName, betaChannel, v1alpha1.ApprovalAutomatic) + // time.Sleep(5 * time.Minute) subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionHasInstallPlanChecker) require.NoError(t, err) require.NotNil(t, subscription) diff --git a/test/e2e/util_test.go b/test/e2e/util_test.go index 4e32324360..3047c3cc91 100644 --- a/test/e2e/util_test.go +++ b/test/e2e/util_test.go @@ -237,7 +237,7 @@ func createFieldNotEqualSelector(field string, names ...string) string { func cleanupOLM(t *testing.T, namespace string) { var immediate int64 = 0 crc := newCRClient(t) - //c := newKubeClient(t) + c := newKubeClient(t) // Cleanup non persistent OLM CRs t.Log("cleaning up any remaining non persistent resources...") @@ -250,7 +250,7 @@ func cleanupOLM(t *testing.T, namespace string) { // error: the server does not allow this method on the requested resource // Cleanup non persistent configmaps - //require.NoError(t, c.KubernetesInterface().CoreV1().ConfigMaps(namespace).DeleteCollection(deleteOptions, metav1.ListOptions{FieldSelector: nonPersistentConfigMapsFieldSelector})) + require.NoError(t, c.KubernetesInterface().CoreV1().Pods(namespace).DeleteCollection(deleteOptions, metav1.ListOptions{})) } func buildCatalogSourceCleanupFunc(t *testing.T, crc versioned.Interface, namespace string, catalogSource *v1alpha1.CatalogSource) cleanupFunc { From 316612a87d342c264cf9262bc5dc8ae2e1b18d6a Mon Sep 17 00:00:00 2001 From: njhale Date: Tue, 29 Jan 2019 10:29:25 -0500 Subject: [PATCH 2/3] fix(e2e): ensure all subscriptions are deleted in replacing test --- test/e2e/installplan_e2e_test.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/test/e2e/installplan_e2e_test.go b/test/e2e/installplan_e2e_test.go index 91a183f6ca..42a81c5a57 100644 --- a/test/e2e/installplan_e2e_test.go +++ b/test/e2e/installplan_e2e_test.go @@ -532,7 +532,7 @@ func TestCreateInstallPlanWithPreExistingCRDOwners(t *testing.T) { fetchedInstallPlan, err := fetchInstallPlan(t, crc, installPlanName, completeOrFailedFunc) require.NoError(t, err) t.Logf("Install plan %s fetched with status %s", fetchedInstallPlan.GetName(), fetchedInstallPlan.Status.Phase) - require.True(t, completeOrFailedFunc(fetchedInstallPlan)) + require.Equal(t, v1alpha1.InstallPlanPhaseComplete, fetchedInstallPlan.Status.Phase) // Ensure that the desired resources have been created expectedSteps := map[registry.ResourceKey]struct{}{ @@ -561,13 +561,12 @@ func TestCreateInstallPlanWithPreExistingCRDOwners(t *testing.T) { require.Equal(t, 0, len(expectedSteps), "Actual resource steps do not match expected") // Update the subscription resource to point to the beta CSV - err = crc.OperatorsV1alpha1().Subscriptions(testNamespace).Delete(subscriptionName, metav1.NewDeleteOptions(0)) + err = crc.OperatorsV1alpha1().Subscriptions(testNamespace).DeleteCollection(metav1.NewDeleteOptions(0), metav1.ListOptions{}) require.NoError(t, err) // existing cleanup should remove this createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, mainCatalogSourceName, mainPackageName, betaChannel, v1alpha1.ApprovalAutomatic) - // time.Sleep(5 * time.Minute) subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionHasInstallPlanChecker) require.NoError(t, err) require.NotNil(t, subscription) From d8999685991f0ba15ea2d6dda45297f49d6f57b1 Mon Sep 17 00:00:00 2001 From: njhale Date: Tue, 29 Jan 2019 13:21:35 -0500 Subject: [PATCH 3/3] fix(subscriptions): add check for csv manifest dif before applying new installplans --- Makefile | 1 + go.mod | 5 ++- go.sum | 6 +++ .../operators/v1alpha1/installplan_types.go | 37 +++++++++++++++++++ pkg/controller/operators/catalog/operator.go | 31 +++++++++++++--- .../operators/catalog/operator_test.go | 5 ++- pkg/lib/operatorclient/mock_client.go | 2 +- .../github.com/golang/groupcache/lru/lru.go | 2 +- vendor/modules.txt | 6 +-- 9 files changed, 82 insertions(+), 13 deletions(-) diff --git a/Makefile b/Makefile index aae8017e77..0fa054d17e 100644 --- a/Makefile +++ b/Makefile @@ -153,6 +153,7 @@ container-mockgen: docker cp temp-mockgen:/go/src/github.com/operator-framework/operator-lifecycle-manager/pkg/fakes/client-go/listers/. ./pkg/fakes/client-go/listers docker cp temp-mockgen:/go/src/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes/. ./pkg/lib/operatorlister/operatorlisterfakes docker cp temp-mockgen:/go/src/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient/mock_client.go ./pkg/lib/operatorclient/mock_client.go + docker cp temp-mockgen:/go/src/github.com/operator-framework/operator-lifecycle-manager/pkg/fakes/. ./pkg/fakes/. docker rm temp-mockgen # Must be run in gopath: https://github.com/kubernetes/kubernetes/issues/67566 diff --git a/go.mod b/go.mod index 0d49224916..7ba625f81f 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,7 @@ module github.com/operator-framework/operator-lifecycle-manager require ( + github.com/coreos/bbolt v1.3.2 // indirect github.com/coreos/etcd v3.3.10+incompatible // indirect github.com/coreos/go-semver v0.2.0 github.com/coreos/go-systemd v0.0.0-20181031085051-9002847aa142 // indirect @@ -12,12 +13,13 @@ require ( github.com/go-openapi/strfmt v0.18.0 // indirect github.com/go-openapi/validate v0.18.0 // indirect github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b + github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef // indirect github.com/golang/mock v1.1.1 github.com/google/btree v0.0.0-20180813153112-4030bb1f1f0c // indirect github.com/google/gofuzz v0.0.0-20170612174753-24818f796faf // indirect github.com/googleapis/gnostic v0.2.0 // indirect github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 // indirect - github.com/grpc-ecosystem/grpc-gateway v1.6.3 + github.com/grpc-ecosystem/grpc-gateway v1.6.3 // indirect github.com/imdario/mergo v0.3.6 // indirect github.com/inconshreveable/mousetrap v1.0.0 // indirect github.com/json-iterator/go v1.1.5 // indirect @@ -37,6 +39,7 @@ require ( github.com/tmc/grpc-websocket-proxy v0.0.0-20190109142713-0ad062ec5ee5 // indirect github.com/ugorji/go/codec v0.0.0-20181022190402-e5e69e061d4f // indirect github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 // indirect + go.etcd.io/bbolt v1.3.2 // indirect go.uber.org/atomic v1.3.2 // indirect go.uber.org/multierr v1.1.0 // indirect go.uber.org/zap v1.9.1 // indirect diff --git a/go.sum b/go.sum index 00d3f82bfc..5f616507d4 100644 --- a/go.sum +++ b/go.sum @@ -20,6 +20,8 @@ github.com/boltdb/bolt v1.3.1/go.mod h1:clJnj/oiGkjum5o1McbSZDSLxVThjynRyGBgiAx2 github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDkc90ppPyw= github.com/coreos/bbolt v1.3.0 h1:HIgH5xUWXT914HCI671AxuTTqjj64UOFr7pHn48LUTI= github.com/coreos/bbolt v1.3.0/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= +github.com/coreos/bbolt v1.3.2 h1:wZwiHHUieZCquLkDL0B8UhzreNWsPHooDAG3q34zk0s= +github.com/coreos/bbolt v1.3.2/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk= github.com/coreos/etcd v3.3.9+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= github.com/coreos/etcd v3.3.10+incompatible h1:KjVWqrZ5U0wa3CxY2AxlH6/UcB+PK2td1DcsYhA+HRs= github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE= @@ -89,6 +91,8 @@ github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b/go.mod h1:SBH7ygxi8pfU github.com/golang/groupcache v0.0.0-20180924190550-6f2cf27854a4/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/groupcache v0.0.0-20181024230925-c65c006176ff h1:kOkM9whyQYodu09SJ6W3NCsHG7crFaJILQ22Gozp3lg= github.com/golang/groupcache v0.0.0-20181024230925-c65c006176ff/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= +github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef h1:veQD95Isof8w9/WXiA+pa3tz3fJXkt5B7QaRBrM62gk= +github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef/go.mod h1:cIg4eruTrX1D+g88fzRXU5OdNfaM+9IcxsU14FzY7Hc= github.com/golang/lint v0.0.0-20180702182130-06c8688daad7/go.mod h1:tluoj9z5200jBnyusfRPU2LqT6J+DAorxEvtC7LHB+E= github.com/golang/mock v1.1.1 h1:G5FRp8JnTd7RQH5kemVNlMeyXQAztQ3mOWV95KxsXH8= github.com/golang/mock v1.1.1/go.mod h1:oTYuIxOrZwtPieC+H1uAHpcLFnEyAGVDL/k47Jfbm0A= @@ -206,6 +210,8 @@ github.com/ugorji/go/codec v0.0.0-20181022190402-e5e69e061d4f/go.mod h1:VFNgLljT github.com/xiang90/probing v0.0.0-20160813154853-07dd2e8dfe18/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU= github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2 h1:eY9dn8+vbi4tKz5Qo6v2eYzo7kUS51QINcR5jNpbZS8= github.com/xiang90/probing v0.0.0-20190116061207-43a291ad63a2/go.mod h1:UETIi67q53MR2AWcXfiuqkDkRtnGDLqkBTpCHuJHxtU= +go.etcd.io/bbolt v1.3.2 h1:Z/90sZLPOeCy2PwprqkFa25PdkusRzaj9P8zm/KNyvk= +go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU= go.uber.org/atomic v1.3.2 h1:2Oa65PReHzfn29GpvgsYwloV9AVFHPDk8tYxt2c2tr4= go.uber.org/atomic v1.3.2/go.mod h1:gD2HeocX3+yG+ygLZcrzQJaqmWj9AIm7n08wl/qW/PE= go.uber.org/multierr v1.1.0 h1:HoEmRHQPVSqub6w2z2d2EOVs2fjyFRGyofhKuyDq0QI= diff --git a/pkg/api/apis/operators/v1alpha1/installplan_types.go b/pkg/api/apis/operators/v1alpha1/installplan_types.go index 4321368555..96a90b7c6a 100644 --- a/pkg/api/apis/operators/v1alpha1/installplan_types.go +++ b/pkg/api/apis/operators/v1alpha1/installplan_types.go @@ -145,6 +145,43 @@ type Step struct { Status StepStatus `json:"status"` } +// ManifestsMatch returns true if the CSV manifests in the StepResources of the given list of steps +// matches those in the InstallPlanStatus. +func (s *InstallPlanStatus) CSVManifestsMatch(steps []*Step) bool { + if s.Plan == nil && steps == nil { + return true + } + if s.Plan == nil || steps == nil { + return false + } + + manifests := make(map[string]struct{}) + for _, step := range s.Plan { + resource := step.Resource + if resource.Kind != ClusterServiceVersionKind { + continue + } + manifests[resource.Manifest] = struct{}{} + } + + for _, step := range steps { + resource := step.Resource + if resource.Kind != ClusterServiceVersionKind { + continue + } + if _, ok := manifests[resource.Manifest]; !ok { + return false + } + delete(manifests, resource.Manifest) + } + + if len(manifests) == 0 { + return true + } + + return false +} + func (s *Step) String() string { return fmt.Sprintf("%s: %s (%s)", s.Resolving, s.Resource, s.Status) } diff --git a/pkg/controller/operators/catalog/operator.go b/pkg/controller/operators/catalog/operator.go index 0acf0cb11b..6bc7a21b0a 100644 --- a/pkg/controller/operators/catalog/operator.go +++ b/pkg/controller/operators/catalog/operator.go @@ -47,8 +47,7 @@ const ( serviceKind = "Service" roleKind = "Role" roleBindingKind = "RoleBinding" - - generatedByKey = "olm/generated-by" + generatedByKey = "olm.generated-by" ) // for test stubbing and for ensuring standardization of timezones to UTC @@ -580,9 +579,9 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error { break } } - installplanReference, err := o.createInstallPlan(namespace, subs, installPlanApproval, steps) + installplanReference, err := o.ensureInstallPlan(logger, namespace, subs, installPlanApproval, steps) if err != nil { - logger.WithError(err).Debug("error creating installplan") + logger.WithError(err).Debug("error ensuring installplan") return err } @@ -679,8 +678,6 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub // this indicates it was newly resolved by another operator, and we should reference that installplan in the status ipName, ok := sub.GetAnnotations()[generatedByKey] if !ok { - // err := fmt.Errorf("no installplan reference or %s annotation found", generatedByKey) - // logger.WithField("err", err.Error()).Error("an error occurred while associating a subscription with an installplan") return sub, nil } @@ -744,6 +741,28 @@ func (o *Operator) updateSubscriptionSetInstallPlanState(namespace string, subs return nil } +func (o *Operator) ensureInstallPlan(logger *logrus.Entry, namespace string, subs []*v1alpha1.Subscription, installPlanApproval v1alpha1.Approval, steps []*v1alpha1.Step) (*v1alpha1.InstallPlanReference, error) { + if len(steps) == 0 { + return nil, nil + } + + // Check if any existing installplans are creating the same resources + installPlans, err := o.lister.OperatorsV1alpha1().InstallPlanLister().InstallPlans(namespace).List(labels.Everything()) + if err != nil { + return nil, err + } + + for _, installPlan := range installPlans { + if installPlan.Status.CSVManifestsMatch(steps) { + logger.Infof("found InstallPlan with matching manifests: %s", installPlan.GetName()) + return o.referenceForInstallPlan(installPlan), nil + } + } + logger.Warn("no installplan found with matching manifests, creating new one") + + return o.createInstallPlan(namespace, subs, installPlanApproval, steps) +} + func (o *Operator) createInstallPlan(namespace string, subs []*v1alpha1.Subscription, installPlanApproval v1alpha1.Approval, steps []*v1alpha1.Step) (*v1alpha1.InstallPlanReference, error) { if len(steps) == 0 { return nil, nil diff --git a/pkg/controller/operators/catalog/operator_test.go b/pkg/controller/operators/catalog/operator_test.go index 8e34b8ad84..ee59db60e6 100644 --- a/pkg/controller/operators/catalog/operator_test.go +++ b/pkg/controller/operators/catalog/operator_test.go @@ -488,7 +488,8 @@ func NewFakeOperator(clientObjs []runtime.Object, k8sObjs []runtime.Object, extO podInformer := informerFactory.Core().V1().Pods() configMapInformer := informerFactory.Core().V1().ConfigMaps() subscriptionInformer := externalversions.NewSharedInformerFactoryWithOptions(clientFake, wakeupInterval, externalversions.WithNamespace(namespace)).Operators().V1alpha1().Subscriptions() - + installPlanInformer := externalversions.NewSharedInformerFactoryWithOptions(clientFake, wakeupInterval, externalversions.WithNamespace(namespace)).Operators().V1alpha1().InstallPlans() + // register informers registryInformers := []cache.SharedIndexInformer{ roleInformer.Informer(), @@ -498,6 +499,7 @@ func NewFakeOperator(clientObjs []runtime.Object, k8sObjs []runtime.Object, extO podInformer.Informer(), configMapInformer.Informer(), subscriptionInformer.Informer(), + installPlanInformer.Informer(), } // register listers @@ -509,6 +511,7 @@ func NewFakeOperator(clientObjs []runtime.Object, k8sObjs []runtime.Object, extO lister.CoreV1().RegisterPodLister(namespace, podInformer.Lister()) lister.CoreV1().RegisterConfigMapLister(namespace, configMapInformer.Lister()) lister.OperatorsV1alpha1().RegisterSubscriptionLister(namespace, subscriptionInformer.Lister()) + lister.OperatorsV1alpha1().RegisterInstallPlanLister(namespace, installPlanInformer.Lister()) // Create the new operator queueOperator, err := queueinformer.NewOperatorFromClient(opClientFake, logrus.New()) diff --git a/pkg/lib/operatorclient/mock_client.go b/pkg/lib/operatorclient/mock_client.go index 7d62909fd8..418fc781c8 100644 --- a/pkg/lib/operatorclient/mock_client.go +++ b/pkg/lib/operatorclient/mock_client.go @@ -1,5 +1,5 @@ // Code generated by MockGen. DO NOT EDIT. -// SourceClient: ./pkg/lib/operatorclient/client.go +// Source: ./pkg/lib/operatorclient/client.go // Package operatorclient is a generated GoMock package. package operatorclient diff --git a/vendor/github.com/golang/groupcache/lru/lru.go b/vendor/github.com/golang/groupcache/lru/lru.go index 532cc45e6d..eac1c7664f 100644 --- a/vendor/github.com/golang/groupcache/lru/lru.go +++ b/vendor/github.com/golang/groupcache/lru/lru.go @@ -25,7 +25,7 @@ type Cache struct { // an item is evicted. Zero means no limit. MaxEntries int - // OnEvicted optionally specificies a callback function to be + // OnEvicted optionally specifies a callback function to be // executed when an entry is purged from the cache. OnEvicted func(key Key, value interface{}) diff --git a/vendor/modules.txt b/vendor/modules.txt index e8848aae28..8a9848e881 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -71,7 +71,7 @@ github.com/gogo/protobuf/gogoproto github.com/gogo/protobuf/protoc-gen-gogo/descriptor # github.com/golang/glog v0.0.0-20160126235308-23def4e6c14b github.com/golang/glog -# github.com/golang/groupcache v0.0.0-20181024230925-c65c006176ff +# github.com/golang/groupcache v0.0.0-20190129154638-5b532d6fd5ef github.com/golang/groupcache/lru # github.com/golang/mock v1.1.1 github.com/golang/mock/mockgen @@ -336,6 +336,7 @@ k8s.io/apimachinery/pkg/api/equality k8s.io/apimachinery/pkg/api/validation k8s.io/apimachinery/pkg/util/yaml k8s.io/apimachinery/pkg/util/framer +k8s.io/apimachinery/pkg/util/rand k8s.io/apimachinery/pkg/apis/meta/v1beta1 k8s.io/apimachinery/pkg/util/mergepatch k8s.io/apimachinery/third_party/forked/golang/json @@ -343,13 +344,13 @@ k8s.io/apimachinery/pkg/api/validation/path k8s.io/apimachinery/pkg/apis/meta/v1/validation k8s.io/apimachinery/pkg/util/uuid k8s.io/apimachinery/third_party/forked/golang/reflect -k8s.io/apimachinery/pkg/util/rand # k8s.io/apiserver v0.0.0-20181026151315-13cfe3978170 k8s.io/apiserver/pkg/server k8s.io/apiserver/pkg/util/logs k8s.io/apiserver/pkg/authentication/serviceaccount k8s.io/apiserver/pkg/authentication/user k8s.io/apiserver/pkg/authorization/authorizer +k8s.io/apiserver/pkg/storage/names k8s.io/apiserver/pkg/endpoints/openapi k8s.io/apiserver/pkg/registry/rest k8s.io/apiserver/pkg/server/options @@ -379,7 +380,6 @@ k8s.io/apiserver/pkg/server/mux k8s.io/apiserver/pkg/server/routes k8s.io/apiserver/pkg/server/storage k8s.io/apiserver/pkg/util/feature -k8s.io/apiserver/pkg/storage/names k8s.io/apiserver/pkg/admission/initializer k8s.io/apiserver/pkg/admission/metrics k8s.io/apiserver/pkg/apis/apiserver