Refactor some HRQ code for readability

I found some of this code very difficult to read, and there was a bunch
of stuff in the 'utils' package that was being exported but not used
(this is because the library was originally copied from K8s, but it's
been heavily modified since then). Hopefully others find these cleanups
useful to.

I did simplify the TryUseResources function to allow decreasing usage.
This opens up a *very* slight race condition at the cost of making the
function more robust to future changes in K8s and significantly easier
to understand and test.

Author: adrianludwin

internal/forest/namespacehrq.go

@@ -36,18 +36,12 @@ type usage struct {
 	subtree v1.ResourceList
 }
 
-// TryUseResources checks resource limits in the namespace and its ancestors
-// when given proposed absolute (not delta) resource usages in the namespace. If
-// the proposed resource usages are the same as the current usages, of course
-// it's allowed and we do nothing. If there are any changes in the usages, we
-// only check to see if the proposed changing usages are allowed. If any of them
-// exceed resource limits, it returns an error; otherwise, it returns nil.
-// Callers of this method are responsible for updating resource usage status of
-// the HierarchicalResourceQuota objects.
-//
-// If the proposed resource usages changes are allowed, the method also updates
-// in-memory local resource usages of the current namespace and the subtree
-// resource usages of its ancestors (including itself).
+// TryUseResources checks resource limits in the namespace and its ancestors when given proposed
+// absolute (not delta) resource usages in the namespace. If there are any changes in the usages, we
+// only check to see if any proposed increases take us over any limits. If any of them exceed
+// resource limits, it returns an error suitable to display to end users; otherwise, it updates the
+// in-memory usages of both this namespace as well as all its ancestors. Callers of this method are
+// responsible for updating resource usage status of the HierarchicalResourceQuota objects.
 //
 // TryUseResources is called by the HRQ admission controller to decide if a ResourceQuota.Status
 // update issued by the K8s ResourceQuota admission controller is allowed. Since UseResources()
@@ -62,38 +56,21 @@ type usage struct {
 // etcd) but the apiserver runs a cleanup process that occasionally syncs up actual usage with the
 // usage recorded in RQs. When the RQs are changed, we'll be updated too.
 //
-// Based on observations, the K8s ResourceQuota admission controller is called only
-// when a resource is consumed, not when a resource is released. Therefore, in most cases,
-// the proposed resource usages that the HRQ admission controller received should
-// be larger than in-memory resource usages.
-//
-// The proposed usage can be smaller when in-memory resource usages are out of sync
-// with ResourceQuota.Status.Used. In this case, TryUseResources will still update
-// in-memory usages.
-//
-// For example, when a user deletes a pod that consumes
-// 100m cpu and immediately creates another pod that consumes 1m cpu in a namespace,
-// the HRQ ResourceQuota reconciler will be triggered by ResourceQuota.Status changes twice:
-// 1) when pod is deleted.
-// 2) when pod is created.
-// The HRQ ResourceQuota reconciler will compare `local` with ResourceQuota.Status.Used
-// and will update `local` (and `subtree` in namespace and its ancestors) if it
-// sees `local` is different from ResourceQuota.Status.Used.
+// Based on observations, the K8s ResourceQuota admission controller is called only when a resource
+// is consumed, not when a resource is released. Therefore, in most cases, the proposed resource
+// usages that the HRQ admission controller received should be larger than in-memory resource
+// usages. However, this function is robust to (that is, always allows) decreases as well, mainly
+// because it's easier to test - plus, who knows, the K8s behaviour may change in the future.
 //
-// The HRQ admission coontroller will be triggered once when the pod is created.
-// If the HRQ admission coontroller locks the in-memory forest before step 1)
-// of the HRQ ResourceQuota reconciler, the HRQ admission controller will see proposed
-// usages are smaller than in-memory usages and will update in-memory usages.
-// When ResourceQuota reconciler receives the lock later, it will notice the
-// ResourceQuota.Status.Used is the same as in-memory usages and will not
-// update in-memory usages.
+// This may allow one weird case where a user may be allowed to use something they weren't supposed
+// to. Let's say you're well over your limit, and then in quick succession, some resources are
+// deleted, and some _fewer_ are added, but enough to still go over the limit. In that case, there's
+// a race condition between this function being called, and the RQ reconciler updating the baseline
+// resource usage. If this function wins, it will look like resource usage is decreasing, and will
+// be incorrectly allowed. If the RQ reconciler runs first, we'll see that the usage is incorrectly
+// _increasing_ and it will be disallowed. However, I think the simplicity of not trying to prevent
+// this (hopefully very unlikely) corner case is more valuable than trying to catch it.
 func (n *Namespace) TryUseResources(rl v1.ResourceList) error {
-	// The proposed resource usages are allowed if they are the same as current
-	// resource usages in the namespace.
-	if utils.Equals(rl, n.quotas.used.local) {
-		return nil
-	}
-
 	if err := n.canUseResources(rl); err != nil {
 		// At least one of the proposed usage exceeds resource limits.
 		return err
@@ -114,28 +91,32 @@ func (n *Namespace) TryUseResources(rl v1.ResourceList) error {
 func (n *Namespace) canUseResources(u v1.ResourceList) error {
 	// For each resource, delta = proposed usage - current usage.
 	delta := utils.Subtract(u, n.quotas.used.local)
-	delta = utils.OmitZeroQuantity(delta)
+	// Only consider *increasing* deltas; see comments to TryUseResources for details.
+	increases := utils.OmitLTEZero(delta)
 
 	for _, nsnm := range n.AncestryNames() {
 		ns := n.forest.Get(nsnm)
-		r := utils.Copy(ns.quotas.used.subtree)
-		r = utils.AddIfExists(delta, r)
-		allowed, nm, exceeded := checkLimits(ns.quotas.limits, r)
-		if !allowed {
-			// Construct the error message similar to the RQ exceeded quota error message -
-			// "exceeded quota: gke-hc-hrq, requested: configmaps=1, used: configmaps=2, limited: configmaps=2"
-			msg := fmt.Sprintf("exceeded hierarchical quota in namespace %q: %q", ns.name, nm)
-			for _, er := range exceeded {
-				rnm := er.String()
-				// Get the requested, used, limited quantity of the exceeded resource.
-				rq := delta[er]
-				uq := ns.quotas.used.subtree[er]
-				lq := ns.quotas.limits[nm][er]
-				msg += fmt.Sprintf(", requested: %s=%v, used: %s=%v, limited: %s=%v",
-					rnm, &rq, rnm, &uq, rnm, &lq)
-			}
-			return fmt.Errorf(msg)
+		// Use AddIfExists (not Add) because we want to ignore any resources that aren't increasing when
+		// checking against the limits.
+		proposed := utils.AddIfExists(increases, ns.quotas.used.subtree)
+		allowed, nm, exceeded := checkLimits(ns.quotas.limits, proposed)
+		if allowed {
+			continue
 		}
+
+		// Construct the error message similar to the RQ exceeded quota error message -
+		// "exceeded quota: gke-hc-hrq, requested: configmaps=1, used: configmaps=2, limited: configmaps=2"
+		msg := fmt.Sprintf("exceeded hierarchical quota in namespace %q: %q", ns.name, nm)
+		for _, er := range exceeded {
+			rnm := er.String()
+			// Get the requested, used, limited quantity of the exceeded resource.
+			rq := increases[er]
+			uq := ns.quotas.used.subtree[er]
+			lq := ns.quotas.limits[nm][er]
+			msg += fmt.Sprintf(", requested: %s=%v, used: %s=%v, limited: %s=%v",
+				rnm, &rq, rnm, &uq, rnm, &lq)
+		}
+		return fmt.Errorf(msg)
 	}
 
 	return nil
@@ -159,8 +140,10 @@ func (n *Namespace) canUseResources(u v1.ResourceList) error {
 //  usages to the new ancestors following a parent update
 func (n *Namespace) UseResources(newUsage v1.ResourceList) {
 	oldUsage := n.quotas.used.local
-	// We only consider limited usages.
-	newUsage = utils.CleanupUnneeded(newUsage, n.Limits())
+
+	// We only store the usages we care about
+	newUsage = utils.FilterUnlimited(newUsage, n.Limits())
+
 	// Early exit if there's no usages change. It's safe because the forest would
 	// remain unchanged and the caller would always enqueue all ancestor HRQs.
 	if utils.Equals(oldUsage, newUsage) {
@@ -181,7 +164,7 @@ func (n *Namespace) UseResources(newUsage v1.ResourceList) {
 
 		// Get the new subtree usage and remove no longer limited usages.
 		newSubUsg := utils.Add(delta, ns.quotas.used.subtree)
-		ns.UpdateSubtreeUsages(newSubUsg)
+		ns.quotas.used.subtree = utils.FilterUnlimited(newSubUsg, ns.Limits())
 	}
 }
 
@@ -236,9 +219,13 @@ func (n *Namespace) GetSubtreeUsages() v1.ResourceList {
 	return u
 }
 
-// UpdateSubtreeUsages sets the subtree resource usages.
-func (n *Namespace) UpdateSubtreeUsages(rl v1.ResourceList) {
-	n.quotas.used.subtree = utils.CleanupUnneeded(rl, n.Limits())
+// TestOnlySetSubtreeUsage overwrites the actual, calculated subtree usages and replaces them with
+// arbitrary garbage. Needless to say, you should never call this, unless you're testing HNC's
+// ability to recover from arbitrary garbage.
+//
+// The passed-in arg is used as-is, not copied. This is test code, so deal with it 😎
+func (n *Namespace) TestOnlySetSubtreeUsage(rl v1.ResourceList) {
+	n.quotas.used.subtree = rl
 }
 
 // RemoveLimits removes limits specified by the HierarchicalResourceQuota object

internal/forest/namespacehrq_test.go

@@ -151,35 +151,13 @@ func TestTryUseResources(t *testing.T) {
 		req:      testReq{ns: "bar", use: "secrets 1 pods 1"},
 		expected: usages{"foo": "secrets 2", "bar": "secrets 1 pods 1"},
 	}, {
-		// Note: we are expecting an error here because this is a unit test for
-		// the `TryUseResources` function. In reality, negative resource usage
-		// changes won't get an error since K8s resource quota admission
-		// controller is not called, thus neither our webhook nor this function
-		// will be called to prevent users from deleting exceeded resources.
-		name: "deny decrease if it still exceeds limits",
+		name: "allow decrease if it still exceeds limits",
 		setup: []testNS{
 			{nm: "foo", limits: "secrets 2", local: "secrets 1"},
 			{nm: "bar", ancs: "foo", limits: "secrets 100", local: "secrets 4"},
 		},
-		req: testReq{ns: "bar", use: "secrets 2"},
-		// In reality it won't show negative number since neither K8s rq validator
-		// nor our rq validator will be called to output this message.
-		error: []wantError{{ns: "foo", nm: "fooHrq", exceeded: []exceeded{{name: "secrets", requested: "-2", used: "5", limited: "2"}}}},
-		// Usages should not be updated.
-		expected: usages{"foo": "secrets 5", "bar": "secrets 4"},
-	}, {
-		// Note: we are expecting an error here for the same reason in the above test.
-		name: "deny decrease if it still exceeds limits, while not affecting other resource usages",
-		setup: []testNS{
-			{nm: "foo", limits: "secrets 2", local: "secrets 1"},
-			{nm: "bar", ancs: "foo", limits: "secrets 100 pods 2", local: "secrets 4 pods 1"},
-		},
-		req: testReq{ns: "bar", use: "secrets 2 pods 1"},
-		// In reality it won't show negative number since neither K8s rq validator
-		// nor our rq validator will be called to output this message.
-		error: []wantError{{ns: "foo", nm: "fooHrq", exceeded: []exceeded{{name: "secrets", requested: "-2", used: "5", limited: "2"}}}},
-		// Usages should not be updated.
-		expected: usages{"foo": "secrets 5", "bar": "secrets 4 pods 1"},
+		req:      testReq{ns: "bar", use: "secrets 2"},
+		expected: usages{"foo": "secrets 3", "bar": "secrets 2"},
 	}}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
@@ -251,53 +229,33 @@ func TestCanUseResource(t *testing.T) {
 		req:  testReq{ns: "bar", use: "secrets 3 pods 1"},
 		want: []wantError{{ns: "foo", nm: "fooHrq", exceeded: []exceeded{{name: "secrets", requested: "2", used: "2", limited: "2"}}}},
 	}, {
-		// Note: we are expecting an error here because this is a unit test for
-		// the `CanUseResources` function. In reality, negative resource usage
-		// changes won't get an error since K8s resource quota admission
-		// controller is not called, thus neither our webhook nor this function
-		// will be called to prevent users from deleting exceeded resources.
-		name: "deny decrease exceeding local limit when has parent",
+		name: "allow decrease exceeding local limit when has parent",
 		setup: []testNS{
 			{nm: "foo", limits: "secrets 2", local: "secrets 1"},
 			{nm: "bar", ancs: "foo", limits: "pods 2", local: "pods 4"},
 		},
 		req: testReq{ns: "bar", use: "pods 3"},
-		// In reality it won't show negative number since neither K8s rq validator
-		// nor our rq validator will be called to output this message.
-		want: []wantError{{ns: "bar", nm: "barHrq", exceeded: []exceeded{{name: "pods", requested: "-1", used: "4", limited: "2"}}}},
 	}, {
-		// Note: we are expecting an error here for the same reason in the above test.
-		name: "deny decrease exceeding local limit when has parent, while not affecting other resource usages",
+		name: "allow decrease exceeding local limit when has parent, while not affecting other resource usages",
 		setup: []testNS{
 			{nm: "foo", limits: "secrets 2", local: "secrets 1"},
 			{nm: "bar", ancs: "foo", limits: "secrets 100 pods 2", local: "secrets 1 pods 4"},
 		},
 		req: testReq{ns: "bar", use: "secrets 1 pods 3"},
-		// In reality it won't show negative number since neither K8s rq validator
-		// nor our rq validator will be called to output this message.
-		want: []wantError{{ns: "bar", nm: "barHrq", exceeded: []exceeded{{name: "pods", requested: "-1", used: "4", limited: "2"}}}},
 	}, {
-		// Note: we are expecting an error here for the same reason in the above test.
-		name: "deny decrease exceeding parent limit when has parent",
+		name: "allow decrease exceeding parent limit when has parent",
 		setup: []testNS{
 			{nm: "foo", limits: "secrets 2", local: "secrets 1"},
 			{nm: "bar", ancs: "foo", limits: "secrets 100", local: "secrets 4"},
 		},
 		req: testReq{ns: "bar", use: "secrets 3"},
-		// In reality it won't show negative number since neither K8s rq validator
-		// nor our rq validator will be called to output this message.
-		want: []wantError{{ns: "foo", nm: "fooHrq", exceeded: []exceeded{{name: "secrets", requested: "-1", used: "5", limited: "2"}}}},
 	}, {
-		// Note: we are expecting an error here for the same reason in the above test.
-		name: "deny decrease exceeding parent limit when has parent, while not affecting other resource usages",
+		name: "allow decrease exceeding parent limit when has parent, while not affecting other resource usages",
 		setup: []testNS{
 			{nm: "foo", limits: "secrets 2", local: "secrets 1"},
 			{nm: "bar", ancs: "foo", limits: "secrets 100 pods 2", local: "secrets 4 pods 2"},
 		},
 		req: testReq{ns: "bar", use: "secrets 3 pods 2"},
-		// In reality it won't show negative number since neither K8s rq validator
-		// nor our rq validator will be called to output this message.
-		want: []wantError{{ns: "foo", nm: "fooHrq", exceeded: []exceeded{{name: "secrets", requested: "-1", used: "5", limited: "2"}}}},
 	}, {
 		name: "deny multiple resources exceeding limits",
 		setup: []testNS{

internal/hrq/hrqreconciler.go

@@ -141,15 +141,9 @@ func (r *HierarchicalResourceQuotaReconciler) syncUsages(inst *api.HierarchicalR
 	}
 	ns := r.Forest.Get(inst.GetNamespace())
 
-	// Get all usage counts in this namespace
-	usages := ns.GetSubtreeUsages()
-
-	// Get the names of all resource types being limited by this particular HRQ
-	nms := utils.ResourceNames(inst.Spec.Hard)
-
 	// Filter the usages to only include the resource types being limited by this HRQ and write those
 	// usages back to the HRQ status.
-	inst.Status.Used = utils.Mask(usages, nms)
+	inst.Status.Used = utils.FilterUnlimited(ns.GetSubtreeUsages(), inst.Spec.Hard)
 }
 
 func isDeleted(inst *api.HierarchicalResourceQuota) bool {

internal/hrq/hrqreconciler_test.go

@@ -230,7 +230,7 @@ var _ = Describe("HRQ reconciler tests", func() {
 func forestSetSubtreeUsages(ns string, args ...string) {
 	TestForest.Lock()
 	defer TestForest.Unlock()
-	TestForest.Get(ns).UpdateSubtreeUsages(argsToResourceList(0, args...))
+	TestForest.Get(ns).TestOnlySetSubtreeUsage(argsToResourceList(0, args...))
 }
 
 func getHRQStatus(ctx context.Context, ns, nm string) func() v1.ResourceList {

internal/hrq/utils/resources.go

@@ -95,25 +95,18 @@ func Subtract(a v1.ResourceList, b v1.ResourceList) v1.ResourceList {
 	return result
 }
 
-// OmitZeroQuantity returns a list omitting zero quantity resources.
-func OmitZeroQuantity(a v1.ResourceList) v1.ResourceList {
+// OmitLTEZero returns a list omitting zero or negative quantity resources.
+func OmitLTEZero(a v1.ResourceList) v1.ResourceList {
 	for n, q := range a {
 		if q.IsZero() {
 			delete(a, n)
+		} else if q.AsApproximateFloat64() <= 0 {
+			delete(a, n)
 		}
 	}
 	return a
 }
 
-// Copy returns a deep copy of a ResourceList.
-func Copy(rl v1.ResourceList) v1.ResourceList {
-	rs := make(v1.ResourceList)
-	for k, v := range rl {
-		rs[k] = v.DeepCopy()
-	}
-	return rs
-}
-
 // Min returns the result of Min(a, b) (i.e., smallest resource quantity) for
 // each named resource. If a resource only exists in one but not all ResourceLists
 // inputs, it will be returned.
@@ -136,32 +129,22 @@ func Min(a v1.ResourceList, b v1.ResourceList) v1.ResourceList {
 	return result
 }
 
-// Contains returns true if the specified item is in the list of items
-func Contains(items []v1.ResourceName, item v1.ResourceName) bool {
-	for _, i := range items {
-		if i == item {
-			return true
-		}
-	}
-	return false
-}
-
-// CleanupUnneeded cleans up the raw usages by omitting not limited usages.
-func CleanupUnneeded(rawUsages v1.ResourceList, limits v1.ResourceList) v1.ResourceList {
-	return Mask(rawUsages, ResourceNames(limits))
+// FilterUnlimited cleans up the raw usages by omitting not limited usages.
+func FilterUnlimited(rawUsages v1.ResourceList, limits v1.ResourceList) v1.ResourceList {
+	return mask(rawUsages, resourceNames(limits))
 }
 
-// ResourceNames returns a list of all resource names in the ResourceList
-func ResourceNames(resources v1.ResourceList) []v1.ResourceName {
+// resourceNames returns a list of all resource names in the ResourceList
+func resourceNames(resources v1.ResourceList) []v1.ResourceName {
 	result := []v1.ResourceName{}
 	for resourceName := range resources {
 		result = append(result, resourceName)
 	}
 	return result
 }
 
-// Mask returns a new resource list that only has the values with the specified names
-func Mask(resources v1.ResourceList, names []v1.ResourceName) v1.ResourceList {
+// mask returns a new resource list that only has the values with the specified names
+func mask(resources v1.ResourceList, names []v1.ResourceName) v1.ResourceList {
 	nameSet := toSet(names)
 	result := v1.ResourceList{}
 	for key, value := range resources {