Merge pull request kubernetes-retired#237 from adrianludwin/hrq-refactor

Refactor some HRQ code for readability

Author: k8s-ci-robot

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 {