diff --git a/internal/hrq/hrqreconciler.go b/internal/hrq/hrqreconciler.go index a63baa6be..1a46024d8 100644 --- a/internal/hrq/hrqreconciler.go +++ b/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") } diff --git a/internal/hrq/hrqreconciler_test.go b/internal/hrq/hrqreconciler_test.go index 854fec6e7..b04b2f7ed 100644 --- a/internal/hrq/hrqreconciler_test.go +++ b/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()) diff --git a/internal/hrq/rqreconciler.go b/internal/hrq/rqreconciler.go index 4b7fdc5a5..ef567632c 100644 --- a/internal/hrq/rqreconciler.go +++ b/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. diff --git a/internal/hrq/rqreconciler_test.go b/internal/hrq/rqreconciler_test.go index 7888675aa..8f2488e6e 100644 --- a/internal/hrq/rqreconciler_test.go +++ b/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") diff --git a/internal/hrq/utils/scopedhrq.go b/internal/hrq/utils/scopedhrq.go index 862bd508c..b7e3946e1 100644 --- a/internal/hrq/utils/scopedhrq.go +++ b/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] +} diff --git a/internal/hrq/utils/scopedhrq_test.go b/internal/hrq/utils/scopedhrq_test.go new file mode 100644 index 000000000..9ced26b96 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/test/e2e/pfn_hrq_test.go b/test/e2e/pfn_hrq_test.go index 24c84e145..677ca2aae 100644 --- a/test/e2e/pfn_hrq_test.go +++ b/test/e2e/pfn_hrq_test.go @@ -1,6 +1,8 @@ package e2e import ( + "crypto/md5" + "encoding/hex" "fmt" "strconv" "strings" @@ -10,6 +12,7 @@ import ( "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" + "sigs.k8s.io/hierarchical-namespaces/internal/hrq/utils" "sigs.k8s.io/yaml" "github.com/google/uuid" @@ -58,23 +61,25 @@ var _ = Describe("Scoped Hierarchical Resource Quota", Label("pfnet"), func() { }) It("should create RQs with correct limits in the descendants (including itself) for Scoped HRQs", func() { - hrqA := setScopedHRQ("a-hrq", parentNs, corev1.ResourceList{corev1.ResourcePods: resource.MustParse("3")}, &scopeSelector) - hrqB := setScopedHRQ("b-hrq", childNs, corev1.ResourceList{corev1.ResourcePods: resource.MustParse("2")}, &scopeSelector) + hrqA := setScopedHRQ("same-name-hrq", parentNs, corev1.ResourceList{corev1.ResourcePods: resource.MustParse("3")}, &scopeSelector) + hrqB := setScopedHRQ("same-name-hrq", childNs, corev1.ResourceList{corev1.ResourcePods: resource.MustParse("2")}, &scopeSelector) - rqAName := api.ResourceQuotaSingletonName + "-" + hrqA.Name - rqBName := api.ResourceQuotaSingletonName + "-" + hrqB.Name + rqAName := api.ResourceQuotaSingletonName + "-" + parentNs + "-" + hrqA.Name + "-" + md5Hash(parentNs+"/"+hrqA.Name) + rqBName := api.ResourceQuotaSingletonName + "-" + childNs + "-" + hrqB.Name + "-" + md5Hash(childNs+"/"+hrqB.Name) FieldShouldContain("resourcequota", parentNs, rqAName, ".spec.hard", "pods:3") + FieldShouldContain("resourcequota", childNs, rqAName, ".spec.hard", "pods:3") FieldShouldContain("resourcequota", childNs, rqBName, ".spec.hard", "pods:2") expect := selectorStr(priorityName) FieldShouldContain("resourcequota", parentNs, rqAName, ".spec.scopeSelector", expect) + FieldShouldContain("resourcequota", childNs, rqAName, ".spec.scopeSelector", expect) FieldShouldContain("resourcequota", childNs, rqBName, ".spec.scopeSelector", expect) }) It("should remove obsolete (empty) RQ if there's no longer a Scoped HRQ in the ancestor", func() { hrq := setScopedHRQ("a-hrq", parentNs, corev1.ResourceList{corev1.ResourcePods: resource.MustParse("3")}, &scopeSelector) - rqName := api.ResourceQuotaSingletonName + "-" + hrq.Name + rqName := api.ResourceQuotaSingletonName + "-" + parentNs + "-" + hrq.Name + "-" + md5Hash(parentNs+"/"+hrq.Name) MustRun("kubectl delete hrq -n", parentNs, hrq.Name) @@ -110,6 +115,24 @@ var _ = Describe("Scoped Hierarchical Resource Quota", Label("pfnet"), func() { FieldShouldContain("hrq", parentNs, hrq.Name, ".status.used", "pods:1") }) + + It("should remove the legacy RQ", func() { + hrqName := "legacy-hrq" + + // Legacy RQ remains before the new RQ is created + legacyRQ := mustCreateLegacyRQ(parentNs, hrqName, corev1.ResourceList{corev1.ResourcePods: resource.MustParse("1")}) + RunShouldContain(legacyRQ.Name, propagationTime, "kubectl get resourcequota -n", parentNs) + + // Create the HRQ + setScopedHRQ(hrqName, parentNs, corev1.ResourceList{corev1.ResourcePods: resource.MustParse("1")}, &scopeSelector) + + // Confirm the new RQ is created + newRQName := api.ResourceQuotaSingletonName + "-" + parentNs + "-" + hrqName + "-" + md5Hash(parentNs+"/"+hrqName) + RunShouldContain(newRQName, propagationTime, "kubectl get resourcequota -n", parentNs, newRQName) + + // Legacy RQ is removed + RunShouldNotContain(legacyRQ.Name, propagationTime, "kubectl get resourcequota -n", parentNs) + }) }) func mustCreatePod(prefix, nsnm string) (corev1.Pod, error) { @@ -213,6 +236,32 @@ func createSubNS(parent, prefix string) string { return nsName } +func mustCreateLegacyRQ(ns, hrqName string, resourceList corev1.ResourceList) corev1.ResourceQuota { + hrq := corev1.ResourceQuota{ + TypeMeta: metav1.TypeMeta{ + Kind: "ResourceQuota", + APIVersion: "v1", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: utils.LegacyScopedRQName(hrqName), + Namespace: ns, + Labels: map[string]string{ + api.HRQLabelCleanup: "true", + }, + }, + Spec: corev1.ResourceQuotaSpec{ + Hard: resourceList, + }, + } + manifest, err := yaml.Marshal(hrq) + Expect(err).NotTo(HaveOccurred()) + + MustApplyYAML(string(manifest)) + RunShouldContain(hrq.Name, propagationTime, "kubectl get resourcequota -n", ns, hrq.Name) + + return hrq +} + func genPriorityScopeSelector() (corev1.ScopeSelector, string, func()) { priority := uuid.New().String() err := TryRun("kubectl create priorityclass", priority, "--value=100") @@ -236,3 +285,8 @@ func genPriorityScopeSelector() (corev1.ScopeSelector, string, func()) { func selectorStr(priorityName string) string { return "map[matchExpressions:[map[operator:In scopeName:PriorityClass values:[" + priorityName + "]]]]" } + +func md5Hash(s string) string { + hash := md5.Sum([]byte(s)) + return hex.EncodeToString(hash[:]) +}