From 912356a095225ccaf36f0f24e2a64d3b42b6d522 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Mon, 29 Apr 2024 16:13:15 +0200 Subject: [PATCH 1/9] Add sidecar containers to resource requests computation --- pkg/util/limitrange/limitrange.go | 22 +++++++++++++------ pkg/util/limitrange/limitrange_test.go | 30 ++++++++++++++++++++++---- 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/pkg/util/limitrange/limitrange.go b/pkg/util/limitrange/limitrange.go index 943e0a2ccf..e98bf936ae 100644 --- a/pkg/util/limitrange/limitrange.go +++ b/pkg/util/limitrange/limitrange.go @@ -81,20 +81,30 @@ func (s Summary) validatePodSpecContainers(containers []corev1.Container, path * } // TotalRequests computes the total resource requests of a pod. -// total = sum(max(sum(.containers[].requests), initContainers[].requests), overhead) +// total = Sum(Max( Max(nonSidecarInitContainers) + Sum(SidecarContainers), Sum(SidecarContainers) + Sum(Containers) ), overhead) func TotalRequests(ps *corev1.PodSpec) corev1.ResourceList { - total := corev1.ResourceList{} - // add the resource from the main containers + sumContainers := corev1.ResourceList{} for i := range ps.Containers { - total = resource.MergeResourceListKeepSum(total, ps.Containers[i].Resources.Requests) + sumContainers = resource.MergeResourceListKeepSum(sumContainers, ps.Containers[i].Resources.Requests) } - // take into account the maximum of any init containers + // take into account the maximum of any non sidecar init containers and add the sidecar containers + maxNonSidecarInitContainers := corev1.ResourceList{} + sumSidecarContainers := corev1.ResourceList{} for i := range ps.InitContainers { - total = resource.MergeResourceListKeepMax(total, ps.InitContainers[i].Resources.Requests) + // Is the init container a sidecar container? + if ps.InitContainers[i].RestartPolicy != nil && *ps.InitContainers[i].RestartPolicy == corev1.ContainerRestartPolicyAlways { + sumSidecarContainers = resource.MergeResourceListKeepSum(sumSidecarContainers, ps.InitContainers[i].Resources.Requests) + } else { + maxNonSidecarInitContainers = resource.MergeResourceListKeepMax(maxNonSidecarInitContainers, ps.InitContainers[i].Resources.Requests) + } } + total := resource.MergeResourceListKeepMax( + resource.MergeResourceListKeepSum(maxNonSidecarInitContainers, sumSidecarContainers), + resource.MergeResourceListKeepSum(sumSidecarContainers, sumContainers), + ) // add the overhead total = resource.MergeResourceListKeepSum(total, ps.Overhead) return total diff --git a/pkg/util/limitrange/limitrange_test.go b/pkg/util/limitrange/limitrange_test.go index eed8c66460..1a9594f781 100644 --- a/pkg/util/limitrange/limitrange_test.go +++ b/pkg/util/limitrange/limitrange_test.go @@ -23,6 +23,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/field" + "k8s.io/utils/ptr" testingutil "sigs.k8s.io/kueue/pkg/util/testing" ) @@ -193,6 +194,16 @@ func TestTotalRequest(t *testing.T) { }, }, }, + { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceMemory: resource.MustParse("2Gi"), + "example.com/gpu": resource.MustParse("2"), + }, + }, + RestartPolicy: ptr.To(corev1.ContainerRestartPolicyAlways), + }, } cases := map[string]struct { podSpec *corev1.PodSpec @@ -210,7 +221,7 @@ func TestTotalRequest(t *testing.T) { }, "one init wants more": { podSpec: &corev1.PodSpec{ - InitContainers: containers[2:], + InitContainers: containers[2:4], Containers: containers[:2], }, want: corev1.ResourceList{ @@ -219,6 +230,17 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("2"), }, }, + "include sidecar container": { + podSpec: &corev1.PodSpec{ + InitContainers: containers[2:], + Containers: containers[:2], + }, + want: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("5000m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + "example.com/gpu": resource.MustParse("4"), + }, + }, "adds overhead": { podSpec: &corev1.PodSpec{ InitContainers: containers[2:], @@ -230,9 +252,9 @@ func TestTotalRequest(t *testing.T) { }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("5000m"), - corev1.ResourceMemory: resource.MustParse("3Gi"), - "example.com/gpu": resource.MustParse("3"), + corev1.ResourceCPU: resource.MustParse("6000m"), + corev1.ResourceMemory: resource.MustParse("5Gi"), + "example.com/gpu": resource.MustParse("5"), }, }, } From 4c147e5599d81af26f87d763342cc0584e141124 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Thu, 2 May 2024 12:32:48 +0200 Subject: [PATCH 2/9] Refactor init containers resource computation --- pkg/util/limitrange/limitrange.go | 34 ++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/pkg/util/limitrange/limitrange.go b/pkg/util/limitrange/limitrange.go index e98bf936ae..50a9693964 100644 --- a/pkg/util/limitrange/limitrange.go +++ b/pkg/util/limitrange/limitrange.go @@ -88,18 +88,8 @@ func TotalRequests(ps *corev1.PodSpec) corev1.ResourceList { for i := range ps.Containers { sumContainers = resource.MergeResourceListKeepSum(sumContainers, ps.Containers[i].Resources.Requests) } - - // take into account the maximum of any non sidecar init containers and add the sidecar containers - maxNonSidecarInitContainers := corev1.ResourceList{} - sumSidecarContainers := corev1.ResourceList{} - for i := range ps.InitContainers { - // Is the init container a sidecar container? - if ps.InitContainers[i].RestartPolicy != nil && *ps.InitContainers[i].RestartPolicy == corev1.ContainerRestartPolicyAlways { - sumSidecarContainers = resource.MergeResourceListKeepSum(sumSidecarContainers, ps.InitContainers[i].Resources.Requests) - } else { - maxNonSidecarInitContainers = resource.MergeResourceListKeepMax(maxNonSidecarInitContainers, ps.InitContainers[i].Resources.Requests) - } - } + maxNonSidecarInitContainers := calculateRegularInitContainersResources(ps.InitContainers) + sumSidecarContainers := calculateSidecarContainersResources(ps.InitContainers) total := resource.MergeResourceListKeepMax( resource.MergeResourceListKeepSum(maxNonSidecarInitContainers, sumSidecarContainers), @@ -110,6 +100,26 @@ func TotalRequests(ps *corev1.PodSpec) corev1.ResourceList { return total } +func calculateRegularInitContainersResources(initContainers []corev1.Container) corev1.ResourceList { + total := corev1.ResourceList{} + for i := range initContainers { + if initContainers[i].RestartPolicy == nil || *initContainers[i].RestartPolicy != corev1.ContainerRestartPolicyAlways { + total = resource.MergeResourceListKeepMax(total, initContainers[i].Resources.Requests) + } + } + return total +} + +func calculateSidecarContainersResources(initContainers []corev1.Container) corev1.ResourceList { + total := corev1.ResourceList{} + for i := range initContainers { + if initContainers[i].RestartPolicy != nil && *initContainers[i].RestartPolicy == corev1.ContainerRestartPolicyAlways { + total = resource.MergeResourceListKeepSum(total, initContainers[i].Resources.Requests) + } + } + return total +} + // ValidatePodSpec verifies if the provided podSpec (ps) first into the boundaries of the summary (s). func (s Summary) ValidatePodSpec(ps *corev1.PodSpec, path *field.Path) []string { reasons := []string{} From e33853584b84234e5697bb5d66fbf346fbed9f4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Thu, 2 May 2024 15:07:31 +0200 Subject: [PATCH 3/9] Refactor tests cases using Container wrapper --- pkg/util/limitrange/limitrange_test.go | 186 +++++++++++++------------ pkg/util/testing/wrappers.go | 37 +++++ 2 files changed, 137 insertions(+), 86 deletions(-) diff --git a/pkg/util/limitrange/limitrange_test.go b/pkg/util/limitrange/limitrange_test.go index 1a9594f781..8ff38ba71c 100644 --- a/pkg/util/limitrange/limitrange_test.go +++ b/pkg/util/limitrange/limitrange_test.go @@ -23,7 +23,6 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" "k8s.io/utils/field" - "k8s.io/utils/ptr" testingutil "sigs.k8s.io/kueue/pkg/util/testing" ) @@ -161,57 +160,22 @@ func TestSummarize(t *testing.T) { } func TestTotalRequest(t *testing.T) { - containers := []corev1.Container{ - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - }, - }, - }, - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1500m"), - "example.com/gpu": resource.MustParse("2"), - }, - }, - }, - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - }, - }, - }, - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1500m"), - "example.com/gpu": resource.MustParse("2"), - }, - }, - }, - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - "example.com/gpu": resource.MustParse("2"), - }, - }, - RestartPolicy: ptr.To(corev1.ContainerRestartPolicyAlways), - }, - } cases := map[string]struct { podSpec *corev1.PodSpec want corev1.ResourceList }{ "sum up main containers": { podSpec: &corev1.PodSpec{ - Containers: containers[:2], + Containers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), + }, }, want: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("2500m"), @@ -221,8 +185,26 @@ func TestTotalRequest(t *testing.T) { }, "one init wants more": { podSpec: &corev1.PodSpec{ - InitContainers: containers[2:4], - Containers: containers[:2], + InitContainers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "4"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), + }, + Containers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), + }, }, want: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("4000m"), @@ -232,8 +214,32 @@ func TestTotalRequest(t *testing.T) { }, "include sidecar container": { podSpec: &corev1.PodSpec{ - InitContainers: containers[2:], - Containers: containers[:2], + InitContainers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "4"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + WithResourceReq("example.com/gpu", "2"). + AsSidecar(). + Obj(), + }, + Containers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), + }, }, want: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("5000m"), @@ -243,8 +249,32 @@ func TestTotalRequest(t *testing.T) { }, "adds overhead": { podSpec: &corev1.PodSpec{ - InitContainers: containers[2:], - Containers: containers[:2], + InitContainers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "4"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + WithResourceReq("example.com/gpu", "2"). + AsSidecar(). + Obj(), + }, + Containers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), + }, Overhead: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("1"), corev1.ResourceMemory: resource.MustParse("1Gi"), @@ -271,42 +301,26 @@ func TestTotalRequest(t *testing.T) { func TestValidatePodSpec(t *testing.T) { podSpec := &corev1.PodSpec{ Containers: []corev1.Container{ - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - "example.com/mainContainerGpu": resource.MustParse("2"), - }, - }, - }, - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1500m"), - "example.com/gpu": resource.MustParse("2"), - }, - }, - }, + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + WithResourceReq("example.com/mainContainerGpu", "2"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + Obj(), }, InitContainers: []corev1.Container{ - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4"), - corev1.ResourceMemory: resource.MustParse("2Gi"), - }, - }, - }, - { - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("1500m"), - "example.com/gpu": resource.MustParse("2"), - "example.com/initContainerGpu": resource.MustParse("2"), - }, - }, - }, + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "4"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq("example.com/gpu", "2"). + WithResourceReq("example.com/initContainerGpu", "2"). + Obj(), }, Overhead: corev1.ResourceList{ corev1.ResourceCPU: resource.MustParse("1"), diff --git a/pkg/util/testing/wrappers.go b/pkg/util/testing/wrappers.go index 6efa611e7e..3401049d93 100644 --- a/pkg/util/testing/wrappers.go +++ b/pkg/util/testing/wrappers.go @@ -32,6 +32,7 @@ import ( kueuealpha "sigs.k8s.io/kueue/apis/kueue/v1alpha1" kueue "sigs.k8s.io/kueue/apis/kueue/v1beta1" + utilResource "sigs.k8s.io/kueue/pkg/util/resource" ) // PriorityClassWrapper wraps a PriorityClass. @@ -1029,3 +1030,39 @@ func (mkc *MultiKueueClusterWrapper) Generation(num int64) *MultiKueueClusterWra mkc.ObjectMeta.Generation = num return mkc } + +// ContainerWrapper wraps a corev1.Container. +type ContainerWrapper struct{ corev1.Container } + +// MakeContainer wraps a ContainerWrapper with an empty ResourceList. +func MakeContainer() *ContainerWrapper { + return &ContainerWrapper{ + corev1.Container{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{}, + }, + }, + } +} + +// Obj returns the inner corev1.Container. +func (c *ContainerWrapper) Obj() *corev1.Container { + return &c.Container +} + +// WithResourceReq appends a resource request to the container. +func (c *ContainerWrapper) WithResourceReq(resourceName corev1.ResourceName, quantity string) *ContainerWrapper { + requests := utilResource.MergeResourceListKeepFirst(c.Container.Resources.Requests, corev1.ResourceList{ + resourceName: resource.MustParse(quantity), + }) + c.Container.Resources.Requests = requests + + return c +} + +// AsSidecar makes the container a sidecar when used as an Init Container. +func (c *ContainerWrapper) AsSidecar() *ContainerWrapper { + c.Container.RestartPolicy = ptr.To(corev1.ContainerRestartPolicyAlways) + + return c +} From 471e39cda6dfbe571d87ad4bf77b78f3bb8b81c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Thu, 2 May 2024 16:04:35 +0200 Subject: [PATCH 4/9] Update test cases --- pkg/util/limitrange/limitrange_test.go | 79 +++++++++++++++++++++++--- 1 file changed, 71 insertions(+), 8 deletions(-) diff --git a/pkg/util/limitrange/limitrange_test.go b/pkg/util/limitrange/limitrange_test.go index 8ff38ba71c..77792e5c53 100644 --- a/pkg/util/limitrange/limitrange_test.go +++ b/pkg/util/limitrange/limitrange_test.go @@ -164,7 +164,7 @@ func TestTotalRequest(t *testing.T) { podSpec *corev1.PodSpec want corev1.ResourceList }{ - "sum up main containers": { + "pod without init containers": { podSpec: &corev1.PodSpec{ Containers: []corev1.Container{ *testingutil.MakeContainer(). @@ -183,7 +183,7 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("2"), }, }, - "one init wants more": { + "pod only with regular init containers": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -212,38 +212,101 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("2"), }, }, - "include sidecar container": { + "pod only with sidecar containers": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). WithResourceReq(corev1.ResourceCPU, "4"). WithResourceReq(corev1.ResourceMemory, "2Gi"). + AsSidecar(). Obj(), *testingutil.MakeContainer(). WithResourceReq(corev1.ResourceCPU, "1500m"). WithResourceReq("example.com/gpu", "2"). + AsSidecar(). Obj(), + }, + Containers: []corev1.Container{ *testingutil.MakeContainer(). WithResourceReq(corev1.ResourceCPU, "1"). WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1500m"). WithResourceReq("example.com/gpu", "2"). + Obj(), + }, + }, + want: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("8000m"), + corev1.ResourceMemory: resource.MustParse("4Gi"), + "example.com/gpu": resource.MustParse("4"), + }, + }, + "pod only with init containers": { + podSpec: &corev1.PodSpec{ + InitContainers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "1Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "2"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "3"). + WithResourceReq(corev1.ResourceMemory, "3Gi"). + AsSidecar(). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "4"). + WithResourceReq(corev1.ResourceMemory, "4Gi"). AsSidecar(). Obj(), }, - Containers: []corev1.Container{ + }, + want: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("9"), + corev1.ResourceMemory: resource.MustParse("9Gi"), + }, + }, + "pod with regular init and sidecar containers": { + podSpec: &corev1.PodSpec{ + InitContainers: []corev1.Container{ *testingutil.MakeContainer(). WithResourceReq(corev1.ResourceCPU, "1"). + WithResourceReq(corev1.ResourceMemory, "1Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "2"). WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). - WithResourceReq("example.com/gpu", "2"). + WithResourceReq(corev1.ResourceCPU, "3"). + WithResourceReq(corev1.ResourceMemory, "3Gi"). + AsSidecar(). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "4"). + WithResourceReq(corev1.ResourceMemory, "4Gi"). + AsSidecar(). + Obj(), + }, + Containers: []corev1.Container{ + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "2"). + WithResourceReq(corev1.ResourceMemory, "2Gi"). + Obj(), + *testingutil.MakeContainer(). + WithResourceReq(corev1.ResourceCPU, "4"). + WithResourceReq("example.com/gpu", "4"). Obj(), }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("5000m"), - corev1.ResourceMemory: resource.MustParse("4Gi"), + corev1.ResourceCPU: resource.MustParse("13"), + corev1.ResourceMemory: resource.MustParse("9Gi"), "example.com/gpu": resource.MustParse("4"), }, }, From 7ba8d95bd744caef4994b94a3bc2932334adeca8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Thu, 2 May 2024 18:53:45 +0200 Subject: [PATCH 5/9] Refactor sidecar condition --- pkg/util/limitrange/limitrange.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/pkg/util/limitrange/limitrange.go b/pkg/util/limitrange/limitrange.go index 50a9693964..9f813447fe 100644 --- a/pkg/util/limitrange/limitrange.go +++ b/pkg/util/limitrange/limitrange.go @@ -103,7 +103,7 @@ func TotalRequests(ps *corev1.PodSpec) corev1.ResourceList { func calculateRegularInitContainersResources(initContainers []corev1.Container) corev1.ResourceList { total := corev1.ResourceList{} for i := range initContainers { - if initContainers[i].RestartPolicy == nil || *initContainers[i].RestartPolicy != corev1.ContainerRestartPolicyAlways { + if !isSidecarContainer(initContainers[i]) { total = resource.MergeResourceListKeepMax(total, initContainers[i].Resources.Requests) } } @@ -113,13 +113,17 @@ func calculateRegularInitContainersResources(initContainers []corev1.Container) func calculateSidecarContainersResources(initContainers []corev1.Container) corev1.ResourceList { total := corev1.ResourceList{} for i := range initContainers { - if initContainers[i].RestartPolicy != nil && *initContainers[i].RestartPolicy == corev1.ContainerRestartPolicyAlways { + if isSidecarContainer(initContainers[i]) { total = resource.MergeResourceListKeepSum(total, initContainers[i].Resources.Requests) } } return total } +func isSidecarContainer(container corev1.Container) bool { + return container.RestartPolicy != nil && *container.RestartPolicy == corev1.ContainerRestartPolicyAlways +} + // ValidatePodSpec verifies if the provided podSpec (ps) first into the boundaries of the summary (s). func (s Summary) ValidatePodSpec(ps *corev1.PodSpec, path *field.Path) []string { reasons := []string{} From b216698fb19833dd112c59d0506c6b861a1974e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Thu, 2 May 2024 19:38:03 +0200 Subject: [PATCH 6/9] Update tests --- pkg/util/limitrange/limitrange_test.go | 38 +++++++++++++------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/pkg/util/limitrange/limitrange_test.go b/pkg/util/limitrange/limitrange_test.go index 77792e5c53..7074f177cb 100644 --- a/pkg/util/limitrange/limitrange_test.go +++ b/pkg/util/limitrange/limitrange_test.go @@ -164,7 +164,7 @@ func TestTotalRequest(t *testing.T) { podSpec *corev1.PodSpec want corev1.ResourceList }{ - "pod without init containers": { + "pod without init containers. request sum(containers)": { podSpec: &corev1.PodSpec{ Containers: []corev1.Container{ *testingutil.MakeContainer(). @@ -172,18 +172,18 @@ func TestTotalRequest(t *testing.T) { WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). Obj(), }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("2500m"), + corev1.ResourceCPU: resource.MustParse("2.5"), corev1.ResourceMemory: resource.MustParse("2Gi"), "example.com/gpu": resource.MustParse("2"), }, }, - "pod only with regular init containers": { + "pod only with regular init containers. request max(regularContainers)": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -191,7 +191,7 @@ func TestTotalRequest(t *testing.T) { WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). Obj(), }, @@ -201,18 +201,18 @@ func TestTotalRequest(t *testing.T) { WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). Obj(), }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("4000m"), + corev1.ResourceCPU: resource.MustParse("4"), corev1.ResourceMemory: resource.MustParse("2Gi"), "example.com/gpu": resource.MustParse("2"), }, }, - "pod only with sidecar containers": { + "pod only with sidecar containers. request sum(sidecar) + sum(containers)": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -221,7 +221,7 @@ func TestTotalRequest(t *testing.T) { AsSidecar(). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). AsSidecar(). Obj(), @@ -232,18 +232,18 @@ func TestTotalRequest(t *testing.T) { WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). Obj(), }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("8000m"), + corev1.ResourceCPU: resource.MustParse("8"), corev1.ResourceMemory: resource.MustParse("4Gi"), "example.com/gpu": resource.MustParse("4"), }, }, - "pod only with init containers": { + "pod only with regular init and sidecar containers. request max(regularContainers) + sum(sidecar)": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -271,7 +271,7 @@ func TestTotalRequest(t *testing.T) { corev1.ResourceMemory: resource.MustParse("9Gi"), }, }, - "pod with regular init and sidecar containers": { + "pod with regular init and sidecar containers. request max(max(regularCointainers) + sum(sidecar), sum(sidecar) + sum(containers))": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -310,7 +310,7 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("4"), }, }, - "adds overhead": { + "adds overhead. request max(max(regularCointainers) + sum(sidecar), sum(sidecar) + sum(containers)) + overhead": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -318,7 +318,7 @@ func TestTotalRequest(t *testing.T) { WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). Obj(), *testingutil.MakeContainer(). @@ -334,7 +334,7 @@ func TestTotalRequest(t *testing.T) { WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). Obj(), }, @@ -345,7 +345,7 @@ func TestTotalRequest(t *testing.T) { }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("6000m"), + corev1.ResourceCPU: resource.MustParse("6"), corev1.ResourceMemory: resource.MustParse("5Gi"), "example.com/gpu": resource.MustParse("5"), }, @@ -370,7 +370,7 @@ func TestValidatePodSpec(t *testing.T) { WithResourceReq("example.com/mainContainerGpu", "2"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). Obj(), }, @@ -380,7 +380,7 @@ func TestValidatePodSpec(t *testing.T) { WithResourceReq(corev1.ResourceMemory, "2Gi"). Obj(), *testingutil.MakeContainer(). - WithResourceReq(corev1.ResourceCPU, "1500m"). + WithResourceReq(corev1.ResourceCPU, "1.5"). WithResourceReq("example.com/gpu", "2"). WithResourceReq("example.com/initContainerGpu", "2"). Obj(), From 48490a688d791c02239baf7b82994d9f781a8b08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Mon, 6 May 2024 17:02:47 +0200 Subject: [PATCH 7/9] Update formula to compute resource requests --- pkg/util/limitrange/limitrange.go | 29 +++++++++++++++----------- pkg/util/limitrange/limitrange_test.go | 16 +++++++------- 2 files changed, 25 insertions(+), 20 deletions(-) diff --git a/pkg/util/limitrange/limitrange.go b/pkg/util/limitrange/limitrange.go index 9f813447fe..bb3dba3a54 100644 --- a/pkg/util/limitrange/limitrange.go +++ b/pkg/util/limitrange/limitrange.go @@ -81,18 +81,14 @@ func (s Summary) validatePodSpecContainers(containers []corev1.Container, path * } // TotalRequests computes the total resource requests of a pod. -// total = Sum(Max( Max(nonSidecarInitContainers) + Sum(SidecarContainers), Sum(SidecarContainers) + Sum(Containers) ), overhead) +// total = Max( Max(each InitContainerUse) , Sum(SidecarContainers) + Sum(Containers) ) + pod overhead func TotalRequests(ps *corev1.PodSpec) corev1.ResourceList { - // add the resource from the main containers - sumContainers := corev1.ResourceList{} - for i := range ps.Containers { - sumContainers = resource.MergeResourceListKeepSum(sumContainers, ps.Containers[i].Resources.Requests) - } - maxNonSidecarInitContainers := calculateRegularInitContainersResources(ps.InitContainers) + sumContainers := calculateMainContainersResources(ps.Containers) + maxInitContainers := calculateInitContainersResources(ps.InitContainers) sumSidecarContainers := calculateSidecarContainersResources(ps.InitContainers) total := resource.MergeResourceListKeepMax( - resource.MergeResourceListKeepSum(maxNonSidecarInitContainers, sumSidecarContainers), + maxInitContainers, resource.MergeResourceListKeepSum(sumSidecarContainers, sumContainers), ) // add the overhead @@ -100,12 +96,21 @@ func TotalRequests(ps *corev1.PodSpec) corev1.ResourceList { return total } -func calculateRegularInitContainersResources(initContainers []corev1.Container) corev1.ResourceList { +func calculateMainContainersResources(containers []corev1.Container) corev1.ResourceList { + total := corev1.ResourceList{} + for i := range containers { + total = resource.MergeResourceListKeepSum(total, containers[i].Resources.Requests) + } + return total +} + +func calculateInitContainersResources(initContainers []corev1.Container) corev1.ResourceList { total := corev1.ResourceList{} for i := range initContainers { - if !isSidecarContainer(initContainers[i]) { - total = resource.MergeResourceListKeepMax(total, initContainers[i].Resources.Requests) - } + sumSidecarContainers := calculateSidecarContainersResources(initContainers[:i]) + initContainerUse := resource.MergeResourceListKeepSum(sumSidecarContainers, initContainers[i].Resources.Requests) + + total = resource.MergeResourceListKeepMax(total, initContainerUse) } return total } diff --git a/pkg/util/limitrange/limitrange_test.go b/pkg/util/limitrange/limitrange_test.go index 7074f177cb..536f5b0f68 100644 --- a/pkg/util/limitrange/limitrange_test.go +++ b/pkg/util/limitrange/limitrange_test.go @@ -183,7 +183,7 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("2"), }, }, - "pod only with regular init containers. request max(regularContainers)": { + "pod only with regular init containers. request max( max(each initContainerUse), sum(containers) )": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -212,7 +212,7 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("2"), }, }, - "pod only with sidecar containers. request sum(sidecar) + sum(containers)": { + "pod only with sidecar containers. request max( max(each initContainerUse), sum(sidecarContainers) + sum(containers) )": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -243,7 +243,7 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("4"), }, }, - "pod only with regular init and sidecar containers. request max(regularContainers) + sum(sidecar)": { + "pod only with regular init and sidecar containers. request max( max(each InitContainerUse), sum(sidecarContainers) )": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -267,11 +267,11 @@ func TestTotalRequest(t *testing.T) { }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("9"), - corev1.ResourceMemory: resource.MustParse("9Gi"), + corev1.ResourceCPU: resource.MustParse("7"), + corev1.ResourceMemory: resource.MustParse("7Gi"), }, }, - "pod with regular init and sidecar containers. request max(max(regularCointainers) + sum(sidecar), sum(sidecar) + sum(containers))": { + "pod with regular init and sidecar containers. request max( max(each InitContainer), sum(sidecarContainers) + sum(containers) )": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -310,7 +310,7 @@ func TestTotalRequest(t *testing.T) { "example.com/gpu": resource.MustParse("4"), }, }, - "adds overhead. request max(max(regularCointainers) + sum(sidecar), sum(sidecar) + sum(containers)) + overhead": { + "adds overhead. request max( max(each InitContainer), sum(sidecarContainers) + sum(containers) ) + overhead": { podSpec: &corev1.PodSpec{ InitContainers: []corev1.Container{ *testingutil.MakeContainer(). @@ -345,7 +345,7 @@ func TestTotalRequest(t *testing.T) { }, }, want: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("6"), + corev1.ResourceCPU: resource.MustParse("5"), corev1.ResourceMemory: resource.MustParse("5Gi"), "example.com/gpu": resource.MustParse("5"), }, From e44dbca56df18a61385f0eabb46cba906a02c2a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Tue, 7 May 2024 14:15:15 +0200 Subject: [PATCH 8/9] Make init containers resource computation linear --- pkg/util/limitrange/limitrange.go | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/pkg/util/limitrange/limitrange.go b/pkg/util/limitrange/limitrange.go index bb3dba3a54..0b8e8ac322 100644 --- a/pkg/util/limitrange/limitrange.go +++ b/pkg/util/limitrange/limitrange.go @@ -81,7 +81,12 @@ func (s Summary) validatePodSpecContainers(containers []corev1.Container, path * } // TotalRequests computes the total resource requests of a pod. -// total = Max( Max(each InitContainerUse) , Sum(SidecarContainers) + Sum(Containers) ) + pod overhead +// +// total = Max( Max(each InitContainerUse) , Sum(SidecarContainers) + Sum(Containers) ) + pod overhead +// +// where: +// +// InitContainerUse(i) = Sum(SidecarContainers with index < i) + InitContainer(i) func TotalRequests(ps *corev1.PodSpec) corev1.ResourceList { sumContainers := calculateMainContainersResources(ps.Containers) maxInitContainers := calculateInitContainersResources(ps.InitContainers) @@ -105,16 +110,27 @@ func calculateMainContainersResources(containers []corev1.Container) corev1.Reso } func calculateInitContainersResources(initContainers []corev1.Container) corev1.ResourceList { + incrementalSum := calculateSidecarContainersIncrementalSum(initContainers) total := corev1.ResourceList{} for i := range initContainers { - sumSidecarContainers := calculateSidecarContainersResources(initContainers[:i]) - initContainerUse := resource.MergeResourceListKeepSum(sumSidecarContainers, initContainers[i].Resources.Requests) - + initContainerUse := resource.MergeResourceListKeepSum(incrementalSum[i], initContainers[i].Resources.Requests) total = resource.MergeResourceListKeepMax(total, initContainerUse) } return total } +func calculateSidecarContainersIncrementalSum(initContainers []corev1.Container) []corev1.ResourceList { + results := make([]corev1.ResourceList, len(initContainers)) + lastResult := corev1.ResourceList{} + for i := 1; i < len(initContainers); i++ { + if isSidecarContainer(initContainers[i-1]) { + lastResult = resource.MergeResourceListKeepSum(results[i-1], initContainers[i-1].Resources.Requests) + } + results[i] = lastResult + } + return results +} + func calculateSidecarContainersResources(initContainers []corev1.Container) corev1.ResourceList { total := corev1.ResourceList{} for i := range initContainers { From 053c150ff587524c855e277dfa4f7679acd5dae6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Irving=20Mondrag=C3=B3n?= Date: Tue, 7 May 2024 15:42:07 +0200 Subject: [PATCH 9/9] Update init containers resource computation --- pkg/util/limitrange/limitrange.go | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/pkg/util/limitrange/limitrange.go b/pkg/util/limitrange/limitrange.go index 0b8e8ac322..98fbde5faf 100644 --- a/pkg/util/limitrange/limitrange.go +++ b/pkg/util/limitrange/limitrange.go @@ -110,25 +110,17 @@ func calculateMainContainersResources(containers []corev1.Container) corev1.Reso } func calculateInitContainersResources(initContainers []corev1.Container) corev1.ResourceList { - incrementalSum := calculateSidecarContainersIncrementalSum(initContainers) - total := corev1.ResourceList{} + maxInitContainerUsage := corev1.ResourceList{} + sidecarRunningUsage := corev1.ResourceList{} for i := range initContainers { - initContainerUse := resource.MergeResourceListKeepSum(incrementalSum[i], initContainers[i].Resources.Requests) - total = resource.MergeResourceListKeepMax(total, initContainerUse) - } - return total -} - -func calculateSidecarContainersIncrementalSum(initContainers []corev1.Container) []corev1.ResourceList { - results := make([]corev1.ResourceList, len(initContainers)) - lastResult := corev1.ResourceList{} - for i := 1; i < len(initContainers); i++ { - if isSidecarContainer(initContainers[i-1]) { - lastResult = resource.MergeResourceListKeepSum(results[i-1], initContainers[i-1].Resources.Requests) + if isSidecarContainer(initContainers[i]) { + sidecarRunningUsage = resource.MergeResourceListKeepSum(sidecarRunningUsage, initContainers[i].Resources.Requests) + } else { + initContainerUse := resource.MergeResourceListKeepSum(initContainers[i].Resources.Requests, sidecarRunningUsage) + maxInitContainerUsage = resource.MergeResourceListKeepMax(maxInitContainerUsage, initContainerUse) } - results[i] = lastResult } - return results + return maxInitContainerUsage } func calculateSidecarContainersResources(initContainers []corev1.Container) corev1.ResourceList {