From cc1db44b66f45c2818ca4e5f609562fa7dafba91 Mon Sep 17 00:00:00 2001 From: Rizwana777 Date: Tue, 22 Oct 2024 17:38:02 +0530 Subject: [PATCH] Minor fixes Signed-off-by: Rizwana777 --- api/v1alpha1/argorollouts_types.go | 3 +- api/v1alpha1/zz_generated.deepcopy.go | 6 +++- controllers/deployment.go | 14 +++++--- controllers/deployment_test.go | 33 +++++++++++++++++-- docs/crd_reference.md | 13 ++++++++ .../cluster_scoped_rollouts_test.go | 1 + tests/e2e/rollout_tests_all.go | 6 ++-- 7 files changed, 65 insertions(+), 11 deletions(-) diff --git a/api/v1alpha1/argorollouts_types.go b/api/v1alpha1/argorollouts_types.go index d1381d6..92a999e 100644 --- a/api/v1alpha1/argorollouts_types.go +++ b/api/v1alpha1/argorollouts_types.go @@ -57,7 +57,7 @@ type RolloutManagerSpec struct { Plugins Plugins `json:"plugins,omitempty"` // HA options for High Availability support for Rollouts. - HA RolloutManagerHASpec `json:"ha,omitempty"` + HA *RolloutManagerHASpec `json:"ha,omitempty"` } // Plugin is used to integrate traffic management and metric plugins into the Argo Rollouts controller. For more information on these plugins, see the upstream Argo Rollouts documentation. @@ -77,6 +77,7 @@ type Plugins struct { Metric []Plugin `json:"metric,omitempty"` } +// RolloutManagerHASpec specifies HA options for High Availability support for Rollouts. type RolloutManagerHASpec struct { // Enabled will toggle HA support globally for RolloutManager. Enabled bool `json:"enabled"` diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index d807b0a..bd53ec7 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -200,7 +200,11 @@ func (in *RolloutManagerSpec) DeepCopyInto(out *RolloutManagerSpec) { (*in).DeepCopyInto(*out) } in.Plugins.DeepCopyInto(&out.Plugins) - out.HA = in.HA + if in.HA != nil { + in, out := &in.HA, &out.HA + *out = new(RolloutManagerHASpec) + **out = **in + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RolloutManagerSpec. diff --git a/controllers/deployment.go b/controllers/deployment.go index 15e298b..f075af5 100644 --- a/controllers/deployment.go +++ b/controllers/deployment.go @@ -45,7 +45,7 @@ func generateDesiredRolloutsDeployment(cr rolloutsmanagerv1alpha1.RolloutManager // Default number of replicas is 1, update it to 2 if HA is enabled var replicas int32 = 1 - if cr.Spec.HA.Enabled { + if cr.Spec.HA != nil && cr.Spec.HA.Enabled { replicas = 2 } @@ -70,7 +70,7 @@ func generateDesiredRolloutsDeployment(cr rolloutsmanagerv1alpha1.RolloutManager }, } - if cr.Spec.HA.Enabled { + if cr.Spec.HA != nil && cr.Spec.HA.Enabled { desiredDeployment.Spec.Template.Spec.Affinity = &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ @@ -431,7 +431,7 @@ func normalizeDeployment(inputParam appsv1.Deployment, cr rolloutsmanagerv1alpha // Default number of replicas is 1, update it to 2 if HA is enabled var replicas int32 = 1 - if cr.Spec.HA.Enabled { + if cr.Spec.HA != nil && cr.Spec.HA.Enabled { replicas = 2 } @@ -461,6 +461,10 @@ func normalizeDeployment(inputParam appsv1.Deployment, cr rolloutsmanagerv1alpha }, } + if input.Spec.Replicas == nil { + return appsv1.Deployment{}, fmt.Errorf("missing .spec.replicas") + } + if len(input.Spec.Template.Spec.Containers) != 1 { return appsv1.Deployment{}, fmt.Errorf("incorrect number of .spec.template.spec.containers") } @@ -509,7 +513,7 @@ func normalizeDeployment(inputParam appsv1.Deployment, cr rolloutsmanagerv1alpha inputContainer.Env = make([]corev1.EnvVar, 0) } - if cr.Spec.HA.Enabled { + if input.Spec.Template.Spec.Affinity != nil && input.Spec.Template.Spec.Affinity.PodAffinity != nil { res.Spec.Template.Spec.Affinity = &corev1.Affinity{ PodAntiAffinity: &corev1.PodAntiAffinity{ PreferredDuringSchedulingIgnoredDuringExecution: []corev1.WeightedPodAffinityTerm{ @@ -648,7 +652,7 @@ func getRolloutsCommandArgs(cr rolloutsmanagerv1alpha1.RolloutManager) []string args = append(args, "--namespaced") } - if cr.Spec.HA.Enabled { + if cr.Spec.HA != nil && cr.Spec.HA.Enabled { args = append(args, "--leader-elect", "true") } diff --git a/controllers/deployment_test.go b/controllers/deployment_test.go index b825dbb..1c5c560 100644 --- a/controllers/deployment_test.go +++ b/controllers/deployment_test.go @@ -339,6 +339,15 @@ var _ = Describe("Deployment Test", func() { Expect(r.Client.Get(context.Background(), client.ObjectKeyFromObject(&updatedDeplFromClient), &updatedDeplFromClient)).To(Succeed()) Expect(areEqual(*updatedDepl, updatedDeplFromClient, a)).To(BeTrue(), "resource on cluster should match the resource we called Update with") + // The '.spec.replicas' value is not being set directly through the CR. + // Instead, we're applying the default value of 2. + // This 'if' condition checks whether the replicas value is set to 2 when HA is enabled. + if a.Spec.HA != nil && a.Spec.HA.Enabled { + Expect(areEqual(*updatedDepl, updatedDeplFromClient, a)).To(BeTrue()) + // Return here to skip further steps in the test for '.spec.replicas' entry + return + } + Expect(areEqual(updatedDeplFromClient, expectedDepl, a)).ToNot(BeTrue(), "resource on cluster should NOT match the original Deployment that was created by the call to reconcileRolloutsDeployment") By("calling reconcileRolloutsDeployment again, it should revert the change back to default") @@ -416,12 +425,28 @@ var _ = Describe("Deployment Test", func() { }, } }), + Entry(".spec.replicas", func(deployment *appsv1.Deployment) { + a.Spec = v1alpha1.RolloutManagerSpec{ + HA: &v1alpha1.RolloutManagerHASpec{ + Enabled: true, + }, + } + Expect(r.Client.Update(context.Background(), &a)).To(Succeed()) + var replicas int32 + deployment.Spec.Replicas = &replicas + }), ) It("should contain two replicas and the --leader-elect argument set to true, and verify that the anti-affinity rule is added by default when HA is enabled", func() { - a.Spec.HA.Enabled = true + a.Spec = v1alpha1.RolloutManagerSpec{ + HA: &v1alpha1.RolloutManagerHASpec{ + Enabled: true, + }, + } replicas := int32(2) + Expect(r.Client.Update(ctx, &a)).To(Succeed()) + By("calling reconcileRolloutsDeployment to create the initial set of rollout resources") Expect(r.reconcileRolloutsDeployment(ctx, a, *sa)).To(Succeed()) @@ -642,6 +667,10 @@ var _ = Describe("normalizeDeployment tests to verify that an error is returned" {Name: "volume1", MountPath: "/mnt/volume1"}, } }, "incorrect volume mounts"), + + Entry("Spec.Replicas is nil", func() { + deployment.Spec.Replicas = nil + }, "missing .spec.replicas"), ) }) @@ -767,7 +796,7 @@ func deploymentCR(name string, namespace string, rolloutsSelectorLabel string, v } setRolloutsLabelsAndAnnotationsToObject(&deploymentCR.ObjectMeta, rolloutManager) replicas := int32(1) - if rolloutManager.Spec.HA.Enabled { + if rolloutManager.Spec.HA != nil && rolloutManager.Spec.HA.Enabled { replicas = 2 } deploymentCR.Spec = appsv1.DeploymentSpec{ diff --git a/docs/crd_reference.md b/docs/crd_reference.md index a546229..4c67755 100644 --- a/docs/crd_reference.md +++ b/docs/crd_reference.md @@ -152,4 +152,17 @@ spec: - name: "argoproj-labs/sample-prometheus" location: https://github.com/argoproj-labs/sample-rollouts-metric-plugin/releases/download/v0.0.3/metric-plugin-linux-amd64 sha256: a597a017a9a1394a31b3cbc33e08a071c88f0bd8 +``` +### RolloutManager example with HA enabled + +``` yaml +apiVersion: argoproj.io/v1alpha1 +kind: RolloutManager +metadata: + name: argo-rollout + labels: + example: with-ha +spec: + ha: + enabled: true ``` \ No newline at end of file diff --git a/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go b/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go index 122bfbb..191f4fb 100644 --- a/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go +++ b/tests/e2e/cluster-scoped/cluster_scoped_rollouts_test.go @@ -389,5 +389,6 @@ var _ = Describe("Cluster-scoped RolloutManager tests", func() { Eventually(clusterRoleView, "1m", "1s").ShouldNot((k8s.ExistByName(k8sClient))) Eventually(clusterRoleEdit, "1m", "1s").ShouldNot((k8s.ExistByName(k8sClient))) }) + }) }) diff --git a/tests/e2e/rollout_tests_all.go b/tests/e2e/rollout_tests_all.go index 96cf2db..00c5c84 100644 --- a/tests/e2e/rollout_tests_all.go +++ b/tests/e2e/rollout_tests_all.go @@ -794,7 +794,7 @@ func RunRolloutsTests(namespaceScopedParam bool) { }, Spec: rolloutsmanagerv1alpha1.RolloutManagerSpec{ NamespaceScoped: namespaceScopedParam, - HA: rolloutsmanagerv1alpha1.RolloutManagerHASpec{ + HA: &rolloutsmanagerv1alpha1.RolloutManagerHASpec{ Enabled: true, }, }, @@ -809,7 +809,9 @@ func RunRolloutsTests(namespaceScopedParam bool) { }, } - Eventually(&depl, "30s", "1s").Should(k8s.ExistByName(k8sClient)) + // In this test we don't check whether RolloutManager has phase: Available, and we don't check if Deployment is ready. + // This is because our E2E tests run in a single node cluster, which prevents HA deployments from being fully scheduled. + Eventually(&depl, "60s", "1s").Should(k8s.ExistByName(k8sClient)) replicas := int32(2) Expect(depl.Spec.Replicas).To(Equal(&replicas))