Repository: argoproj-labs/argocd-operator
Pull request #903 (2023-05-08 18:27:31)

bug: fix proper reconciliation of hpa object
**What type of PR is this?**
> Uncomment only one ` /kind` line, and delete the rest.
> For example, `> /kind bug` would simply become: `/kind bug`

> /kind bug
> /kind chore
> /kind cleanup
> /kind failing-test
> /kind enhancement
> /kind documentation
> /kind code-refactoring


**What does this PR do / why we need it**:
port https://github.com/argoproj-labs/argocd-operator/pull/870 to 0.6 branch
**Have you updated the necessary documentation?**

* [ ] Documentation update is required by this PR.
* [ ] Documentation has been updated.

**Which issue(s) this PR fixes**:

Fixes #?

**How to test changes / Special notes to the reviewer**:



Patch:
diff --git a/api/v1alpha1/argocd_types.go b/api/v1alpha1/argocd_types.go
index fa1c2fa9..8e257ddb 100644
--- a/api/v1alpha1/argocd_types.go
+++ b/api/v1alpha1/argocd_types.go
@@ -39,22 +39,22 @@ func init() {
 // ArgoCD is the Schema for the argocds API
 // +k8s:openapi-gen=true
 // +kubebuilder:subresource:status
-//+operator-sdk:csv:customresourcedefinitions:resources={{ArgoCD,v1alpha1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ArgoCDExport,v1alpha1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ConfigMap,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{CronJob,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Deployment,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Ingress,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Job,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{PersistentVolumeClaim,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Pod,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Prometheus,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ReplicaSet,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Route,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Secret,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Service,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ServiceMonitor,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{StatefulSet,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ArgoCD,v1alpha1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ArgoCDExport,v1alpha1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ConfigMap,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{CronJob,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Deployment,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Ingress,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Job,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{PersistentVolumeClaim,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Pod,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Prometheus,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ReplicaSet,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Route,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Secret,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Service,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ServiceMonitor,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{StatefulSet,v1,""}}
 type ArgoCD struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
@@ -637,7 +637,7 @@ type ArgoCDMonitoringSpec struct {
 	Enabled bool `json:"enabled"`
 }
 
-//ArgoCDNodePlacementSpec is used to specify NodeSelector and Tolerations for Argo CD workloads
+// ArgoCDNodePlacementSpec is used to specify NodeSelector and Tolerations for Argo CD workloads
 type ArgoCDNodePlacementSpec struct {
 	// NodeSelector is a field of PodSpec, it is a map of key value pairs used for node selection
 	NodeSelector map[string]string `json:"nodeSelector,omitempty"`
diff --git a/api/v1alpha1/argocdexport_types.go b/api/v1alpha1/argocdexport_types.go
index eb7e1248..f9e0e222 100644
--- a/api/v1alpha1/argocdexport_types.go
+++ b/api/v1alpha1/argocdexport_types.go
@@ -30,22 +30,22 @@ import (
 // +k8s:openapi-gen=true
 // +kubebuilder:subresource:status
 // +kubebuilder:resource:path=argocdexports,scope=Namespaced
-//+operator-sdk:csv:customresourcedefinitions:resources={{ArgoCD,v1alpha1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ArgoCDExport,v1alpha1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ConfigMap,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{CronJob,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Deployment,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Ingress,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Job,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{PersistentVolumeClaim,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Pod,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Prometheus,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ReplicaSet,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Route,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Secret,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{Service,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{ServiceMonitor,v1,""}}
-//+operator-sdk:csv:customresourcedefinitions:resources={{StatefulSet,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ArgoCD,v1alpha1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ArgoCDExport,v1alpha1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ConfigMap,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{CronJob,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Deployment,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Ingress,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Job,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{PersistentVolumeClaim,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Pod,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Prometheus,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ReplicaSet,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Route,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Secret,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{Service,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{ServiceMonitor,v1,""}}
+// +operator-sdk:csv:customresourcedefinitions:resources={{StatefulSet,v1,""}}
 type ArgoCDExport struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
diff --git a/api/v1alpha1/groupversion_info.go b/api/v1alpha1/groupversion_info.go
index 529170a0..54238ded 100644
--- a/api/v1alpha1/groupversion_info.go
+++ b/api/v1alpha1/groupversion_info.go
@@ -15,8 +15,8 @@ limitations under the License.
 */
 
 // Package v1alpha1 contains API Schema definitions for the argoproj.io v1alpha1 API group
-//+kubebuilder:object:generate=true
-//+groupName=argoproj.io
+// +kubebuilder:object:generate=true
+// +groupName=argoproj.io
 package v1alpha1
 
 import (
diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go
index 2684ac03..0fa61bd8 100644
--- a/controllers/argocd/deployment.go
+++ b/controllers/argocd/deployment.go
@@ -1315,8 +1315,10 @@ func (r *ReconcileArgoCD) reconcileServerDeployment(cr *argoprojv1a1.ArgoCD, use
 			changed = true
 		}
 		if !reflect.DeepEqual(deploy.Spec.Replicas, existing.Spec.Replicas) {
-			existing.Spec.Replicas = deploy.Spec.Replicas
-			changed = true
+			if !cr.Spec.Server.Autoscale.Enabled {
+				existing.Spec.Replicas = deploy.Spec.Replicas
+				changed = true
+			}
 		}
 		if changed {
 			return r.Client.Update(context.TODO(), existing)
diff --git a/controllers/argocd/hpa.go b/controllers/argocd/hpa.go
index b289ca5e..0cee4a3c 100644
--- a/controllers/argocd/hpa.go
+++ b/controllers/argocd/hpa.go
@@ -16,6 +16,7 @@ package argocd
 
 import (
 	"context"
+	"reflect"
 
 	autoscaling "k8s.io/api/autoscaling/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -25,6 +26,12 @@ import (
 	"github.com/argoproj-labs/argocd-operator/controllers/argoutil"
 )
 
+var (
+	maxReplicas int32 = 3
+	minReplicas int32 = 1
+	tcup        int32 = 50
+)
+
 func newHorizontalPodAutoscaler(cr *argoprojv1a1.ArgoCD) *autoscaling.HorizontalPodAutoscaler {
 	return &autoscaling.HorizontalPodAutoscaler{
 		ObjectMeta: metav1.ObjectMeta{
@@ -50,39 +57,54 @@ func newHorizontalPodAutoscalerWithSuffix(suffix string, cr *argoprojv1a1.ArgoCD
 	return newHorizontalPodAutoscalerWithName(nameWithSuffix(suffix, cr), cr)
 }
 
-// reconcileServerHPA will ensure that the HorizontalPodAutoscaler is present for the Argo CD Server component.
+// reconcileServerHPA will ensure that the HorizontalPodAutoscaler is present for the Argo CD Server component, and reconcile any detected changes.
 func (r *ReconcileArgoCD) reconcileServerHPA(cr *argoprojv1a1.ArgoCD) error {
-	hpa := newHorizontalPodAutoscalerWithSuffix("server", cr)
-	if argoutil.IsObjectFound(r.Client, cr.Namespace, hpa.Name, hpa) {
+
+	defaultHPA := newHorizontalPodAutoscalerWithSuffix("server", cr)
+	defaultHPA.Spec = autoscaling.HorizontalPodAutoscalerSpec{
+		MaxReplicas:                    maxReplicas,
+		MinReplicas:                    &minReplicas,
+		TargetCPUUtilizationPercentage: &tcup,
+		ScaleTargetRef: autoscaling.CrossVersionObjectReference{
+			APIVersion: "apps/v1",
+			Kind:       "Deployment",
+			Name:       nameWithSuffix("server", cr),
+		},
+	}
+
+	existingHPA := newHorizontalPodAutoscalerWithSuffix("server", cr)
+	if argoutil.IsObjectFound(r.Client, cr.Namespace, existingHPA.Name, existingHPA) {
 		if !cr.Spec.Server.Autoscale.Enabled {
-			return r.Client.Delete(context.TODO(), hpa) // HorizontalPodAutoscaler found but globally disabled, delete it.
+			return r.Client.Delete(context.TODO(), existingHPA) // HorizontalPodAutoscaler found but globally disabled, delete it.
+		}
+
+		changed := false
+		// HorizontalPodAutoscaler found, reconcile if necessary changes detected
+		if cr.Spec.Server.Autoscale.HPA != nil {
+			if !reflect.DeepEqual(existingHPA.Spec, cr.Spec.Server.Autoscale.HPA) {
+				existingHPA.Spec = *cr.Spec.Server.Autoscale.HPA
+				changed = true
+			}
 		}
-		return nil // HorizontalPodAutoscaler found and configured, nothing do to, move along...
+
+		if changed {
+			return r.Client.Update(context.TODO(), existingHPA)
+		}
+
+		// HorizontalPodAutoscaler found, no changes detected
+		return nil
 	}
 
 	if !cr.Spec.Server.Autoscale.Enabled {
 		return nil // AutoScale not enabled, move along...
 	}
 
+	// AutoScale enabled, no existing HPA found, create
 	if cr.Spec.Server.Autoscale.HPA != nil {
-		hpa.Spec = *cr.Spec.Server.Autoscale.HPA
-	} else {
-		hpa.Spec.MaxReplicas = 3
-
-		var minrReplicas int32 = 1
-		hpa.Spec.MinReplicas = &minrReplicas
-
-		var tcup int32 = 50
-		hpa.Spec.TargetCPUUtilizationPercentage = &tcup
-
-		hpa.Spec.ScaleTargetRef = autoscaling.CrossVersionObjectReference{
-			APIVersion: "apps/v1",
-			Kind:       "Deployment",
-			Name:       nameWithSuffix("server", cr),
-		}
+		defaultHPA.Spec = *cr.Spec.Server.Autoscale.HPA
 	}
 
-	return r.Client.Create(context.TODO(), hpa)
+	return r.Client.Create(context.TODO(), defaultHPA)
 }
 
 // reconcileAutoscalers will ensure that all HorizontalPodAutoscalers are present for the given ArgoCD.
diff --git a/docs/reference/argocd.md b/docs/reference/argocd.md
index 8d1a0427..5a22ff3d 100644
--- a/docs/reference/argocd.md
+++ b/docs/reference/argocd.md
@@ -1303,6 +1303,9 @@ Name | Default | Description
 Enabled | false | Toggle Autoscaling support globally for the Argo CD server component.
 HPA | [Object] | HorizontalPodAutoscaler options for the Argo CD Server component.
 
+!!! note
+    When `.spec.server.autoscale.enabled` is set to `true`, the number of required replicas (if set) in `.spec.server.replicas` will be ignored. The final replica count on the server deployment will be controlled by the Horizontal Pod Autoscaler instead. 
+
 ### Server Command Arguments
 
 Allows a user to pass arguments to Argo CD Server command.


Test patch:
diff --git a/controllers/argocd/deployment_test.go b/controllers/argocd/deployment_test.go
index 7cb5b378..11f1a9ad 100644
--- a/controllers/argocd/deployment_test.go
+++ b/controllers/argocd/deployment_test.go
@@ -87,50 +87,71 @@ func TestReconcileArgoCD_reconcileRepoDeployment_replicas(t *testing.T) {
 func TestReconcileArgoCD_reconcile_ServerDeployment_replicas(t *testing.T) {
 	logf.SetLogger(ZapLogger(true))
 
+	var (
+		initalReplicas  int32 = 4
+		updatedReplicas int32 = 5
+	)
+
 	tests := []struct {
-		name          string
-		replicas      int32
-		autoscale     bool
-		expectedNil   bool
-		expectedValue int32
+		name              string
+		initialReplicas   *int32
+		updatedReplicas   *int32
+		autoscale         bool
+		wantFinalReplicas *int32
 	}{
 		{
-			name:          "replicas field in the spec should reflect the number of replicas on the cluster",
-			replicas:      5,
-			autoscale:     false,
-			expectedNil:   false,
-			expectedValue: 5,
+			name:              "deployment spec replicas initially nil, updated by operator, no autoscale",
+			initialReplicas:   nil,
+			updatedReplicas:   &updatedReplicas,
+			autoscale:         false,
+			wantFinalReplicas: &updatedReplicas,
+		},
+		{
+			name:              "deployment spec replicas initially not nil, updated by operator, no autoscale",
+			initialReplicas:   &initalReplicas,
+			updatedReplicas:   &updatedReplicas,
+			autoscale:         false,
+			wantFinalReplicas: &updatedReplicas,
 		},
 		{
-			name:        "replicas should be overriden by autoscale",
-			replicas:    1,
-			autoscale:   true,
-			expectedNil: true,
+			name:              "deployment spec replicas initially nil, ignored by operator with autoscale",
+			initialReplicas:   nil,
+			updatedReplicas:   &updatedReplicas,
+			autoscale:         true,
+			wantFinalReplicas: nil,
 		},
 	}
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-
 			a := makeTestArgoCD(func(a *argoprojv1alpha1.ArgoCD) {
+				a.Spec.Server.Replicas = test.initialReplicas
 				a.Spec.Server.Autoscale.Enabled = test.autoscale
-				a.Spec.Server.Replicas = &test.replicas
 			})
 			r := makeTestReconciler(t, a)
 
 			err := r.reconcileServerDeployment(a, false)
 			assert.NoError(t, err)
-
 			deployment := &appsv1.Deployment{}
 			err = r.Client.Get(context.TODO(), types.NamespacedName{
 				Name:      "argocd-server",
 				Namespace: testNamespace,
 			}, deployment)
 			assert.NoError(t, err)
-			assert.Equal(t, test.expectedNil, deployment.Spec.Replicas == nil)
-			if deployment.Spec.Replicas != nil {
-				assert.Equal(t, test.expectedValue, *deployment.Spec.Replicas)
-			}
+			assert.Equal(t, test.initialReplicas, deployment.Spec.Replicas)
+
+			a.Spec.Server.Replicas = test.updatedReplicas
+			err = r.reconcileServerDeployment(a, false)
+			assert.NoError(t, err)
+
+			deployment = &appsv1.Deployment{}
+			err = r.Client.Get(context.TODO(), types.NamespacedName{
+				Name:      "argocd-server",
+				Namespace: testNamespace,
+			}, deployment)
+			assert.NoError(t, err)
+			assert.Equal(t, test.wantFinalReplicas, deployment.Spec.Replicas)
+
 		})
 	}
 }
diff --git a/controllers/argocd/hpa_test.go b/controllers/argocd/hpa_test.go
new file mode 100644
index 00000000..1090bff8
--- /dev/null
+++ b/controllers/argocd/hpa_test.go
@@ -0,0 +1,91 @@
+package argocd
+
+import (
+	"context"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	autoscaling "k8s.io/api/autoscaling/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/types"
+	logf "sigs.k8s.io/controller-runtime/pkg/log"
+)
+
+var (
+	min     int32 = 5
+	max     int32 = 8
+	cpuUtil int32 = 45
+)
+
+func TestReconcileHPA(t *testing.T) {
+
+	logf.SetLogger(ZapLogger(true))
+	a := makeTestArgoCD()
+	r := makeTestReconciler(t, a)
+
+	existingHPA := newHorizontalPodAutoscalerWithSuffix("server", a)
+
+	defaultHPASpec := autoscaling.HorizontalPodAutoscalerSpec{
+		MaxReplicas:                    maxReplicas,
+		MinReplicas:                    &minReplicas,
+		TargetCPUUtilizationPercentage: &tcup,
+		ScaleTargetRef: autoscaling.CrossVersionObjectReference{
+			APIVersion: "apps/v1",
+			Kind:       "Deployment",
+			Name:       nameWithSuffix("server", a),
+		},
+	}
+
+	updatedHPASpec := autoscaling.HorizontalPodAutoscalerSpec{
+		MaxReplicas:                    max,
+		MinReplicas:                    &min,
+		TargetCPUUtilizationPercentage: &cpuUtil,
+		ScaleTargetRef: autoscaling.CrossVersionObjectReference{
+			APIVersion: "apps/v1",
+			Kind:       "Deployment",
+			Name:       nameWithSuffix("server", a),
+		},
+	}
+
+	err := r.Client.Get(context.TODO(), types.NamespacedName{
+		Name:      "argocd-server",
+		Namespace: testNamespace,
+	}, existingHPA)
+	assert.True(t, errors.IsNotFound(err))
+
+	a.Spec.Server.Autoscale.Enabled = true
+
+	err = r.reconcileServerHPA(a)
+	assert.NoError(t, err)
+
+	err = r.Client.Get(context.TODO(), types.NamespacedName{
+		Name:      "argocd-server",
+		Namespace: testNamespace,
+	}, existingHPA)
+	assert.NoError(t, err)
+	assert.Equal(t, defaultHPASpec, existingHPA.Spec)
+
+	a.Spec.Server.Autoscale.HPA = &updatedHPASpec
+
+	err = r.reconcileServerHPA(a)
+	assert.NoError(t, err)
+
+	err = r.Client.Get(context.TODO(), types.NamespacedName{
+		Name:      "argocd-server",
+		Namespace: testNamespace,
+	}, existingHPA)
+	assert.NoError(t, err)
+	assert.Equal(t, updatedHPASpec, existingHPA.Spec)
+
+	a.Spec.Server.Autoscale.Enabled = false
+
+	err = r.reconcileServerHPA(a)
+	assert.NoError(t, err)
+
+	err = r.Client.Get(context.TODO(), types.NamespacedName{
+		Name:      "argocd-server",
+		Namespace: testNamespace,
+	}, existingHPA)
+	assert.True(t, errors.IsNotFound(err))
+
+}