Update quota status with limits even when calculating errors

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",

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

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()

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",

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)
 }

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)
+		}
+	}
+}