diff --git a/controllers/deployment.go b/controllers/deployment.go index c12640f..fdf78b8 100644 --- a/controllers/deployment.go +++ b/controllers/deployment.go @@ -239,6 +239,15 @@ func identifyDeploymentDifference(x appsv1.Deployment, y appsv1.Deployment) stri return "" } +// defaultRolloutsContainerResources return the default resource constaints set on containers, when the RolloutManager CR does not have resource constraints set. +func defaultRolloutsContainerResources() corev1.ResourceRequirements { + return corev1.ResourceRequirements{ + Limits: corev1.ResourceList{ + corev1.ResourceEphemeralStorage: resource.MustParse("1Gi"), + }, + } +} + func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) corev1.Container { // NOTE: When updating this function, ensure that normalizeDeployment is updated as well. See that function for details. @@ -251,11 +260,8 @@ func rolloutsContainer(cr rolloutsmanagerv1alpha1.RolloutManager) corev1.Contain containerResources := cr.Spec.ControllerResources if containerResources == nil { - containerResources = &corev1.ResourceRequirements{ - Limits: corev1.ResourceList{ - corev1.ResourceEphemeralStorage: resource.MustParse("1Gi"), - }, - } + defaultContainerResources := defaultRolloutsContainerResources() + containerResources = &defaultContainerResources } return corev1.Container{ diff --git a/controllers/deployment_test.go b/controllers/deployment_test.go index 45d5c2a..dbca5a7 100644 --- a/controllers/deployment_test.go +++ b/controllers/deployment_test.go @@ -47,16 +47,6 @@ var _ = Describe("Deployment Test", func() { fetchedDeployment := &appsv1.Deployment{} Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) - a.Spec.ControllerResources = &corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("100m"), - corev1.ResourceMemory: resource.MustParse("100Mi"), - }, - Limits: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("500m"), - corev1.ResourceMemory: resource.MustParse("500Mi"), - }, - } expectedDeployment := deploymentCR(DefaultArgoRolloutsResourceName, a.Namespace, DefaultArgoRolloutsResourceName, []string{"plugin-bin", "tmp"}, "linux", DefaultArgoRolloutsResourceName, a) By("verify that the fetched Deployment matches the desired one") @@ -69,6 +59,7 @@ var _ = Describe("Deployment Test", func() { Expect(fetchedDeployment.Spec.Template.Spec.Tolerations).To(Equal(expectedDeployment.Spec.Template.Spec.Tolerations)) Expect(fetchedDeployment.Spec.Template.Spec.SecurityContext).To(Equal(expectedDeployment.Spec.Template.Spec.SecurityContext)) Expect(fetchedDeployment.Spec.Template.Spec.Volumes).To(Equal(expectedDeployment.Spec.Template.Spec.Volumes)) + Expect(fetchedDeployment.Spec.Template.Spec.Containers[0].Resources).To(Equal(expectedDeployment.Spec.Template.Spec.Containers[0].Resources)) }) When("Rollouts Deployment already exists, but then is modified away from default values", func() { @@ -111,6 +102,91 @@ var _ = Describe("Deployment Test", func() { }) }) + When("RolloutManagerCR has custom controller resources defined", func() { + + It("should create a Deployment that uses those controller resources", func() { + + By("setting resource requirements on RolloutsManager CR") + a.Spec.ControllerResources = &corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("100Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("500Mi"), + }, + } + 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()) + + By("fetching the Deployment") + fetchedDeployment := &appsv1.Deployment{} + Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) + + By("verifying that the fetched Deployment matches the desired one") + Expect(fetchedDeployment.Spec.Template.Spec.Containers[0].Resources).To(Equal(*a.Spec.ControllerResources)) + }) + + defaultContainerResources := defaultRolloutsContainerResources() + + nonDefaultContainerResourcesValue := &corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("100m"), + corev1.ResourceMemory: resource.MustParse("100Mi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("500m"), + corev1.ResourceMemory: resource.MustParse("500Mi"), + }, + } + + otherNonDefault := corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1m"), + corev1.ResourceMemory: resource.MustParse("1Mi"), + }, + } + + DescribeTable("Deployment CR should always be updated to be consistent with RolloutsManager .spec.controllerResources field, in both default and non-default cases", func(initialDeployment *corev1.ResourceRequirements, crValue *corev1.ResourceRequirements, expectedDeployment *corev1.ResourceRequirements) { + + By("calling reconcileRolloutsDeployment to create the initial set of rollout resources") + Expect(r.reconcileRolloutsDeployment(ctx, a, *sa)).To(Succeed()) + + By("updating the default Deployment to resource value defined in 'initialDeployment'") + fetchedDeployment := &appsv1.Deployment{} + Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) + + fetchedDeployment.Spec.Template.Spec.Containers[0].Resources = *initialDeployment + Expect(r.Update(ctx, fetchedDeployment)).To(Succeed()) + + if crValue != nil { + By("setting resource requirements on RolloutsManager CR to 'crValue'") + a.Spec.ControllerResources = crValue + Expect(r.Client.Update(ctx, &a)).To(Succeed()) + } + + By("calling reconcileRolloutsDeployment") + Expect(r.reconcileRolloutsDeployment(ctx, a, *sa)).To(Succeed()) + + By("fetching the Deployment") + fetchedDeployment = &appsv1.Deployment{} + Expect(fetchObject(ctx, r.Client, a.Namespace, DefaultArgoRolloutsResourceName, fetchedDeployment)).To(Succeed()) + + By("verifying that the fetched Deployment resource requirements matches the desired resource requirements") + Expect(fetchedDeployment.Spec.Template.Spec.Containers[0].Resources).To(Equal(*expectedDeployment)) + + }, + Entry("default deployment, with a empty CR .spec.containerResources -> no change in deployment from default", &defaultContainerResources, nil, &defaultContainerResources), + Entry("default deployment, with CR non-default value in .spec.containerResources -> deployment should now have value from CR", &defaultContainerResources, nonDefaultContainerResourcesValue, nonDefaultContainerResourcesValue), + Entry("deployment with non-default container resources, empty value in CR .spec.containerResources -> Deployment should revert to default value from CR", nonDefaultContainerResourcesValue, nil, &defaultContainerResources), + Entry("deployment with a different non-default container resources, non-default value in CR .spec.containerResources -> Deployment should use CR value", &otherNonDefault, nonDefaultContainerResourcesValue, nonDefaultContainerResourcesValue), + ) + + }) + When("Rollouts deployment already exists, but then RolloutManager is modified in a way that requires updating either .spec.selector of the existing Deployment", func() { It("should cause the existing Deployment to be deleted, and a new Deployment to be created with the updated .spec.selector", func() { @@ -326,7 +402,7 @@ var _ = Describe("Deployment Test", func() { Entry(".spec.template.spec.volumes", func(deployment *appsv1.Deployment) { deployment.Spec.Template.Spec.Volumes = []corev1.Volume{{Name: "my-volume"}} }), - Entry(".spec.template.spec.containes.resources", func(deployment *appsv1.Deployment) { + Entry(".spec.template.spec.containers.resources", func(deployment *appsv1.Deployment) { deployment.Spec.Template.Spec.Containers[0].Resources = corev1.ResourceRequirements{ Requests: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("100m"), diff --git a/tests/e2e/rollout_tests_all.go b/tests/e2e/rollout_tests_all.go index e5bce36..dd72df9 100644 --- a/tests/e2e/rollout_tests_all.go +++ b/tests/e2e/rollout_tests_all.go @@ -445,11 +445,13 @@ func RunRolloutsTests(namespaceScopedParam bool) { }) }) - When("A RolloutManager specifies controller resources", func() { + When("A RolloutManager specifies controller resources under .spec.controllerResources", func() { It("should create the controller with the correct resources requests/limits", func() { - rolloutsManager := rolloutsmanagerv1alpha1.RolloutManager{ + By("creating a RolloutManager containing resource requirements") + + rmWithResources := rolloutsmanagerv1alpha1.RolloutManager{ ObjectMeta: metav1.ObjectMeta{ Name: "basic-rollouts-manager-with-resources", Namespace: fixture.TestE2ENamespace, @@ -469,16 +471,39 @@ func RunRolloutsTests(namespaceScopedParam bool) { }, } - Expect(k8sClient.Create(ctx, &rolloutsManager)).To(Succeed()) + Expect(k8sClient.Create(ctx, &rmWithResources)).To(Succeed()) - Eventually(rolloutsManager, "1m", "1s").Should(rolloutManagerFixture.HavePhase(rolloutsmanagerv1alpha1.PhaseAvailable)) + Eventually(rmWithResources, "1m", "1s").Should(rolloutManagerFixture.HavePhase(rolloutsmanagerv1alpha1.PhaseAvailable)) deployment := appsv1.Deployment{ - ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultArgoRolloutsResourceName, Namespace: rolloutManager.Namespace}, + ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultArgoRolloutsResourceName, Namespace: rmWithResources.Namespace}, } Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(&deployment), &deployment)).To(Succeed()) - Expect(deployment.Spec.Template.Spec.Containers[0].Resources).To(Equal(*rolloutsManager.Spec.ControllerResources)) + Expect(deployment.Spec.Template.Spec.Containers[0].Resources).To(Equal(*rmWithResources.Spec.ControllerResources)) + + By("updating RolloutManager to use a different CPU limit") + + err := k8s.UpdateWithoutConflict(ctx, &rmWithResources, k8sClient, func(obj client.Object) { + rm, ok := obj.(*rolloutsmanagerv1alpha1.RolloutManager) + Expect(ok).To(BeTrue()) + + rm.Spec.ControllerResources.Limits[corev1.ResourceCPU] = resource.MustParse("555m") + + }) + Expect(err).ToNot(HaveOccurred()) + + Eventually(func() bool { + deployment := appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: controllers.DefaultArgoRolloutsResourceName, Namespace: rmWithResources.Namespace}, + } + if err := k8sClient.Get(ctx, client.ObjectKeyFromObject(&deployment), &deployment); err != nil { + return false + } + return deployment.Spec.Template.Spec.Containers[0].Resources.Limits[corev1.ResourceCPU] == resource.MustParse("555m") + + }, "1m", "1s").Should(BeTrue(), "Deployment should switch to the new CPU limit on update of RolloutManager CR") + }) }) })