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.

Sign up for GitHub

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

Jump to bottom

bugfix: nodeport quota check failure result into failing to create a clusterip service #97451

Merged
merged 1 commit into from Jan 7, 2021
Merged

bugfix: nodeport quota check failure result into failing to create a clusterip service #97451

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions pkg/quota/v1/evaluator/core/BUILD
View file
Open in desktop
Expand Up @@ -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",
Expand All @@ -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",
],
)

Expand Down
27 changes: 24 additions & 3 deletions pkg/quota/v1/evaluator/core/services.go
View file
Open in desktop
Expand Up @@ -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
Expand Down Expand Up @@ -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:
liggitt marked this conversation as resolved.
Show resolved Hide resolved
// 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)
Expand Down
142 changes: 133 additions & 9 deletions pkg/quota/v1/evaluator/core/services_test.go
View file
Open in desktop
Expand Up @@ -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) {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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": {
pacoxu marked this conversation as resolved.
Show resolved Hide resolved
service: &api.Service{
Spec: api.ServiceSpec{
Expand Down Expand Up @@ -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)
}
})
}
}

Expand Down
5 changes: 4 additions & 1 deletion staging/src/k8s.io/apiserver/pkg/admission/plugin/resourcequota/controller.go
View file
Open in desktop
Expand Up @@ -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
}

Expand Down
11 changes: 11 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/quota/v1/resources.go
View file
Open in desktop
Expand Up @@ -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{}
Expand Down
44 changes: 44 additions & 0 deletions staging/src/k8s.io/apiserver/pkg/quota/v1/resources_test.go
View file
Open in desktop
Expand Up @@ -287,6 +287,50 @@ func TestIsZero(t *testing.T) {
}
}

func TestRemoveZeros(t *testing.T) {
pacoxu marked this conversation as resolved.
Show resolved Hide resolved
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
Expand Down
24 changes: 20 additions & 4 deletions test/e2e/apimachinery/resource_quota.go
View file
Open in desktop
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please add the test case for the scenario described in the PR description:

"when nodeport quota usage exceeds the quota limit, cluster ip service should still be allowed if cluster ip service quota still has some left."

ginkgo.By("Ensuring resource quota status captures service creation")
usedResources = v1.ResourceList{}
usedResources[v1.ResourceQuotas] = resource.MustParse(strconv.Itoa(c + 1))
pacoxu marked this conversation as resolved.
Show resolved Hide resolved
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)
})
Expand Down Expand Up @@ -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,
Expand All @@ -1636,6 +1651,7 @@ func newTestServiceForQuota(name string, serviceType v1.ServiceType) *v1.Service
Port: 80,
TargetPort: intstr.FromInt(80),
}},
AllocateLoadBalancerNodePorts: pointer.BoolPtr(allocateLoadBalancerNodePorts),
},
}
}
Expand Down