fix: Reconcile redis resource limits when HA is enabled (#915)

* Fix redis resources reconciliation when HA is enabled Signed-off-by: Siddhesh Ghadi <[email protected]> * Apply resource limit updates on all redis containers Signed-off-by: Siddhesh Ghadi <[email protected]> * Redis should use resource limits from .spec.ha when ha is enabled. Signed-off-by: Siddhesh Ghadi <[email protected]> * Add e2e tests for redis ha Signed-off-by: Siddhesh Ghadi <[email protected]> * Add missing .spec.ha docs Signed-off-by: Siddhesh Ghadi <[email protected]> * Add empty ha test directory to resolve ci failures Signed-off-by: Siddhesh Ghadi <[email protected]> * Move redis ha test to ha directory Since ha testing requires minimum 3 node cluster, moving them to ha directory for better separation. Signed-off-by: Siddhesh Ghadi <[email protected]> * Add a doc note about redis resource limits when ha is enabled Signed-off-by: Siddhesh Ghadi <[email protected]> --------- Signed-off-by: Siddhesh Ghadi <[email protected]>
argoproj-labs · May 26, 2023 · 4913ddc · 4913ddc
1 parent 19b9bdc
commit 4913ddc
Show file tree

Hide file tree

Showing 24 changed files with 344 additions and 128 deletions.
diff --git a/.gitignore b/.gitignore
@@ -31,6 +31,7 @@ build/_output
 
 kubeconfig
 kuttl-test.json
+e2e.json
 
 # Desktop Services Store in macOS system
 .DS_Store
diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go
@@ -672,32 +672,6 @@ func (r *ReconcileArgoCD) reconcileRedisDeployment(cr *argoprojv1a1.ArgoCD, useT
 func (r *ReconcileArgoCD) reconcileRedisHAProxyDeployment(cr *argoprojv1a1.ArgoCD) error {
 	deploy := newDeploymentWithSuffix("redis-ha-haproxy", "redis", cr)
 
-	existing := newDeploymentWithSuffix("redis-ha-haproxy", "redis", cr)
-	if argoutil.IsObjectFound(r.Client, cr.Namespace, existing.Name, existing) {
-		if !cr.Spec.HA.Enabled {
-			// Deployment exists but HA enabled flag has been set to false, delete the Deployment
-			return r.Client.Delete(context.TODO(), existing)
-		}
-		changed := false
-		actualImage := existing.Spec.Template.Spec.Containers[0].Image
-		desiredImage := getRedisHAProxyContainerImage(cr)
-
-		if actualImage != desiredImage {
-			existing.Spec.Template.Spec.Containers[0].Image = desiredImage
-			existing.Spec.Template.ObjectMeta.Labels["image.upgraded"] = time.Now().UTC().Format("01022006-150406-MST")
-			changed = true
-		}
-		updateNodePlacement(existing, deploy, &changed)
-		if changed {
-			return r.Client.Update(context.TODO(), existing)
-		}
-		return nil // Deployment found, do nothing
-	}
-
-	if !cr.Spec.HA.Enabled {
-		return nil // HA not enabled, do nothing.
-	}
-
 	deploy.Spec.Template.Spec.Affinity = &corev1.Affinity{
 		PodAntiAffinity: &corev1.PodAntiAffinity{
 			PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{
@@ -747,7 +721,7 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyDeployment(cr *argoprojv1a1.ArgoC
 				Name:          "redis",
 			},
 		},
-		Resources: getRedisHAProxyResources(cr),
+		Resources: getRedisHAResources(cr),
 		SecurityContext: &corev1.SecurityContext{
 			AllowPrivilegeEscalation: boolPtr(false),
 			Capabilities: &corev1.Capabilities{
@@ -784,7 +758,7 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyDeployment(cr *argoprojv1a1.ArgoC
 		ImagePullPolicy: corev1.PullIfNotPresent,
 		Name:            "config-init",
 		Env:             proxyEnvVars(),
-		Resources:       getRedisHAProxyResources(cr),
+		Resources:       getRedisHAResources(cr),
 		SecurityContext: &corev1.SecurityContext{
 			AllowPrivilegeEscalation: boolPtr(false),
 			Capabilities: &corev1.Capabilities{
@@ -857,6 +831,43 @@ func (r *ReconcileArgoCD) reconcileRedisHAProxyDeployment(cr *argoprojv1a1.ArgoC
 		return err
 	}
 
+	existing := newDeploymentWithSuffix("redis-ha-haproxy", "redis", cr)
+	if argoutil.IsObjectFound(r.Client, cr.Namespace, existing.Name, existing) {
+		if !cr.Spec.HA.Enabled {
+			// Deployment exists but HA enabled flag has been set to false, delete the Deployment
+			return r.Client.Delete(context.TODO(), existing)
+		}
+		changed := false
+		actualImage := existing.Spec.Template.Spec.Containers[0].Image
+		desiredImage := getRedisHAProxyContainerImage(cr)
+
+		if actualImage != desiredImage {
+			existing.Spec.Template.Spec.Containers[0].Image = desiredImage
+			existing.Spec.Template.ObjectMeta.Labels["image.upgraded"] = time.Now().UTC().Format("01022006-150406-MST")
+			changed = true
+		}
+		updateNodePlacement(existing, deploy, &changed)
+
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.Containers[0].Resources, existing.Spec.Template.Spec.Containers[0].Resources) {
+			existing.Spec.Template.Spec.Containers[0].Resources = deploy.Spec.Template.Spec.Containers[0].Resources
+			changed = true
+		}
+
+		if !reflect.DeepEqual(deploy.Spec.Template.Spec.InitContainers[0].Resources, existing.Spec.Template.Spec.InitContainers[0].Resources) {
+			existing.Spec.Template.Spec.InitContainers[0].Resources = deploy.Spec.Template.Spec.InitContainers[0].Resources
+			changed = true
+		}
+
+		if changed {
+			return r.Client.Update(context.TODO(), existing)
+		}
+		return nil // Deployment found, do nothing
+	}
+
+	if !cr.Spec.HA.Enabled {
+		return nil // HA not enabled, do nothing.
+	}
+
 	if err := controllerutil.SetControllerReference(cr, deploy, r.Scheme); err != nil {
 		return err
 	}

diff --git a/controllers/argocd/deployment_test.go b/controllers/argocd/deployment_test.go
@@ -644,6 +644,7 @@ func TestReconcileArgoCD_reconcileDeployments_HA_proxy_with_resources(t *testing
 	})
 	r := makeTestReconciler(t, a)
 
+	// test resource is Created on reconciliation
 	assert.NoError(t, r.reconcileRedisHAProxyDeployment(a))
 
 	deployment := &appsv1.Deployment{}
@@ -667,6 +668,31 @@ func TestReconcileArgoCD_reconcileDeployments_HA_proxy_with_resources(t *testing
 	}
 	assert.Equal(t, deployment.Spec.Template.Spec.Containers[0].Resources, testResources)
 	assert.Equal(t, deployment.Spec.Template.Spec.InitContainers[0].Resources, testResources)
+
+	// test resource is Updated on reconciliation
+	newResources := corev1.ResourceRequirements{
+		Requests: corev1.ResourceList{
+			corev1.ResourceMemory: resourcev1.MustParse("256Mi"),
+			corev1.ResourceCPU:    resourcev1.MustParse("500m"),
+		},
+		Limits: corev1.ResourceList{
+			corev1.ResourceMemory: resourcev1.MustParse("512Mi"),
+			corev1.ResourceCPU:    resourcev1.MustParse("1"),
+		},
+	}
+	a.Spec.HA.Resources = &newResources
+	assert.NoError(t, r.reconcileRedisHAProxyDeployment(a))
+
+	assert.NoError(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      a.Name + "-redis-ha-haproxy",
+			Namespace: a.Namespace,
+		},
+		deployment))
+
+	assert.Equal(t, deployment.Spec.Template.Spec.Containers[0].Resources, newResources)
+	assert.Equal(t, deployment.Spec.Template.Spec.InitContainers[0].Resources, newResources)
 }
 
 func TestReconcileArgoCD_reconcileRepoDeployment_updatesVolumeMounts(t *testing.T) {

diff --git a/controllers/argocd/statefulset.go b/controllers/argocd/statefulset.go
@@ -93,35 +93,6 @@ func newStatefulSetWithSuffix(suffix string, component string, cr *argoprojv1a1.
 func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoprojv1a1.ArgoCD) error {
 	ss := newStatefulSetWithSuffix("redis-ha-server", "redis", cr)
 
-	existing := newStatefulSetWithSuffix("redis-ha-server", "redis", cr)
-	if argoutil.IsObjectFound(r.Client, cr.Namespace, existing.Name, existing) {
-		if !cr.Spec.HA.Enabled {
-			// StatefulSet exists but HA enabled flag has been set to false, delete the StatefulSet
-			return r.Client.Delete(context.TODO(), existing)
-		}
-
-		desiredImage := getRedisHAContainerImage(cr)
-		changed := false
-		updateNodePlacementStateful(existing, ss, &changed)
-		for i, container := range existing.Spec.Template.Spec.Containers {
-			if container.Image != desiredImage {
-				existing.Spec.Template.Spec.Containers[i].Image = getRedisHAContainerImage(cr)
-				existing.Spec.Template.ObjectMeta.Labels["image.upgraded"] = time.Now().UTC().Format("01022006-150406-MST")
-				changed = true
-			}
-		}
-
-		if changed {
-			return r.Client.Update(context.TODO(), existing)
-		}
-
-		return nil // StatefulSet found, do nothing
-	}
-
-	if !cr.Spec.HA.Enabled {
-		return nil // HA not enabled, do nothing.
-	}
-
 	ss.Spec.PodManagementPolicy = appsv1.OrderedReadyPodManagement
 	ss.Spec.Replicas = getRedisHAReplicas(cr)
 	ss.Spec.Selector = &metav1.LabelSelector{
@@ -204,7 +175,7 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoprojv1a1.ArgoCD) err
 				SuccessThreshold:    int32(1),
 				TimeoutSeconds:      int32(15),
 			},
-			Resources: getRedisResources(cr),
+			Resources: getRedisHAResources(cr),
 			SecurityContext: &corev1.SecurityContext{
 				AllowPrivilegeEscalation: boolPtr(false),
 				Capabilities: &corev1.Capabilities{
@@ -275,7 +246,7 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoprojv1a1.ArgoCD) err
 				SuccessThreshold:    int32(1),
 				TimeoutSeconds:      int32(15),
 			},
-			Resources: getRedisResources(cr),
+			Resources: getRedisHAResources(cr),
 			SecurityContext: &corev1.SecurityContext{
 				AllowPrivilegeEscalation: boolPtr(false),
 				Capabilities: &corev1.Capabilities{
@@ -326,7 +297,7 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoprojv1a1.ArgoCD) err
 		Image:           getRedisHAContainerImage(cr),
 		ImagePullPolicy: corev1.PullIfNotPresent,
 		Name:            "config-init",
-		Resources:       getRedisResources(cr),
+		Resources:       getRedisHAResources(cr),
 		SecurityContext: &corev1.SecurityContext{
 			AllowPrivilegeEscalation: boolPtr(false),
 			Capabilities: &corev1.Capabilities{
@@ -417,6 +388,45 @@ func (r *ReconcileArgoCD) reconcileRedisStatefulSet(cr *argoprojv1a1.ArgoCD) err
 		return err
 	}
 
+	existing := newStatefulSetWithSuffix("redis-ha-server", "redis", cr)
+	if argoutil.IsObjectFound(r.Client, cr.Namespace, existing.Name, existing) {
+		if !cr.Spec.HA.Enabled {
+			// StatefulSet exists but HA enabled flag has been set to false, delete the StatefulSet
+			return r.Client.Delete(context.TODO(), existing)
+		}
+
+		desiredImage := getRedisHAContainerImage(cr)
+		changed := false
+		updateNodePlacementStateful(existing, ss, &changed)
+		for i, container := range existing.Spec.Template.Spec.Containers {
+			if container.Image != desiredImage {
+				existing.Spec.Template.Spec.Containers[i].Image = getRedisHAContainerImage(cr)
+				existing.Spec.Template.ObjectMeta.Labels["image.upgraded"] = time.Now().UTC().Format("01022006-150406-MST")
+				changed = true
+			}
+
+			if !reflect.DeepEqual(ss.Spec.Template.Spec.Containers[i].Resources, existing.Spec.Template.Spec.Containers[i].Resources) {
+				existing.Spec.Template.Spec.Containers[i].Resources = ss.Spec.Template.Spec.Containers[i].Resources
+				changed = true
+			}
+		}
+
+		if !reflect.DeepEqual(ss.Spec.Template.Spec.InitContainers[0].Resources, existing.Spec.Template.Spec.InitContainers[0].Resources) {
+			existing.Spec.Template.Spec.InitContainers[0].Resources = ss.Spec.Template.Spec.InitContainers[0].Resources
+			changed = true
+		}
+
+		if changed {
+			return r.Client.Update(context.TODO(), existing)
+		}
+
+		return nil // StatefulSet found, do nothing
+	}
+
+	if !cr.Spec.HA.Enabled {
+		return nil // HA not enabled, do nothing.
+	}
+
 	if err := controllerutil.SetControllerReference(cr, ss, r.Scheme); err != nil {
 		return err
 	}

diff --git a/controllers/argocd/statefulset_test.go b/controllers/argocd/statefulset_test.go
@@ -93,9 +93,24 @@ func TestReconcileArgoCD_reconcileRedisStatefulSet_HA_enabled(t *testing.T) {
 	// test resource is Updated on reconciliation
 	a.Spec.Redis.Image = testRedisImage
 	a.Spec.Redis.Version = testRedisImageVersion
+	newResources := corev1.ResourceRequirements{
+		Requests: corev1.ResourceList{
+			corev1.ResourceMemory: resourcev1.MustParse("256Mi"),
+			corev1.ResourceCPU:    resourcev1.MustParse("500m"),
+		},
+		Limits: corev1.ResourceList{
+			corev1.ResourceMemory: resourcev1.MustParse("512Mi"),
+			corev1.ResourceCPU:    resourcev1.MustParse("1"),
+		},
+	}
+	a.Spec.HA.Resources = &newResources
 	assert.NoError(t, r.reconcileRedisStatefulSet(a))
 	assert.NoError(t, r.Client.Get(context.TODO(), types.NamespacedName{Name: s.Name, Namespace: a.Namespace}, s))
-	assert.Equal(t, s.Spec.Template.Spec.Containers[0].Image, fmt.Sprintf("%s:%s", testRedisImage, testRedisImageVersion))
+	for _, container := range s.Spec.Template.Spec.Containers {
+		assert.Equal(t, container.Image, fmt.Sprintf("%s:%s", testRedisImage, testRedisImageVersion))
+		assert.Equal(t, container.Resources, newResources)
+	}
+	assert.Equal(t, s.Spec.Template.Spec.InitContainers[0].Resources, newResources)
 
 	// test resource is Deleted, when HA is disabled
 	a.Spec.HA.Enabled = false

diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go
@@ -512,8 +512,8 @@ func getRedisResources(cr *argoprojv1a1.ArgoCD) corev1.ResourceRequirements {
 	return resources
 }
 
-// getRedisHAProxyResources will return the ResourceRequirements for the Redis HA Proxy.
-func getRedisHAProxyResources(cr *argoprojv1a1.ArgoCD) corev1.ResourceRequirements {
+// getRedisHAResources will return the ResourceRequirements for the Redis HA.
+func getRedisHAResources(cr *argoprojv1a1.ArgoCD) corev1.ResourceRequirements {
 	resources := corev1.ResourceRequirements{}
 
 	// Allow override of resource requirements from CR

diff --git a/docs/reference/api.html.md b/docs/reference/api.html.md
@@ -1088,6 +1088,41 @@ Resource Types:
             <p>Enabled will toggle HA support globally for Argo CD.</p>
          </td>
       </tr>
+      <tr>
+         <td>
+            <code>redisProxyImage</code></br>
+            <em>
+            string
+            </em>
+         </td>
+         <td>
+            <p>The Redis HAProxy container image. This overrides the "ARGOCD_REDIS_HA_PROXY_IMAGE" environment variable.</p>
+         </td>
+      </tr>
+      <tr>
+         <td>
+            <code>redisProxyVersion</code></br>
+            <em>
+            string
+            </em>
+         </td>
+         <td>
+            <p>The tag to use for the Redis HAProxy container image.</p>
+         </td>
+      </tr>
+      <tr>
+         <td>
+            <code>resources</code></br>
+            <em>
+            <a href="https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.24/#resourcerequirements-v1-core">
+            Kubernetes core/v1.ResourceRequirements
+            </a>
+            </em>
+         </td>
+         <td>
+            <p>Resources defines the Compute Resources required by the container for Redis HA.</p>
+         </td>
+      </tr>
    </tbody>
 </table>
 <h3 id="argoproj.io/v1alpha1.ArgoCDImportSpec">ArgoCDImportSpec</h3>

diff --git a/docs/reference/argocd.md b/docs/reference/argocd.md
@@ -421,6 +421,7 @@ Name | Default | Description
 Enabled | `false` | Toggle High Availability support globally for Argo CD.
 RedisProxyImage | `haproxy` | The Redis HAProxy container image. This overrides the `ARGOCD_REDIS_HA_PROXY_IMAGE`environment variable.
 RedisProxyVersion | `2.0.4` | The tag to use for the Redis HAProxy container image.
+Resources | [Empty] | The container compute resources.
 
 ### HA Example
 

diff --git a/docs/usage/ha.md b/docs/usage/ha.md
@@ -4,6 +4,9 @@ The Argo CD operator supports high availability through the mechanism described
 
 To enable HA for an Argo CD cluster, include the `ha` section in the `ArgoCD` Custom Resource.
 
+!!! note
+    When `ha` is enabled, changes to `.spec.redis.resources` doesn't have any effect. Redis resource limits can be set using `.spec.ha.resources`.
+
 ``` yaml
 apiVersion: argoproj.io/v1alpha1
 kind: ArgoCD

diff --git a/tests/ha/1-003_validate_redis_ha/kuttl-test-redis-ha.yaml b/tests/ha/1-003_validate_redis_ha/kuttl-test-redis-ha.yaml
diff --git a/tests/ha/1-020_validate_redis_ha_nonha/01-assert.yaml b/tests/ha/1-020_validate_redis_ha_nonha/01-assert.yaml
@@ -0,0 +1,23 @@
+apiVersion: kuttl.dev/v1beta1
+kind: TestAssert
+timeout: 720
+---
+apiVersion: v1
+kind: Service
+metadata:
+  name: example-argocd-redis
+---
+apiVersion: argoproj.io/v1alpha1
+kind: ArgoCD
+metadata:
+  name: example-argocd
+status:
+  phase: Available
+  redis: Running
+---
+apiVersion: apps/v1
+kind: Deployment
+metadata:
+  name: example-argocd-redis
+status:
+  readyReplicas: 1
diff --git a/tests/ha/1-020_validate_redis_ha_nonha/01-basic.yaml b/tests/ha/1-020_validate_redis_ha_nonha/01-basic.yaml
@@ -0,0 +1,7 @@
+apiVersion: argoproj.io/v1alpha1
+kind: ArgoCD
+metadata:
+  name: example-argocd
+  labels:
+    example: basic
+spec: {}
diff --git a/...te_redis_ha_nonha_services/01-errors.yaml → ...20_validate_redis_ha_nonha/01-errors.yaml b/...te_redis_ha_nonha_services/01-errors.yaml → ...20_validate_redis_ha_nonha/01-errors.yaml