Merge pull request operator-framework#688 from ecordell/command-pod

fix(reconciler): set command in pod spec of registry images

README.md

@@ -105,6 +105,8 @@ spec:
 
 This will keep the etcd `ClusterServiceVersion` up to date as new versions become available in the catalog.
 
+Catalogs are served internally over a grpc interface to OLM from [operator-registry](https://github.com/operator-framework/operator-registry) pods.
+
 ### User Interface
 
 Use the OpenShift admin console (compatible with upstream Kubernetes) to interact with and visualize the resources managed by OLM. Create subscriptions, approve install plans, identify Operator-managed resources, and more.

go.sum

@@ -288,7 +288,7 @@ k8s.io/apimachinery v0.0.0-20190118094746-1525e4dadd2d/go.mod h1:ccL7Eh7zubPUSh9
 k8s.io/apiserver v0.0.0-20181026151315-13cfe3978170 h1:CqI85nZvPaV+7JFono0nAOGOx2brocqefcOhDPVhHKI=
 k8s.io/apiserver v0.0.0-20181026151315-13cfe3978170/go.mod h1:6bqaTSOSJavUIXUtfaR9Os9JtTCm8ZqH2SUl2S60C4w=
 k8s.io/client-go v0.0.0-20180718001006-59698c7d9724/go.mod h1:7vJpHMYJwNQCWgzmNV+VYUl1zCObLyodBc8nIyt8L5s=
-k8s.io/client-go v8.0.0+incompatible h1:tTI4hRmb1DRMl4fG6Vclfdi6nTM82oIrTT7HfitmxC4=
+k8s.io/client-go v8.0.0+incompatible h1:2pUaSg2x6iEHr8cia6zmWhoCXG1EDG9TCx9s//Aq7HY=
 k8s.io/client-go v8.0.0+incompatible/go.mod h1:7vJpHMYJwNQCWgzmNV+VYUl1zCObLyodBc8nIyt8L5s=
 k8s.io/code-generator v0.0.0-20181203235156-f8cba74510f3 h1:f/Aa24HPnPEDWia884BCF94E1b29KYjOTVTHcBzvT2Q=
 k8s.io/code-generator v0.0.0-20181203235156-f8cba74510f3/go.mod h1:MYiN+ZJZ9HkETbgVZdWw2AsuAi9PZ4V80cwfuf2axe8=

pkg/controller/operators/catalog/operator.go

@@ -47,7 +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
@@ -356,7 +356,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() {
@@ -535,7 +539,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(),
@@ -545,16 +550,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 {
@@ -565,30 +581,36 @@ 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.ensureInstallPlan(logger, namespace, subs, installPlanApproval, steps)
-	if err != nil {
-		logger.WithError(err).Debug("error ensuring installplan")
-		return err
-	}
 
-	if err := o.updateSubscriptionSetInstallPlanState(namespace, subs, installplanReference); err != nil {
-		logger.WithError(err).Debug("error ensuring subscription installplan state")
-		return err
+		installPlanReference, err := o.ensureInstallPlan(logger, namespace, subs, installPlanApproval, steps)
+		if err != nil {
+			logger.WithError(err).Debug("error ensuring 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
 	}
+
 	return nil
 }
 
@@ -656,7 +678,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
 	}
@@ -667,9 +689,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")
@@ -678,13 +700,13 @@ 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 {
-		return sub, nil
+		return sub, false, nil
 	}
 
 	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
+		return nil, false, err
 	}
 	logger.WithField("installplan", ipName).Debug("found installplan that generated subscription")
 
@@ -693,15 +715,15 @@ func (o *Operator) ensureSubscriptionInstallPlanState(logger *logrus.Entry, sub
 	out.Status.State = v1alpha1.SubscriptionStateUpgradePending
 	updated, err := o.client.OperatorsV1alpha1().Subscriptions(sub.GetNamespace()).UpdateStatus(out)
 	if err != nil {
-		return nil, err
+		return nil, false, err
 	}
 
-	return updated, nil
+	return updated, true, 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{})
@@ -716,29 +738,34 @@ 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
+		}
+		if _, subErr := o.client.OperatorsV1alpha1().Subscriptions(namespace).UpdateStatus(sub); subErr != nil {
+			err = subErr
 		}
 	}
-	return nil
+	return err
 }
 
 func (o *Operator) ensureInstallPlan(logger *logrus.Entry, 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,
 					},
 				},
 			},

pkg/controller/registry/reconciler/configmap.go

@@ -91,9 +91,9 @@ func (s *configMapCatalogSourceDecorator) Pod(image string) *v1.Pod {
 		Spec: v1.PodSpec{
 			Containers: []v1.Container{
 				{
-					Name:  "configmap-registry-server",
-					Image: image,
-					Args:  []string{"-c", s.Spec.ConfigMap, "-n", s.GetNamespace()},
+					Name:    "configmap-registry-server",
+					Image:   image,
+					Command: []string{"configmap-server", "-c", s.Spec.ConfigMap, "-n", s.GetNamespace()},
 					Ports: []v1.ContainerPort{
 						{
 							Name:          "grpc",