From 75a7887c18bc8348d24459ace52ac59cd90053e2 Mon Sep 17 00:00:00 2001 From: Wei Huang Date: Sun, 30 Apr 2023 15:30:12 -0700 Subject: [PATCH 1/2] Fix incorrect calculation for ResourceQuota with PriorityClass as its scope --- pkg/quota/v1/evaluator/core/pods.go | 4 + pkg/quota/v1/evaluator/core/pods_test.go | 228 +++++++++++++++++++++++ 2 files changed, 232 insertions(+) diff --git a/pkg/quota/v1/evaluator/core/pods.go b/pkg/quota/v1/evaluator/core/pods.go index f85fbde45d33..37b83234e64b 100644 --- a/pkg/quota/v1/evaluator/core/pods.go +++ b/pkg/quota/v1/evaluator/core/pods.go @@ -321,6 +321,10 @@ func podMatchesScopeFunc(selector corev1.ScopedResourceSelectorRequirement, obje case corev1.ResourceQuotaScopeNotBestEffort: return !isBestEffort(pod), nil case corev1.ResourceQuotaScopePriorityClass: + if len(selector.Operator) == 0 && selector.Values == nil { + // this is just checking for existence of a priorityClass on the pod + return len(pod.Spec.PriorityClassName) != 0, nil + } return podMatchesSelector(pod, selector) case corev1.ResourceQuotaScopeCrossNamespacePodAffinity: return usesCrossNamespacePodAffinity(pod), nil diff --git a/pkg/quota/v1/evaluator/core/pods_test.go b/pkg/quota/v1/evaluator/core/pods_test.go index d74f7392adb6..d5dd36f35bf8 100644 --- a/pkg/quota/v1/evaluator/core/pods_test.go +++ b/pkg/quota/v1/evaluator/core/pods_test.go @@ -17,6 +17,7 @@ limitations under the License. package core import ( + "fmt" "testing" "time" @@ -24,9 +25,11 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" quota "k8s.io/apiserver/pkg/quota/v1" "k8s.io/apiserver/pkg/quota/v1/generic" + "k8s.io/client-go/tools/cache" api "k8s.io/kubernetes/pkg/apis/core" "k8s.io/kubernetes/pkg/util/node" "k8s.io/utils/clock" @@ -537,6 +540,146 @@ func TestPodEvaluatorUsage(t *testing.T) { } } +func TestPodEvaluatorUsageStats(t *testing.T) { + cpu1 := api.ResourceList{api.ResourceCPU: resource.MustParse("1")} + tests := []struct { + name string + objs []runtime.Object + quotaScopes []corev1.ResourceQuotaScope + quotaScopeSelector *corev1.ScopeSelector + want corev1.ResourceList + }{ + { + name: "nil case", + }, + { + name: "all pods in running state", + objs: []runtime.Object{ + makePod("p1", "", cpu1, api.PodRunning), + makePod("p2", "", cpu1, api.PodRunning), + }, + want: corev1.ResourceList{ + corev1.ResourcePods: resource.MustParse("2"), + corev1.ResourceName("count/pods"): resource.MustParse("2"), + corev1.ResourceCPU: resource.MustParse("2"), + corev1.ResourceRequestsCPU: resource.MustParse("2"), + corev1.ResourceLimitsCPU: resource.MustParse("2"), + }, + }, + { + name: "one pods in terminal state", + objs: []runtime.Object{ + makePod("p1", "", cpu1, api.PodRunning), + makePod("p2", "", cpu1, api.PodSucceeded), + }, + want: corev1.ResourceList{ + corev1.ResourcePods: resource.MustParse("1"), + corev1.ResourceName("count/pods"): resource.MustParse("2"), + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceRequestsCPU: resource.MustParse("1"), + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }, + }, + { + name: "partial pods matching quotaScopeSelector", + objs: []runtime.Object{ + makePod("p1", "high-priority", cpu1, api.PodRunning), + makePod("p2", "high-priority", cpu1, api.PodSucceeded), + makePod("p3", "low-priority", cpu1, api.PodRunning), + }, + quotaScopeSelector: &corev1.ScopeSelector{ + MatchExpressions: []corev1.ScopedResourceSelectorRequirement{ + { + ScopeName: corev1.ResourceQuotaScopePriorityClass, + Operator: corev1.ScopeSelectorOpIn, + Values: []string{"high-priority"}, + }, + }, + }, + want: corev1.ResourceList{ + corev1.ResourcePods: resource.MustParse("1"), + corev1.ResourceName("count/pods"): resource.MustParse("2"), + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceRequestsCPU: resource.MustParse("1"), + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }, + }, + { + name: "partial pods matching quotaScopeSelector - w/ scopeName specified", + objs: []runtime.Object{ + makePod("p1", "high-priority", cpu1, api.PodRunning), + makePod("p2", "high-priority", cpu1, api.PodSucceeded), + makePod("p3", "low-priority", cpu1, api.PodRunning), + }, + quotaScopes: []corev1.ResourceQuotaScope{ + corev1.ResourceQuotaScopePriorityClass, + }, + quotaScopeSelector: &corev1.ScopeSelector{ + MatchExpressions: []corev1.ScopedResourceSelectorRequirement{ + { + ScopeName: corev1.ResourceQuotaScopePriorityClass, + Operator: corev1.ScopeSelectorOpIn, + Values: []string{"high-priority"}, + }, + }, + }, + want: corev1.ResourceList{ + corev1.ResourcePods: resource.MustParse("1"), + corev1.ResourceName("count/pods"): resource.MustParse("2"), + corev1.ResourceCPU: resource.MustParse("1"), + corev1.ResourceRequestsCPU: resource.MustParse("1"), + corev1.ResourceLimitsCPU: resource.MustParse("1"), + }, + }, + { + name: "partial pods matching quotaScopeSelector - w/ multiple scopeNames specified", + objs: []runtime.Object{ + makePod("p1", "high-priority", cpu1, api.PodRunning), + makePod("p2", "high-priority", cpu1, api.PodSucceeded), + makePod("p3", "low-priority", cpu1, api.PodRunning), + makePod("p4", "high-priority", nil, api.PodFailed), + }, + quotaScopes: []corev1.ResourceQuotaScope{ + corev1.ResourceQuotaScopePriorityClass, + corev1.ResourceQuotaScopeBestEffort, + }, + quotaScopeSelector: &corev1.ScopeSelector{ + MatchExpressions: []corev1.ScopedResourceSelectorRequirement{ + { + ScopeName: corev1.ResourceQuotaScopePriorityClass, + Operator: corev1.ScopeSelectorOpIn, + Values: []string{"high-priority"}, + }, + }, + }, + want: corev1.ResourceList{ + corev1.ResourceName("count/pods"): resource.MustParse("1"), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + gvr := corev1.SchemeGroupVersion.WithResource("pods") + listerForPod := map[schema.GroupVersionResource]cache.GenericLister{ + gvr: newGenericLister(gvr.GroupResource(), tt.objs), + } + evaluator := NewPodEvaluator(mockListerForResourceFunc(listerForPod), testingclock.NewFakeClock(time.Now())) + usageStatsOption := quota.UsageStatsOptions{ + Scopes: tt.quotaScopes, + ScopeSelector: tt.quotaScopeSelector, + } + actual, err := evaluator.UsageStats(usageStatsOption) + if err != nil { + t.Error(err) + } + if !quota.Equals(tt.want, actual.Used) { + t.Errorf("expected: %v, actual: %v", tt.want, actual.Used) + } + }) + } +} + func TestPodEvaluatorMatchingScopes(t *testing.T) { fakeClock := testingclock.NewFakeClock(time.Now()) evaluator := NewPodEvaluator(nil, fakeClock) @@ -750,3 +893,88 @@ func TestPodEvaluatorMatchingScopes(t *testing.T) { }) } } + +func BenchmarkPodMatchesScopeFunc(b *testing.B) { + pod, _ := toExternalPodOrError(makePod("p1", "high-priority", + api.ResourceList{api.ResourceCPU: resource.MustParse("1")}, api.PodRunning)) + + tests := []struct { + name string + selector corev1.ScopedResourceSelectorRequirement + }{ + { + name: "PriorityClass selector w/o operator", + selector: corev1.ScopedResourceSelectorRequirement{ + ScopeName: corev1.ResourceQuotaScopePriorityClass, + }, + }, + { + name: "PriorityClass selector w/ 'Exists' operator", + selector: corev1.ScopedResourceSelectorRequirement{ + ScopeName: corev1.ResourceQuotaScopePriorityClass, + Operator: corev1.ScopeSelectorOpExists, + }, + }, + { + name: "BestEfforts selector w/o operator", + selector: corev1.ScopedResourceSelectorRequirement{ + ScopeName: corev1.ResourceQuotaScopeBestEffort, + }, + }, + { + name: "BestEfforts selector w/ 'Exists' operator", + selector: corev1.ScopedResourceSelectorRequirement{ + ScopeName: corev1.ResourceQuotaScopeBestEffort, + Operator: corev1.ScopeSelectorOpExists, + }, + }, + } + + for _, tt := range tests { + b.Run(tt.name, func(b *testing.B) { + for n := 0; n < b.N; n++ { + _, _ = podMatchesScopeFunc(tt.selector, pod) + } + }) + } +} + +func mockListerForResourceFunc(listerForResource map[schema.GroupVersionResource]cache.GenericLister) quota.ListerForResourceFunc { + return func(gvr schema.GroupVersionResource) (cache.GenericLister, error) { + lister, found := listerForResource[gvr] + if !found { + return nil, fmt.Errorf("no lister found for resource %v", gvr) + } + return lister, nil + } +} + +func newGenericLister(groupResource schema.GroupResource, items []runtime.Object) cache.GenericLister { + store := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{"namespace": cache.MetaNamespaceIndexFunc}) + for _, item := range items { + store.Add(item) + } + return cache.NewGenericLister(store, groupResource) +} + +func makePod(name, pcName string, resList api.ResourceList, phase api.PodPhase) *api.Pod { + return &api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Spec: api.PodSpec{ + PriorityClassName: pcName, + Containers: []api.Container{ + { + Resources: api.ResourceRequirements{ + Requests: resList, + Limits: resList, + }, + }, + }, + }, + Status: api.PodStatus{ + Phase: phase, + }, + } +} From b6ec911f35ac3a67f546c1e1c846dbe2ec302f5b Mon Sep 17 00:00:00 2001 From: Wei Huang Date: Thu, 4 May 2023 16:55:32 -0700 Subject: [PATCH 2/2] benchmark test to evaluate the overhead of podMatchesScopeFunc --- pkg/quota/v1/evaluator/core/pods.go | 5 +++-- .../src/k8s.io/apiserver/pkg/quota/v1/generic/evaluator.go | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/quota/v1/evaluator/core/pods.go b/pkg/quota/v1/evaluator/core/pods.go index 37b83234e64b..d7ee78280e6b 100644 --- a/pkg/quota/v1/evaluator/core/pods.go +++ b/pkg/quota/v1/evaluator/core/pods.go @@ -321,8 +321,9 @@ func podMatchesScopeFunc(selector corev1.ScopedResourceSelectorRequirement, obje case corev1.ResourceQuotaScopeNotBestEffort: return !isBestEffort(pod), nil case corev1.ResourceQuotaScopePriorityClass: - if len(selector.Operator) == 0 && selector.Values == nil { - // this is just checking for existence of a priorityClass on the pod + if selector.Operator == corev1.ScopeSelectorOpExists { + // This is just checking for existence of a priorityClass on the pod, + // no need to take the overhead of selector parsing/evaluation. return len(pod.Spec.PriorityClassName) != 0, nil } return podMatchesSelector(pod, selector) diff --git a/staging/src/k8s.io/apiserver/pkg/quota/v1/generic/evaluator.go b/staging/src/k8s.io/apiserver/pkg/quota/v1/generic/evaluator.go index 55b31a745a05..e122248f861c 100644 --- a/staging/src/k8s.io/apiserver/pkg/quota/v1/generic/evaluator.go +++ b/staging/src/k8s.io/apiserver/pkg/quota/v1/generic/evaluator.go @@ -199,7 +199,7 @@ func CalculateUsageStats(options quota.UsageStatsOptions, // need to verify that the item matches the set of scopes matchesScopes := true for _, scope := range options.Scopes { - innerMatch, err := scopeFunc(corev1.ScopedResourceSelectorRequirement{ScopeName: scope}, item) + innerMatch, err := scopeFunc(corev1.ScopedResourceSelectorRequirement{ScopeName: scope, Operator: corev1.ScopeSelectorOpExists}, item) if err != nil { return result, nil }