Skip to content

Commit

Permalink
Resource constraint unit and E2E test updates
Browse files Browse the repository at this point in the history 
Signed-off-by: Jonathan West <jonwest@redhat.com>
  • Loading branch information
@jgwest
jgwest committed Jul 31, 2024
1 parent 3992ad3 commit c4aa6df
Show file tree
Hide file tree
Showing 3 changed files with 129 additions and 22 deletions.
16 changes: 11 additions & 5 deletions controllers/deployment.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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{
Expand Down
98 changes: 87 additions & 11 deletions controllers/deployment_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand All @@ -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() {
Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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"),
Expand Down
37 changes: 31 additions & 6 deletions tests/e2e/rollout_tests_all.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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")

})
})
})
Expand Down

0 comments on commit c4aa6df

Please sign in to comment.