diff --git a/pkg/features/features.go b/pkg/features/features.go index 312507699..9fa7b2380 100644 --- a/pkg/features/features.go +++ b/pkg/features/features.go @@ -55,6 +55,8 @@ const ( ElasticQuotaIgnorePodOverhead featuregate.Feature = "ElasticQuotaIgnorePodOverhead" // ElasticQuotaGuaranteeUsage enable guarantee the quota usage + // In some specific scenarios, resources that have been allocated to users are considered + // to belong to the users and will not be preempted back. ElasticQuotaGuaranteeUsage featuregate.Feature = "ElasticQuotaGuaranteeUsage" // DisableDefaultQuota disable default quota. diff --git a/pkg/util/resource.go b/pkg/util/resource.go index 77aa5f5a2..f58e38175 100644 --- a/pkg/util/resource.go +++ b/pkg/util/resource.go @@ -257,8 +257,8 @@ func IsZoneListResourceEqual(a, b v1alpha1.ZoneList, resourceNames ...string) bo return true } -// LessThanOrEqualEnhanced is different quotav1.LessThanOrEqual. It will compare non-exist value in b -func LessThanOrEqualEnhanced(a corev1.ResourceList, b corev1.ResourceList) bool { +// LessThanOrEqualCompletely is different quotav1.LessThanOrEqual. It will compare non-exist value in b +func LessThanOrEqualCompletely(a corev1.ResourceList, b corev1.ResourceList) bool { result := true delta := quotav1.Subtract(a, b) for _, value := range delta { diff --git a/pkg/util/resource_test.go b/pkg/util/resource_test.go index 4a629c5da..6ceba4c01 100644 --- a/pkg/util/resource_test.go +++ b/pkg/util/resource_test.go @@ -1142,7 +1142,7 @@ func TestLessThanOrEqualEnhanced(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.expect, LessThanOrEqualEnhanced(tt.a, tt.b)) + assert.Equal(t, tt.expect, LessThanOrEqualCompletely(tt.a, tt.b)) }) } } diff --git a/pkg/webhook/elasticquota/pod_check.go b/pkg/webhook/elasticquota/pod_check.go index 8597bf1ac..d91336c79 100644 --- a/pkg/webhook/elasticquota/pod_check.go +++ b/pkg/webhook/elasticquota/pod_check.go @@ -96,38 +96,38 @@ func GetQuotaName(pod *corev1.Pod, kubeClient client.Client) string { return extension.DefaultQuotaName } -func ListQuotaBoundPods(kubeClient client.Client, quota *v1alpha1.ElasticQuota) (*corev1.PodList, error) { +// hasQuotaBoundedPods returns true if the quota has bounded pods. +func hasQuotaBoundedPods(kubeClient client.Client, quotaName string, namespaces []string) (bool, error) { podList := &corev1.PodList{} if err := kubeClient.List(context.TODO(), podList, &client.ListOptions{ - FieldSelector: fields.OneTermEqualSelector("label.quotaName", quota.Name), + FieldSelector: fields.OneTermEqualSelector("label.quotaName", quotaName), }, utilclient.DisableDeepCopy); err != nil { - return nil, err + return false, err } if len(podList.Items) > 0 { - return podList, nil + return true, nil } if err := kubeClient.List(context.TODO(), podList, &client.ListOptions{ - Namespace: quota.Name, + Namespace: quotaName, }, utilclient.DisableDeepCopy); err != nil { - return nil, err + return false, err } if len(podList.Items) > 0 { - return podList, nil + return true, nil } - namespaces := extension.GetAnnotationQuotaNamespaces(quota) for _, namespace := range namespaces { if err := kubeClient.List(context.TODO(), podList, &client.ListOptions{ Namespace: namespace, }, utilclient.DisableDeepCopy); err != nil { - return nil, err + return false, err } if len(podList.Items) > 0 { - return podList, nil + return true, nil } } - return nil, nil + return false, nil } diff --git a/pkg/webhook/elasticquota/quota_topology.go b/pkg/webhook/elasticquota/quota_topology.go index 098958d71..6e301b666 100644 --- a/pkg/webhook/elasticquota/quota_topology.go +++ b/pkg/webhook/elasticquota/quota_topology.go @@ -129,8 +129,9 @@ func (qt *quotaTopology) ValidUpdateQuota(oldQuota, newQuota *v1alpha1.ElasticQu return err } + oldAnnotationNamespaces := extension.GetAnnotationQuotaNamespaces(oldQuota) newQuotaInfo := NewQuotaInfoFromQuota(newQuota) - if err := qt.validateQuotaTopology(oldQuotaInfo, newQuotaInfo, oldQuota); err != nil { + if err := qt.validateQuotaTopology(oldQuotaInfo, newQuotaInfo, oldAnnotationNamespaces); err != nil { return err } @@ -140,7 +141,6 @@ func (qt *quotaTopology) ValidUpdateQuota(oldQuota, newQuota *v1alpha1.ElasticQu qt.quotaHierarchyInfo[newQuotaInfo.ParentName][newQuotaInfo.Name] = struct{}{} } - oldAnnotationNamespaces := extension.GetAnnotationQuotaNamespaces(oldQuota) for _, namespace := range oldAnnotationNamespaces { delete(qt.namespaceToQuotaMap, namespace) } diff --git a/pkg/webhook/elasticquota/quota_topology_check.go b/pkg/webhook/elasticquota/quota_topology_check.go index 40cea88ba..b7c18ed60 100644 --- a/pkg/webhook/elasticquota/quota_topology_check.go +++ b/pkg/webhook/elasticquota/quota_topology_check.go @@ -66,36 +66,37 @@ func (qt *quotaTopology) validateQuotaSelfItem(quota *v1alpha1.ElasticQuota) err return nil } -// validateQuotaTopology checks the quotaInfo's topology with its parent and its children (when update calls the function). -func (qt *quotaTopology) validateQuotaTopology(oldQuotaInfo, quotaInfo *QuotaInfo, oldQuota *v1alpha1.ElasticQuota) error { - if err := qt.checkIsParentChange(oldQuotaInfo, quotaInfo, oldQuota); err != nil { +// validateQuotaTopology checks the quotaInfo's topology with its parent and its children. +// oldQuotaInfo is null wben validate a new create request, and is the current quotaInfo when validate a update request. +func (qt *quotaTopology) validateQuotaTopology(oldQuotaInfo, newQuotaInfo *QuotaInfo, oldNamespaces []string) error { + if err := qt.checkIsParentChange(oldQuotaInfo, newQuotaInfo, oldNamespaces); err != nil { return err } - if err := qt.checkTreeID(oldQuotaInfo, quotaInfo); err != nil { + if err := qt.checkTreeID(oldQuotaInfo, newQuotaInfo); err != nil { return err } // if the quotaInfo's parent is root and its IsParent is false, the following checks will be true, just return nil. - if quotaInfo.ParentName == extension.RootQuotaName && !quotaInfo.IsParent { + if newQuotaInfo.ParentName == extension.RootQuotaName && !newQuotaInfo.IsParent { return nil } - if err := qt.checkParentQuotaInfo(quotaInfo.Name, quotaInfo.ParentName); err != nil { + if err := qt.checkParentQuotaInfo(newQuotaInfo.Name, newQuotaInfo.ParentName); err != nil { return err } - if err := qt.checkSubAndParentGroupMaxQuotaKeySame(quotaInfo); err != nil { + if err := qt.checkSubAndParentGroupMaxQuotaKeySame(newQuotaInfo); err != nil { return err } - if err := qt.checkMinQuotaSum(quotaInfo); err != nil { + if err := qt.checkMinQuotaValidate(newQuotaInfo); err != nil { return err } if utilfeature.DefaultFeatureGate.Enabled(features.ElasticQuotaGuaranteeUsage) { - if err := qt.checkGuaranteedForMin(quotaInfo); err != nil { - return fmt.Errorf("%v %v", err.Error(), quotaInfo.Name) + if err := qt.checkGuaranteedForMin(newQuotaInfo); err != nil { + return fmt.Errorf("%v %v", err.Error(), newQuotaInfo.Name) } } @@ -134,7 +135,7 @@ func (qt *quotaTopology) checkTreeID(oldQuotaInfo, quotaInfo *QuotaInfo) error { return nil } -func (qt *quotaTopology) checkIsParentChange(oldQuotaInfo, quotaInfo *QuotaInfo, oldQuota *v1alpha1.ElasticQuota) error { +func (qt *quotaTopology) checkIsParentChange(oldQuotaInfo, quotaInfo *QuotaInfo, oldNamespaces []string) error { // means create quota, no need check if oldQuotaInfo == nil || oldQuotaInfo.IsParent == quotaInfo.IsParent { return nil @@ -145,11 +146,11 @@ func (qt *quotaTopology) checkIsParentChange(oldQuotaInfo, quotaInfo *QuotaInfo, } if quotaInfo.IsParent { - podList, err := ListQuotaBoundPods(qt.client, oldQuota) + hasPods, err := hasQuotaBoundedPods(qt.client, oldQuotaInfo.Name, oldNamespaces) if err != nil { return err } - if len(podList.Items) > 0 { + if hasPods { return fmt.Errorf("quota has bound pods, isParent is forbidden to modify as true, quotaName: %v", oldQuotaInfo.Name) } } @@ -157,6 +158,7 @@ func (qt *quotaTopology) checkIsParentChange(oldQuotaInfo, quotaInfo *QuotaInfo, return nil } +// checkParentQuotaInfo check parent exist func (qt *quotaTopology) checkParentQuotaInfo(quotaName, parentName string) error { if parentName != extension.RootQuotaName { parentInfo, find := qt.quotaInfoMap[parentName] @@ -197,39 +199,43 @@ func (qt *quotaTopology) checkSubAndParentGroupMaxQuotaKeySame(quotaInfo *QuotaI quotaInfo.Name, name) } } else { - return fmt.Errorf("BUG quotaInfoMap and quotaTree information out of sync, losed :%v", name) + return fmt.Errorf("internal error: quotaInfoMap and quotaTree information out of sync, losed :%v", name) } } return nil } -// checkMinQuotaSum parent's minQuota should be larger or equal than the sum of allChildren's minQuota. -func (qt *quotaTopology) checkMinQuotaSum(quotaInfo *QuotaInfo) error { - if quotaInfo.ParentName != extension.RootQuotaName { - childMinSumNotIncludeSelf, err := qt.getChildMinQuotaSumExceptSpecificChild(quotaInfo.ParentName, quotaInfo.Name) +// checkMinQuotaValidate will do two checks: +// 1. the sum of brothers' minquota should less than or equal to parentMinQuota. +// 2. the sum of children's minquota should less than or equal to newQuotaMin. +func (qt *quotaTopology) checkMinQuotaValidate(newQuotaInfo *QuotaInfo) error { + // check brothers' minquota sum + if newQuotaInfo.ParentName != extension.RootQuotaName { + childMinSumNotIncludeSelf, err := qt.getChildMinQuotaSumExceptSpecificChild(newQuotaInfo.ParentName, newQuotaInfo.Name) if err != nil { return fmt.Errorf("checkMinQuotaSum failed: %v", err) } - childMinSumIncludeSelf := quotav1.Add(childMinSumNotIncludeSelf, quotaInfo.CalculateInfo.Min) - if !util.LessThanOrEqualEnhanced(childMinSumIncludeSelf, qt.quotaInfoMap[quotaInfo.ParentName].CalculateInfo.Min) { - return fmt.Errorf("checkMinQuotaSum allChildren SumMinQuota > parentMinQuota, parent: %v", quotaInfo.ParentName) + childMinSumIncludeSelf := quotav1.Add(childMinSumNotIncludeSelf, newQuotaInfo.CalculateInfo.Min) + if !util.LessThanOrEqualCompletely(childMinSumIncludeSelf, qt.quotaInfoMap[newQuotaInfo.ParentName].CalculateInfo.Min) { + return fmt.Errorf("checkMinQuotaSum all brothers' MinQuota > parent MinQuota, parent: %v", newQuotaInfo.ParentName) } } - children, exist := qt.quotaHierarchyInfo[quotaInfo.Name] + // check children's minquota sum + children, exist := qt.quotaHierarchyInfo[newQuotaInfo.Name] if !exist || len(children) == 0 { return nil } - childMinSum, err := qt.getChildMinQuotaSumExceptSpecificChild(quotaInfo.Name, "") + childMinSum, err := qt.getChildMinQuotaSumExceptSpecificChild(newQuotaInfo.Name, "") if err != nil { return fmt.Errorf("checkMinQuotaSum failed:%v", err) } - if !util.LessThanOrEqualEnhanced(childMinSum, quotaInfo.CalculateInfo.Min) { - return fmt.Errorf("checkMinQuotaSum allChildrn SumMinQuota > MinQuota, parent: %v", quotaInfo.Name) + if !util.LessThanOrEqualCompletely(childMinSum, newQuotaInfo.CalculateInfo.Min) { + return fmt.Errorf("checkMinQuotaSum all children's MinQuota > current MinQuota, current: %v", newQuotaInfo.Name) } return nil @@ -241,12 +247,12 @@ func (qt *quotaTopology) getChildMinQuotaSumExceptSpecificChild(parentName, skip return allChildQuotaSum, nil } - childQuotaList, exist := qt.quotaHierarchyInfo[parentName] + children, exist := qt.quotaHierarchyInfo[parentName] if !exist { return nil, fmt.Errorf("not found child quota list, parent: %v", parentName) } - for childName := range childQuotaList { + for childName := range children { if childName == skipQuota { continue } @@ -334,8 +340,8 @@ func (qt *quotaTopology) checkGuaranteedForMin(quotaInfo *QuotaInfo) error { return nil } - // if the new min less than guaranteed. allow it. - if util.LessThanOrEqualEnhanced(quotaInfo.CalculateInfo.Min, quotaInfo.CalculateInfo.Guaranteed) { + // if the new min less than guaranteed, which means that no more resource guarantee is needed, so it is allowed directly. + if util.LessThanOrEqualCompletely(quotaInfo.CalculateInfo.Min, quotaInfo.CalculateInfo.Guaranteed) { return nil } newGuaranteed := quotav1.Max(quotaInfo.CalculateInfo.Min, quotaInfo.CalculateInfo.Guaranteed) @@ -343,6 +349,10 @@ func (qt *quotaTopology) checkGuaranteedForMin(quotaInfo *QuotaInfo) error { return qt.checkParentGuaranteed(newGuaranteed, quotaInfo.Name, quotaInfo.ParentName) } +// Guaranteed resources are searched starting from the parent node of the current node until the root node of the tree. +// We need to meet guaranteed resources at least at a certain level to guarantee the resources of the current node. +// During the search process, there may be situations where min is not used up at the intermediate nodes. +// Therefore, we need to recursively accumulate the guaranteed resources that need to be satisfied until the root node is reached. func (qt *quotaTopology) checkParentGuaranteed(newGuarantee v1.ResourceList, self, parentName string) error { if parentName == extension.RootQuotaName { return fmt.Errorf("tree root quota %v can't guarantee for min", self) @@ -353,26 +363,26 @@ func (qt *quotaTopology) checkParentGuaranteed(newGuarantee v1.ResourceList, sel return fmt.Errorf("parent %v not found", parentName) } - childQuotaList, ok := qt.quotaHierarchyInfo[parentName] + children, ok := qt.quotaHierarchyInfo[parentName] if !ok { return fmt.Errorf("child quota list not found, parent: %v", parentName) } - allChildGuaranteed := newGuarantee - for childName := range childQuotaList { + allChildrenGuaranteed := newGuarantee + for childName := range children { if childName == self { continue } if quotaInfo, exist := qt.quotaInfoMap[childName]; exist { - allChildGuaranteed = quotav1.Add(allChildGuaranteed, quotaInfo.CalculateInfo.Guaranteed) + allChildrenGuaranteed = quotav1.Add(allChildrenGuaranteed, quotaInfo.CalculateInfo.Guaranteed) } else { return fmt.Errorf("BUG quotaInfoMap and quotaTree information out of sync, losed :%v", childName) } } - newParentGuaranteed := quotav1.Max(parentInfo.CalculateInfo.Min, allChildGuaranteed) - if util.LessThanOrEqualEnhanced(newParentGuaranteed, parentInfo.CalculateInfo.Guaranteed) { + newParentGuaranteed := quotav1.Max(parentInfo.CalculateInfo.Min, allChildrenGuaranteed) + if util.LessThanOrEqualCompletely(newParentGuaranteed, parentInfo.CalculateInfo.Guaranteed) { return nil } diff --git a/pkg/webhook/elasticquota/quota_topology_test.go b/pkg/webhook/elasticquota/quota_topology_test.go index ff1ae3fd3..d231cce8d 100644 --- a/pkg/webhook/elasticquota/quota_topology_test.go +++ b/pkg/webhook/elasticquota/quota_topology_test.go @@ -329,6 +329,14 @@ func TestQuotaTopology_checkMinQuotaSum(t *testing.T) { Min(MakeResourceList().CPU(16).Mem(12800).Obj()).IsParent(false).Obj(), err: fmt.Errorf("error"), }, + { + name: "childQuotaInfo not satisfy", + quota: MakeQuota("temp").Max(MakeResourceList().CPU(120).Mem(1048576).Obj()). + Min(MakeResourceList().CPU(16).Mem(12800).Obj()).IsParent(false).Obj(), + subQuota: MakeQuota("sub-1").ParentName("temp").Max(MakeResourceList().CPU(120).Mem(12800).Obj()). + Min(MakeResourceList().CPU(16).Mem(12801).Obj()).IsParent(false).Obj(), + err: fmt.Errorf("error"), + }, { name: "satisfy", quota: MakeQuota("temp").Max(MakeResourceList().CPU(120).Mem(1048576).Obj()). @@ -363,7 +371,7 @@ func TestQuotaTopology_checkMinQuotaSum(t *testing.T) { if tt.eraseSub { delete(qt.quotaInfoMap, tt.subQuota.Name) } - err := qt.checkMinQuotaSum(quota) + err := qt.checkMinQuotaValidate(quota) if (tt.err != nil && err == nil) || (tt.err == nil && err != nil) { t.Errorf("error") }