fix(installplans): only create installplans if something new has been

resolved.

This solves an issue where duplicate installplans are sometimes created.

It also fixes an issue where a new installplan may be created if the
catalog operator restarts

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,6 +13,7 @@ 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
@@ -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

pkg/controller/operators/catalog/operator.go

@@ -355,7 +355,11 @@ func (o *Operator) syncCatalogSources(obj interface{}) (syncError error) {
 		}
 
 		if wasOwned := ownerutil.EnsureOwner(configMap, catsrc); !wasOwned {
-			o.OpClient.KubernetesInterface().CoreV1().ConfigMaps(configMap.GetNamespace()).Update(configMap)
+			configMap, err = o.OpClient.KubernetesInterface().CoreV1().ConfigMaps(configMap.GetNamespace()).Update(configMap)
+			if err != nil {
+				return fmt.Errorf("unable to write owner onto catalog source configmap")
+			}
+			logger.Debug("adopted configmap")
 		}
 
 		if catsrc.Status.ConfigMapResource == nil || catsrc.Status.ConfigMapResource.UID != configMap.GetUID() || catsrc.Status.ConfigMapResource.ResourceVersion != configMap.GetResourceVersion() {
@@ -534,7 +538,8 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 		return err
 	}
 
-	shouldUpdate := false
+	// TODO: parallel
+	subscriptionUpdated := false
 	for _, sub := range subs {
 		logger := logger.WithFields(logrus.Fields{
 			"sub":     sub.GetName(),
@@ -544,16 +549,27 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 		})
 
 		// ensure the installplan reference is correct
-		sub, err := o.ensureSubscriptionInstallPlanState(logger, sub)
+		sub, changedIp, err := o.ensureSubscriptionInstallPlanState(logger, sub)
 		if err != nil {
 			return err
 		}
+		subscriptionUpdated = subscriptionUpdated || changedIp
 
 		// record the current state of the desired corresponding CSV in the status. no-op if we don't know the csv yet.
-		sub, err = o.ensureSubscriptionCSVState(logger, sub)
+		sub, changedCSV, err := o.ensureSubscriptionCSVState(logger, sub)
 		if err != nil {
 			return err
 		}
+
+		subscriptionUpdated = subscriptionUpdated || changedCSV
+	}
+	if subscriptionUpdated {
+		logger.Debug("subscriptions were updated, wait for a new resolution")
+		return nil
+	}
+
+	shouldUpdate := false
+	for _, sub := range subs {
 		shouldUpdate = shouldUpdate || !o.nothingToUpdate(logger, sub)
 	}
 	if !shouldUpdate {
@@ -564,28 +580,37 @@ func (o *Operator) syncResolvingNamespace(obj interface{}) error {
 	logger.Debug("resolving subscriptions in namespace")
 
 	// resolve a set of steps to apply to a cluster, a set of subscriptions to create/update, and any errors
-	steps, subs, err := o.resolver.ResolveSteps(namespace, resolver.NewNamespaceSourceQuerier(resolverSources))
+	steps, updatedSubs, err := o.resolver.ResolveSteps(namespace, resolver.NewNamespaceSourceQuerier(resolverSources))
 	if err != nil {
 		return err
 	}
 
-	// any subscription in the namespace with manual approval will force generated installplans to be manual
-	// TODO: this is an odd artifact of the older resolver, and will probably confuse users. approval mode could be on the operatorgroup?
-	installPlanApproval := v1alpha1.ApprovalAutomatic
-	for _, sub := range subs {
-		if sub.Spec.InstallPlanApproval == v1alpha1.ApprovalManual {
-			installPlanApproval = v1alpha1.ApprovalManual
-			break
+	// create installplan if anything updated
+	if len(updatedSubs) > 0 {
+		logger.Debug("resolution caused subscription changes, creating installplan")
+		// any subscription in the namespace with manual approval will force generated installplans to be manual
+		// TODO: this is an odd artifact of the older resolver, and will probably confuse users. approval mode could be on the operatorgroup?
+		installPlanApproval := v1alpha1.ApprovalAutomatic
+		for _, sub := range subs {
+			if sub.Spec.InstallPlanApproval == v1alpha1.ApprovalManual {
+				installPlanApproval = v1alpha1.ApprovalManual
+				break
+			}
 		}
-	}
-	installplanReference, err := o.createInstallPlan(namespace, subs, installPlanApproval, steps)
-	if err != nil {
-		logger.WithError(err).Debug("error creating installplan")
-		return err
+		installPlanReference, err := o.createInstallPlan(namespace, subs, installPlanApproval, steps)
+		if err != nil {
+			logger.WithError(err).Debug("error creating installplan")
+			return err
+		}
+		if err := o.updateSubscriptionStatus(namespace, updatedSubs, installPlanReference); err != nil {
+			logger.WithError(err).Debug("error ensuring subscription installplan state")
+			return err
+		}
+		return nil
 	}
 
-	if err := o.updateSubscriptionSetInstallPlanState(namespace, subs, installplanReference); err != nil {
-		logger.WithError(err).Debug("error ensuring subscription installplan state")
+	if err := o.updateSubscriptionStatus(namespace, subs, nil); err != nil {
+		logger.WithError(err).Debug("error updating subscription status state")
 		return err
 	}
 	return nil
@@ -655,7 +680,7 @@ func (o *Operator) ensureResolverSources(logger *logrus.Entry, namespace string)
 
 func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscription) bool {
 	// Only sync if catalog has been updated since last sync time
-	if o.sourcesLastUpdate.Before(&sub.Status.LastUpdated) && sub.Status.State == v1alpha1.SubscriptionStateAtLatest {
+	if o.sourcesLastUpdate.Before(&sub.Status.LastUpdated) && sub.Status.State != v1alpha1.SubscriptionStateNone {
 		logger.Debugf("skipping update: no new updates to catalog since last sync at %s", sub.Status.LastUpdated.String())
 		return true
 	}
@@ -666,9 +691,9 @@ func (o *Operator) nothingToUpdate(logger *logrus.Entry, sub *v1alpha1.Subscript
 	return false
 }
 
-func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, error) {
+func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, bool, error) {
 	if sub.Status.Install != nil {
-		return sub, nil
+		return sub, false, nil
 	}
 
 	logger.Debug("checking for existing installplan")
@@ -679,7 +704,7 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub
 	if err != nil {
 		logger.WithError(err).Debug("couldn't get installplans")
 		// if we can't list, just continue processing
-		return sub, nil
+		return sub, false, nil
 	}
 
 	out := sub.DeepCopy()
@@ -692,21 +717,21 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub
 				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
+					return nil, false, err
 				} else {
-					return updated, nil
+					return updated, true, nil
 				}
 			}
 		}
 	}
 	logger.Debug("did not find subscription in steps of existing installplan")
 
-	return sub, nil
+	return sub, false, nil
 }
 
-func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, error) {
+func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha1.Subscription) (*v1alpha1.Subscription, bool, error) {
 	if sub.Status.CurrentCSV == "" {
-		return sub, nil
+		return sub, false, nil
 	}
 
 	_, err := o.client.OperatorsV1alpha1().ClusterServiceVersions(sub.GetNamespace()).Get(sub.Status.CurrentCSV, metav1.GetOptions{})
@@ -721,29 +746,32 @@ func (o *Operator) ensureSubscriptionCSVState(logger *logrus.Entry, sub *v1alpha
 
 	if sub.Status.State == out.Status.State && sub.Status.InstalledCSV == out.Status.InstalledCSV {
 		// The subscription status represents the cluster state
-		return sub, nil
+		return sub, false, nil
 	}
 	out.Status.LastUpdated = timeNow()
 
 	// Update Subscription with status of transition. Log errors if we can't write them to the status.
 	if sub, err = o.client.OperatorsV1alpha1().Subscriptions(out.GetNamespace()).UpdateStatus(out); err != nil {
 		logger.WithError(err).Info("error updating subscription status")
-		return nil, fmt.Errorf("error updating Subscription status: " + err.Error())
+		return nil, false, fmt.Errorf("error updating Subscription status: " + err.Error())
 	}
 
 	// subscription status represents cluster state
-	return sub, nil
+	return sub, true, nil
 }
 
-func (o *Operator) updateSubscriptionSetInstallPlanState(namespace string, subs []*v1alpha1.Subscription, installPlanRef *v1alpha1.InstallPlanReference) error {
+func (o *Operator) updateSubscriptionStatus(namespace string, subs []*v1alpha1.Subscription, installPlanRef *v1alpha1.InstallPlanReference) error {
 	// TODO: parallel, sync waitgroup
+	var err error
 	for _, sub := range subs {
-		sub.Status.Install = installPlanRef
-		if _, err := o.client.OperatorsV1alpha1().Subscriptions(namespace).UpdateStatus(sub); err != nil {
-			return err
+		sub.Status.LastUpdated = timeNow()
+		if installPlanRef != nil {
+			sub.Status.Install = installPlanRef
+			sub.Status.State = v1alpha1.SubscriptionStateUpgradePending
 		}
+		_, err = o.client.OperatorsV1alpha1().Subscriptions(namespace).UpdateStatus(sub)
 	}
-	return nil
+	return err
 }
 
 func (o *Operator) createInstallPlan(namespace string, subs []*v1alpha1.Subscription, installPlanApproval v1alpha1.Approval, steps []*v1alpha1.Step) (*v1alpha1.InstallPlanReference, error) {

pkg/controller/operators/catalog/subscriptions_test.go

@@ -3,6 +3,7 @@ package catalog
 import (
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 	"k8s.io/api/core/v1"
@@ -16,6 +17,9 @@ import (
 )
 
 func TestSyncSubscriptions(t *testing.T) {
+	nowTime := metav1.Date(2018, time.January, 26, 20, 40, 0, 0, time.UTC)
+	timeNow = func() metav1.Time { return nowTime }
+
 	testNamespace := "testNamespace"
 	type fields struct {
 		sourcesLastUpdate metav1.Time
@@ -78,6 +82,10 @@ func TestSyncSubscriptions(t *testing.T) {
 				},
 				resolveSubs: []*v1alpha1.Subscription{
 					{
+						TypeMeta: metav1.TypeMeta{
+							Kind:       v1alpha1.SubscriptionKind,
+							APIVersion: v1alpha1.SchemeGroupVersion.String(),
+						},
 						ObjectMeta: metav1.ObjectMeta{
 							Name:      "sub",
 							Namespace: testNamespace,
@@ -125,11 +133,12 @@ func TestSyncSubscriptions(t *testing.T) {
 					},
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "csv.v.1",
-						State:      "SubscriptionStateAtLatest",
+						State:      v1alpha1.SubscriptionStateUpgradePending,
 						Install: &v1alpha1.InstallPlanReference{
 							Kind:       v1alpha1.InstallPlanKind,
 							APIVersion: v1alpha1.InstallPlanAPIVersion,
 						},
+						LastUpdated: nowTime,
 					},
 				},
 			},
@@ -207,6 +216,10 @@ func TestSyncSubscriptions(t *testing.T) {
 				},
 				resolveSubs: []*v1alpha1.Subscription{
 					{
+						TypeMeta: metav1.TypeMeta{
+							Kind:       v1alpha1.SubscriptionKind,
+							APIVersion: v1alpha1.SubscriptionCRDAPIVersion,
+						},
 						ObjectMeta: metav1.ObjectMeta{
 							Name:      "sub",
 							Namespace: testNamespace,
@@ -254,11 +267,12 @@ func TestSyncSubscriptions(t *testing.T) {
 					},
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "csv.v.2",
-						State:      "SubscriptionStateAtLatest",
+						State:      v1alpha1.SubscriptionStateUpgradePending,
 						Install: &v1alpha1.InstallPlanReference{
 							Kind:       v1alpha1.InstallPlanKind,
 							APIVersion: v1alpha1.InstallPlanAPIVersion,
 						},
+						LastUpdated: nowTime,
 					},
 				},
 			},
@@ -360,6 +374,10 @@ func TestSyncSubscriptions(t *testing.T) {
 				},
 				resolveSubs: []*v1alpha1.Subscription{
 					{
+						TypeMeta: metav1.TypeMeta{
+							Kind:       v1alpha1.SubscriptionKind,
+							APIVersion: v1alpha1.SubscriptionCRDAPIVersion,
+						},
 						ObjectMeta: metav1.ObjectMeta{
 							Name:      "sub",
 							Namespace: testNamespace,
@@ -407,11 +425,12 @@ func TestSyncSubscriptions(t *testing.T) {
 					},
 					Status: v1alpha1.SubscriptionStatus{
 						CurrentCSV: "csv.v.2",
-						State:      "SubscriptionStateAtLatest",
+						State:      v1alpha1.SubscriptionStateUpgradePending,
 						Install: &v1alpha1.InstallPlanReference{
 							Kind:       v1alpha1.InstallPlanKind,
 							APIVersion: v1alpha1.InstallPlanAPIVersion,
 						},
+						LastUpdated: nowTime,
 					},
 				},
 			},

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,20 +336,21 @@ 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
 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