From 15867d9e8a1faf007f6df563c26a9b5e8744b2a1 Mon Sep 17 00:00:00 2001 From: pacoxu Date: Tue, 22 Dec 2020 19:19:15 +0800 Subject: [PATCH] bugfix: check Spec.AllocateLoadBalancerNodePorts for nodeport and skip zero usage in delta evaluator Signed-off-by: pacoxu When Spec.AllocateLoadBalancerNodePorts is "false" NodePort shall not be included when computing quota for type:LoadBalancer. Co-authored-by: uablrek --- pkg/quota/v1/evaluator/core/BUILD | 4 + pkg/quota/v1/evaluator/core/services.go | 27 +++- pkg/quota/v1/evaluator/core/services_test.go | 142 ++++++++++++++++-- .../plugin/resourcequota/controller.go | 5 +- .../apiserver/pkg/quota/v1/resources.go | 11 ++ .../apiserver/pkg/quota/v1/resources_test.go | 44 ++++++ test/e2e/apimachinery/resource_quota.go | 24 ++- 7 files changed, 240 insertions(+), 17 deletions(-) diff --git a/pkg/quota/v1/evaluator/core/BUILD b/pkg/quota/v1/evaluator/core/BUILD index 63d948d8308..e5f02d9a77d 100644 --- a/pkg/quota/v1/evaluator/core/BUILD +++ b/pkg/quota/v1/evaluator/core/BUILD @@ -46,6 +46,7 @@ go_test( embed = [":go_default_library"], deps = [ "//pkg/apis/core:go_default_library", + "//pkg/features:go_default_library", "//pkg/util/node:go_default_library", "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", @@ -54,6 +55,9 @@ go_test( "//staging/src/k8s.io/apimachinery/pkg/util/clock:go_default_library", "//staging/src/k8s.io/apiserver/pkg/quota/v1:go_default_library", "//staging/src/k8s.io/apiserver/pkg/quota/v1/generic:go_default_library", + "//staging/src/k8s.io/apiserver/pkg/util/feature:go_default_library", + "//staging/src/k8s.io/component-base/featuregate/testing:go_default_library", + "//vendor/k8s.io/utils/pointer:go_default_library", ], ) diff --git a/pkg/quota/v1/evaluator/core/services.go b/pkg/quota/v1/evaluator/core/services.go index d90afe0b4f8..ab5c8a9f48e 100644 --- a/pkg/quota/v1/evaluator/core/services.go +++ b/pkg/quota/v1/evaluator/core/services.go @@ -26,8 +26,10 @@ import ( "k8s.io/apiserver/pkg/admission" quota "k8s.io/apiserver/pkg/quota/v1" "k8s.io/apiserver/pkg/quota/v1/generic" + "k8s.io/apiserver/pkg/util/feature" api "k8s.io/kubernetes/pkg/apis/core" k8s_api_v1 "k8s.io/kubernetes/pkg/apis/core/v1" + "k8s.io/kubernetes/pkg/features" ) // the name used for object count quota @@ -128,14 +130,33 @@ func (p *serviceEvaluator) Usage(item runtime.Object) (corev1.ResourceList, erro value := resource.NewQuantity(int64(ports), resource.DecimalSI) result[corev1.ResourceServicesNodePorts] = *value case corev1.ServiceTypeLoadBalancer: - // load balancer services need to count node ports and load balancers - value := resource.NewQuantity(int64(ports), resource.DecimalSI) - result[corev1.ResourceServicesNodePorts] = *value + // load balancer services need to count node ports. If creation of node ports + // is suppressed only ports with explicit NodePort values are counted. + // nodeports won't be allocated yet, so we can't simply count the actual values. + // We need to look at the intent. + if feature.DefaultFeatureGate.Enabled(features.ServiceLBNodePortControl) && + svc.Spec.AllocateLoadBalancerNodePorts != nil && + *svc.Spec.AllocateLoadBalancerNodePorts == false { + result[corev1.ResourceServicesNodePorts] = *portsWithNodePorts(svc) + } else { + value := resource.NewQuantity(int64(ports), resource.DecimalSI) + result[corev1.ResourceServicesNodePorts] = *value + } result[corev1.ResourceServicesLoadBalancers] = *(resource.NewQuantity(1, resource.DecimalSI)) } return result, nil } +func portsWithNodePorts(svc *corev1.Service) *resource.Quantity { + count := 0 + for _, p := range svc.Spec.Ports { + if p.NodePort != 0 { + count++ + } + } + return resource.NewQuantity(int64(count), resource.DecimalSI) +} + // UsageStats calculates aggregate usage for the object. func (p *serviceEvaluator) UsageStats(options quota.UsageStatsOptions) (quota.UsageStats, error) { return generic.CalculateUsageStats(options, p.listFuncByNamespace, generic.MatchesNoScopeFunc, p.Usage) diff --git a/pkg/quota/v1/evaluator/core/services_test.go b/pkg/quota/v1/evaluator/core/services_test.go index b275f23a8a2..fa8f6246411 100644 --- a/pkg/quota/v1/evaluator/core/services_test.go +++ b/pkg/quota/v1/evaluator/core/services_test.go @@ -24,7 +24,11 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" quota "k8s.io/apiserver/pkg/quota/v1" "k8s.io/apiserver/pkg/quota/v1/generic" + "k8s.io/apiserver/pkg/util/feature" + featuregatetesting "k8s.io/component-base/featuregate/testing" api "k8s.io/kubernetes/pkg/apis/core" + "k8s.io/kubernetes/pkg/features" + utilpointer "k8s.io/utils/pointer" ) func TestServiceEvaluatorMatchesResources(t *testing.T) { @@ -52,8 +56,9 @@ func TestServiceEvaluatorMatchesResources(t *testing.T) { func TestServiceEvaluatorUsage(t *testing.T) { evaluator := NewServiceEvaluator(nil) testCases := map[string]struct { - service *api.Service - usage corev1.ResourceList + service *api.Service + usage corev1.ResourceList + serviceLBNodePortControlEnabled bool }{ "loadbalancer": { service: &api.Service{ @@ -86,6 +91,27 @@ func TestServiceEvaluatorUsage(t *testing.T) { generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"), }, }, + "loadbalancer_2_ports": { + service: &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeLoadBalancer, + Ports: []api.ServicePort{ + { + Port: 27443, + }, + { + Port: 27444, + }, + }, + }, + }, + usage: corev1.ResourceList{ + corev1.ResourceServicesNodePorts: resource.MustParse("2"), + corev1.ResourceServicesLoadBalancers: resource.MustParse("1"), + corev1.ResourceServices: resource.MustParse("1"), + generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"), + }, + }, "clusterip": { service: &api.Service{ Spec: api.ServiceSpec{ @@ -138,15 +164,113 @@ func TestServiceEvaluatorUsage(t *testing.T) { generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"), }, }, + "nodeports-disabled": { + service: &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeLoadBalancer, + Ports: []api.ServicePort{ + { + Port: 27443, + }, + { + Port: 27444, + }, + }, + AllocateLoadBalancerNodePorts: utilpointer.BoolPtr(false), + }, + }, + usage: corev1.ResourceList{ + corev1.ResourceServices: resource.MustParse("1"), + corev1.ResourceServicesNodePorts: resource.MustParse("0"), + corev1.ResourceServicesLoadBalancers: resource.MustParse("1"), + generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"), + }, + serviceLBNodePortControlEnabled: true, + }, + "nodeports-default-enabled": { + service: &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeLoadBalancer, + Ports: []api.ServicePort{ + { + Port: 27443, + NodePort: 32001, + }, + { + Port: 27444, + NodePort: 32002, + }, + }, + AllocateLoadBalancerNodePorts: nil, + }, + }, + usage: corev1.ResourceList{ + corev1.ResourceServices: resource.MustParse("1"), + corev1.ResourceServicesNodePorts: resource.MustParse("2"), + corev1.ResourceServicesLoadBalancers: resource.MustParse("1"), + generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"), + }, + }, + "nodeports-explicitly-enabled": { + service: &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeLoadBalancer, + Ports: []api.ServicePort{ + { + Port: 27443, + }, + { + Port: 27444, + }, + }, + AllocateLoadBalancerNodePorts: utilpointer.BoolPtr(true), + }, + }, + usage: corev1.ResourceList{ + corev1.ResourceServices: resource.MustParse("1"), + corev1.ResourceServicesNodePorts: resource.MustParse("2"), + corev1.ResourceServicesLoadBalancers: resource.MustParse("1"), + generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"), + }, + serviceLBNodePortControlEnabled: true, + }, + "nodeports-disabled-but-specified": { + service: &api.Service{ + Spec: api.ServiceSpec{ + Type: api.ServiceTypeLoadBalancer, + Ports: []api.ServicePort{ + { + Port: 27443, + NodePort: 32001, + }, + { + Port: 27444, + NodePort: 32002, + }, + }, + AllocateLoadBalancerNodePorts: utilpointer.BoolPtr(false), + }, + }, + usage: corev1.ResourceList{ + corev1.ResourceServices: resource.MustParse("1"), + corev1.ResourceServicesNodePorts: resource.MustParse("2"), + corev1.ResourceServicesLoadBalancers: resource.MustParse("1"), + generic.ObjectCountQuotaResourceNameFor(schema.GroupResource{Resource: "services"}): resource.MustParse("1"), + }, + serviceLBNodePortControlEnabled: true, + }, } for testName, testCase := range testCases { - actual, err := evaluator.Usage(testCase.service) - if err != nil { - t.Errorf("%s unexpected error: %v", testName, err) - } - if !quota.Equals(testCase.usage, actual) { - t.Errorf("%s expected: %v, actual: %v", testName, testCase.usage, actual) - } + t.Run(testName, func(t *testing.T) { + defer featuregatetesting.SetFeatureGateDuringTest(t, feature.DefaultFeatureGate, features.ServiceLBNodePortControl, testCase.serviceLBNodePortControlEnabled)() + actual, err := evaluator.Usage(testCase.service) + if err != nil { + t.Errorf("%s unexpected error: %v", testName, err) + } + if !quota.Equals(testCase.usage, actual) { + t.Errorf("%s expected: %v, actual: %v", testName, testCase.usage, actual) + } + }) } } diff --git a/staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/controller.go b/staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/controller.go index 622c7bcfd50..51d3123eab3 100644 --- a/staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/controller.go +++ b/staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/controller.go @@ -510,7 +510,10 @@ func CheckRequest(quotas []corev1.ResourceQuota, a admission.Attributes, evaluat } } - if quota.IsZero(deltaUsage) { + // ignore items in deltaUsage with zero usage + deltaUsage = quota.RemoveZeros(deltaUsage) + // if there is no remaining non-zero usage, short-circuit and return + if len(deltaUsage) == 0 { return quotas, nil } diff --git a/staging/src/k8s.io/apiserver/pkg/quota/v1/resources.go b/staging/src/k8s.io/apiserver/pkg/quota/v1/resources.go index 3c2927d738b..b6647192099 100644 --- a/staging/src/k8s.io/apiserver/pkg/quota/v1/resources.go +++ b/staging/src/k8s.io/apiserver/pkg/quota/v1/resources.go @@ -226,6 +226,17 @@ func IsZero(a corev1.ResourceList) bool { return true } +// RemoveZeros returns a new resource list that only has no zero values +func RemoveZeros(a corev1.ResourceList) corev1.ResourceList { + result := corev1.ResourceList{} + for key, value := range a { + if !value.IsZero() { + result[key] = value + } + } + return result +} + // IsNegative returns the set of resource names that have a negative value. func IsNegative(a corev1.ResourceList) []corev1.ResourceName { results := []corev1.ResourceName{} diff --git a/staging/src/k8s.io/apiserver/pkg/quota/v1/resources_test.go b/staging/src/k8s.io/apiserver/pkg/quota/v1/resources_test.go index b8c85a1574e..ae8d21320c7 100644 --- a/staging/src/k8s.io/apiserver/pkg/quota/v1/resources_test.go +++ b/staging/src/k8s.io/apiserver/pkg/quota/v1/resources_test.go @@ -287,6 +287,50 @@ func TestIsZero(t *testing.T) { } } +func TestRemoveZeros(t *testing.T) { + testCases := map[string]struct { + a corev1.ResourceList + expected corev1.ResourceList + }{ + "empty": { + a: corev1.ResourceList{}, + expected: corev1.ResourceList{}, + }, + "all-zeros": { + a: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("0"), + corev1.ResourceMemory: resource.MustParse("0"), + }, + expected: corev1.ResourceList{}, + }, + "some-zeros": { + a: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("0"), + corev1.ResourceMemory: resource.MustParse("0"), + corev1.ResourceStorage: resource.MustParse("100Gi"), + }, + expected: corev1.ResourceList{ + corev1.ResourceStorage: resource.MustParse("100Gi"), + }, + }, + "non-zero": { + a: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("200m"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + expected: corev1.ResourceList{ + corev1.ResourceCPU: resource.MustParse("200m"), + corev1.ResourceMemory: resource.MustParse("1Gi"), + }, + }, + } + for testName, testCase := range testCases { + if result := RemoveZeros(testCase.a); !Equals(result, testCase.expected) { + t.Errorf("%s expected: %v, actual: %v", testName, testCase.expected, result) + } + } +} + func TestIsNegative(t *testing.T) { testCases := map[string]struct { a corev1.ResourceList diff --git a/test/e2e/apimachinery/resource_quota.go b/test/e2e/apimachinery/resource_quota.go index 3e0571c9e69..83c3cf2e77a 100644 --- a/test/e2e/apimachinery/resource_quota.go +++ b/test/e2e/apimachinery/resource_quota.go @@ -36,6 +36,7 @@ import ( "k8s.io/kubernetes/test/e2e/framework" "k8s.io/kubernetes/test/utils/crd" imageutils "k8s.io/kubernetes/test/utils/image" + "k8s.io/utils/pointer" "github.com/onsi/ginkgo" ) @@ -100,23 +101,37 @@ var _ = SIGDescribe("ResourceQuota", func() { framework.ExpectNoError(err) ginkgo.By("Creating a Service") - service := newTestServiceForQuota("test-service", v1.ServiceTypeClusterIP) + service := newTestServiceForQuota("test-service", v1.ServiceTypeClusterIP, false) service, err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Create(context.TODO(), service, metav1.CreateOptions{}) framework.ExpectNoError(err) + ginkgo.By("Creating a NodePort Service") + nodeport := newTestServiceForQuota("test-service-np", v1.ServiceTypeNodePort, false) + nodeport, err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Create(context.TODO(), nodeport, metav1.CreateOptions{}) + framework.ExpectNoError(err) + + ginkgo.By("Not allowing a LoadBalancer Service with NodePort to be created that exceeds remaining quota") + loadbalancer := newTestServiceForQuota("test-service-lb", v1.ServiceTypeLoadBalancer, false) + _, err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Create(context.TODO(), loadbalancer, metav1.CreateOptions{}) + framework.ExpectError(err) + ginkgo.By("Ensuring resource quota status captures service creation") usedResources = v1.ResourceList{} usedResources[v1.ResourceQuotas] = resource.MustParse(strconv.Itoa(c + 1)) - usedResources[v1.ResourceServices] = resource.MustParse("1") + usedResources[v1.ResourceServices] = resource.MustParse("2") + usedResources[v1.ResourceServicesNodePorts] = resource.MustParse("1") err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) framework.ExpectNoError(err) - ginkgo.By("Deleting a Service") + ginkgo.By("Deleting Services") err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Delete(context.TODO(), service.Name, metav1.DeleteOptions{}) framework.ExpectNoError(err) + err = f.ClientSet.CoreV1().Services(f.Namespace.Name).Delete(context.TODO(), nodeport.Name, metav1.DeleteOptions{}) + framework.ExpectNoError(err) ginkgo.By("Ensuring resource quota status released usage") usedResources[v1.ResourceServices] = resource.MustParse("0") + usedResources[v1.ResourceServicesNodePorts] = resource.MustParse("0") err = waitForResourceQuota(f.ClientSet, f.Namespace.Name, quotaName, usedResources) framework.ExpectNoError(err) }) @@ -1625,7 +1640,7 @@ func newTestReplicaSetForQuota(name, image string, replicas int32) *appsv1.Repli } // newTestServiceForQuota returns a simple service -func newTestServiceForQuota(name string, serviceType v1.ServiceType) *v1.Service { +func newTestServiceForQuota(name string, serviceType v1.ServiceType, allocateLoadBalancerNodePorts bool) *v1.Service { return &v1.Service{ ObjectMeta: metav1.ObjectMeta{ Name: name, @@ -1636,6 +1651,7 @@ func newTestServiceForQuota(name string, serviceType v1.ServiceType) *v1.Service Port: 80, TargetPort: intstr.FromInt(80), }}, + AllocateLoadBalancerNodePorts: pointer.BoolPtr(allocateLoadBalancerNodePorts), }, } }