Multiple scoped HRQs with the same name (#24)

Author: ryotarai

internal/hrq/hrqreconciler.go

@@ -96,7 +96,11 @@ func (r *HierarchicalResourceQuotaReconciler) Reconcile(ctx context.Context, req
 
 	rqName := api.ResourceQuotaSingletonName
 	if r.Forest.IsMarkedAsScopedHRQ(req.NamespacedName) {
-		rqName = utils.ScopedRQName(inst.GetName())
+		rqName, err = utils.ScopedRQName(inst.GetNamespace(), inst.GetName())
+		if err != nil {
+			log.Error(err, "Couldn't get the scoped RQ name")
+			return ctrl.Result{}, err
+		}
 	}
 
 	// Enqueue ResourceQuota objects in the current namespace and its descendants
@@ -116,6 +120,8 @@ func (r *HierarchicalResourceQuotaReconciler) Reconcile(ctx context.Context, req
 // forest. The first return value is true if the HRQ object is updated; the
 // second return value is true if the forest is updated.
 func (r *HierarchicalResourceQuotaReconciler) syncWithForest(log logr.Logger, inst *api.HierarchicalResourceQuota) (bool, bool, error) {
+	var err error
+
 	r.Forest.Lock()
 	defer r.Forest.Unlock()
 
@@ -126,10 +132,16 @@ func (r *HierarchicalResourceQuotaReconciler) syncWithForest(log logr.Logger, in
 	if isScopedHRQ {
 		log.Info("Marking HRQ as scoped", "name", inst.GetName(), "namespace", inst.GetNamespace())
 		r.Forest.MarkScopedRQ(nn)
-		rqName = utils.ScopedRQName(inst.GetName())
+		rqName, err = utils.ScopedRQName(inst.GetNamespace(), inst.GetName())
+		if err != nil {
+			return false, false, err
+		}
 	} else if r.Forest.IsMarkedAsScopedHRQ(nn) {
 		log.Info("Detect the Scoped HRQ because of the mark")
-		rqName = utils.ScopedRQName(inst.GetName())
+		rqName, err = utils.ScopedRQName(inst.GetNamespace(), inst.GetName())
+		if err != nil {
+			return false, false, err
+		}
 	} else {
 		log.Info("HRQ is a singleton")
 	}

internal/hrq/hrqreconciler_test.go

@@ -150,7 +150,11 @@ var _ = Describe("HRQ reconciler tests", func() {
 
 		// Scoped HRQs don't affect the result of TestCheckHRQDrift.
 		forestOverrideSubtreeUsages("hrq-selector", "cpu", "3")
-		updateRQUsage(ctx, fooName, utils.ScopedRQName("hrq-selector"), "cpu", "5")
+
+		scopedRQ, err := utils.ScopedRQName(fooName, "hrq-selector")
+		Expect(err).NotTo(HaveOccurred())
+		updateRQUsage(ctx, fooName, scopedRQ, "cpu", "5")
+
 		drift, err = TestCheckHRQDrift()
 		Expect(err).NotTo(HaveOccurred())
 		Expect(drift).Should(BeFalse())

internal/hrq/rqreconciler.go

@@ -83,6 +83,19 @@ func (r *ResourceQuotaReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 	}
 
 	isSingleton := utils.IsSingletonRQ(inst)
+	isLegacyScoped := utils.IsLegacyScopedRQ(inst)
+
+	if isLegacyScoped {
+		// Ignore legacy scoped RQs
+		// It will be deleted in the reconcile loop for the new RQ.
+		return ctrl.Result{}, nil
+	}
+
+	if !notFound && !isSingleton { // scoped RQ exists
+		if err := r.deleteLegacyScopedRQ(ctx, log, inst); err != nil {
+			return ctrl.Result{}, err
+		}
+	}
 
 	r.Forest.Lock()
 	ns := r.Forest.Get(inst.ObjectMeta.Namespace)
@@ -100,43 +113,31 @@ func (r *ResourceQuotaReconciler) Reconcile(ctx context.Context, req ctrl.Reques
 
 	log.Info("Reconciling ResourceQuota", "name", fmt.Sprintf("%s/%s", inst.GetNamespace(), inst.GetName()), "limits", inst.Spec.Hard, "usages", inst.Status.Used, "updated", updated)
 
+	var hrq *api.HierarchicalResourceQuota
+
 	// Delete the obsolete singleton and early exit if the new limits are empty.
 	if inst.Spec.Hard == nil {
 		return ctrl.Result{}, r.deleteRQ(ctx, log, inst)
 	} else if !isSingleton && notFound {
-		hrq := &api.HierarchicalResourceQuota{}
-		hrqName, err := utils.ScopedHRQNameFromHRQName(inst.Name)
+		hrqnnm, err := r.findHRQNameForRQ(inst)
 		if err != nil {
-			return ctrl.Result{}, fmt.Errorf("while getting hrq name: %w", err)
+			return ctrl.Result{}, fmt.Errorf("while finding hrq name for new RQ: %w", err)
 		}
 
-		cursorNm := ns
-		var found bool
-		for {
-			if cursorNm == nil {
-				break
-			}
-
-			hrqnnm := types.NamespacedName{Namespace: cursorNm.Name(), Name: hrqName}
-			err := r.Get(ctx, hrqnnm, hrq)
-			if err == nil {
-				found = true
-				break
-			}
+		hrq = &api.HierarchicalResourceQuota{}
+		err = r.Get(ctx, hrqnnm, hrq)
+		if err != nil {
 			if apierrors.IsNotFound(err) {
-				cursorNm = cursorNm.Parent()
-				continue
+				return ctrl.Result{}, fmt.Errorf("the parent hrq not found: %s", hrqnnm)
+			} else {
+				return ctrl.Result{}, fmt.Errorf("getting hrq %s: %w", hrqnnm, err)
 			}
-
-			return ctrl.Result{}, fmt.Errorf("while getting hrq: %w", err)
-		}
-		if !found {
-			return ctrl.Result{}, fmt.Errorf("the parent hrq not found: %s", hrqName)
 		}
 
 		log.Info("Found the parent HRQ", "namespace", hrq.Namespace, "name", hrq.Name)
 
 		inst.Spec.ScopeSelector = hrq.Spec.ScopeSelector
+		utils.SetLabelsAnnotationsForScopedRQ(inst, hrq.Namespace, hrq.Name)
 	}
 
 	// We only need to write back to the apiserver if the spec has changed
@@ -229,6 +230,38 @@ func (r *ResourceQuotaReconciler) getAncestorHRQs(inst *v1.ResourceQuota) []type
 	return names
 }
 
+// findHRQNameForRQ finds HRQ name and namespace for a new RQ by searching through namespace and ancestor HRQs
+func (r *ResourceQuotaReconciler) findHRQNameForRQ(inst *v1.ResourceQuota) (types.NamespacedName, error) {
+	hrqnnm, err := utils.ScopedHRQNameFromRQ(inst)
+	if err == nil {
+		return hrqnnm, nil
+	}
+
+	// New RQs that don't have the labels yet, so we need to search through the forest
+	r.Forest.Lock()
+	defer r.Forest.Unlock()
+
+	ns := r.Forest.Get(inst.Namespace)
+
+	// Check current namespace and all ancestors
+	namespaces := []string{inst.Namespace}
+	namespaces = append(namespaces, ns.AncestryNames()...)
+
+	for _, nsnm := range namespaces {
+		ancestorNS := r.Forest.Get(nsnm)
+		for _, hrqName := range ancestorNS.HRQNames() {
+			expectedRQName, err := utils.ScopedRQName(nsnm, hrqName)
+			if err != nil {
+				continue
+			}
+			if expectedRQName == inst.Name {
+				return types.NamespacedName{Namespace: nsnm, Name: hrqName}, nil
+			}
+		}
+	}
+	return types.NamespacedName{}, fmt.Errorf("no matching HRQ found for RQ: %s", inst.Name)
+}
+
 // deleteRQ deletes a resource quota on the apiserver and a quota in on-memory if it exists. Otherwise,
 // do nothing.
 func (r *ResourceQuotaReconciler) deleteRQ(ctx context.Context, log logr.Logger, inst *v1.ResourceQuota) error {
@@ -313,6 +346,28 @@ func (r *ResourceQuotaReconciler) syncResourceLimits(ns *forest.Namespace, inst
 	return true
 }
 
+// deleteLegacyScopedRQ deletes the legacy scoped RQ if it exists.
+func (r *ResourceQuotaReconciler) deleteLegacyScopedRQ(ctx context.Context, log logr.Logger, newRQ *v1.ResourceQuota) error {
+	hrqnnm, err := utils.ScopedHRQNameFromRQ(newRQ)
+	if err != nil {
+		return fmt.Errorf("get HRQ name from RQ: %w", err)
+	}
+
+	legacyRQName := utils.LegacyScopedRQName(hrqnnm.Name)
+
+	legacyRQ, err := r.getRQ(ctx, newRQ.Namespace, legacyRQName)
+	if err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		} else {
+			return err
+		}
+	}
+
+	log.Info("Deleting legacy scoped RQ", "legacyRQ", legacyRQ.Name, "namespace", legacyRQ.Namespace)
+	return r.deleteRQ(ctx, log, legacyRQ)
+}
+
 // OnChangeNamespace enqueues the singleton in a specific namespace to trigger the reconciliation of
 // the singleton 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/rqreconciler_test.go

@@ -68,7 +68,8 @@ var _ = Describe("RQ reconciler tests", func() {
 		setHRQ(ctx, barHRQName, barName, nil, "secrets", "100", "cpu", "50")
 		setHRQ(ctx, bazHRQName, bazName, nil, "pods", "1")
 		hrqWithSelectorName := "hrq-with-selector"
-		rqName := fmt.Sprintf("%s-%s", api.ResourceQuotaSingletonName, hrqWithSelectorName)
+		rqName, err := utils.ScopedRQName(fooName, hrqWithSelectorName)
+		Expect(err).NotTo(HaveOccurred())
 		setHRQ(ctx, hrqWithSelectorName, fooName, &highPrioritySelector, "cpu", "4", "pods", "2")
 
 		Eventually(getRQHard(ctx, fooName, api.ResourceQuotaSingletonName)).Should(equalRL("secrets", "6", "pods", "3"))
@@ -89,7 +90,8 @@ var _ = Describe("RQ reconciler tests", func() {
 		setHRQ(ctx, bazHRQName, bazName, nil, "pods", "1")
 
 		hrqWithSelectorName := "hrq-with-selector"
-		rqName := fmt.Sprintf("%s-%s", api.ResourceQuotaSingletonName, hrqWithSelectorName)
+		rqName, err := utils.ScopedRQName(fooName, hrqWithSelectorName)
+		Expect(err).NotTo(HaveOccurred())
 		setHRQ(ctx, hrqWithSelectorName, fooName, &highPrioritySelector, "cpu", "6", "pods", "3")
 		setHRQ(ctx, hrqWithSelectorName, barName, &highPrioritySelector, "cpu", "100", "pods", "50")
 		setHRQ(ctx, hrqWithSelectorName, bazName, &highPrioritySelector, "pods", "1")

internal/hrq/utils/scopedhrq.go

@@ -1,12 +1,21 @@
 package utils
 
 import (
+	"crypto/md5"
+	"encoding/hex"
 	"fmt"
 	"strings"
 
 	v1 "k8s.io/api/core/v1"
-
+	"k8s.io/apimachinery/pkg/types"
+	"k8s.io/apimachinery/pkg/util/validation"
 	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
+	"sigs.k8s.io/hierarchical-namespaces/internal/metadata"
+)
+
+const (
+	hrqNameLabel      = "hnc.x-k8s.io/hrq-name"
+	hrqNamespaceLabel = "hnc.x-k8s.io/hrq-namespace"
 )
 
 func IsSingletonRQ(rq *v1.ResourceQuota) bool {
@@ -17,15 +26,77 @@ func IsScopedRQ(rq *v1.ResourceQuota) bool {
 	return !IsSingletonRQ(rq)
 }
 
-func ScopedRQName(hrqName string) string {
+// IsHNCManagedRQ checks if an RQ has the HNC cleanup label
+func IsHNCManagedRQ(rq *v1.ResourceQuota) bool {
+	if label, ok := metadata.GetLabel(rq, api.HRQLabelCleanup); ok {
+		return label == "true"
+	}
+	return false
+}
+
+// IsLegacyScopedRQ checks if an RQ is a legacy scoped RQ (old format without namespace)
+// and is managed by HNC
+func IsLegacyScopedRQ(rq *v1.ResourceQuota) bool {
+	if IsSingletonRQ(rq) {
+		return false
+	}
+	// Only consider RQs managed by HNC for migration
+	if !IsHNCManagedRQ(rq) {
+		return false
+	}
+	_, nsLabelFound := metadata.GetLabel(rq, hrqNamespaceLabel)
+	_, nameLabelFound := metadata.GetLabel(rq, hrqNameLabel)
+	return !nsLabelFound || !nameLabelFound
+}
+
+// LegacyScopedRQName generates the legacy RQ name for backward compatibility
+func LegacyScopedRQName(hrqName string) string {
 	return api.ResourceQuotaSingletonName + "-" + hrqName
 }
 
-func ScopedHRQNameFromHRQName(rqName string) (string, error) {
+// HRQNameFromLegacyRQName extracts HRQ name from legacy RQ name
+func HRQNameFromLegacyRQName(rqName string) (string, error) {
+	if rqName == api.ResourceQuotaSingletonName {
+		return "", fmt.Errorf("invalid legacy RQ name: %s", rqName)
+	}
+
 	hrqName := strings.TrimPrefix(rqName, api.ResourceQuotaSingletonName+"-")
-	if hrqName == api.ResourceQuotaSingletonName {
-		return "", fmt.Errorf("invalid ScopedHRQ name: %s", hrqName)
+	if hrqName == rqName {
+		return "", fmt.Errorf("not a legacy scoped RQ name: %s", rqName)
 	}
 
 	return hrqName, nil
 }
+
+func ScopedRQName(hrqNamespace string, hrqName string) (string, error) {
+	hash := md5.Sum([]byte(fmt.Sprintf("%s/%s", hrqNamespace, hrqName)))
+	hashStr := hex.EncodeToString(hash[:])
+
+	namespaceAndName := truncate(
+		fmt.Sprintf("%s-%s", hrqNamespace, hrqName),
+		uint(validation.DNS1123SubdomainMaxLength-len(hashStr)-len(api.ResourceQuotaSingletonName)-2),
+	)
+
+	return fmt.Sprintf("%s-%s-%s", api.ResourceQuotaSingletonName, namespaceAndName, hashStr), nil
+}
+
+func ScopedHRQNameFromRQ(rq *v1.ResourceQuota) (types.NamespacedName, error) {
+	namespace, nsOK := metadata.GetLabel(rq, hrqNamespaceLabel)
+	name, nameOK := metadata.GetLabel(rq, hrqNameLabel)
+	if nsOK && nameOK {
+		return types.NamespacedName{Namespace: namespace, Name: name}, nil
+	}
+	return types.NamespacedName{}, fmt.Errorf("no matching HRQ found for RQ: %s", rq.Name)
+}
+
+func SetLabelsAnnotationsForScopedRQ(rq *v1.ResourceQuota, hrqNamespace string, hrqName string) {
+	metadata.SetLabel(rq, hrqNamespaceLabel, hrqNamespace)
+	metadata.SetLabel(rq, hrqNameLabel, hrqName)
+}
+
+func truncate(s string, n uint) string {
+	if uint(len(s)) <= n {
+		return s
+	}
+	return s[:n]
+}

internal/hrq/utils/scopedhrq_test.go

@@ -0,0 +1,37 @@
+package utils
+
+import "testing"
+
+func TestScopedRQName(t *testing.T) {
+	tests := []struct {
+		name         string
+		hrqNamespace string
+		hrqName      string
+		want         string
+	}{
+		{
+			name:         "short",
+			hrqNamespace: "default",
+			hrqName:      "test",
+			want:         "hrq.hnc.x-k8s.io-default-test-1b5cb9615ea99c0edaf5b1f157ce3997",
+		},
+		{
+			name:         "too_long",
+			hrqNamespace: "default",
+			hrqName:      "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+			want:         "hrq.hnc.x-k8s.io-default-123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345-dc2d54f49ea75bc9da92c7272bc626d7", // 253 chars
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			got, err := ScopedRQName(test.hrqNamespace, test.hrqName)
+			if err != nil {
+				t.Errorf("ScopedRQName(%s, %s) = %v", test.hrqNamespace, test.hrqName, err)
+			}
+			if got != test.want {
+				t.Errorf("ScopedRQName(%s, %s) = %s, want %s", test.hrqNamespace, test.hrqName, got, test.want)
+			}
+		})
+	}
+}