Merge pull request #30330 from derekwaynecarr/fix-quota-constraints-funcs

Automatic merge from submit-queue

Fix quota constraints funcs for pvc and service objects

Fixes https://github.com/kubernetes/kubernetes/issues/30315

pkg/quota/evaluator/core/persistent_volume_claims.go

@@ -17,6 +17,9 @@ limitations under the License.
 package core
 
 import (
+	"fmt"
+	"strings"
+
 	"k8s.io/kubernetes/pkg/admission"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/resource"
@@ -24,6 +27,7 @@ import (
 	"k8s.io/kubernetes/pkg/quota"
 	"k8s.io/kubernetes/pkg/quota/generic"
 	"k8s.io/kubernetes/pkg/runtime"
+	"k8s.io/kubernetes/pkg/util/sets"
 )
 
 // NewPersistentVolumeClaimEvaluator returns an evaluator that can evaluate persistent volume claims
@@ -37,7 +41,7 @@ func NewPersistentVolumeClaimEvaluator(kubeClient clientset.Interface) quota.Eva
 		},
 		MatchedResourceNames: allResources,
 		MatchesScopeFunc:     generic.MatchesNoScopeFunc,
-		ConstraintsFunc:      generic.ObjectCountConstraintsFunc(api.ResourcePersistentVolumeClaims),
+		ConstraintsFunc:      PersistentVolumeClaimConstraintsFunc,
 		UsageFunc:            PersistentVolumeClaimUsageFunc,
 		ListFuncByNamespace: func(namespace string, options api.ListOptions) (runtime.Object, error) {
 			return kubeClient.Core().PersistentVolumeClaims(namespace).List(options)
@@ -58,3 +62,24 @@ func PersistentVolumeClaimUsageFunc(object runtime.Object) api.ResourceList {
 	}
 	return result
 }
+
+// PersistentVolumeClaimConstraintsFunc verifies that all required resources are present on the claim
+// In addition, it validates that the resources are valid (i.e. requests < limits)
+func PersistentVolumeClaimConstraintsFunc(required []api.ResourceName, object runtime.Object) error {
+	pvc, ok := object.(*api.PersistentVolumeClaim)
+	if !ok {
+		return fmt.Errorf("unexpected input object %v", object)
+	}
+
+	requiredSet := quota.ToSet(required)
+	missingSet := sets.NewString()
+	pvcUsage := PersistentVolumeClaimUsageFunc(pvc)
+	pvcSet := quota.ToSet(quota.ResourceNames(pvcUsage))
+	if diff := requiredSet.Difference(pvcSet); len(diff) > 0 {
+		missingSet.Insert(diff.List()...)
+	}
+	if len(missingSet) == 0 {
+		return nil
+	}
+	return fmt.Errorf("must specify %s", strings.Join(missingSet.List(), ","))
+}

pkg/quota/evaluator/core/persistent_volume_claims_test.go

@@ -0,0 +1,149 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package core
+
+import (
+	"testing"
+
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/api/resource"
+	"k8s.io/kubernetes/pkg/api/unversioned"
+	"k8s.io/kubernetes/pkg/client/clientset_generated/internalclientset/fake"
+	"k8s.io/kubernetes/pkg/quota"
+)
+
+func testVolumeClaim(name string, namespace string, spec api.PersistentVolumeClaimSpec) *api.PersistentVolumeClaim {
+	return &api.PersistentVolumeClaim{
+		ObjectMeta: api.ObjectMeta{Name: name, Namespace: namespace},
+		Spec:       spec,
+	}
+}
+
+func TestPersistentVolumeClaimsConstraintsFunc(t *testing.T) {
+	validClaim := testVolumeClaim("foo", "ns", api.PersistentVolumeClaimSpec{
+		Selector: &unversioned.LabelSelector{
+			MatchExpressions: []unversioned.LabelSelectorRequirement{
+				{
+					Key:      "key2",
+					Operator: "Exists",
+				},
+			},
+		},
+		AccessModes: []api.PersistentVolumeAccessMode{
+			api.ReadWriteOnce,
+			api.ReadOnlyMany,
+		},
+		Resources: api.ResourceRequirements{
+			Requests: api.ResourceList{
+				api.ResourceName(api.ResourceStorage): resource.MustParse("10G"),
+			},
+		},
+	})
+	missingStorage := testVolumeClaim("foo", "ns", api.PersistentVolumeClaimSpec{
+		Selector: &unversioned.LabelSelector{
+			MatchExpressions: []unversioned.LabelSelectorRequirement{
+				{
+					Key:      "key2",
+					Operator: "Exists",
+				},
+			},
+		},
+		AccessModes: []api.PersistentVolumeAccessMode{
+			api.ReadWriteOnce,
+			api.ReadOnlyMany,
+		},
+		Resources: api.ResourceRequirements{
+			Requests: api.ResourceList{},
+		},
+	})
+
+	testCases := map[string]struct {
+		pvc      *api.PersistentVolumeClaim
+		required []api.ResourceName
+		err      string
+	}{
+		"missing storage": {
+			pvc:      missingStorage,
+			required: []api.ResourceName{api.ResourceRequestsStorage},
+			err:      `must specify requests.storage`,
+		},
+		"valid-claim-quota-storage": {
+			pvc:      validClaim,
+			required: []api.ResourceName{api.ResourceRequestsStorage},
+		},
+		"valid-claim-quota-pvc": {
+			pvc:      validClaim,
+			required: []api.ResourceName{api.ResourcePersistentVolumeClaims},
+		},
+		"valid-claim-quota-storage-and-pvc": {
+			pvc:      validClaim,
+			required: []api.ResourceName{api.ResourceRequestsStorage, api.ResourcePersistentVolumeClaims},
+		},
+	}
+	for testName, test := range testCases {
+		err := PersistentVolumeClaimConstraintsFunc(test.required, test.pvc)
+		switch {
+		case err != nil && len(test.err) == 0,
+			err == nil && len(test.err) != 0,
+			err != nil && test.err != err.Error():
+			t.Errorf("%s unexpected error: %v", testName, err)
+		}
+	}
+}
+
+func TestPersistentVolumeClaimEvaluatorUsage(t *testing.T) {
+	validClaim := testVolumeClaim("foo", "ns", api.PersistentVolumeClaimSpec{
+		Selector: &unversioned.LabelSelector{
+			MatchExpressions: []unversioned.LabelSelectorRequirement{
+				{
+					Key:      "key2",
+					Operator: "Exists",
+				},
+			},
+		},
+		AccessModes: []api.PersistentVolumeAccessMode{
+			api.ReadWriteOnce,
+			api.ReadOnlyMany,
+		},
+		Resources: api.ResourceRequirements{
+			Requests: api.ResourceList{
+				api.ResourceName(api.ResourceStorage): resource.MustParse("10Gi"),
+			},
+		},
+	})
+
+	kubeClient := fake.NewSimpleClientset()
+	evaluator := NewPersistentVolumeClaimEvaluator(kubeClient)
+	testCases := map[string]struct {
+		pvc   *api.PersistentVolumeClaim
+		usage api.ResourceList
+	}{
+		"pvc-usage": {
+			pvc: validClaim,
+			usage: api.ResourceList{
+				api.ResourceRequestsStorage:        resource.MustParse("10Gi"),
+				api.ResourcePersistentVolumeClaims: resource.MustParse("1"),
+			},
+		},
+	}
+	for testName, testCase := range testCases {
+		actual := evaluator.Usage(testCase.pvc)
+		if !quota.Equals(testCase.usage, actual) {
+			t.Errorf("%s expected: %v, actual: %v", testName, testCase.usage, actual)
+		}
+	}
+}

pkg/quota/evaluator/core/services.go

@@ -17,6 +17,9 @@ limitations under the License.
 package core
 
 import (
+	"fmt"
+	"strings"
+
 	"k8s.io/kubernetes/pkg/admission"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/resource"
@@ -24,6 +27,7 @@ import (
 	"k8s.io/kubernetes/pkg/quota"
 	"k8s.io/kubernetes/pkg/quota/generic"
 	"k8s.io/kubernetes/pkg/runtime"
+	"k8s.io/kubernetes/pkg/util/sets"
 )
 
 // NewServiceEvaluator returns an evaluator that can evaluate service quotas
@@ -42,7 +46,7 @@ func NewServiceEvaluator(kubeClient clientset.Interface) quota.Evaluator {
 		},
 		MatchedResourceNames: allResources,
 		MatchesScopeFunc:     generic.MatchesNoScopeFunc,
-		ConstraintsFunc:      generic.ObjectCountConstraintsFunc(api.ResourceServices),
+		ConstraintsFunc:      ServiceConstraintsFunc,
 		UsageFunc:            ServiceUsageFunc,
 		ListFuncByNamespace: func(namespace string, options api.ListOptions) (runtime.Object, error) {
 			return kubeClient.Core().Services(namespace).List(options)
@@ -54,12 +58,17 @@ func NewServiceEvaluator(kubeClient clientset.Interface) quota.Evaluator {
 func ServiceUsageFunc(object runtime.Object) api.ResourceList {
 	result := api.ResourceList{}
 	if service, ok := object.(*api.Service); ok {
+		// default service usage
 		result[api.ResourceServices] = resource.MustParse("1")
+		result[api.ResourceServicesLoadBalancers] = resource.MustParse("0")
+		result[api.ResourceServicesNodePorts] = resource.MustParse("0")
 		switch service.Spec.Type {
 		case api.ServiceTypeNodePort:
+			// node port services need to count node ports
 			value := resource.NewQuantity(int64(len(service.Spec.Ports)), resource.DecimalSI)
 			result[api.ResourceServicesNodePorts] = *value
 		case api.ServiceTypeLoadBalancer:
+			// load balancer services need to count load balancers
 			result[api.ResourceServicesLoadBalancers] = resource.MustParse("1")
 		}
 	}
@@ -85,3 +94,24 @@ func GetQuotaServiceType(service *api.Service) api.ServiceType {
 	}
 	return api.ServiceType("")
 }
+
+// ServiceConstraintsFunc verifies that all required resources are captured in service usage.
+func ServiceConstraintsFunc(required []api.ResourceName, object runtime.Object) error {
+	service, ok := object.(*api.Service)
+	if !ok {
+		return fmt.Errorf("unexpected input object %v", object)
+	}
+
+	requiredSet := quota.ToSet(required)
+	missingSet := sets.NewString()
+	serviceUsage := ServiceUsageFunc(service)
+	serviceSet := quota.ToSet(quota.ResourceNames(serviceUsage))
+	if diff := requiredSet.Difference(serviceSet); len(diff) > 0 {
+		missingSet.Insert(diff.List()...)
+	}
+
+	if len(missingSet) == 0 {
+		return nil
+	}
+	return fmt.Errorf("must specify %s", strings.Join(missingSet.List(), ","))
+}

pkg/quota/evaluator/core/services_test.go

@@ -53,6 +53,7 @@ func TestServiceEvaluatorUsage(t *testing.T) {
 				},
 			},
 			usage: api.ResourceList{
+				api.ResourceServicesNodePorts:     resource.MustParse("0"),
 				api.ResourceServicesLoadBalancers: resource.MustParse("1"),
 				api.ResourceServices:              resource.MustParse("1"),
 			},
@@ -64,7 +65,9 @@ func TestServiceEvaluatorUsage(t *testing.T) {
 				},
 			},
 			usage: api.ResourceList{
-				api.ResourceServices: resource.MustParse("1"),
+				api.ResourceServices:              resource.MustParse("1"),
+				api.ResourceServicesNodePorts:     resource.MustParse("0"),
+				api.ResourceServicesLoadBalancers: resource.MustParse("0"),
 			},
 		},
 		"nodeports": {
@@ -79,8 +82,9 @@ func TestServiceEvaluatorUsage(t *testing.T) {
 				},
 			},
 			usage: api.ResourceList{
-				api.ResourceServices:          resource.MustParse("1"),
-				api.ResourceServicesNodePorts: resource.MustParse("1"),
+				api.ResourceServices:              resource.MustParse("1"),
+				api.ResourceServicesNodePorts:     resource.MustParse("1"),
+				api.ResourceServicesLoadBalancers: resource.MustParse("0"),
 			},
 		},
 		"multi-nodeports": {
@@ -98,8 +102,9 @@ func TestServiceEvaluatorUsage(t *testing.T) {
 				},
 			},
 			usage: api.ResourceList{
-				api.ResourceServices:          resource.MustParse("1"),
-				api.ResourceServicesNodePorts: resource.MustParse("2"),
+				api.ResourceServices:              resource.MustParse("1"),
+				api.ResourceServicesNodePorts:     resource.MustParse("2"),
+				api.ResourceServicesLoadBalancers: resource.MustParse("0"),
 			},
 		},
 	}
@@ -110,3 +115,66 @@ func TestServiceEvaluatorUsage(t *testing.T) {
 		}
 	}
 }
+
+func TestServiceConstraintsFunc(t *testing.T) {
+	testCases := map[string]struct {
+		service  *api.Service
+		required []api.ResourceName
+		err      string
+	}{
+		"loadbalancer": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeLoadBalancer,
+				},
+			},
+			required: []api.ResourceName{api.ResourceServicesLoadBalancers},
+		},
+		"clusterip": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeClusterIP,
+				},
+			},
+			required: []api.ResourceName{api.ResourceServicesLoadBalancers, api.ResourceServices},
+		},
+		"nodeports": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Port: 27443,
+						},
+					},
+				},
+			},
+			required: []api.ResourceName{api.ResourceServicesNodePorts},
+		},
+		"multi-nodeports": {
+			service: &api.Service{
+				Spec: api.ServiceSpec{
+					Type: api.ServiceTypeNodePort,
+					Ports: []api.ServicePort{
+						{
+							Port: 27443,
+						},
+						{
+							Port: 27444,
+						},
+					},
+				},
+			},
+			required: []api.ResourceName{api.ResourceServicesNodePorts},
+		},
+	}
+	for testName, test := range testCases {
+		err := ServiceConstraintsFunc(test.required, test.service)
+		switch {
+		case err != nil && len(test.err) == 0,
+			err == nil && len(test.err) != 0,
+			err != nil && test.err != err.Error():
+			t.Errorf("%s unexpected error: %v", testName, err)
+		}
+	}
+}