Skip to content

Commit 08c1486

Browse files
MarSikMarSik
committed
OCPBUGS-52169: <carry>: Workload partitioning of static init containers
The pod modification routine that prepares containers for Workload Partitioning quits early when it encounters a container with no resources specified. This causes a logical leak of resource capacity on the node, because the Pod is left untouched and still reports its full resource requests to the scheduler. It is however not using them, because the logic that moves the container to the management partitions works just fine. The end result is lowered node capacity for scheduling. Signed-off-by: Martin Sivak <[email protected]>
1 parent c3a6a36 commit 08c1486

File tree

2 files changed

+189
-17
lines changed

2 files changed

+189
-17
lines changed

‎pkg/kubelet/managed/managed.go

+18-3
Original file line numberDiff line numberDiff line change
@@ -117,14 +117,29 @@ func GenerateResourceName(workloadName string) v1.ResourceName {
117117
func updateContainers(workloadName string, pod *v1.Pod) error {
118118
updateContainer := func(container *v1.Container) error {
119119
if container.Resources.Requests == nil {
120-
return fmt.Errorf("managed container %v does not have Resource.Requests", container.Name)
120+
// Nothing to modify, but that is OK, it will not
121+
// change the QoS class of the modified Pod
122+
return nil
121123
}
122-
if _, ok := container.Resources.Requests[v1.ResourceCPU]; !ok {
124+
125+
_, cpuOk := container.Resources.Requests[v1.ResourceCPU]
126+
_, memoryOk := container.Resources.Requests[v1.ResourceMemory]
127+
128+
// When both cpu and memory requests are missing, there is nothing
129+
// to do
130+
if !cpuOk && !memoryOk {
131+
return nil
132+
}
133+
134+
// Both memory and cpu have to be set to make sure stripping them
135+
// will not change the QoS class of the Pod
136+
if !cpuOk {
123137
return fmt.Errorf("managed container %v does not have cpu requests", container.Name)
124138
}
125-
if _, ok := container.Resources.Requests[v1.ResourceMemory]; !ok {
139+
if !memoryOk {
126140
return fmt.Errorf("managed container %v does not have memory requests", container.Name)
127141
}
142+
128143
if container.Resources.Limits == nil {
129144
container.Resources.Limits = v1.ResourceList{}
130145
}

‎pkg/kubelet/managed/managed_test.go

+171-14
Original file line numberDiff line numberDiff line change
@@ -19,17 +19,6 @@ func TestModifyStaticPodForPinnedManagementErrorStates(t *testing.T) {
1919
pod *v1.Pod
2020
expectedError error
2121
}{
22-
{
23-
pod: createPod(workloadAnnotations, nil,
24-
&v1.Container{
25-
Name: "nginx",
26-
Image: "test/image",
27-
Resources: v1.ResourceRequirements{
28-
Requests: nil,
29-
},
30-
}),
31-
expectedError: fmt.Errorf("managed container nginx does not have Resource.Requests"),
32-
},
3322
{
3423
pod: createPod(workloadAnnotations, nil,
3524
&v1.Container{
@@ -129,6 +118,7 @@ func TestStaticPodManaged(t *testing.T) {
129118
pod *v1.Pod
130119
expectedAnnotations map[string]string
131120
isGuaranteed bool
121+
isBestEffort bool
132122
}{
133123
{
134124
pod: &v1.Pod{
@@ -270,6 +260,164 @@ func TestStaticPodManaged(t *testing.T) {
270260
"resources.workload.openshift.io/c1": `{"cpushares":20,"cpulimit":100}`,
271261
},
272262
},
263+
{
264+
pod: &v1.Pod{
265+
TypeMeta: metav1.TypeMeta{
266+
Kind: "Pod",
267+
APIVersion: "",
268+
},
269+
ObjectMeta: metav1.ObjectMeta{
270+
Name: "test",
271+
UID: "12345",
272+
Namespace: "mynamespace",
273+
Annotations: map[string]string{
274+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
275+
},
276+
},
277+
Spec: v1.PodSpec{
278+
Containers: []v1.Container{
279+
{
280+
Name: "c1",
281+
Image: "test/nginx",
282+
Resources: v1.ResourceRequirements{},
283+
},
284+
},
285+
SecurityContext: &v1.PodSecurityContext{},
286+
},
287+
Status: v1.PodStatus{
288+
Phase: v1.PodPending,
289+
},
290+
},
291+
expectedAnnotations: map[string]string{
292+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
293+
},
294+
isBestEffort: true,
295+
},
296+
{
297+
pod: &v1.Pod{
298+
TypeMeta: metav1.TypeMeta{
299+
Kind: "Pod",
300+
APIVersion: "",
301+
},
302+
ObjectMeta: metav1.ObjectMeta{
303+
Name: "test",
304+
UID: "12345",
305+
Namespace: "mynamespace",
306+
Annotations: map[string]string{
307+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
308+
},
309+
},
310+
Spec: v1.PodSpec{
311+
Containers: []v1.Container{
312+
{
313+
Name: "c1",
314+
Image: "test/nginx",
315+
Resources: v1.ResourceRequirements{
316+
Requests: v1.ResourceList{
317+
v1.ResourceName("dummy"): resource.MustParse("20m"),
318+
},
319+
},
320+
},
321+
},
322+
SecurityContext: &v1.PodSecurityContext{},
323+
},
324+
Status: v1.PodStatus{
325+
Phase: v1.PodPending,
326+
},
327+
},
328+
expectedAnnotations: map[string]string{
329+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
330+
},
331+
isBestEffort: true,
332+
},
333+
{
334+
pod: &v1.Pod{
335+
TypeMeta: metav1.TypeMeta{
336+
Kind: "Pod",
337+
APIVersion: "",
338+
},
339+
ObjectMeta: metav1.ObjectMeta{
340+
Name: "test",
341+
UID: "12345",
342+
Namespace: "mynamespace",
343+
Annotations: map[string]string{
344+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
345+
},
346+
},
347+
Spec: v1.PodSpec{
348+
Containers: []v1.Container{
349+
{
350+
Name: "c1",
351+
Image: "test/nginx",
352+
Resources: v1.ResourceRequirements{},
353+
},
354+
},
355+
InitContainers: []v1.Container{
356+
{
357+
Name: "ic1",
358+
Image: "test/nginx",
359+
Resources: v1.ResourceRequirements{},
360+
},
361+
},
362+
SecurityContext: &v1.PodSecurityContext{},
363+
},
364+
Status: v1.PodStatus{
365+
Phase: v1.PodPending,
366+
},
367+
},
368+
expectedAnnotations: map[string]string{
369+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
370+
},
371+
isBestEffort: true,
372+
},
373+
{
374+
pod: &v1.Pod{
375+
TypeMeta: metav1.TypeMeta{
376+
Kind: "Pod",
377+
APIVersion: "",
378+
},
379+
ObjectMeta: metav1.ObjectMeta{
380+
Name: "test",
381+
UID: "12345",
382+
Namespace: "mynamespace",
383+
Annotations: map[string]string{
384+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
385+
},
386+
},
387+
Spec: v1.PodSpec{
388+
Containers: []v1.Container{
389+
{
390+
Name: "c1",
391+
Image: "test/nginx",
392+
Resources: v1.ResourceRequirements{
393+
Requests: v1.ResourceList{
394+
v1.ResourceName("dummy"): resource.MustParse("20m"),
395+
},
396+
},
397+
},
398+
},
399+
InitContainers: []v1.Container{
400+
{
401+
Name: "ic1",
402+
Image: "test/nginx",
403+
Resources: v1.ResourceRequirements{
404+
Requests: v1.ResourceList{
405+
v1.ResourceName("dummy"): resource.MustParse("20m"),
406+
},
407+
},
408+
},
409+
},
410+
SecurityContext: &v1.PodSecurityContext{},
411+
},
412+
Status: v1.PodStatus{
413+
Phase: v1.PodPending,
414+
},
415+
},
416+
expectedAnnotations: map[string]string{
417+
"target.workload.openshift.io/management": `{"effect": "PreferredDuringScheduling"}`,
418+
},
419+
isBestEffort: true,
420+
},
273421
{
274422
pod: &v1.Pod{
275423
TypeMeta: metav1.TypeMeta{
@@ -481,15 +629,24 @@ func TestStaticPodManaged(t *testing.T) {
481629
if container.Resources.Requests.Cpu().String() != "0" && !tc.isGuaranteed {
482630
t.Errorf("cpu requests should be 0 got %v", container.Resources.Requests.Cpu().String())
483631
}
484-
if container.Resources.Requests.Memory().String() == "0" && !tc.isGuaranteed {
632+
if container.Resources.Requests.Memory().String() == "0" && !tc.isGuaranteed && !tc.isBestEffort {
485633
t.Errorf("memory requests were %v but should be %v", container.Resources.Requests.Memory().String(), container.Resources.Requests.Memory().String())
486634
}
487-
if _, exists := container.Resources.Requests[GenerateResourceName(workloadName)]; !exists && !tc.isGuaranteed {
635+
if container.Resources.Requests.Memory().String() != "0" && !tc.isGuaranteed && tc.isBestEffort {
636+
t.Errorf("memory requests should be 0 got %v", container.Resources.Requests.Memory().String())
637+
}
638+
if _, exists := container.Resources.Requests[GenerateResourceName(workloadName)]; !exists && !tc.isGuaranteed && !tc.isBestEffort {
488639
t.Errorf("managed capacity label missing from pod %v and container %v", tc.pod.Name, container.Name)
489640
}
490-
if _, exists := container.Resources.Limits[GenerateResourceName(workloadName)]; !exists && !tc.isGuaranteed {
641+
if _, exists := container.Resources.Limits[GenerateResourceName(workloadName)]; !exists && !tc.isGuaranteed && !tc.isBestEffort {
491642
t.Errorf("managed capacity label missing from pod %v and container %v limits", tc.pod.Name, container.Name)
492643
}
644+
if _, exists := container.Resources.Requests[GenerateResourceName(workloadName)]; exists && tc.isBestEffort {
645+
t.Errorf("managed capacity label present in best-effort pod %v and container %v requests", tc.pod.Name, container.Name)
646+
}
647+
if _, exists := container.Resources.Limits[GenerateResourceName(workloadName)]; exists && tc.isBestEffort {
648+
t.Errorf("managed capacity label present in best-effort pod %v and container %v limits", tc.pod.Name, container.Name)
649+
}
493650
}
494651
}
495652
}

0 commit comments

Comments
 (0)