Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(controller): Allow to configure main container resources #4656

Merged
merged 6 commits into from
Dec 9, 2020
Merged

feat(controller): Allow to configure main container resources #4656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
96d76ef
feat(controller): Allow to configure main container resources
terrytangyuan Dec 7, 2020
fbe7ffd
Rename to MainContainer
terrytangyuan Dec 7, 2020
5e9a121
Address comments
terrytangyuan Dec 8, 2020
47f7339
Fix import order
terrytangyuan Dec 8, 2020
cd8e652
Check tmpl.Container
terrytangyuan Dec 8, 2020
3e494cb
Merge branch 'master' into main-ctr-resources-config
terrytangyuan Dec 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ type Config struct {
// DEPRECATED: use `executor.resources` in configmap instead
ExecutorResources *apiv1.ResourceRequirements `json:"executorResources,omitempty"`

// MainContainer holds container customization for the main container
MainContainer *apiv1.Container `json:"mainContainer,omitempty"`

// KubeConfig specifies a kube config file for the wait & init containers
KubeConfig *KubeConfig `json:"kubeConfig,omitempty"`

Expand Down
6 changes: 6 additions & 0 deletions workflow/controller/workflowpod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,12 @@ func (woc *wfOperationCtx) createWorkflowPod(nodeName string, mainCtr apiv1.Cont
wfSpec := woc.execWf.Spec.DeepCopy()

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") {
mainCtr.Resources = woc.controller.Config.MainContainer.Resources
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The workflow spec should take priority over the config.
  2. Test?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback. Just updated the PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can ignore podSpePatch, but you should check tmpl.Container

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you clarify why we need to check tmpl.Container? The goal of this PR is to customize main container no matter what's in the template (similar to what's currently done for executor).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that would be contrary to how we do it for other item - the user should be able to override a global setting like this

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually another goal of this PR is to set reasonable default resources for the main container.

I've added a check on tmpl.Container. Let me know if I've missed anything here.

}

var activeDeadlineSeconds *int64
wfDeadline := woc.getWorkflowDeadline()
Expand Down
28 changes: 28 additions & 0 deletions workflow/controller/workflowpod_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (

"github.com/stretchr/testify/assert"
apiv1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/utils/pointer"
"sigs.k8s.io/yaml"
Expand Down Expand Up @@ -1146,6 +1147,33 @@ func TestPodSpecPatch(t *testing.T) {

}

func TestMainContainerCustomization(t *testing.T) {
mainCtrSpec := &apiv1.Container{
Name: "main",
Resources: apiv1.ResourceRequirements{
Limits: apiv1.ResourceList{
apiv1.ResourceCPU: resource.MustParse("0.200"),
apiv1.ResourceMemory: resource.MustParse("512Mi"),
},
},
}
// podSpecPatch in workflow spec takes precedence over the main container's
// configuration in controller so here we respect what's specified in podSpecPatch.
wf := unmarshalWF(helloWorldWfWithPatch)
woc := newWoc(*wf)
woc.controller.Config.MainContainer = mainCtrSpec
mainCtr := woc.execWf.Spec.Templates[0].Container
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())

wf = unmarshalWF(helloWorldWf)
woc = newWoc(*wf)
woc.controller.Config.MainContainer = mainCtrSpec
mainCtr = woc.execWf.Spec.Templates[0].Container
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())
}

var helloWindowsWf = `
apiVersion: argoproj.io/v1alpha1
kind: Workflow
Expand Down