fix(controller): Fix incorrect main container customization precedenc…

…e and isResourcesSpecified check (#4681) Signed-off-by: terrytangyuan <terrytangyuan@gmail.com>
argoproj · Dec 17, 2020 · 4fb0d96 · 4fb0d96
1 parent 1aac79e
commit 4fb0d96
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 5 deletions.
diff --git a/workflow/controller/workflowpod.go b/workflow/controller/workflowpod.go
@@ -152,11 +152,13 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont
 
 	mainCtr.Name = common.MainContainerName
 	// Allow customization of main container resources.
-	if isResourcesSpecified(woc.controller.Config.MainContainer) &&
-		// Container resources in workflow spec takes precedence over the main container's configuration in controller.
-		!(isResourcesSpecified(tmpl.Container) && tmpl.Container.Name == "main") {
+	if isResourcesSpecified(woc.controller.Config.MainContainer) {
 		mainCtr.Resources = woc.controller.Config.MainContainer.Resources
 	}
+	// Container resources in workflow spec takes precedence over the main container's configuration in controller.
+	if isResourcesSpecified(tmpl.Container) && tmpl.Container.Name == common.MainContainerName {
+		mainCtr.Resources = tmpl.Container.Resources
+	}
 
 	var activeDeadlineSeconds *int64
 	wfDeadline := woc.getWorkflowDeadline()
@@ -607,7 +609,7 @@ func (woc *wfOperationCtx) newExecContainer(name string, tmpl *wfv1.Template) *a
 }
 
 func isResourcesSpecified(ctr *apiv1.Container) bool {
-	return ctr != nil && (ctr.Resources.Limits.Cpu() != nil || ctr.Resources.Limits.Memory() != nil)
+	return ctr != nil && len(ctr.Resources.Limits) != 0
 }
 
 // addMetadata applies metadata specified in the template

diff --git a/workflow/controller/workflowpod_test.go b/workflow/controller/workflowpod_test.go
@@ -1145,7 +1145,7 @@ func TestPodSpecPatch(t *testing.T) {
 
 func TestMainContainerCustomization(t *testing.T) {
 	mainCtrSpec := &apiv1.Container{
-		Name: "main",
+		Name: common.MainContainerName,
 		Resources: apiv1.ResourceRequirements{
 			Limits: apiv1.ResourceList{
 				apiv1.ResourceCPU:    resource.MustParse("0.200"),
@@ -1162,12 +1162,68 @@ func TestMainContainerCustomization(t *testing.T) {
 	pod, _ := woc.createWorkflowPod(wf.Name, *mainCtr, &wf.Spec.Templates[0], &createWorkflowPodOpts{})
 	assert.Equal(t, "0.800", pod.Spec.Containers[1].Resources.Limits.Cpu().AsDec().String())
 
+	// The main container's resources should be changed since the existing
+	// container's resources are not specified.
 	wf = unmarshalWF(helloWorldWf)
 	woc = newWoc(*wf)
 	woc.controller.Config.MainContainer = mainCtrSpec
 	mainCtr = woc.execWf.Spec.Templates[0].Container
+	mainCtr.Resources = apiv1.ResourceRequirements{Limits: apiv1.ResourceList{}}
 	pod, _ = woc.createWorkflowPod(wf.Name, *mainCtr, &wf.Spec.Templates[0], &createWorkflowPodOpts{})
 	assert.Equal(t, "0.200", pod.Spec.Containers[1].Resources.Limits.Cpu().AsDec().String())
+
+	// Workflow spec's main container takes precedence over config in controller
+	// so here the main container resources remain unchanged.
+	wf = unmarshalWF(helloWorldWf)
+	woc = newWoc(*wf)
+	woc.controller.Config.MainContainer = mainCtrSpec
+	mainCtr = woc.execWf.Spec.Templates[0].Container
+	wf.Spec.Templates[0].Container.Name = common.MainContainerName
+	wf.Spec.Templates[0].Container.Resources = apiv1.ResourceRequirements{
+		Limits: apiv1.ResourceList{
+			apiv1.ResourceCPU:    resource.MustParse("0.900"),
+			apiv1.ResourceMemory: resource.MustParse("512Mi"),
+		},
+	}
+	pod, _ = woc.createWorkflowPod(wf.Name, *mainCtr, &wf.Spec.Templates[0], &createWorkflowPodOpts{})
+	assert.Equal(t, "0.900", pod.Spec.Containers[1].Resources.Limits.Cpu().AsDec().String())
+
+	// If the name of the container in the workflow spec is not "main",
+	// the main container resources should remain unchanged.
+	wf = unmarshalWF(helloWorldWf)
+	woc = newWoc(*wf)
+	mainCtr = woc.execWf.Spec.Templates[0].Container
+	mainCtr.Resources = apiv1.ResourceRequirements{
+		Limits: apiv1.ResourceList{
+			apiv1.ResourceCPU:    resource.MustParse("0.100"),
+			apiv1.ResourceMemory: resource.MustParse("512Mi"),
+		},
+	}
+	wf.Spec.Templates[0].Container.Name = "non-main"
+	wf.Spec.Templates[0].Container.Resources = apiv1.ResourceRequirements{
+		Limits: apiv1.ResourceList{
+			apiv1.ResourceCPU:    resource.MustParse("0.900"),
+			apiv1.ResourceMemory: resource.MustParse("512Mi"),
+		},
+	}
+	pod, _ = woc.createWorkflowPod(wf.Name, *mainCtr, &wf.Spec.Templates[0], &createWorkflowPodOpts{})
+	assert.Equal(t, "0.100", pod.Spec.Containers[1].Resources.Limits.Cpu().AsDec().String())
+}
+
+func TestIsResourcesSpecified(t *testing.T) {
+	wf := unmarshalWF(helloWorldWf)
+	woc := newWoc(*wf)
+	mainCtr := woc.execWf.Spec.Templates[0].Container
+	mainCtr.Resources = apiv1.ResourceRequirements{Limits: apiv1.ResourceList{}}
+	assert.False(t, isResourcesSpecified(mainCtr))
+
+	mainCtr.Resources = apiv1.ResourceRequirements{
+		Limits: apiv1.ResourceList{
+			apiv1.ResourceCPU:    resource.MustParse("0.900"),
+			apiv1.ResourceMemory: resource.MustParse("512Mi"),
+		},
+	}
+	assert.True(t, isResourcesSpecified(mainCtr))
 }
 
 var helloWindowsWf = `