You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "0yukali0 (via GitHub)" <gi...@apache.org> on 2023/05/25 05:40:12 UTC
[GitHub] [yunikorn-core] 0yukali0 commented on a diff in pull request #538: [YUNIKORN-1607] Configuration processing should support limit hierarc…
0yukali0 commented on code in PR #538:
URL: https://github.com/apache/yunikorn-core/pull/538#discussion_r1205008606
##########
pkg/common/configs/configvalidator.go:
##########
@@ -139,6 +139,72 @@ func checkQueueResource(cur QueueConfig, parentM *resources.Resource) (*resource
return curG, nil
}
+func checkLimitResource(cur QueueConfig, parent *QueueConfig, users map[string]*resources.Resource, groups map[string]*resources.Resource) error {
+ // Collect user and group limits of parent queue
+ if parent != nil {
+ for _, limit := range parent.Limits {
+ for _, user := range limit.Users {
+ var err error
+ users[user], err = resources.NewResourceFromConf(limit.MaxResources)
+ if err != nil {
+ return err
+ }
+ }
+ for _, group := range limit.Groups {
+ var err error
+ groups[group], err = resources.NewResourceFromConf(limit.MaxResources)
+ if err != nil {
+ return err
+ }
+ }
+ }
+ }
+
+ // compare user & group limit setting between the current queue and parent queue
+ for _, limit := range cur.Limits {
+ for _, user := range limit.Users {
+ curM, err := resources.NewResourceFromConf(limit.MaxResources)
+ if err != nil {
+ return err
+ }
+ // Is user limit setting exists?
+ if _, ok := users[user]; ok {
Review Comment:
Use a variable to reduce the access count of map repeatedly.
```
maxApplicationsBound, ok := users[user]
```
##########
pkg/common/configs/configvalidator.go:
##########
@@ -153,6 +219,56 @@ func checkQueueMaxApplications(cur QueueConfig) error {
return nil
}
+func checkLimitMaxApplications(cur QueueConfig, parent *QueueConfig, users map[string]uint64, groups map[string]uint64) error {
+ // Collect user and group limits of parent queue
+ if parent != nil {
+ for _, limit := range parent.Limits {
+ for _, user := range limit.Users {
+ users[user] = limit.MaxApplications
+ }
+ for _, group := range limit.Groups {
+ groups[group] = limit.MaxApplications
+ }
+ }
+ }
+
+ // compare user & group limit setting between the current queue and parent queue
+ for _, limit := range cur.Limits {
+ for _, user := range limit.Users {
+ curM := limit.MaxApplications
+ // Is user limit setting exists?
+ if _, ok := users[user]; ok {
Review Comment:
The reason is above.
##########
pkg/common/configs/configvalidator.go:
##########
@@ -139,6 +139,72 @@ func checkQueueResource(cur QueueConfig, parentM *resources.Resource) (*resource
return curG, nil
}
+func checkLimitResource(cur QueueConfig, parent *QueueConfig, users map[string]*resources.Resource, groups map[string]*resources.Resource) error {
+ // Collect user and group limits of parent queue
+ if parent != nil {
+ for _, limit := range parent.Limits {
+ for _, user := range limit.Users {
+ var err error
+ users[user], err = resources.NewResourceFromConf(limit.MaxResources)
+ if err != nil {
+ return err
+ }
+ }
+ for _, group := range limit.Groups {
+ var err error
+ groups[group], err = resources.NewResourceFromConf(limit.MaxResources)
+ if err != nil {
+ return err
+ }
+ }
+ }
+ }
+
+ // compare user & group limit setting between the current queue and parent queue
+ for _, limit := range cur.Limits {
+ for _, user := range limit.Users {
+ curM, err := resources.NewResourceFromConf(limit.MaxResources)
+ if err != nil {
+ return err
+ }
+ // Is user limit setting exists?
+ if _, ok := users[user]; ok {
+ if !users[user].FitInMaxUndef(curM) {
+ return fmt.Errorf("user %s max resource %s of queue %s is greater than immediate or ancestor parent maximum resource %s", user, curM.String(), cur.Name, users[user].String())
+ }
+ // Override with min resource
+ users[user] = resources.ComponentWiseMinPermissive(curM, users[user])
+ } else {
+ users[user] = curM
+ }
+ }
+ for _, group := range limit.Groups {
Review Comment:
The reason is above.
##########
pkg/common/configs/configvalidator.go:
##########
@@ -153,6 +219,56 @@ func checkQueueMaxApplications(cur QueueConfig) error {
return nil
}
+func checkLimitMaxApplications(cur QueueConfig, parent *QueueConfig, users map[string]uint64, groups map[string]uint64) error {
+ // Collect user and group limits of parent queue
+ if parent != nil {
+ for _, limit := range parent.Limits {
+ for _, user := range limit.Users {
+ users[user] = limit.MaxApplications
+ }
+ for _, group := range limit.Groups {
+ groups[group] = limit.MaxApplications
+ }
+ }
+ }
+
+ // compare user & group limit setting between the current queue and parent queue
+ for _, limit := range cur.Limits {
+ for _, user := range limit.Users {
+ curM := limit.MaxApplications
+ // Is user limit setting exists?
+ if _, ok := users[user]; ok {
+ if users[user] != 0 && (users[user] < curM || curM == 0) {
+ return fmt.Errorf("user %s max applications %v of queue %s is greater than immediate or ancestor parent max applications %v", user, curM, cur.Name, users[user])
+ }
+ users[user] = uint64(math.Min(float64(curM), float64(users[user])))
+ } else {
+ users[user] = curM
+ }
+ }
+ for _, group := range limit.Groups {
+ curM := limit.MaxApplications
+ // Is group limit setting exists?
+ if _, ok := groups[group]; ok {
Review Comment:
The reason is above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org