Skip to content

Commit 28dea49

Browse files
ndixitandixita
committed
Limit Range changes to validate against Pod Level Resources
1 parent 99a6153 commit 28dea49

File tree

2 files changed

+134
-22
lines changed

2 files changed

+134
-22
lines changed

‎plugin/pkg/admission/limitranger/admission.go‎

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,14 +32,17 @@ import (
3232
"k8s.io/apimachinery/pkg/labels"
3333
"k8s.io/apimachinery/pkg/runtime"
3434
utilerrors "k8s.io/apimachinery/pkg/util/errors"
35+
"k8s.io/apimachinery/pkg/util/sets"
3536
"k8s.io/apiserver/pkg/admission"
3637
genericadmissioninitailizer "k8s.io/apiserver/pkg/admission/initializer"
38+
"k8s.io/apiserver/pkg/util/feature"
3739
"k8s.io/client-go/informers"
3840
"k8s.io/client-go/kubernetes"
3941
corev1listers "k8s.io/client-go/listers/core/v1"
4042
"k8s.io/utils/lru"
4143

4244
api "k8s.io/kubernetes/pkg/apis/core"
45+
"k8s.io/kubernetes/pkg/features"
4346
)
4447

4548
const (
@@ -528,8 +531,11 @@ func PodValidateLimitFunc(limitRange *corev1.LimitRange, pod *api.Pod) error {
528531

529532
// enforce pod limits on init containers
530533
if limitType == corev1.LimitTypePod {
531-
podRequests := podRequests(pod)
532-
podLimits := podLimits(pod)
534+
opts := podResourcesOptions{
535+
PodLevelResourcesEnabled: feature.DefaultFeatureGate.Enabled(features.PodLevelResources),
536+
}
537+
podRequests := podRequests(pod, opts)
538+
podLimits := podLimits(pod, opts)
533539
for k, v := range limit.Min {
534540
if err := minConstraint(string(limitType), string(k), v, podRequests, podLimits); err != nil {
535541
errs = append(errs, err)
@@ -550,13 +556,22 @@ func PodValidateLimitFunc(limitRange *corev1.LimitRange, pod *api.Pod) error {
550556
return utilerrors.NewAggregate(errs)
551557
}
552558

559+
type podResourcesOptions struct {
560+
// PodLevelResourcesEnabled indicates that the PodLevelResources feature gate is
561+
// enabled.
562+
PodLevelResourcesEnabled bool
563+
}
564+
553565
// podRequests is a simplified version of pkg/api/v1/resource/PodRequests that operates against the core version of
554566
// pod. Any changes to that calculation should be reflected here.
555567
// NOTE: We do not want to check status resources here, only the spec. This is equivalent to setting
556568
// UseStatusResources=false in the common helper.
557569
// TODO: Maybe we can consider doing a partial conversion of the pod to a v1
558570
// type and then using the pkg/api/v1/resource/PodRequests.
559-
func podRequests(pod *api.Pod) api.ResourceList {
571+
// TODO(ndixita): PodRequests method exists in
572+
// staging/src/k8s.io/component-helpers/resource/helpers.go. Refactor the code to
573+
// avoid duplicating podRequests method.
574+
func podRequests(pod *api.Pod, opts podResourcesOptions) api.ResourceList {
560575
reqs := api.ResourceList{}
561576

562577
for _, container := range pod.Spec.Containers {
@@ -589,6 +604,19 @@ func podRequests(pod *api.Pod) api.ResourceList {
589604
}
590605

591606
maxResourceList(reqs, initContainerReqs)
607+
608+
// If PodLevelResources feature is enabled and resources are set at pod-level,
609+
// override aggregated container requests of resources supported by pod-level
610+
// resources with quantities specified at pod-level.
611+
if opts.PodLevelResourcesEnabled && pod.Spec.Resources != nil {
612+
for resourceName, quantity := range pod.Spec.Resources.Requests {
613+
if isSupportedPodLevelResource(resourceName) {
614+
// override with pod-level resource requests
615+
reqs[resourceName] = quantity
616+
}
617+
}
618+
}
619+
592620
return reqs
593621
}
594622

@@ -598,7 +626,10 @@ func podRequests(pod *api.Pod) api.ResourceList {
598626
// UseStatusResources=false in the common helper.
599627
// TODO: Maybe we can consider doing a partial conversion of the pod to a v1
600628
// type and then using the pkg/api/v1/resource/PodLimits.
601-
func podLimits(pod *api.Pod) api.ResourceList {
629+
// TODO(ndixita): PodLimits method exists in
630+
// staging/src/k8s.io/component-helpers/resource/helpers.go. Refactor the code to
631+
// avoid duplicating podLimits method.
632+
func podLimits(pod *api.Pod, opts podResourcesOptions) api.ResourceList {
602633
limits := api.ResourceList{}
603634

604635
for _, container := range pod.Spec.Containers {
@@ -628,9 +659,35 @@ func podLimits(pod *api.Pod) api.ResourceList {
628659

629660
maxResourceList(limits, initContainerLimits)
630661

662+
// If PodLevelResources feature is enabled and resources are set at pod-level,
663+
// override aggregated container limits of resources supported by pod-level
664+
// resources with quantities specified at pod-level.
665+
if opts.PodLevelResourcesEnabled && pod.Spec.Resources != nil {
666+
for resourceName, quantity := range pod.Spec.Resources.Limits {
667+
if isSupportedPodLevelResource(resourceName) {
668+
// override with pod-level resource limits
669+
limits[resourceName] = quantity
670+
}
671+
}
672+
}
673+
631674
return limits
632675
}
633676

677+
var supportedPodLevelResources = sets.New(api.ResourceCPU, api.ResourceMemory)
678+
679+
// isSupportedPodLevelResources checks if a given resource is supported by pod-level
680+
// resource management through the PodLevelResources feature. Returns true if
681+
// the resource is supported.
682+
// isSupportedPodLevelResource method exists in
683+
// staging/src/k8s.io/component-helpers/resource/helpers.go.
684+
// isSupportedPodLevelResource is added here to avoid conversion of v1.
685+
// Pod to api.Pod.
686+
// TODO(ndixita): Find alternatives to avoid duplicating the code.
687+
func isSupportedPodLevelResource(name api.ResourceName) bool {
688+
return supportedPodLevelResources.Has(name)
689+
}
690+
634691
// addResourceList adds the resources in newList to list.
635692
func addResourceList(list, newList api.ResourceList) {
636693
for name, quantity := range newList {

‎plugin/pkg/admission/limitranger/admission_test.go‎

Lines changed: 73 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -158,6 +158,12 @@ func validLimitRangeNoDefaults() corev1.LimitRange {
158158
return externalLimitRange
159159
}
160160

161+
func validPodWithPodLevelResources(name string, numContainers int, containerResources api.ResourceRequirements, podResources api.ResourceRequirements) api.Pod {
162+
pod := validPod(name, numContainers, containerResources)
163+
pod.Spec.Resources = &podResources
164+
return pod
165+
}
166+
161167
func validPod(name string, numContainers int, resources api.ResourceRequirements) api.Pod {
162168
pod := api.Pod{
163169
ObjectMeta: metav1.ObjectMeta{Name: name, Namespace: "test"},
@@ -280,8 +286,9 @@ func TestMergePodResourceRequirements(t *testing.T) {
280286

281287
func TestPodLimitFunc(t *testing.T) {
282288
type testCase struct {
283-
pod api.Pod
284-
limitRange corev1.LimitRange
289+
pod api.Pod
290+
limitRange corev1.LimitRange
291+
podLevelResourcesEnabled bool
285292
}
286293

287294
successCases := []testCase{
@@ -453,17 +460,42 @@ func TestPodLimitFunc(t *testing.T) {
453460
pod: validPod("pod-max-local-ephemeral-storage-ratio", 3, getResourceRequirements(getLocalStorageResourceList("300Mi"), getLocalStorageResourceList("450Mi"))),
454461
limitRange: createLimitRange(api.LimitTypePod, api.ResourceList{}, getLocalStorageResourceList("2Gi"), api.ResourceList{}, api.ResourceList{}, getLocalStorageResourceList("1.5")),
455462
},
463+
{
464+
pod: validPodWithPodLevelResources("pod-level-resources-with-min-max", 3, getResourceRequirements(getComputeResourceList("100m", "60Mi"), getComputeResourceList("200m", "100Mi")),
465+
getResourceRequirements(getComputeResourceList("200m", "180Mi"), getComputeResourceList("400m", "200Mi")),
466+
),
467+
limitRange: createLimitRange(api.LimitTypePod, api.ResourceList{}, getComputeResourceList("400m", "200Mi"), api.ResourceList{}, api.ResourceList{}, api.ResourceList{}),
468+
podLevelResourcesEnabled: true,
469+
},
470+
{
471+
pod: validPodWithPodLevelResources("pod-level-requests-with-min", 3, getResourceRequirements(getComputeResourceList("50m", "60Mi"), getComputeResourceList("", "")),
472+
getResourceRequirements(getComputeResourceList("160m", "200Mi"), getComputeResourceList("", "")),
473+
),
474+
limitRange: createLimitRange(api.LimitTypePod, getComputeResourceList("160m", "200Mi"), getComputeResourceList("", ""), api.ResourceList{}, api.ResourceList{}, api.ResourceList{}),
475+
podLevelResourcesEnabled: true,
476+
},
477+
{
478+
pod: validPodWithPodLevelResources("pod-level-limits-with-max", 3, getResourceRequirements(getComputeResourceList("", ""), getComputeResourceList("50m", "60Mi")),
479+
getResourceRequirements(getComputeResourceList("", ""), getComputeResourceList("160m", "200Mi")),
480+
),
481+
limitRange: createLimitRange(api.LimitTypePod, getComputeResourceList("", ""), getComputeResourceList("160m", "200Mi"), api.ResourceList{}, api.ResourceList{}, api.ResourceList{}),
482+
podLevelResourcesEnabled: true,
483+
},
456484
}
457485
for i := range successCases {
458486
test := successCases[i]
459-
err := PodMutateLimitFunc(&test.limitRange, &test.pod)
460-
if err != nil {
461-
t.Errorf("Unexpected error for pod: %s, %v", test.pod.Name, err)
462-
}
463-
err = PodValidateLimitFunc(&test.limitRange, &test.pod)
464-
if err != nil {
465-
t.Errorf("Unexpected error for pod: %s, %v", test.pod.Name, err)
466-
}
487+
t.Run(test.pod.Name, func(t *testing.T) {
488+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodLevelResources, test.podLevelResourcesEnabled)
489+
err := PodMutateLimitFunc(&test.limitRange, &test.pod)
490+
if err != nil {
491+
t.Errorf("Unexpected error for pod: %s, %v", test.pod.Name, err)
492+
}
493+
494+
err = PodValidateLimitFunc(&test.limitRange, &test.pod)
495+
if err != nil {
496+
t.Errorf("Unexpected error for pod: %s, %v", test.pod.Name, err)
497+
}
498+
})
467499
}
468500

469501
errorCases := []testCase{
@@ -641,18 +673,41 @@ func TestPodLimitFunc(t *testing.T) {
641673
pod: withRestartableInitContainer(getComputeResourceList("1500m", ""), api.ResourceList{},
642674
validPod("ctr-max-cpu-limit-restartable-init-container", 1, getResourceRequirements(getComputeResourceList("1000m", ""), getComputeResourceList("1500m", "")))),
643675
limitRange: createLimitRange(api.LimitTypePod, api.ResourceList{}, getComputeResourceList("2", ""), api.ResourceList{}, api.ResourceList{}, api.ResourceList{}),
676+
}, {
677+
pod: validPodWithPodLevelResources("pod-level-resources-exceeding-max", 3, getResourceRequirements(getComputeResourceList("100m", "60Mi"), getComputeResourceList("200m", "100Mi")),
678+
getResourceRequirements(getComputeResourceList("200m", "180Mi"), getComputeResourceList("500m", "280Mi")),
679+
),
680+
limitRange: createLimitRange(api.LimitTypePod, api.ResourceList{}, getComputeResourceList("400m", "200Mi"), api.ResourceList{}, api.ResourceList{}, api.ResourceList{}),
681+
podLevelResourcesEnabled: true,
682+
},
683+
{
684+
pod: validPodWithPodLevelResources("pod-level-requests-less-than-min", 3, getResourceRequirements(getComputeResourceList("50m", "60Mi"), getComputeResourceList("", "")),
685+
getResourceRequirements(getComputeResourceList("100m", "200Mi"), getComputeResourceList("", "")),
686+
),
687+
limitRange: createLimitRange(api.LimitTypePod, getComputeResourceList("160m", "200Mi"), getComputeResourceList("", ""), api.ResourceList{}, api.ResourceList{}, api.ResourceList{}),
688+
podLevelResourcesEnabled: true,
689+
},
690+
{
691+
pod: validPodWithPodLevelResources("pod-level-limits-exceeding-max", 3, getResourceRequirements(getComputeResourceList("", ""), getComputeResourceList("50m", "60Mi")),
692+
getResourceRequirements(getComputeResourceList("", ""), getComputeResourceList("160m", "300Mi")),
693+
),
694+
limitRange: createLimitRange(api.LimitTypePod, getComputeResourceList("", ""), getComputeResourceList("160m", "200Mi"), api.ResourceList{}, api.ResourceList{}, api.ResourceList{}),
695+
podLevelResourcesEnabled: true,
644696
},
645697
}
646698
for i := range errorCases {
647699
test := errorCases[i]
648-
err := PodMutateLimitFunc(&test.limitRange, &test.pod)
649-
if err != nil {
650-
t.Errorf("Unexpected error for pod: %s, %v", test.pod.Name, err)
651-
}
652-
err = PodValidateLimitFunc(&test.limitRange, &test.pod)
653-
if err == nil {
654-
t.Errorf("Expected error for pod: %s", test.pod.Name)
655-
}
700+
t.Run(test.pod.Name, func(t *testing.T) {
701+
featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.PodLevelResources, test.podLevelResourcesEnabled)
702+
err := PodMutateLimitFunc(&test.limitRange, &test.pod)
703+
if err != nil {
704+
t.Errorf("Unexpected error for pod: %s, %v", test.pod.Name, err)
705+
}
706+
err = PodValidateLimitFunc(&test.limitRange, &test.pod)
707+
if err == nil {
708+
t.Errorf("Expected error for pod: %s", test.pod.Name)
709+
}
710+
})
656711
}
657712
}
658713

0 commit comments

Comments
 (0)