Rework cleanupOrphanedDeployments

Signed-off-by: Vu Dinh <[email protected]>

Makefile

@@ -141,7 +141,7 @@ container-codegen:
 	docker rm temp-codegen
 
 container-mockgen:
-	docker build -t olm:mockgen -f mockgen.Dockerfile .
+	docker build -t olm:mockgen -f mockgen.Dockerfile . --no-cache
 	docker run --name temp-mockgen olm:mockgen /bin/true
 	docker cp temp-mockgen:/go/src/github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers/wrappersfakes/. ./pkg/api/wrappers/wrappersfakes
 	docker cp temp-mockgen:/go/src/github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister/operatorlisterfakes/. ./pkg/lib/operatorlister/operatorlisterfakes
@@ -173,7 +173,7 @@ release:
 	rm -rf manifests
 	mkdir manifests
 	cp -R deploy/ocp/manifests/$(ver)/. manifests
-	find ./manifests -type f -exec sed -i "/^#/d" {} \; 
+	find ./manifests -type f -exec sed -i "/^#/d" {} \;
 	find ./manifests -type f -exec sed -i "1{/---/d}" {} \;
 
 package: olmref=$(shell docker inspect --format='{{index .RepoDigests 0}}' quay.io/operator-framework/olm:$(ver))

pkg/api/wrappers/deployment_install_client.go

@@ -8,6 +8,7 @@ import (
 	rbacv1 "k8s.io/api/rbac/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/labels"
 
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister"
@@ -25,6 +26,7 @@ type InstallStrategyDeploymentInterface interface {
 	DeleteDeployment(name string) error
 	GetServiceAccountByName(serviceAccountName string) (*corev1.ServiceAccount, error)
 	FindAnyDeploymentsMatchingNames(depNames []string) ([]*appsv1.Deployment, error)
+	FindAnyDeploymentsMatchingLabels(label labels.Selector) ([]*appsv1.Deployment, error)
 }
 
 type InstallStrategyDeploymentClientForNamespace struct {
@@ -118,3 +120,13 @@ func (c *InstallStrategyDeploymentClientForNamespace) FindAnyDeploymentsMatching
 	}
 	return deployments, nil
 }
+
+func (c *InstallStrategyDeploymentClientForNamespace) FindAnyDeploymentsMatchingLabels(label labels.Selector) ([]*appsv1.Deployment, error) {
+	deployments, err := c.opLister.AppsV1().DeploymentLister().Deployments(c.Namespace).List(label)
+	// Any errors other than !exists are propagated up
+	if !apierrors.IsNotFound(err) {
+		return deployments, err
+	}
+
+	return deployments, nil
+}

pkg/controller/install/deployment.go

@@ -7,6 +7,7 @@ import (
 	appsv1 "k8s.io/api/apps/v1"
 	rbac "k8s.io/api/rbac/v1"
 
+	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/apis/operators/v1alpha1"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/api/wrappers"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 )
@@ -113,14 +114,8 @@ func (i *StrategyDeploymentInstaller) Install(s Strategy) error {
 		return err
 	}
 
-	if i.previousStrategy != nil {
-		previous, ok := i.previousStrategy.(*StrategyDetailsDeployment)
-		if !ok {
-			return fmt.Errorf("couldn't parse old install %s strategy with deployment installer", previous.GetStrategyName())
-		}
-		return i.cleanupPrevious(strategy, previous)
-	}
-	return nil
+	// Clean up orphaned deployments
+	return i.cleanupOrphanedDeployments(strategy.DeploymentSpecs)
 }
 
 // CheckInstalled can return nil (installed), or errors
@@ -181,3 +176,39 @@ func (i *StrategyDeploymentInstaller) checkForDeployments(deploymentSpecs []Stra
 	}
 	return nil
 }
+
+// Clean up orphaned deployments after reinstalling deployments process
+func (i *StrategyDeploymentInstaller) cleanupOrphanedDeployments(deploymentSpecs []StrategyDeploymentSpec) error {
+	// Map of deployments
+	depNames := map[string]string{}
+	for _, dep := range deploymentSpecs {
+		depNames[dep.Name] = dep.Name
+	}
+
+	// Check the owner is a CSV
+	csv, ok := i.owner.(*v1alpha1.ClusterServiceVersion)
+	if !ok {
+		return fmt.Errorf("owner %s is not a CSV", i.owner.GetName())
+	}
+
+	// Get existing deployments in CSV's namespace and owned by CSV
+	existingDeployments, err := i.strategyClient.FindAnyDeploymentsMatchingLabels(ownerutil.CSVOwnerSelector(csv))
+	if err != nil {
+		return err
+	}
+
+	// compare existing deployments to deployments in CSV's spec to see if any need to be deleted
+	for _, d := range existingDeployments {
+		if _, exists := depNames[d.GetName()]; !exists {
+			if ownerutil.IsOwnedBy(d, i.owner) {
+				log.Infof("found an orphaned deployment %s in namespace %s", d.GetName(), i.owner.GetNamespace())
+				if err := i.strategyClient.DeleteDeployment(d.GetName()); err != nil {
+					log.Warnf("error cleaning up deployment %s", d.GetName())
+					return err
+				}
+			}
+		}
+	}
+
+	return nil
+}

pkg/controller/install/deployment_test.go

@@ -332,3 +332,115 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 		})
 	}
 }
+
+func TestInstallStrategyDeploymentCleanupDeployments(t *testing.T) {
+	var (
+		mockOwner = v1alpha1.ClusterServiceVersion{
+			TypeMeta: metav1.TypeMeta{
+				Kind:       v1alpha1.ClusterServiceVersionKind,
+				APIVersion: v1alpha1.ClusterServiceVersionAPIVersion,
+			},
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "clusterserviceversion-owner",
+				Namespace: "olm-test-deployment",
+			},
+		}
+		mockOwnerRefs = []metav1.OwnerReference{{
+			APIVersion:         v1alpha1.ClusterServiceVersionAPIVersion,
+			Kind:               v1alpha1.ClusterServiceVersionKind,
+			Name:               mockOwner.GetName(),
+			UID:                mockOwner.UID,
+			Controller:         &ownerutil.NotController,
+			BlockOwnerDeletion: &ownerutil.DontBlockOwnerDeletion,
+		}}
+	)
+
+	type inputs struct {
+		strategyDeploymentSpecs []StrategyDeploymentSpec
+	}
+	type setup struct {
+		existingDeployments []*appsv1.Deployment
+	}
+	type cleanupMock struct {
+		deletedDeployment appsv1.Deployment
+		returnError       error
+	}
+	tests := []struct {
+		description string
+		inputs      inputs
+		setup       setup
+		cleanupMock cleanupMock
+		output      error
+	}{
+		{
+			description: "cleanup successfully",
+			inputs: inputs{
+				strategyDeploymentSpecs: []StrategyDeploymentSpec{
+					{
+						Name: "test-deployment-1",
+						Spec: appsv1.DeploymentSpec{},
+					},
+				},
+			},
+			setup: setup{
+				existingDeployments: []*appsv1.Deployment{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name:            "test-deployment-2",
+							Namespace:       mockOwner.GetNamespace(),
+							OwnerReferences: mockOwnerRefs,
+							Labels: map[string]string{
+								"olm.owner":           mockOwner.GetName(),
+								"olm.owner.namespace": mockOwner.GetNamespace(),
+							},
+						},
+					},
+				},
+			},
+			cleanupMock: cleanupMock{
+				deletedDeployment: appsv1.Deployment{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:            "test-deployment-2",
+						Namespace:       mockOwner.GetNamespace(),
+						OwnerReferences: mockOwnerRefs,
+						Labels: map[string]string{
+							"olm.owner":           mockOwner.GetName(),
+							"olm.owner.namespace": mockOwner.GetNamespace(),
+						},
+					},
+					Spec: appsv1.DeploymentSpec{
+						Template: corev1.PodTemplateSpec{
+							ObjectMeta: metav1.ObjectMeta{
+								Annotations: map[string]string{},
+							},
+						},
+					},
+				},
+				returnError: nil,
+			},
+			output: nil,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.description, func(t *testing.T) {
+			fakeClient := new(clientfakes.FakeInstallStrategyDeploymentInterface)
+			installer := &StrategyDeploymentInstaller{
+				strategyClient: fakeClient,
+				owner:          &mockOwner,
+			}
+
+			fakeClient.FindAnyDeploymentsMatchingLabelsReturns(
+				tt.setup.existingDeployments, nil,
+			)
+			fakeClient.DeleteDeploymentReturns(tt.cleanupMock.returnError)
+			defer func() {
+				deletedDep := fakeClient.DeleteDeploymentArgsForCall(0)
+				require.Equal(t, tt.cleanupMock.deletedDeployment.GetName(), deletedDep)
+			}()
+
+			result := installer.cleanupOrphanedDeployments(tt.inputs.strategyDeploymentSpecs)
+			assert.Equal(t, tt.output, result)
+		})
+	}
+}

pkg/controller/operators/olm/operator.go

@@ -953,10 +953,6 @@ func (a *Operator) transitionCSVState(in v1alpha1.ClusterServiceVersion) (out *v
 			}
 		}
 
-		// Clean up deployments in case some become orphaned
-		if cleanupErr := a.cleanupOrphanedDeployments(logger, out); cleanupErr == nil {
-			logger.WithField("strategy", out.Spec.InstallStrategy.StrategyName).Infof("clean up deployments successful")
-		}
 	case v1alpha1.CSVPhaseSucceeded:
 		installer, strategy, _ := a.parseStrategiesAndUpdateStatus(out)
 		if strategy == nil {
@@ -1458,7 +1454,6 @@ func (a *Operator) ensureDeploymentAnnotations(logger *logrus.Entry, csv *v1alph
 	return utilerrors.NewAggregate(updateErrs)
 }
 
-<<<<<<< HEAD
 // ensureLabels merges a label set with a CSV's labels and attempts to update the CSV if the merged set differs from the CSV's original labels.
 func (a *Operator) ensureLabels(in *v1alpha1.ClusterServiceVersion, labelSets ...labels.Set) (*v1alpha1.ClusterServiceVersion, error) {
 	csvLabelSet := labels.Set(in.GetLabels())
@@ -1476,44 +1471,4 @@ func (a *Operator) ensureLabels(in *v1alpha1.ClusterServiceVersion, labelSets ..
 	out.SetLabels(merged)
 	out, err := a.client.OperatorsV1alpha1().ClusterServiceVersions(out.GetNamespace()).Update(out)
 	return out, err
-=======
-// Clean up orphaned deployments after reinstalling deployments process
-func (a *Operator) cleanupOrphanedDeployments(logger *logrus.Entry, csv *v1alpha1.ClusterServiceVersion) error {
-	// Extract the InstallStrategy for the deployment
-	strategy, err := a.resolver.UnmarshalStrategy(csv.Spec.InstallStrategy)
-	if err != nil {
-		logger.Warn("could not parse install strategy while cleaning up CSV deployment")
-		return nil
-	}
-
-	// Assume the strategy is for a deployment
-	strategyDetailsDeployment, ok := strategy.(*install.StrategyDetailsDeployment)
-	if !ok {
-		logger.Warnf("could not cast install strategy as type %T", strategyDetailsDeployment)
-		return nil
-	}
-
-	depNames := map[string]string{}
-	for _, dep := range strategyDetailsDeployment.DeploymentSpecs {
-		depNames[dep.Name] = dep.Name
-	}
-	// Get existing deployments in CSV's namespace and owned by CSV
-	existingDeployments, err := a.lister.AppsV1().DeploymentLister().Deployments(csv.GetNamespace()).List(ownerutil.CSVOwnerSelector(csv))
-	if err != nil {
-		return err
-	}
-
-	// compare existing deployments to deployments in CSV's spec to see if any need to be deleted
-	for _, d := range existingDeployments {
-		if _, exists := depNames[d.GetName()]; !exists {
-			logger.Infof("found an orphaned deployment %s in namespace %s", d.GetName(), csv.GetNamespace())
-			if err := a.OpClient.DeleteDeployment(csv.GetNamespace(), d.GetName(), &metav1.DeleteOptions{}); err != nil {
-				logger.Warnf("error cleaning up deployment %s", d.GetName())
-				return err
-			}
-		}
-	}
-
-	return nil
->>>>>>> fix(deployment): Clean up orphaned deployments
 }