diff --git a/internal/webhook/globalresourcequota/calculation_test.go b/internal/webhook/globalresourcequota/calculation_test.go index a209dc81..785b9365 100644 --- a/internal/webhook/globalresourcequota/calculation_test.go +++ b/internal/webhook/globalresourcequota/calculation_test.go @@ -24,6 +24,7 @@ import ( capsulev1beta2 "github.com/projectcapsule/capsule/api/v1beta2" "github.com/projectcapsule/capsule/pkg/api/meta" + "github.com/projectcapsule/capsule/pkg/runtime/configuration" "github.com/projectcapsule/capsule/pkg/runtime/selectors" ) @@ -269,8 +270,34 @@ func TestValidateHardLimitAgainstAllocatedUsage(t *testing.T) { }, allocated); err == nil { t.Fatal("hard limit below allocated usage was accepted") } - if err := validateHardLimit(corev1.ResourceList{}, allocated); err == nil { - t.Fatal("allocated resource was removed from hard limit") + if err := validateHardLimit(corev1.ResourceList{}, allocated); err != nil { + t.Fatalf("allocated resource removal was rejected: %v", err) + } +} + +func TestGlobalResourceQuotaAllowsRemovingAllocatedResource(t *testing.T) { + t.Parallel() + + oldQuota := globalQuotaForTest("shared", corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("8"), + corev1.ResourceRequestsMemory: resource.MustParse("8Gi"), + }) + oldQuota.Status.Total.Used = corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("4"), + } + ledger := initializedLedger(types.NamespacedName{ + Namespace: configuration.ControllerNamespace(), + Name: oldQuota.GetLedgerName(), + }, oldQuota, corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("6"), + }) + + newQuota := oldQuota.DeepCopy() + delete(newQuota.Spec.Quota.Hard, corev1.ResourceLimitsCPU) + + request := globalResourceQuotaUpdateRequest(t, oldQuota, newQuota) + if response := validateGlobalResourceQuotaRequest(context.Background(), ledgerClient(t, ledger), request); response != nil { + t.Fatalf("allocated resource removal was rejected: %#v", response) } } diff --git a/internal/webhook/tenant/validation/rule_quota_test.go b/internal/webhook/tenant/validation/rule_quota_test.go index cc8bb941..bdb48b02 100644 --- a/internal/webhook/tenant/validation/rule_quota_test.go +++ b/internal/webhook/tenant/validation/rule_quota_test.go @@ -73,6 +73,24 @@ func TestValidateRuleQuotaUpdates(t *testing.T) { } }) + t.Run("removal with allocation", func(t *testing.T) { + old := oldTenant.DeepCopy() + old.Spec.Rules[0].Quota[0].Hard[corev1.ResourceRequestsCPU] = resource.MustParse("1") + quota := tenantutils.RuleGlobalResourceQuota(old, 0, 0) + quota.UID = types.UID("removal-global-quota-uid") + quota.Status.Total.Used = corev1.ResourceList{ + corev1.ResourceServices: resource.MustParse("2"), + } + ledger := ruleQuotaLedger(quota, "4") + reader := fake.NewClientBuilder().WithScheme(scheme).WithObjects(quota, ledger).Build() + + updated := old.DeepCopy() + delete(updated.Spec.Rules[0].Quota[0].Hard, corev1.ResourceServices) + if response := validateRuleQuotaUpdates(context.Background(), reader, updated, old); response != nil { + t.Fatalf("allocated rule quota resource removal was rejected: %#v", response) + } + }) + t.Run("scope change with decrease", func(t *testing.T) { oldScoped := ruleQuotaTenant("8") oldScoped.Spec.Rules[0].NamespaceSelector = &metav1.LabelSelector{ diff --git a/pkg/runtime/quota/validation.go b/pkg/runtime/quota/validation.go index 8245ce8a..5bc92106 100644 --- a/pkg/runtime/quota/validation.go +++ b/pkg/runtime/quota/validation.go @@ -9,9 +9,10 @@ import ( corev1 "k8s.io/api/core/v1" ) -// ValidateHardLimit rejects removal or reduction of a hard resource below an -// already allocated quantity. Path identifies the hard-limit field in the -// returned validation error. +// ValidateHardLimit rejects an explicitly configured hard resource below an +// already allocated quantity. Resources omitted from hard are no longer +// governed by the quota and are therefore ignored. Path identifies the +// hard-limit field in the returned validation error. func ValidateHardLimit(path string, hard, allocated corev1.ResourceList) error { for name, usage := range allocated { if usage.Sign() <= 0 { @@ -20,12 +21,7 @@ func ValidateHardLimit(path string, hard, allocated corev1.ResourceList) error { limit, exists := hard[name] if !exists { - return fmt.Errorf( - "%s[%q] cannot be removed while %s is allocated", - path, - name, - usage.String(), - ) + continue } if limit.Cmp(usage) < 0 { @@ -42,10 +38,11 @@ func ValidateHardLimit(path string, hard, allocated corev1.ResourceList) error { return nil } -// ValidateHardLimitScopeChange prevents a quota from reducing or removing a -// hard limit in the same update that changes its namespace selection. Usage -// from newly selected namespaces is not represented by the quota's previous -// status yet, so the scope must reconcile before a safe lower bound is known. +// ValidateHardLimitScopeChange prevents a quota from reducing a retained hard +// limit in the same update that changes its namespace selection. An omitted +// resource is no longer governed by the quota and is ignored. Usage from newly +// selected namespaces is not represented by the quota's previous status yet, +// so the scope must reconcile before a safe lower bound is known. func ValidateHardLimitScopeChange( path string, hard corev1.ResourceList, @@ -59,11 +56,7 @@ func ValidateHardLimitScopeChange( for name, previousLimit := range previous { limit, exists := hard[name] if !exists { - return fmt.Errorf( - "%s[%q] cannot be removed while namespace selectors are changing; update the selectors first and wait for usage reconciliation", - path, - name, - ) + continue } if limit.Cmp(previousLimit) < 0 { diff --git a/pkg/runtime/quota/validation_test.go b/pkg/runtime/quota/validation_test.go index 25662882..be847733 100644 --- a/pkg/runtime/quota/validation_test.go +++ b/pkg/runtime/quota/validation_test.go @@ -11,6 +11,57 @@ import ( "k8s.io/apimachinery/pkg/api/resource" ) +func TestValidateHardLimit(t *testing.T) { + t.Parallel() + + allocated := corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("4"), + } + + tests := []struct { + name string + hard corev1.ResourceList + wantErr string + }{ + { + name: "rejects configured limit below allocation", + hard: corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("3"), + }, + wantErr: "cannot be reduced to 3 while 4 is allocated", + }, + { + name: "allows configured limit equal to allocation", + hard: corev1.ResourceList{ + corev1.ResourceLimitsCPU: resource.MustParse("4"), + }, + }, + { + name: "allows removal of allocated resource", + hard: corev1.ResourceList{}, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + err := ValidateHardLimit("spec.quota.hard", test.hard, allocated) + if test.wantErr == "" { + if err != nil { + t.Fatalf("ValidateHardLimit() error = %v", err) + } + + return + } + + if err == nil || !strings.Contains(err.Error(), test.wantErr) { + t.Fatalf("ValidateHardLimit() error = %v, want containing %q", err, test.wantErr) + } + }) + } +} + func TestValidateHardLimitScopeChange(t *testing.T) { t.Parallel() @@ -33,10 +84,9 @@ func TestValidateHardLimitScopeChange(t *testing.T) { wantErr: "cannot be reduced from 8 to 0 while namespace selectors are changing", }, { - name: "rejects removal while scope changes", + name: "allows removal while scope changes", hard: corev1.ResourceList{}, scopeChanged: true, - wantErr: "cannot be removed while namespace selectors are changing", }, { name: "allows equal limit while scope changes",