From 422b134b8c38a54faf553a20a2620a92afc8b37f Mon Sep 17 00:00:00 2001 From: njhale Date: Sun, 27 Jan 2019 13:41:29 -0500 Subject: [PATCH] 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 {