Skip to content

Commit c84f639

Browse files
committed
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.
1 parent 1e29578 commit c84f639

File tree

6 files changed

+35
-29
lines changed

6 files changed

+35
-29
lines changed

go.mod

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ require (
1717
github.com/google/gofuzz v0.0.0-20170612174753-24818f796faf // indirect
1818
github.com/googleapis/gnostic v0.2.0 // indirect
1919
github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 // indirect
20-
github.com/grpc-ecosystem/grpc-gateway v1.6.3 // indirect
20+
github.com/grpc-ecosystem/grpc-gateway v1.6.3
2121
github.com/imdario/mergo v0.3.6 // indirect
2222
github.com/inconshreveable/mousetrap v1.0.0 // indirect
2323
github.com/json-iterator/go v1.1.5 // indirect

go.sum

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ github.com/client9/misspell v0.3.4/go.mod h1:qj6jICC3Q7zFZvVWo7KLAzC3yx5G7kyvSDk
2121
github.com/coreos/bbolt v1.3.0 h1:HIgH5xUWXT914HCI671AxuTTqjj64UOFr7pHn48LUTI=
2222
github.com/coreos/bbolt v1.3.0/go.mod h1:iRUV2dpdMOn7Bo10OQBFzIJO9kkE559Wcmn+qkEiiKk=
2323
github.com/coreos/etcd v3.3.9+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE=
24-
github.com/coreos/etcd v3.3.10+incompatible h1:jFneRYjIvLMLhDLCzuTuU4rSJUjRplcJQ7pD7MnhC04=
24+
github.com/coreos/etcd v3.3.10+incompatible h1:KjVWqrZ5U0wa3CxY2AxlH6/UcB+PK2td1DcsYhA+HRs=
2525
github.com/coreos/etcd v3.3.10+incompatible/go.mod h1:uF7uidLiAD3TWHmW31ZFd/JWoc32PjwdhPthX9715RE=
2626
github.com/coreos/go-semver v0.2.0 h1:3Jm3tLmsgAYcjC+4Up7hJrFBPr+n7rAqYeSw/SZazuY=
2727
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
107107
github.com/gregjones/httpcache v0.0.0-20180305231024-9cad4c3443a7/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
108108
github.com/gregjones/httpcache v0.0.0-20181110185634-c63ab54fda8f h1:ShTPMJQes6tubcjzGMODIVG5hlrCeImaBnZzKF2N8SM=
109109
github.com/gregjones/httpcache v0.0.0-20181110185634-c63ab54fda8f/go.mod h1:FecbI9+v66THATjSRHfNgh1IVFe/9kFxbXtjV0ctIMA=
110-
github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:Iju5GlWwrvL6UBg4zJJt3btmonfrMlCDdsejg4CZE7c=
110+
github.com/grpc-ecosystem/go-grpc-middleware v1.0.0 h1:BWIsLfhgKhV5g/oF34aRjniBHLTZe5DNekSjbAjIS6c=
111111
github.com/grpc-ecosystem/go-grpc-middleware v1.0.0/go.mod h1:FiyG127CGDf3tlThmgyCl78X/SZQqEOJBCDaAfeWzPs=
112112
github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 h1:Ovs26xHkKqVztRpIrF/92BcuyuQ/YW4NSIpoGtfXNho=
113113
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
294294
k8s.io/kube-openapi v0.0.0-20181031203759-72693cb1fadd h1:ggv/Vfza0i5xuhUZyYyxcc25AmQvHY8Zi1C2m8WgBvA=
295295
k8s.io/kube-openapi v0.0.0-20181031203759-72693cb1fadd/go.mod h1:BXM9ceUBTj2QnfH2MK1odQs778ajze1RxcmP6S8RVVc=
296296
k8s.io/kubernetes v1.11.7-beta.0.0.20181219023948-b875d52ea96d/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk=
297-
k8s.io/kubernetes v1.11.8-beta.0.0.20190124204751-3a10094374f2 h1:CzIOMOEjH+sQw35LY1Gl0jwthkyOojzaq2HIeYZYOrM=
297+
k8s.io/kubernetes v1.11.8-beta.0.0.20190124204751-3a10094374f2 h1:Q4hIsjqTbRprTaPk+gVDUuVipXpGJtTz7Lg2FS3xpmw=
298298
k8s.io/kubernetes v1.11.8-beta.0.0.20190124204751-3a10094374f2/go.mod h1:ocZa8+6APFNC2tX1DZASIbocyYT5jHzqFVsY5aoB7Jk=

pkg/controller/operators/catalog/operator.go

Lines changed: 26 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ const (
4747
serviceKind = "Service"
4848
roleKind = "Role"
4949
roleBindingKind = "RoleBinding"
50+
51+
generatedByKey = "olm/generated-by"
5052
)
5153

5254
// for test stubbing and for ensuring standardization of timezones to UTC
@@ -675,33 +677,29 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub
675677

676678
// check if there's an installplan that created this subscription (only if it doesn't have a reference yet)
677679
// this indicates it was newly resolved by another operator, and we should reference that installplan in the status
678-
ips, err := o.lister.OperatorsV1alpha1().InstallPlanLister().InstallPlans(sub.GetNamespace()).List(labels.Everything())
679-
if err != nil {
680-
logger.WithError(err).Debug("couldn't get installplans")
681-
// if we can't list, just continue processing
680+
ipName, ok := sub.GetAnnotations()[generatedByKey]
681+
if !ok {
682+
// err := fmt.Errorf("no installplan reference or %s annotation found", generatedByKey)
683+
// logger.WithField("err", err.Error()).Error("an error occurred while associating a subscription with an installplan")
682684
return sub, nil
683685
}
684686

685-
out := sub.DeepCopy()
687+
ip, err := o.lister.OperatorsV1alpha1().InstallPlanLister().InstallPlans(sub.GetNamespace()).Get(ipName)
688+
if err != nil {
689+
logger.WithField("installplan", ipName).Warn("unable to get installplan from cache")
690+
return nil, err
691+
}
692+
logger.WithField("installplan", ipName).Debug("found installplan that generated subscription")
686693

687-
for _, ip := range ips {
688-
for _, step := range ip.Status.Plan {
689-
// TODO: is this enough? should we check equality of pkg/channel?
690-
if step != nil && step.Resource.Kind == v1alpha1.SubscriptionKind && step.Resource.Name == sub.GetName() {
691-
logger.WithField("installplan", ip.GetName()).Debug("found subscription in steps of existing installplan")
692-
out.Status.Install = o.referenceForInstallPlan(ip)
693-
out.Status.State = v1alpha1.SubscriptionStateUpgradePending
694-
if updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(out); err != nil {
695-
return nil, err
696-
} else {
697-
return updated, nil
698-
}
699-
}
700-
}
694+
out := sub.DeepCopy()
695+
out.Status.Install = o.referenceForInstallPlan(ip)
696+
out.Status.State = v1alpha1.SubscriptionStateUpgradePending
697+
updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(out)
698+
if err != nil {
699+
return nil, err
701700
}
702-
logger.Debug("did not find subscription in steps of existing installplan")
703701

704-
return sub, nil
702+
return updated, nil
705703
}
706704

707705
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 {
1001999
return errorwrap.Wrapf(err, "error parsing step manifest: %s", step.Resource.Name)
10021000
}
10031001

1002+
// Add the InstallPlan's name as an annotation
1003+
if annotations := sub.GetAnnotations(); annotations != nil {
1004+
annotations[generatedByKey] = plan.GetName()
1005+
} else {
1006+
sub.SetAnnotations(map[string]string{generatedByKey: plan.GetName()})
1007+
}
1008+
10041009
// Attempt to create the Subscription
10051010
sub.SetNamespace(namespace)
10061011
_, err = o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).Create(&sub)

pkg/controller/registry/reconciler/configmap.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -106,15 +106,15 @@ func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod {
106106
Command: []string{"grpc_health_probe", "-addr=localhost:50051"},
107107
},
108108
},
109-
InitialDelaySeconds: 5,
109+
InitialDelaySeconds: 1,
110110
},
111111
LivenessProbe: &v1.Probe{
112112
Handler: v1.Handler{
113113
Exec: &v1.ExecAction{
114114
Command: []string{"grpc_health_probe", "-addr=localhost:50051"},
115115
},
116116
},
117-
InitialDelaySeconds: 10,
117+
InitialDelaySeconds: 2,
118118
},
119119
},
120120
},

test/e2e/installplan_e2e_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -567,6 +567,7 @@ func TestCreateInstallPlanWithPreExistingCRDOwners(t *testing.T) {
567567
// existing cleanup should remove this
568568
createSubscriptionForCatalog(t, crc, testNamespace, subscriptionName, mainCatalogSourceName, mainPackageName, betaChannel, v1alpha1.ApprovalAutomatic)
569569

570+
// time.Sleep(5 * time.Minute)
570571
subscription, err = fetchSubscription(t, crc, testNamespace, subscriptionName, subscriptionHasInstallPlanChecker)
571572
require.NoError(t, err)
572573
require.NotNil(t, subscription)

test/e2e/util_test.go

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -237,7 +237,7 @@ func createFieldNotEqualSelector(field string, names ...string) string {
237237
func cleanupOLM(t *testing.T, namespace string) {
238238
var immediate int64 = 0
239239
crc := newCRClient(t)
240-
//c := newKubeClient(t)
240+
c := newKubeClient(t)
241241

242242
// Cleanup non persistent OLM CRs
243243
t.Log("cleaning up any remaining non persistent resources...")
@@ -250,7 +250,7 @@ func cleanupOLM(t *testing.T, namespace string) {
250250

251251
// error: the server does not allow this method on the requested resource
252252
// Cleanup non persistent configmaps
253-
//require.NoError(t, c.KubernetesInterface().CoreV1().ConfigMaps(namespace).DeleteCollection(deleteOptions, metav1.ListOptions{FieldSelector: nonPersistentConfigMapsFieldSelector}))
253+
require.NoError(t, c.KubernetesInterface().CoreV1().Pods(namespace).DeleteCollection(deleteOptions, metav1.ListOptions{}))
254254
}
255255

256256
func buildCatalogSourceCleanupFunc(t *testing.T, crc versioned.Interface, namespace string, catalogSource *v1alpha1.CatalogSource) cleanupFunc {

0 commit comments

Comments
 (0)