Merge pull request kubernetes-retired#219 from mzeevi/hrqIssue-214

Subtree usage updated when namespace is removed from subtree

Author: k8s-ci-robot

internal/forest/namespacehrq.go

@@ -4,7 +4,6 @@ import (
 	"fmt"
 
 	v1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/resource"
 
 	"sigs.k8s.io/hierarchical-namespaces/internal/hrq/utils"
 )
@@ -155,6 +154,9 @@ func (n *Namespace) canUseResources(u v1.ResourceList) error {
 //  usages are different from ResourceQuota.Status.Used
 //   - Called by the HRQ Namespace reconciler to remove `local` usages of a
 //  namespace from the subtree usages of the previous ancestors of the namespace.
+//   - Called by the SetParent to remove `local` usages of a namespace from
+//  the subtree usages of the previous ancestors of the namespace and add the
+//  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.
@@ -257,14 +259,3 @@ func (n *Namespace) UpdateLimits(nm string, l v1.ResourceList) bool {
 	n.quotas.limits[nm] = l
 	return true
 }
-
-// DeleteUsages removes the local usages of a namespace. The usages are also
-// removed from the subtree usages of the ancestors of the namespace. The caller
-// should enqueue the ancestor HRQs to update the usages.
-func (n *Namespace) DeleteUsages() {
-	local := v1.ResourceList{}
-	for r := range n.quotas.used.local {
-		local[r] = resource.MustParse("0")
-	}
-	n.UseResources(local)
-}

internal/forest/namespacehrq_test.go

@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"sort"
 	"strings"
+
+	v1 "k8s.io/api/core/v1"
 )
 
 // ChildNames returns a sorted list of names or nil if there are no children.
@@ -31,10 +33,15 @@ func (ns *Namespace) IsAncestor(other *Namespace) bool {
 	return ns.parent.IsAncestor(other)
 }
 
-// SetParent modifies the namespace's parent, including updating the list of children. It may result
-// in a cycle being created; this can be prevented by calling CanSetParent before, or seeing if it
-// happened by calling CycleNames afterwards.
+// SetParent modifies the namespace's parent, including updating the list of children and updating
+// the old and new subtree usage. It may result in a cycle being created; this can be prevented by
+// calling CanSetParent before, or seeing if it happened by calling CycleNames afterwards.
 func (ns *Namespace) SetParent(p *Namespace) {
+	// Remove usage from old subtree.
+	nsUsage := ns.quotas.used.local
+	if nsUsage != nil {
+		ns.UseResources(v1.ResourceList{})
+	}
 	// Remove old parent and cleans it up.
 	if ns.parent != nil {
 		delete(ns.parent.children, ns.name)
@@ -48,6 +55,10 @@ func (ns *Namespace) SetParent(p *Namespace) {
 	if p != nil {
 		p.children[ns.name] = ns
 	}
+	// Add usage to new subtree.
+	if nsUsage != nil {
+		ns.UseResources(nsUsage)
+	}
 }
 
 // CanSetParent returns the empty string if the assignment is currently legal, or a non-empty string

internal/forest/namespacestructure.go

@@ -156,6 +156,35 @@ func isDeleted(inst *api.HierarchicalResourceQuota) bool {
 	return inst.GetCreationTimestamp() == (metav1.Time{})
 }
 
+// allSubtreeHRQs returns a slice of all HRQ objects in a subtree
+func (r *HierarchicalResourceQuotaReconciler) allSubtreeHRQs(ns *forest.Namespace) []api.HierarchicalResourceQuota {
+	insts := []api.HierarchicalResourceQuota{}
+	for _, nsnm := range ns.AncestryNames() {
+		for _, hrqnm := range r.Forest.Get(nsnm).HRQNames() {
+			inst := api.HierarchicalResourceQuota{}
+			inst.ObjectMeta.Name = hrqnm
+			inst.ObjectMeta.Namespace = nsnm
+
+			insts = append(insts, inst)
+		}
+	}
+	return insts
+}
+
+// OnChangeNamespace enqueues all HRQ objects in the subtree for later reconciliation.
+// This is needed so that the HRQ objects are enqueued for reconciliation when there is a
+// change in the tree hierarchy which affects the subtree usage of the HRQ objects.
+// This occurs in a goroutine so the caller doesn't block; since the
+// reconciler is never garbage-collected, this is safe.
+func (r *HierarchicalResourceQuotaReconciler) OnChangeNamespace(log logr.Logger, ns *forest.Namespace) {
+	insts := r.allSubtreeHRQs(ns)
+	go func() {
+		for _, inst := range insts {
+			r.trigger <- event.GenericEvent{Object: &inst}
+		}
+	}()
+}
+
 // Enqueue enqueues a specific HierarchicalResourceQuota object to trigger the reconciliation of the
 // object for a given reason. This occurs in a goroutine so the caller doesn't block; since the
 // reconciler is never garbage-collected, this is safe.

internal/hrq/hrqreconciler.go

@@ -163,6 +163,68 @@ var _ = Describe("HRQ reconciler tests", func() {
 		Eventually(getRQSpec(ctx, barName)).Should(equalRL("secrets", "6", "pods", "3"))
 		Eventually(getRQSpec(ctx, bazName)).Should(equalRL("secrets", "6", "pods", "3"))
 	})
+
+	It("should update usages in status correctly after moving namespace out of subtree", func() {
+		setHRQ(ctx, fooHRQName, fooName, "secrets", "6", "pods", "3")
+		setHRQ(ctx, barHRQName, barName, "secrets", "100", "cpu", "50")
+		setHRQ(ctx, bazHRQName, bazName, "pods", "1")
+		// Simulate the K8s ResourceQuota controller to update usages.
+		updateRQUsage(ctx, fooName, "secrets", "0", "pods", "0")
+		updateRQUsage(ctx, barName, "secrets", "0", "cpu", "0", "pods", "0")
+		updateRQUsage(ctx, bazName, "secrets", "0", "pods", "0")
+
+		// Increase pods count from 0 to 1 in baz and verify that the usage is
+		// increased in foo's HRQ but not bar's (not an ancestor of baz)
+		updateRQUsage(ctx, bazName, "pods", "1")
+		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "0", "pods", "1"))
+		Eventually(getHRQUsed(ctx, barName, barHRQName)).Should(equalRL("secrets", "0", "cpu", "0"))
+		Eventually(getHRQUsed(ctx, bazName, bazHRQName)).Should(equalRL("pods", "1"))
+
+		// Make baz a full namespace by changing its parent to nil
+		bazHier := GetHierarchy(ctx, bazName)
+		bazHier.Spec.Parent = ""
+		UpdateHierarchy(ctx, bazHier)
+
+		Eventually(HasChild(ctx, fooName, bazName)).ShouldNot(Equal(true))
+
+		// Ensure pods usage is decreased on foo after the change in hierarchy
+		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "0", "pods", "0"))
+		Eventually(getHRQUsed(ctx, barName, barHRQName)).Should(equalRL("secrets", "0", "cpu", "0"))
+		Eventually(getHRQUsed(ctx, bazName, bazHRQName)).Should(equalRL("pods", "1"))
+	})
+
+	It("should update usages in status correctly after moving full namespace with limits into hierarchy", func() {
+		// Make bar a full namespace by changing its parent to nil
+		barHier := GetHierarchy(ctx, barName)
+		barHier.Spec.Parent = ""
+		UpdateHierarchy(ctx, barHier)
+
+		Eventually(HasChild(ctx, fooName, barName)).ShouldNot(Equal(true))
+
+		setHRQ(ctx, fooHRQName, fooName, "secrets", "6", "pods", "3")
+		setHRQ(ctx, barHRQName, barName, "secrets", "100", "cpu", "50")
+
+		// Simulate the K8s ResourceQuota controller to update usages.
+		updateRQUsage(ctx, fooName, "secrets", "0", "pods", "0")
+		updateRQUsage(ctx, barName, "secrets", "0", "cpu", "0", "pods", "0")
+
+		// Increase secrets count from 0 to 1 in bar and verify that the usage is
+		// increased in bar's HRQ but not foo's (not an ancestor of baz)
+		updateRQUsage(ctx, barName, "secrets", "1")
+		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "0", "pods", "0"))
+		Eventually(getHRQUsed(ctx, barName, barHRQName)).Should(equalRL("secrets", "1", "cpu", "0"))
+
+		// Make bar a full namespace by changing its parent to nil
+		barHier = GetHierarchy(ctx, barName)
+		barHier.Spec.Parent = fooName
+		UpdateHierarchy(ctx, barHier)
+
+		Eventually(HasChild(ctx, fooName, barName)).Should(Equal(true))
+
+		// Ensure secrets usage is decreased on foo after the change in hierarchy
+		Eventually(getHRQUsed(ctx, fooName, fooHRQName)).Should(equalRL("secrets", "1", "pods", "0"))
+		Eventually(getHRQUsed(ctx, barName, barHRQName)).Should(equalRL("secrets", "1", "cpu", "0"))
+	})
 })
 
 func forestSetSubtreeUsages(ns string, args ...string) {

internal/hrq/hrqreconciler_test.go

@@ -202,8 +202,7 @@ func (r *ResourceQuotaReconciler) deleteSingleton(ctx context.Context, log logr.
 //   and its ancestors.
 // - Updates `hrq.used.local` of the current namespace and `hrq.used.subtree`
 //   of the current namespace and its ancestors based on `ResourceQuota.Status.Used`.
-func (r *ResourceQuotaReconciler) syncWithForest(log logr.Logger,
-	inst *v1.ResourceQuota) bool {
+func (r *ResourceQuotaReconciler) syncWithForest(log logr.Logger, inst *v1.ResourceQuota) bool {
 	r.Forest.Lock()
 	defer r.Forest.Unlock()
 	ns := r.Forest.Get(inst.ObjectMeta.Namespace)

internal/hrq/rqreconciler.go

@@ -89,6 +89,7 @@ func CreateReconcilers(mgr ctrl.Manager, f *forest.Forest, opts Options) error {
 			RQR:    rqr,
 		}
 		rqr.HRQR = hrqr
+		f.AddListener(hrqr)
 
 		if err := rqr.SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("cannot create resource quota reconciler: %s", err.Error())

internal/setup/reconcilers.go

@@ -22,6 +22,7 @@ const (
 	nsA = prefix+"a"
 	nsB = prefix+"b"
 	nsC = prefix+"c"
+	nsD = prefix+"d"
 
 	propagationTime = 5
 	resourceQuotaSingleton = "hrq.hnc.x-k8s.io"
@@ -138,6 +139,55 @@ var _ = PDescribe("Hierarchical Resource Quota", func() {
 		FieldShouldContain("hrq", nsA, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:0")
 	})
 
+	It("should update ancestor HRQ usages if a descendant namespace with limited resources is moved out of subtree", func() {
+		// set up namespaces and subnamespaces
+		CreateNamespace(nsA)
+		CreateSubnamespace(nsB, nsA)
+		CreateSubnamespace(nsC, nsB)
+		CreateNamespace(nsD)
+
+		// set up an HRQ on the root namespaces and on the child namespace
+		setHRQ("a-hrq-persistentvolumeclaims", nsA, "persistentvolumeclaims", "5")
+		setHRQ("a-hrq-secrets", nsA, "secrets", "5")
+
+		setHRQ("a-hrq-persistentvolumeclaims", nsC, "persistentvolumeclaims", "3")
+		setHRQ("a-hrq-secrets", nsC, "secrets", "3")
+
+		setHRQ("a-hrq-persistentvolumeclaims", nsD, "persistentvolumeclaims", "1")
+		setHRQ("a-hrq-secrets", nsD, "secrets", "1")
+
+		// verify usage updates after consuming limited resources in the descendants
+		Eventually(createPVC("b-pvc", nsB)).Should(Succeed())
+		Eventually(createSecret("b-secret", nsB)).Should(Succeed())
+
+		FieldShouldContain("hrq", nsA, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:1")
+		FieldShouldContain("hrq", nsA, "a-hrq-secrets", ".status.used", "secrets:1")
+
+		Eventually(createPVC("d-pvc", nsD)).Should(Succeed())
+		Eventually(createSecret("d-secret", nsD)).Should(Succeed())
+
+		// make full namespace the child of a subnamespace
+		MustRun("kubectl hns set", nsD, "--parent", nsC)
+
+		FieldShouldContain("hrq", nsA, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:2")
+		FieldShouldContain("hrq", nsA, "a-hrq-secrets", ".status.used", "secrets:2")
+		FieldShouldContain("hrq", nsC, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:1")
+		FieldShouldContain("hrq", nsC, "a-hrq-secrets", ".status.used", "secrets:1")
+		FieldShouldContain("hrq", nsD, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:1")
+		FieldShouldContain("hrq", nsD, "a-hrq-secrets", ".status.used", "secrets:1")
+
+		// test moving out of subree the namespace with usages
+		MustRun("kubectl hns set --root", nsD)
+
+		// verify the HRQ usages are updated
+		FieldShouldContain("hrq", nsA, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:1")
+		FieldShouldContain("hrq", nsA, "a-hrq-secrets", ".status.used", "secrets:1")
+		FieldShouldContain("hrq", nsC, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:0")
+		FieldShouldContain("hrq", nsC, "a-hrq-secrets", ".status.used", "secrets:0")
+		FieldShouldContain("hrq", nsD, "a-hrq-persistentvolumeclaims", ".status.used", "persistentvolumeclaims:1")
+		FieldShouldContain("hrq", nsD, "a-hrq-secrets", ".status.used", "secrets:1")
+	})
+
 	It("should update descendant RQ limits or delete them if a namespace with HRQs is deleted", func() {
 		// set up namespaces - 'a' as the parent of 'b' and 'c'.
 		CreateNamespace(nsA)
@@ -306,6 +356,23 @@ spec:
 	}
 }
 
+func createSecret(nm, nsnm string) func() error {
+	return func() error {
+		secret := `# temp file created by hrq_test.go
+apiVersion: v1
+kind: Secret
+metadata:
+  name: ` + nm + `
+  namespace: ` + nsnm + `
+data:
+  key: YmFyCg==`
+		fn := writeTempFile(secret)
+		GinkgoT().Log("Wrote " + fn + ":\n" + secret)
+		defer removeFile(fn)
+		return TryRun("kubectl apply -f", fn)
+	}
+}
+
 func writeTempFile(cxt string) string {
 	f, err := ioutil.TempFile(os.TempDir(), "e2e-test-*.yaml")
 	Expect(err).Should(BeNil())