diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index 75b84259111..7b90fe18632 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -77,8 +77,6 @@ var ( VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{}}, }} - zeroQty = resource.MustParse("0") - // Random byte reader used for pod name generation. // var for testing. randReader = rand.Reader @@ -136,12 +134,8 @@ func MakePod(images pipeline.Images, taskRun *v1alpha1.TaskRun, taskSpec v1alpha initContainers = append(initContainers, entrypointInit) volumes = append(volumes, toolsVolume, downwardVolume) - // Zero out non-max resource requests. - // TODO(#1605): Split this out so it's more easily testable. - maxIndicesByResource := findMaxResourceRequest(taskSpec.Steps, corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage) - for i := range stepContainers { - zeroNonMaxResourceRequests(&stepContainers[i], i, maxIndicesByResource) - } + // Zero out non-max resource requests, move max resource requests to the last step. + stepContainers = resolveResourceRequests(stepContainers) // Add implicit env vars. // They're prepended to the list, so that if the user specified any diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go index a4d3b3f8191..24f6954d3a1 100644 --- a/pkg/pod/pod_test.go +++ b/pkg/pod/pod_test.go @@ -39,9 +39,6 @@ var ( CredsImage: "override-with-creds:latest", ShellImage: "busybox", } - resourceQuantityCmp = cmp.Comparer(func(x, y resource.Quantity) bool { - return x.Cmp(y) == 0 - }) ) func TestMakePod(t *testing.T) { @@ -103,13 +100,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }}, Volumes: append(implicitVolumes, toolsVolume, downwardVolume), }, @@ -160,13 +151,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }}, Volumes: append(implicitVolumes, secretsVolume, toolsVolume, downwardVolume), }, @@ -209,13 +194,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }}, Volumes: append(implicitVolumes, toolsVolume, downwardVolume), SecurityContext: &corev1.PodSecurityContext{ @@ -254,13 +233,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }}, Volumes: append(implicitVolumes, toolsVolume, downwardVolume), }, @@ -293,13 +266,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }}, Volumes: append(implicitVolumes, toolsVolume, downwardVolume), }, @@ -342,13 +309,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: filepath.Join(workspaceDir, "test"), - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }}, Volumes: append(implicitVolumes, toolsVolume, downwardVolume), }, @@ -386,13 +347,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }, { Name: "sidecar-sc-name", Image: "sidecar-image", @@ -445,13 +400,7 @@ func TestMakePod(t *testing.T) { Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("8"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }, { Name: "step-unnamed-1", Image: "image", @@ -470,7 +419,7 @@ func TestMakePod(t *testing.T) { WorkingDir: workspaceDir, Resources: corev1.ResourceRequirements{ Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), + corev1.ResourceCPU: resource.MustParse("8"), corev1.ResourceMemory: resource.MustParse("100Gi"), corev1.ResourceEphemeralStorage: resource.MustParse("0"), }, @@ -540,13 +489,7 @@ script-heredoc-randomly-generated-6nl7g Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{scriptsVolumeMount, toolsMount, downwardMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }, { Name: "step-two", Image: "image", @@ -563,13 +506,7 @@ script-heredoc-randomly-generated-6nl7g Env: implicitEnvVars, VolumeMounts: append([]corev1.VolumeMount{{Name: "i-have-a-volume-mount"}, scriptsVolumeMount, toolsMount}, implicitVolumeMounts...), WorkingDir: workspaceDir, - Resources: corev1.ResourceRequirements{ - Requests: corev1.ResourceList{ - corev1.ResourceCPU: resource.MustParse("0"), - corev1.ResourceMemory: resource.MustParse("0"), - corev1.ResourceEphemeralStorage: resource.MustParse("0"), - }, - }, + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, }}, Volumes: append(implicitVolumes, scriptsVolume, toolsVolume, downwardVolume), }, diff --git a/pkg/pod/resource_request.go b/pkg/pod/resource_request.go new file mode 100644 index 00000000000..2fd4abdb2e8 --- /dev/null +++ b/pkg/pod/resource_request.go @@ -0,0 +1,36 @@ +package pod + +import ( + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +var zeroQty = resource.MustParse("0") + +func allZeroQty() corev1.ResourceList { + return corev1.ResourceList{ + corev1.ResourceCPU: zeroQty, + corev1.ResourceMemory: zeroQty, + corev1.ResourceEphemeralStorage: zeroQty, + } +} + +func resolveResourceRequests(containers []corev1.Container) []corev1.Container { + max := allZeroQty() + for _, c := range containers { + for k, v := range c.Resources.Requests { + if v.Cmp(max[k]) > 0 { + max[k] = v + } + } + } + + // Set resource requests for all steps but the theh last container to + // zero. + for i := range containers[:len(containers)-1] { + containers[i].Resources.Requests = allZeroQty() + } + // Set the last container's request to the max of all resources. + containers[len(containers)-1].Resources.Requests = max + return containers +} diff --git a/pkg/pod/resource_request_test.go b/pkg/pod/resource_request_test.go new file mode 100644 index 00000000000..d838667a628 --- /dev/null +++ b/pkg/pod/resource_request_test.go @@ -0,0 +1,88 @@ +package pod + +import ( + "testing" + + "github.com/google/go-cmp/cmp" + corev1 "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" +) + +var resourceQuantityCmp = cmp.Comparer(func(x, y resource.Quantity) bool { + return x.Cmp(y) == 0 +}) + +func TestResolveResourceRequests(t *testing.T) { + for _, c := range []struct { + desc string + in, want []corev1.Container + }{{ + desc: "three steps, no requests", + in: []corev1.Container{{}, {}, {}}, + want: []corev1.Container{{ + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, + }, { + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, + }, { + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, + }}, + }, { + desc: "requests are moved, limits aren't changed", + in: []corev1.Container{{ + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + }, + }, + }, { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("10Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("11Gi"), + }, + }, + }, { + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("100Gi"), + }, + }, + }}, + want: []corev1.Container{{ + // All zeroed out. + Resources: corev1.ResourceRequirements{Requests: allZeroQty()}, + }, { + // Requests zeroed out, limits remain. + Resources: corev1.ResourceRequirements{ + Requests: allZeroQty(), + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("11Gi"), + }, + }, + }, { + // Requests to the max, limits remain. + Resources: corev1.ResourceRequirements{ + Requests: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("10"), + corev1.ResourceMemory: resource.MustParse("10Gi"), + corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"), + }, + Limits: corev1.ResourceList{ + corev1.ResourceMemory: resource.MustParse("100Gi"), + }, + }, + }}, + }} { + t.Run(c.desc, func(t *testing.T) { + got := resolveResourceRequests(c.in) + if d := cmp.Diff(c.want, got, resourceQuantityCmp); d != "" { + t.Errorf("Diff(-want, +got): %s", d) + } + }) + } +}