Merge pull request #931 from operator-framework/depupdate

feat(csv): detect changes to a deployment and persist them

pkg/controller/install/deployment.go

@@ -6,6 +6,8 @@ import (
 	log "github.com/sirupsen/logrus"
 	appsv1 "k8s.io/api/apps/v1"
 	rbac "k8s.io/api/rbac/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/util/diff"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers"
@@ -61,32 +63,34 @@ func NewStrategyDeploymentInstaller(strategyClient wrappers.InstallStrategyDeplo
 
 func (i *StrategyDeploymentInstaller) installDeployments(deps []StrategyDeploymentSpec) error {
 	for _, d := range deps {
-		dep := &appsv1.Deployment{Spec: d.Spec}
-		dep.SetName(d.Name)
-		dep.SetNamespace(i.owner.GetNamespace())
-
-		// Merge annotations (to avoid losing info from pod template)
-		annotations := map[string]string{}
-		for k, v := range i.templateAnnotations {
-			annotations[k] = v
-		}
-		for k, v := range dep.Spec.Template.GetAnnotations() {
-			annotations[k] = v
-		}
-		dep.Spec.Template.SetAnnotations(annotations)
-
-		ownerutil.AddNonBlockingOwner(dep, i.owner)
-		if err := ownerutil.AddOwnerLabels(dep, i.owner); err != nil {
-			return err
-		}
-		if _, err := i.strategyClient.CreateOrUpdateDeployment(dep); err != nil {
+		if _, err := i.strategyClient.CreateOrUpdateDeployment(i.deploymentForSpec(d.Name, d.Spec)); err != nil {
 			return err
 		}
 	}
 
 	return nil
 }
 
+func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1.DeploymentSpec) *appsv1.Deployment {
+	dep := &appsv1.Deployment{Spec: spec}
+	dep.SetName(name)
+	dep.SetNamespace(i.owner.GetNamespace())
+
+	// Merge annotations (to avoid losing info from pod template)
+	annotations := map[string]string{}
+	for k, v := range i.templateAnnotations {
+		annotations[k] = v
+	}
+	for k, v := range dep.Spec.Template.GetAnnotations() {
+		annotations[k] = v
+	}
+	dep.Spec.Template.SetAnnotations(annotations)
+
+	ownerutil.AddNonBlockingOwner(dep, i.owner)
+	ownerutil.AddOwnerLabelsForKind(dep, i.owner, v1alpha1.ClusterServiceVersionKind)
+	return dep
+}
+
 func (i *StrategyDeploymentInstaller) cleanupPrevious(current *StrategyDetailsDeployment, previous *StrategyDetailsDeployment) error {
 	previousDeploymentsMap := map[string]struct{}{}
 	for _, d := range previous.DeploymentSpecs {
@@ -179,10 +183,44 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []Stra
 				return StrategyError{Reason: StrategyErrReasonAnnotationsMissing, Message: fmt.Sprintf("annotations on deployment don't match. couldn't find %s: %s", key, value)}
 			}
 		}
+
+		// check equality
+		calculated := i.deploymentForSpec(spec.Name, spec.Spec)
+		if !i.equalDeployments(&calculated.Spec, &dep.Spec) {
+			return StrategyError{Reason: StrategyErrDeploymentUpdated, Message: fmt.Sprintf("deployment changed, rolling update with patch: %s\n%#v\n%#v", diff.ObjectDiff(dep.Spec.Template.Spec, calculated.Spec.Template.Spec), calculated.Spec.Template.Spec, dep.Spec.Template.Spec)}
+		}
 	}
 	return nil
 }
 
+func (i *StrategyDeploymentInstaller) equalDeployments(calculated, onCluster *appsv1.DeploymentSpec) bool {
+	// ignore template annotations, OLM injects these elsewhere
+	calculated.Template.Annotations = nil
+
+	// DeepDerivative doesn't treat `0` ints as unset. Stripping them here means we miss changes to these values,
+	// but we don't end up getting bitten by the defaulter for deployments.
+	for i, c := range onCluster.Template.Spec.Containers {
+		o := calculated.Template.Spec.Containers[i]
+		if o.ReadinessProbe != nil {
+			o.ReadinessProbe.InitialDelaySeconds = c.ReadinessProbe.InitialDelaySeconds
+			o.ReadinessProbe.TimeoutSeconds = c.ReadinessProbe.TimeoutSeconds
+			o.ReadinessProbe.PeriodSeconds = c.ReadinessProbe.PeriodSeconds
+			o.ReadinessProbe.SuccessThreshold = c.ReadinessProbe.SuccessThreshold
+			o.ReadinessProbe.FailureThreshold = c.ReadinessProbe.FailureThreshold
+		}
+		if o.LivenessProbe != nil {
+			o.LivenessProbe.InitialDelaySeconds = c.LivenessProbe.InitialDelaySeconds
+			o.LivenessProbe.TimeoutSeconds = c.LivenessProbe.TimeoutSeconds
+			o.LivenessProbe.PeriodSeconds = c.LivenessProbe.PeriodSeconds
+			o.LivenessProbe.SuccessThreshold = c.LivenessProbe.SuccessThreshold
+			o.LivenessProbe.FailureThreshold = c.LivenessProbe.FailureThreshold
+		}
+	}
+
+	// DeepDerivative ensures that, for any non-nil, non-empty value in A, the corresponding value is set in B
+	return equality.Semantic.DeepDerivative(calculated, onCluster)
+}
+
 // Clean up orphaned deployments after reinstalling deployments process
 func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs []StrategyDeploymentSpec) error {
 	// Map of deployments

pkg/controller/install/errors.go

@@ -9,12 +9,15 @@ const (
 	StrategyErrReasonInvalidStrategy    = "InvalidStrategy"
 	StrategyErrReasonTimeout            = "Timeout"
 	StrategyErrReasonUnknown            = "Unknown"
+	StrategyErrBadPatch                 = "PatchUnsuccessful"
+	StrategyErrDeploymentUpdated        = "DeploymentUpdated"
 )
 
 // unrecoverableErrors are the set of errors that mean we can't recover an install strategy
 var unrecoverableErrors = map[string]struct{}{
 	StrategyErrReasonInvalidStrategy: {},
 	StrategyErrReasonTimeout:         {},
+	StrategyErrBadPatch:              {},
 }
 
 // StrategyError is used to represent error types for install strategies

pkg/controller/operators/olm/operator.go

@@ -1271,6 +1271,10 @@ func (a *Operator) updateInstallStatus(csv *v1alpha1.ClusterServiceVersion, inst
 	strategyInstalled, strategyErr := installer.CheckInstalled(strategy)
 	now := a.now()
 
+	if strategyErr != nil {
+		a.logger.WithError(strategyErr).Debug("operator not installed")
+	}
+
 	if strategyInstalled && apiServicesInstalled {
 		// if there's no error, we're successfully running
 		csv.SetPhaseWithEventIfChanged(v1alpha1.CSVPhaseSucceeded, v1alpha1.CSVReasonInstallSuccessful, "install strategy completed with no errors", now, a.recorder)

pkg/lib/ownerutil/util.go

@@ -195,21 +195,26 @@ func OwnerLabel(owner Owner, kind string) map[string]string {
 	}
 }
 
-// AddOwnerLabels adds ownerref-like labels to an object
+// AddOwnerLabels adds ownerref-like labels to an object by inferring the owner kind
 func AddOwnerLabels(object metav1.Object, owner Owner) error {
 	err := InferGroupVersionKind(owner)
 	if err != nil {
 		return err
 	}
+	AddOwnerLabelsForKind(object, owner, owner.GetObjectKind().GroupVersionKind().Kind)
+	return nil
+}
+
+// AddOwnerLabels adds ownerref-like labels to an object, with no inference
+func AddOwnerLabelsForKind(object metav1.Object, owner Owner, kind string) {
 	labels := object.GetLabels()
 	if labels == nil {
 		labels = map[string]string{}
 	}
-	for key, val := range OwnerLabel(owner, owner.GetObjectKind().GroupVersionKind().Kind) {
+	for key, val := range OwnerLabel(owner, kind) {
 		labels[key] = val
 	}
 	object.SetLabels(labels)
-	return nil
 }
 
 // IsOwnedByKindLabel returns whether or not a label exists on the object pointing to an owner of a particular kind

scripts/build_local.sh

@@ -13,7 +13,7 @@ fi
 
 docker build -f local.Dockerfile -t quay.io/operator-framework/olm:local -t quay.io/operator-framework/olm-e2e:local ./bin
 
-if [ -x "$(command -v kind)" ] && [ "kubectl config current-context" -eq "kind" ]; then
+if [ -x "$(command -v kind)" ] && [ "$(kubectl config current-context)" = "kind" ]; then
   kind load docker-image quay.io/operator-framework/olm:local
   kind load docker-image quay.io/operator-framework/olm-e2e:local
 fi

test/e2e/csv_e2e_test.go

@@ -148,7 +148,7 @@ func newNginxDeployment(name string) appsv1.DeploymentSpec {
 				Containers: []corev1.Container{
 					{
 						Name:  genName("nginx"),
-						Image: "bitnami/nginx:latest",
+						Image: "redis",
 						Ports: []corev1.ContainerPort{
 							{
 								ContainerPort: 80,
@@ -2393,6 +2393,172 @@ func TestUpdateCSVMultipleIntermediates(t *testing.T) {
 	require.NoError(t, err)
 }
 
+func TestUpdateCSVInPlace(t *testing.T) {
+	defer cleaner.NotifyTestComplete(t, true)
+
+	c := newKubeClient(t)
+	crc := newCRClient(t)
+
+	// Create dependency first (CRD)
+	crdPlural := genName("ins")
+	crdName := crdPlural + ".cluster.com"
+	cleanupCRD, err := createCRD(c, apiextensions.CustomResourceDefinition{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: crdName,
+		},
+		Spec: apiextensions.CustomResourceDefinitionSpec{
+			Group:   "cluster.com",
+			Version: "v1alpha1",
+			Names: apiextensions.CustomResourceDefinitionNames{
+				Plural:   crdPlural,
+				Singular: crdPlural,
+				Kind:     crdPlural,
+				ListKind: "list" + crdPlural,
+			},
+			Scope: "Namespaced",
+		},
+	})
+
+	// Create "current" CSV
+	nginxName := genName("nginx-")
+	strategy := install.StrategyDetailsDeployment{
+		DeploymentSpecs: []install.StrategyDeploymentSpec{
+			{
+				Name: genName("dep-"),
+				Spec: newNginxDeployment(nginxName),
+			},
+		},
+	}
+	strategyRaw, err := json.Marshal(strategy)
+	require.NoError(t, err)
+
+	require.NoError(t, err)
+	defer cleanupCRD()
+	csv := v1alpha1.ClusterServiceVersion{
+		TypeMeta: metav1.TypeMeta{
+			Kind:       v1alpha1.ClusterServiceVersionKind,
+			APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name: genName("csv"),
+		},
+		Spec: v1alpha1.ClusterServiceVersionSpec{
+			MinKubeVersion: "0.0.0",
+			InstallModes: []v1alpha1.InstallMode{
+				{
+					Type:      v1alpha1.InstallModeTypeOwnNamespace,
+					Supported: true,
+				},
+				{
+					Type:      v1alpha1.InstallModeTypeSingleNamespace,
+					Supported: true,
+				},
+				{
+					Type:      v1alpha1.InstallModeTypeMultiNamespace,
+					Supported: true,
+				},
+				{
+					Type:      v1alpha1.InstallModeTypeAllNamespaces,
+					Supported: true,
+				},
+			},
+			InstallStrategy: v1alpha1.NamedInstallStrategy{
+				StrategyName:    install.InstallStrategyNameDeployment,
+				StrategySpecRaw: strategyRaw,
+			},
+			CustomResourceDefinitions: v1alpha1.CustomResourceDefinitions{
+				Owned: []v1alpha1.CRDDescription{
+					{
+						Name:        crdName,
+						Version:     "v1alpha1",
+						Kind:        crdPlural,
+						DisplayName: crdName,
+						Description: "In the cluster",
+					},
+				},
+			},
+		},
+	}
+
+	cleanupCSV, err := createCSV(t, c, crc, csv, testNamespace, false, true)
+	require.NoError(t, err)
+	defer cleanupCSV()
+
+	// Wait for current CSV to succeed
+	fetchedCSV, err := fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker)
+	require.NoError(t, err)
+
+	// Should have created deployment
+	dep, err := c.GetDeployment(testNamespace, strategy.DeploymentSpecs[0].Name)
+	require.NoError(t, err)
+	require.NotNil(t, dep)
+
+	// Create "updated" CSV with a different image
+	strategyNew := strategy
+	strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers = []corev1.Container{
+		{
+			Name:    genName("hat"),
+			Image:   "quay.io/coreos/mock-extension-apiserver:master",
+			Command: []string{"/bin/mock-extension-apiserver"},
+			Args: []string{
+				"-v=4",
+				"--mock-kinds",
+				"fedora",
+				"--mock-group-version",
+				"group.version",
+				"--secure-port",
+				"5443",
+				"--debug",
+			},
+			Ports: []corev1.ContainerPort{
+				{
+					ContainerPort: 5443,
+				},
+			},
+			ImagePullPolicy: corev1.PullIfNotPresent,
+		},
+	}
+
+	// Also set something outside the spec template - this should be ignored
+	var five int32 = 5
+	strategyNew.DeploymentSpecs[0].Spec.Replicas = &five
+
+	strategyNewRaw, err := json.Marshal(strategyNew)
+	require.NoError(t, err)
+
+	fetchedCSV.Spec.InstallStrategy.StrategySpecRaw = strategyNewRaw
+
+	// Update CSV directly
+	_, err = crc.OperatorsV1alpha1().ClusterServiceVersions(testNamespace).Update(fetchedCSV)
+	require.NoError(t, err)
+
+	// wait for deployment spec to be updated
+	err = wait.Poll(pollInterval, pollDuration, func() (bool, error) {
+		fetched, err := c.GetDeployment(testNamespace, strategyNew.DeploymentSpecs[0].Name)
+		if err != nil {
+			return false, err
+		}
+		fmt.Println("waiting for deployment to update...")
+		return fetched.Spec.Template.Spec.Containers[0].Name == strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Name, nil
+	})
+	require.NoError(t, err)
+
+	// Wait for updated CSV to succeed
+	_, err = fetchCSV(t, crc, csv.Name, testNamespace, csvSucceededChecker)
+	require.NoError(t, err)
+
+	depUpdated, err := c.GetDeployment(testNamespace, strategyNew.DeploymentSpecs[0].Name)
+	require.NoError(t, err)
+	require.NotNil(t, depUpdated)
+
+	// Deployment should have changed even though the CSV is otherwise the same
+	require.Equal(t, depUpdated.Spec.Template.Spec.Containers[0].Name, strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Name)
+	require.Equal(t, depUpdated.Spec.Template.Spec.Containers[0].Image, strategyNew.DeploymentSpecs[0].Spec.Template.Spec.Containers[0].Image)
+
+	// Field updated even though template spec didn't change, because it was part of a template spec change as well
+	require.Equal(t, *depUpdated.Spec.Replicas, *strategyNew.DeploymentSpecs[0].Spec.Replicas)
+}
+
 func TestUpdateCSVMultipleVersionCRD(t *testing.T) {
 	defer cleaner.NotifyTestComplete(t, true)
 

test/e2e/installplan_e2e_test.go

@@ -101,7 +101,7 @@ func newNginxInstallStrategy(name string, permissions []install.StrategyDeployme
 						Spec: corev1.PodSpec{Containers: []corev1.Container{
 							{
 								Name:            genName("nginx"),
-								Image:           "bitnami/nginx:latest",
+								Image:           "redis",
 								Ports:           []corev1.ContainerPort{{ContainerPort: 80}},
 								ImagePullPolicy: corev1.PullIfNotPresent,
 							},

test/e2e/subscription_e2e_test.go

@@ -210,7 +210,7 @@ var (
 						Spec: corev1.PodSpec{Containers: []corev1.Container{
 							{
 								Name:            genName("nginx"),
-								Image:           "bitnami/nginx:latest",
+								Image:           "redis",
 								Ports:           []corev1.ContainerPort{{ContainerPort: 80}},
 								ImagePullPolicy: corev1.PullIfNotPresent,
 							},