From 27cd2be49fdd6f5ae1866c47437e2c56a9616fea Mon Sep 17 00:00:00 2001 From: Jordan Liggitt Date: Wed, 13 Mar 2019 20:13:51 -0700 Subject: [PATCH] Update quota status with limits even when calculating errors --- pkg/controller/resourcequota/BUILD | 1 + .../resource_quota_controller.go | 16 +++- .../resource_quota_controller_test.go | 81 ++++++++++++++++- pkg/quota/v1/BUILD | 1 + pkg/quota/v1/resources.go | 53 ++++++++--- pkg/quota/v1/resources_test.go | 91 +++++++++++++++++++ 6 files changed, 225 insertions(+), 18 deletions(-) diff --git a/pkg/controller/resourcequota/BUILD b/pkg/controller/resourcequota/BUILD index 2728d1cb967..cb2d1beac03 100644 --- a/pkg/controller/resourcequota/BUILD +++ b/pkg/controller/resourcequota/BUILD @@ -54,6 +54,7 @@ go_test( "//staging/src/k8s.io/api/core/v1:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/labels:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", diff --git a/pkg/controller/resourcequota/resource_quota_controller.go b/pkg/controller/resourcequota/resource_quota_controller.go index e87e4cf49a2..bac6b9ebdae 100644 --- a/pkg/controller/resourcequota/resource_quota_controller.go +++ b/pkg/controller/resourcequota/resource_quota_controller.go @@ -30,6 +30,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime/schema" + utilerrors "k8s.io/apimachinery/pkg/util/errors" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" @@ -321,12 +322,12 @@ func (rq *ResourceQuotaController) syncResourceQuotaFromKey(key string) (err err // syncResourceQuota runs a complete sync of resource quota status across all known kinds func (rq *ResourceQuotaController) syncResourceQuota(resourceQuota *v1.ResourceQuota) (err error) { // quota is dirty if any part of spec hard limits differs from the status hard limits - dirty := !apiequality.Semantic.DeepEqual(resourceQuota.Spec.Hard, resourceQuota.Status.Hard) + statusLimitsDirty := !apiequality.Semantic.DeepEqual(resourceQuota.Spec.Hard, resourceQuota.Status.Hard) // dirty tracks if the usage status differs from the previous sync, // if so, we send a new usage with latest status // if this is our first sync, it will be dirty by default, since we need track usage - dirty = dirty || resourceQuota.Status.Hard == nil || resourceQuota.Status.Used == nil + dirty := statusLimitsDirty || resourceQuota.Status.Hard == nil || resourceQuota.Status.Used == nil used := v1.ResourceList{} if resourceQuota.Status.Used != nil { @@ -334,9 +335,12 @@ func (rq *ResourceQuotaController) syncResourceQuota(resourceQuota *v1.ResourceQ } hardLimits := quota.Add(v1.ResourceList{}, resourceQuota.Spec.Hard) + errors := []error{} + newUsage, err := quota.CalculateUsage(resourceQuota.Namespace, resourceQuota.Spec.Scopes, hardLimits, rq.registry, resourceQuota.Spec.ScopeSelector) if err != nil { - return err + // if err is non-nil, remember it to return, but continue updating status with any resources in newUsage + errors = append(errors, err) } for key, value := range newUsage { used[key] = value @@ -359,9 +363,11 @@ func (rq *ResourceQuotaController) syncResourceQuota(resourceQuota *v1.ResourceQ // there was a change observed by this controller that requires we update quota if dirty { _, err = rq.rqClient.ResourceQuotas(usage.Namespace).UpdateStatus(usage) - return err + if err != nil { + errors = append(errors, err) + } } - return nil + return utilerrors.NewAggregate(errors) } // replenishQuota is a replenishment function invoked by a controller to notify that a quota should be recalculated diff --git a/pkg/controller/resourcequota/resource_quota_controller_test.go b/pkg/controller/resourcequota/resource_quota_controller_test.go index 25fae49784d..1d92197b02a 100644 --- a/pkg/controller/resourcequota/resource_quota_controller_test.go +++ b/pkg/controller/resourcequota/resource_quota_controller_test.go @@ -28,6 +28,7 @@ import ( "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" @@ -83,6 +84,23 @@ func newGenericLister(groupResource schema.GroupResource, items []runtime.Object return cache.NewGenericLister(store, groupResource) } +func newErrorLister() cache.GenericLister { + return errorLister{} +} + +type errorLister struct { +} + +func (errorLister) List(selector labels.Selector) (ret []runtime.Object, err error) { + return nil, fmt.Errorf("error listing") +} +func (errorLister) Get(name string) (runtime.Object, error) { + return nil, fmt.Errorf("error getting") +} +func (errorLister) ByNamespace(namespace string) cache.GenericNamespaceLister { + return errorLister{} +} + type quotaController struct { *ResourceQuotaController stop chan struct{} @@ -205,9 +223,11 @@ func newTestPodsWithPriorityClasses() []runtime.Object { func TestSyncResourceQuota(t *testing.T) { testCases := map[string]struct { gvr schema.GroupVersionResource + errorGVR schema.GroupVersionResource items []runtime.Object quota v1.ResourceQuota status v1.ResourceQuotaStatus + expectedError string expectedActionSet sets.String }{ "non-matching-best-effort-scoped-quota": { @@ -699,18 +719,75 @@ func TestSyncResourceQuota(t *testing.T) { expectedActionSet: sets.NewString(), items: []runtime.Object{}, }, + "quota-missing-status-with-calculation-error": { + errorGVR: v1.SchemeGroupVersion.WithResource("pods"), + quota: v1.ResourceQuota{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "rq", + }, + Spec: v1.ResourceQuotaSpec{ + Hard: v1.ResourceList{ + v1.ResourcePods: resource.MustParse("1"), + }, + }, + Status: v1.ResourceQuotaStatus{}, + }, + status: v1.ResourceQuotaStatus{ + Hard: v1.ResourceList{ + v1.ResourcePods: resource.MustParse("1"), + }, + }, + expectedError: "error listing", + expectedActionSet: sets.NewString("update-resourcequotas-status"), + items: []runtime.Object{}, + }, + "quota-missing-status-with-partial-calculation-error": { + gvr: v1.SchemeGroupVersion.WithResource("configmaps"), + errorGVR: v1.SchemeGroupVersion.WithResource("pods"), + quota: v1.ResourceQuota{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "rq", + }, + Spec: v1.ResourceQuotaSpec{ + Hard: v1.ResourceList{ + v1.ResourcePods: resource.MustParse("1"), + v1.ResourceConfigMaps: resource.MustParse("1"), + }, + }, + Status: v1.ResourceQuotaStatus{}, + }, + status: v1.ResourceQuotaStatus{ + Hard: v1.ResourceList{ + v1.ResourcePods: resource.MustParse("1"), + v1.ResourceConfigMaps: resource.MustParse("1"), + }, + Used: v1.ResourceList{ + v1.ResourceConfigMaps: resource.MustParse("0"), + }, + }, + expectedError: "error listing", + expectedActionSet: sets.NewString("update-resourcequotas-status"), + items: []runtime.Object{}, + }, } for testName, testCase := range testCases { kubeClient := fake.NewSimpleClientset(&testCase.quota) listersForResourceConfig := map[schema.GroupVersionResource]cache.GenericLister{ - testCase.gvr: newGenericLister(testCase.gvr.GroupResource(), testCase.items), + testCase.gvr: newGenericLister(testCase.gvr.GroupResource(), testCase.items), + testCase.errorGVR: newErrorLister(), } qc := setupQuotaController(t, kubeClient, mockListerForResourceFunc(listersForResourceConfig), mockDiscoveryFunc) defer close(qc.stop) if err := qc.syncResourceQuota(&testCase.quota); err != nil { - t.Fatalf("test: %s, unexpected error: %v", testName, err) + if len(testCase.expectedError) == 0 || !strings.Contains(err.Error(), testCase.expectedError) { + t.Fatalf("test: %s, unexpected error: %v", testName, err) + } + } else if len(testCase.expectedError) > 0 { + t.Fatalf("test: %s, expected error %q, got none", testName, testCase.expectedError) } actionSet := sets.NewString() diff --git a/pkg/quota/v1/BUILD b/pkg/quota/v1/BUILD index 7b3cb195b78..fe9f7b579d3 100644 --- a/pkg/quota/v1/BUILD +++ b/pkg/quota/v1/BUILD @@ -18,6 +18,7 @@ go_library( "//staging/src/k8s.io/apimachinery/pkg/api/resource:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/runtime/schema:go_default_library", + "//staging/src/k8s.io/apimachinery/pkg/util/errors:go_default_library", "//staging/src/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library", "//staging/src/k8s.io/client-go/tools/cache:go_default_library", diff --git a/pkg/quota/v1/resources.go b/pkg/quota/v1/resources.go index b6aa3210d4b..86984fc1c69 100644 --- a/pkg/quota/v1/resources.go +++ b/pkg/quota/v1/resources.go @@ -17,10 +17,12 @@ limitations under the License. package quota import ( + "sort" "strings" corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" + utilerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" ) @@ -186,7 +188,12 @@ func ResourceNames(resources corev1.ResourceList) []corev1.ResourceName { // Contains returns true if the specified item is in the list of items func Contains(items []corev1.ResourceName, item corev1.ResourceName) bool { - return ToSet(items).Has(string(item)) + for _, i := range items { + if i == item { + return true + } + } + return false } // ContainsPrefix returns true if the specified item has a prefix that contained in given prefix Set @@ -199,15 +206,32 @@ func ContainsPrefix(prefixSet []string, item corev1.ResourceName) bool { return false } -// Intersection returns the intersection of both list of resources +// Intersection returns the intersection of both list of resources, deduped and sorted func Intersection(a []corev1.ResourceName, b []corev1.ResourceName) []corev1.ResourceName { - setA := ToSet(a) - setB := ToSet(b) - setC := setA.Intersection(setB) - result := []corev1.ResourceName{} - for _, resourceName := range setC.List() { - result = append(result, corev1.ResourceName(resourceName)) + result := make([]corev1.ResourceName, 0, len(a)) + for _, item := range a { + if Contains(result, item) { + continue + } + if !Contains(b, item) { + continue + } + result = append(result, item) } + sort.Slice(result, func(i, j int) bool { return result[i] < result[j] }) + return result +} + +// Difference returns the list of resources resulting from a-b, deduped and sorted +func Difference(a []corev1.ResourceName, b []corev1.ResourceName) []corev1.ResourceName { + result := make([]corev1.ResourceName, 0, len(a)) + for _, item := range a { + if Contains(b, item) || Contains(result, item) { + continue + } + result = append(result, item) + } + sort.Slice(result, func(i, j int) bool { return result[i] < result[j] }) return result } @@ -243,7 +267,8 @@ func ToSet(resourceNames []corev1.ResourceName) sets.String { return result } -// CalculateUsage calculates and returns the requested ResourceList usage +// CalculateUsage calculates and returns the requested ResourceList usage. +// If an error is returned, usage only contains the resources which encountered no calculation errors. func CalculateUsage(namespaceName string, scopes []corev1.ResourceQuotaScope, hardLimits corev1.ResourceList, registry Registry, scopeSelector *corev1.ScopeSelector) (corev1.ResourceList, error) { // find the intersection between the hard resources on the quota // and the resources this controller can track to know what we can @@ -257,6 +282,8 @@ func CalculateUsage(namespaceName string, scopes []corev1.ResourceQuotaScope, ha // NOTE: the intersection just removes duplicates since the evaluator match intersects with hard matchedResources := Intersection(hardResources, potentialResources) + errors := []error{} + // sum the observed usage from each evaluator newUsage := corev1.ResourceList{} for _, evaluator := range evaluators { @@ -269,7 +296,11 @@ func CalculateUsage(namespaceName string, scopes []corev1.ResourceQuotaScope, ha usageStatsOptions := UsageStatsOptions{Namespace: namespaceName, Scopes: scopes, Resources: intersection, ScopeSelector: scopeSelector} stats, err := evaluator.UsageStats(usageStatsOptions) if err != nil { - return nil, err + // remember the error + errors = append(errors, err) + // exclude resources which encountered calculation errors + matchedResources = Difference(matchedResources, intersection) + continue } newUsage = Add(newUsage, stats.Used) } @@ -278,5 +309,5 @@ func CalculateUsage(namespaceName string, scopes []corev1.ResourceQuotaScope, ha // merge our observed usage with the quota usage status // if the new usage is different than the last usage, we will need to do an update newUsage = Mask(newUsage, matchedResources) - return newUsage, nil + return newUsage, utilerrors.NewAggregate(errors) } diff --git a/pkg/quota/v1/resources_test.go b/pkg/quota/v1/resources_test.go index 61175c706eb..910c2f51120 100644 --- a/pkg/quota/v1/resources_test.go +++ b/pkg/quota/v1/resources_test.go @@ -17,6 +17,7 @@ limitations under the License. package quota import ( + "reflect" "testing" corev1 "k8s.io/api/core/v1" @@ -319,3 +320,93 @@ func TestIsNegative(t *testing.T) { } } } + +func TestIntersection(t *testing.T) { + testCases := map[string]struct { + a []corev1.ResourceName + b []corev1.ResourceName + expected []corev1.ResourceName + }{ + "empty": { + a: []corev1.ResourceName{}, + b: []corev1.ResourceName{}, + expected: []corev1.ResourceName{}, + }, + "equal": { + a: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + b: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + expected: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + }, + "a has extra": { + a: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + b: []corev1.ResourceName{corev1.ResourceCPU}, + expected: []corev1.ResourceName{corev1.ResourceCPU}, + }, + "b has extra": { + a: []corev1.ResourceName{corev1.ResourceCPU}, + b: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + expected: []corev1.ResourceName{corev1.ResourceCPU}, + }, + "dedupes": { + a: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceMemory}, + b: []corev1.ResourceName{corev1.ResourceCPU}, + expected: []corev1.ResourceName{corev1.ResourceCPU}, + }, + "sorts": { + a: []corev1.ResourceName{corev1.ResourceMemory, corev1.ResourceMemory, corev1.ResourceCPU, corev1.ResourceCPU}, + b: []corev1.ResourceName{corev1.ResourceMemory, corev1.ResourceMemory, corev1.ResourceCPU, corev1.ResourceCPU}, + expected: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + }, + } + for testName, testCase := range testCases { + actual := Intersection(testCase.a, testCase.b) + if !reflect.DeepEqual(actual, testCase.expected) { + t.Errorf("%s expected: %#v, actual: %#v", testName, testCase.expected, actual) + } + } +} + +func TestDifference(t *testing.T) { + testCases := map[string]struct { + a []corev1.ResourceName + b []corev1.ResourceName + expected []corev1.ResourceName + }{ + "empty": { + a: []corev1.ResourceName{}, + b: []corev1.ResourceName{}, + expected: []corev1.ResourceName{}, + }, + "equal": { + a: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + b: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + expected: []corev1.ResourceName{}, + }, + "a has extra": { + a: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + b: []corev1.ResourceName{corev1.ResourceCPU}, + expected: []corev1.ResourceName{corev1.ResourceMemory}, + }, + "b has extra": { + a: []corev1.ResourceName{corev1.ResourceCPU}, + b: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + expected: []corev1.ResourceName{}, + }, + "dedupes": { + a: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceCPU, corev1.ResourceMemory, corev1.ResourceMemory}, + b: []corev1.ResourceName{corev1.ResourceCPU}, + expected: []corev1.ResourceName{corev1.ResourceMemory}, + }, + "sorts": { + a: []corev1.ResourceName{corev1.ResourceMemory, corev1.ResourceMemory, corev1.ResourceCPU, corev1.ResourceCPU}, + b: []corev1.ResourceName{}, + expected: []corev1.ResourceName{corev1.ResourceCPU, corev1.ResourceMemory}, + }, + } + for testName, testCase := range testCases { + actual := Difference(testCase.a, testCase.b) + if !reflect.DeepEqual(actual, testCase.expected) { + t.Errorf("%s expected: %#v, actual: %#v", testName, testCase.expected, actual) + } + } +}