Merge pull request #1745 from awgreene/hco-bug

Bug 1874938: Set RevisionHistoryLimit per Deployment

Author: openshift-merge-robot

pkg/controller/install/deployment.go

@@ -150,8 +150,16 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1
 		return
 	}
 
+	// OLM does not support Rollbacks.
+	// By default, each deployment created by OLM could spawn up to 10 replicaSets.
+	// By setting the deployments revisionHistoryLimit to 1, OLM will only create up
+	// to 2 ReplicaSets per deployment it manages, saving memory.
+	revisionHistoryLimit := int32(1)
+	dep.Spec.RevisionHistoryLimit = &revisionHistoryLimit
+
 	hash = HashDeploymentSpec(dep.Spec)
 	dep.Labels[DeploymentSpecHashLabelKey] = hash
+
 	deployment = dep
 	return
 }

pkg/controller/install/deployment_test.go

@@ -101,6 +101,8 @@ func TestInstallStrategyDeploymentInstallDeployments(t *testing.T) {
 			Controller:         &ownerutil.NotController,
 			BlockOwnerDeletion: &ownerutil.DontBlockOwnerDeletion,
 		}}
+		expectedRevisionHistoryLimit = int32(1)
+		defaultRevisionHistoryLimit  = int32(10)
 	)
 
 	type inputs struct {
@@ -126,11 +128,15 @@ func TestInstallStrategyDeploymentInstallDeployments(t *testing.T) {
 				strategyDeploymentSpecs: []v1alpha1.StrategyDeploymentSpec{
 					{
 						Name: "test-deployment-1",
-						Spec: appsv1.DeploymentSpec{},
+						Spec: appsv1.DeploymentSpec{
+							RevisionHistoryLimit: &defaultRevisionHistoryLimit,
+						},
 					},
 					{
 						Name: "test-deployment-2",
-						Spec: appsv1.DeploymentSpec{},
+						Spec: appsv1.DeploymentSpec{
+							RevisionHistoryLimit: nil,
+						},
 					},
 					{
 						Name: "test-deployment-3",
@@ -168,6 +174,7 @@ func TestInstallStrategyDeploymentInstallDeployments(t *testing.T) {
 							},
 						},
 						Spec: appsv1.DeploymentSpec{
+							RevisionHistoryLimit: &expectedRevisionHistoryLimit,
 							Template: corev1.PodTemplateSpec{
 								ObjectMeta: metav1.ObjectMeta{
 									Annotations: map[string]string{},
@@ -189,6 +196,7 @@ func TestInstallStrategyDeploymentInstallDeployments(t *testing.T) {
 							},
 						},
 						Spec: appsv1.DeploymentSpec{
+							RevisionHistoryLimit: &expectedRevisionHistoryLimit,
 							Template: corev1.PodTemplateSpec{
 								ObjectMeta: metav1.ObjectMeta{
 									Annotations: map[string]string{},
@@ -210,6 +218,7 @@ func TestInstallStrategyDeploymentInstallDeployments(t *testing.T) {
 							},
 						},
 						Spec: appsv1.DeploymentSpec{
+							RevisionHistoryLimit: &expectedRevisionHistoryLimit,
 							Template: corev1.PodTemplateSpec{
 								ObjectMeta: metav1.ObjectMeta{
 									Annotations: map[string]string{},
@@ -234,6 +243,7 @@ func TestInstallStrategyDeploymentInstallDeployments(t *testing.T) {
 					dep := fakeClient.CreateOrUpdateDeploymentArgsForCall(i)
 					expectedDeployment.Spec.Template.Annotations = map[string]string{}
 					require.Equal(t, expectedDeployment.OwnerReferences, dep.OwnerReferences)
+					require.Equal(t, expectedDeployment.Spec.RevisionHistoryLimit, dep.Spec.RevisionHistoryLimit)
 				}(i, m.expectedDeployment)
 			}
 
@@ -299,6 +309,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 		},
 	}
 
+	revisionHistoryLimit := int32(1)
 	for _, tt := range tests {
 		t.Run(tt.description, func(t *testing.T) {
 			fakeClient := new(clientfakes.FakeInstallStrategyDeploymentInterface)
@@ -307,6 +318,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 
 			dep := testDeployment("olm-dep-1", namespace, &mockOwner)
 			dep.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
+			dep.Spec.RevisionHistoryLimit = &revisionHistoryLimit
 			dep.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(dep.Spec)))
 			fakeClient.FindAnyDeploymentsMatchingLabelsReturns(
 				[]*appsv1.Deployment{
@@ -323,6 +335,7 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 
 			deployment := testDeployment("olm-dep-1", namespace, &mockOwner)
 			deployment.Spec.Template.SetAnnotations(map[string]string{"test": "annotation"})
+			deployment.Spec.RevisionHistoryLimit = &revisionHistoryLimit
 			deployment.SetLabels(labels.CloneAndAddLabel(dep.ObjectMeta.GetLabels(), DeploymentSpecHashLabelKey, HashDeploymentSpec(deployment.Spec)))
 			fakeClient.CreateOrUpdateDeploymentReturns(&deployment, tt.createDeploymentErr)
 			defer func() {
@@ -332,7 +345,6 @@ func TestInstallStrategyDeploymentCheckInstallErrors(t *testing.T) {
 			if tt.createDeploymentErr != nil {
 				err := installer.Install(strategy)
 				require.Error(t, err)
-				return
 			}
 		})
 	}

pkg/controller/operators/olm/operator_test.go

@@ -8,7 +8,6 @@ import (
 	"crypto/x509"
 	"crypto/x509/pkix"
 	"fmt"
-	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 	"math"
 	"math/big"
 	"reflect"
@@ -62,6 +61,7 @@ import (
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/ownerutil"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/queueinformer"
 	"github.com/operator-framework/operator-lifecycle-manager/pkg/lib/scoped"
+	opregistry "github.com/operator-framework/operator-registry/pkg/registry"
 )
 
 type TestStrategy struct{}
@@ -326,7 +326,10 @@ func buildFakeAPIIntersectionReconcilerThatReturns(result resolver.APIReconcilia
 }
 
 func deployment(deploymentName, namespace, serviceAccountName string, templateAnnotations map[string]string) *appsv1.Deployment {
-	var singleInstance = int32(1)
+	var (
+		singleInstance       = int32(1)
+		revisionHistoryLimit = int32(1)
+	)
 	return &appsv1.Deployment{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      deploymentName,
@@ -338,7 +341,8 @@ func deployment(deploymentName, namespace, serviceAccountName string, templateAn
 					"app": deploymentName,
 				},
 			},
-			Replicas: &singleInstance,
+			RevisionHistoryLimit: &revisionHistoryLimit,
+			Replicas:             &singleInstance,
 			Template: corev1.PodTemplateSpec{
 				ObjectMeta: metav1.ObjectMeta{
 					Labels: map[string]string{