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.

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

[cherry-pick 0.10.x] leave max container request values at original index #1962

Merged
merged 1 commit into from
Jan 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/pod/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ 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, move max resource requests to the last step.
// Zero out non-max resource requests.
stepContainers = resolveResourceRequests(stepContainers)

// Add implicit env vars.
Expand Down
16 changes: 11 additions & 5 deletions pkg/pod/pod_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -436,10 +436,16 @@ func TestMakePod(t *testing.T) {
"cmd",
"--",
},
Env: implicitEnvVars,
VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...),
WorkingDir: pipeline.WorkspaceDir,
Resources: corev1.ResourceRequirements{Requests: allZeroQty()},
Env: implicitEnvVars,
VolumeMounts: append([]corev1.VolumeMount{toolsMount, downwardMount}, implicitVolumeMounts...),
WorkingDir: pipeline.WorkspaceDir,
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceMemory: zeroQty,
corev1.ResourceEphemeralStorage: zeroQty,
},
},
TerminationMessagePath: "/tekton/termination",
}, {
Name: "step-unnamed-1",
Expand All @@ -461,7 +467,7 @@ func TestMakePod(t *testing.T) {
WorkingDir: pipeline.WorkspaceDir,
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("8"),
corev1.ResourceCPU: zeroQty,
corev1.ResourceMemory: resource.MustParse("100Gi"),
corev1.ResourceEphemeralStorage: zeroQty,
},
Expand Down
30 changes: 23 additions & 7 deletions pkg/pod/resource_request.go
Original file line number Diff line number Diff line change
Expand Up @@ -33,20 +33,36 @@ func allZeroQty() corev1.ResourceList {

func resolveResourceRequests(containers []corev1.Container) []corev1.Container {
max := allZeroQty()
for _, c := range containers {
resourceNames := []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceEphemeralStorage}
maxIndicesByResource := make(map[corev1.ResourceName]int, len(resourceNames))
for _, resourceName := range resourceNames {
maxIndicesByResource[resourceName] = -1
}

// Find max resource requests and associated list indices for
// containers for CPU, memory, and ephemeral storage resources
for i, c := range containers {
for k, v := range c.Resources.Requests {
if v.Cmp(max[k]) > 0 {
maxIndicesByResource[k] = i
max[k] = v
}
}
}

// Set resource requests for all steps but the last container to
// zero.
for i := range containers[:len(containers)-1] {
containers[i].Resources.Requests = allZeroQty()
// Set all non max resource requests to 0. Leave max request at index
// originally defined to account for limit of step.
for i := range containers {
if containers[i].Resources.Requests == nil {
containers[i].Resources.Requests = allZeroQty()
continue
}
for _, resourceName := range resourceNames {
if maxIndicesByResource[resourceName] != i {
containers[i].Resources.Requests[resourceName] = zeroQty
}
}
}
// Set the last container's request to the max of all resources.
containers[len(containers)-1].Resources.Requests = max

return containers
}
123 changes: 117 additions & 6 deletions pkg/pod/resource_request_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,18 +70,61 @@ func TestResolveResourceRequests(t *testing.T) {
},
}},
want: []corev1.Container{{
// All zeroed out.
Resources: corev1.ResourceRequirements{Requests: allZeroQty()},
// ResourceCPU max request
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10"),
corev1.ResourceMemory: zeroQty,
corev1.ResourceEphemeralStorage: zeroQty,
},
},
}, {
// Requests zeroed out, limits remain.
// ResourceMemory max request
Resources: corev1.ResourceRequirements{
Requests: allZeroQty(),
Requests: corev1.ResourceList{
corev1.ResourceCPU: zeroQty,
corev1.ResourceMemory: resource.MustParse("10Gi"),
corev1.ResourceEphemeralStorage: zeroQty,
},
Limits: corev1.ResourceList{
corev1.ResourceMemory: resource.MustParse("11Gi"),
},
},
}, {
// ResourceEphemeralStorage max request
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: zeroQty,
corev1.ResourceMemory: zeroQty,
corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"),
},
Limits: corev1.ResourceList{
corev1.ResourceMemory: resource.MustParse("100Gi"),
},
},
}},
}, {
desc: "Max requests all with step2",
in: []corev1.Container{{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10"),
corev1.ResourceMemory: resource.MustParse("10Gi"),
corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"),
},
},
}, {
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("11"),
corev1.ResourceMemory: resource.MustParse("11Gi"),
corev1.ResourceEphemeralStorage: resource.MustParse("101Gi"),
},
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"),
Expand All @@ -93,7 +136,75 @@ func TestResolveResourceRequests(t *testing.T) {
},
},
}},
}} {
want: []corev1.Container{{
// All zeroed out since step 2 has max requests
Resources: corev1.ResourceRequirements{
Requests: allZeroQty(),
},
}, {
// All max requests
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("11"),
corev1.ResourceMemory: resource.MustParse("11Gi"),
corev1.ResourceEphemeralStorage: resource.MustParse("101Gi"),
},
Limits: corev1.ResourceList{
corev1.ResourceMemory: resource.MustParse("11Gi"),
},
},
}, {
// All zeroed out since step 2 has max requests
Resources: corev1.ResourceRequirements{
Requests: allZeroQty(),
Limits: corev1.ResourceList{
corev1.ResourceMemory: resource.MustParse("100Gi"),
},
},
}},
},
{
desc: "Only one step container with only memory request value filled out",
in: []corev1.Container{{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceMemory: resource.MustParse("10Gi"),
},
},
}},
want: []corev1.Container{{
// ResourceMemory max request set. zeroQty for non set resources.
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: zeroQty,
corev1.ResourceMemory: resource.MustParse("10Gi"),
corev1.ResourceEphemeralStorage: zeroQty,
},
},
}},
}, {
desc: "Only one step container with all request values filled out",
in: []corev1.Container{{
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10"),
corev1.ResourceMemory: resource.MustParse("10Gi"),
corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"),
},
},
}},
want: []corev1.Container{{
// All max values set
Resources: corev1.ResourceRequirements{
Requests: corev1.ResourceList{
corev1.ResourceCPU: resource.MustParse("10"),
corev1.ResourceMemory: resource.MustParse("10Gi"),
corev1.ResourceEphemeralStorage: resource.MustParse("100Gi"),
},
},
}},
},
} {
t.Run(c.desc, func(t *testing.T) {
got := resolveResourceRequests(c.in)
if d := cmp.Diff(c.want, got, resourceQuantityCmp); d != "" {
Expand Down