You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2023/06/15 07:04:18 UTC
[GitHub] [yunikorn-core] zhuqi-lucas opened a new pull request, #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
zhuqi-lucas opened a new pull request, #569:
URL: https://github.com/apache/yunikorn-core/pull/569
### What is this PR for?
The core side change to support ability to specify guaranteedResources via namespace annotations.
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/YUNIKORN/
* Put link here, and add [YUNIKORN-*Jira number*] in PR title, eg. `[YUNIKORN-2] Gang scheduling interface parameters`
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1258236431
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
+ zap.Stringer("maxResource", quotaRes))
Review Comment:
Good suggestion, will address it.
--
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
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1258234375
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
+ zap.Stringer("maxResource", quotaRes))
+ quotaRes = nil // Skip setting quota if it has a value <= 0
+ }
}
- if !resources.StrictlyGreaterThanZero(res) {
- log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
- zap.Stringer("maxResource", res))
+
+ // need to set guaranteed resource: convert json string to resource
+ if guaranteed != "" {
+ guaranteedRes, guaranteedErr = resources.NewResourceFromString(guaranteed)
+ if guaranteedErr != nil {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource conversion failure",
+ zap.String("json guaranteed string", guaranteed),
+ zap.Error(guaranteedErr))
+ } else if !resources.StrictlyGreaterThanZero(guaranteedRes) {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource has at least one 0 value: cannot set queue guaranteed resource",
+ zap.Stringer("guaranteedResource", guaranteedRes))
+ guaranteedRes = nil // Skip setting guaranteed resource if it has a value <= 0
+ }
+ }
+
+ if quotaErr != nil && guaranteedErr != nil {
Review Comment:
Sure, will address the return when logging.
--
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
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1258230294
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
+ zap.Stringer("maxResource", quotaRes))
+ quotaRes = nil // Skip setting quota if it has a value <= 0
+ }
}
- if !resources.StrictlyGreaterThanZero(res) {
- log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
- zap.Stringer("maxResource", res))
+
+ // need to set guaranteed resource: convert json string to resource
+ if guaranteed != "" {
+ guaranteedRes, guaranteedErr = resources.NewResourceFromString(guaranteed)
+ if guaranteedErr != nil {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource conversion failure",
+ zap.String("json guaranteed string", guaranteed),
+ zap.Error(guaranteedErr))
+ } else if !resources.StrictlyGreaterThanZero(guaranteedRes) {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource has at least one 0 value: cannot set queue guaranteed resource",
+ zap.Stringer("guaranteedResource", guaranteedRes))
+ guaranteedRes = nil // Skip setting guaranteed resource if it has a value <= 0
+ }
+ }
+
+ if quotaErr != nil && guaranteedErr != nil {
Review Comment:
I think we will return when both quotaErr and guaranteedErr, for example:
When quotaErr happen but guaranteedErr not happen, we can set for the guranteed resource seperately, every annotaion can set independently
So we only return here when both quotaErr and guaranteedErr happend.
--
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
[GitHub] [yunikorn-core] zhuqi-lucas commented on pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#issuecomment-1632234102
Thank you @manirajv06 for review, i wanted to increase the test coverage here, but it is the log error lines, how can we easily cover the log in unit test?
--
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
[GitHub] [yunikorn-core] codecov[bot] commented on pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#issuecomment-1592485001
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-core/pull/569?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#569](https://app.codecov.io/gh/apache/yunikorn-core/pull/569?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (7c6821e) into [master](https://app.codecov.io/gh/apache/yunikorn-core/commit/68a82820f0d7ff82f4d5d9b44629ae3660a184e1?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (68a8282) will **decrease** coverage by `0.08%`.
> The diff coverage is `63.15%`.
```diff
@@ Coverage Diff @@
## master #569 +/- ##
==========================================
- Coverage 75.15% 75.08% -0.08%
==========================================
Files 72 72
Lines 11637 11658 +21
==========================================
+ Hits 8746 8753 +7
- Misses 2585 2596 +11
- Partials 306 309 +3
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-core/pull/569?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/scheduler/objects/queue.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/569?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL3F1ZXVlLmdv) | `76.13% <26.31%> (-0.77%)` | :arrow_down: |
| [pkg/events/events.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/569?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2V2ZW50cy9ldmVudHMuZ28=) | `100.00% <100.00%> (ø)` | |
| [pkg/scheduler/objects/events.go](https://app.codecov.io/gh/apache/yunikorn-core/pull/569?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL3NjaGVkdWxlci9vYmplY3RzL2V2ZW50cy5nbw==) | `100.00% <100.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
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
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1258239614
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
Review Comment:
No @manirajv06 , it only uses when user annotation it just like quota, every annotation can be used independently.
--
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
[GitHub] [yunikorn-core] manirajv06 commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "manirajv06 (via GitHub)" <gi...@apache.org>.
manirajv06 commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1258465933
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,60 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("Max resource quantities should be greater than zero: cannot set queue max resource",
+ zap.Stringer("maxResource", quotaRes))
+ quotaRes = nil // Skip setting quota if it has a value <= 0
+ }
}
- if !resources.StrictlyGreaterThanZero(res) {
- log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
- zap.Stringer("maxResource", res))
- return
+
+ // need to set guaranteed resource: convert json string to resource
+ if guaranteed != "" {
+ guaranteedRes, guaranteedErr = resources.NewResourceFromString(guaranteed)
+ if guaranteedErr != nil {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource conversion failure",
+ zap.String("json guaranteed string", guaranteed),
+ zap.Error(guaranteedErr))
+ if quotaErr != nil {
Review Comment:
guaranteedErr, not quotaErr :)
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
Review Comment:
If we can treat each annotation independently, then we should not put together in a single if checks. We can have separate if checks for each one.
--
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
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1260837632
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,60 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("Max resource quantities should be greater than zero: cannot set queue max resource",
+ zap.Stringer("maxResource", quotaRes))
+ quotaRes = nil // Skip setting quota if it has a value <= 0
+ }
}
- if !resources.StrictlyGreaterThanZero(res) {
- log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
- zap.Stringer("maxResource", res))
- return
+
+ // need to set guaranteed resource: convert json string to resource
+ if guaranteed != "" {
+ guaranteedRes, guaranteedErr = resources.NewResourceFromString(guaranteed)
+ if guaranteedErr != nil {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource conversion failure",
+ zap.String("json guaranteed string", guaranteed),
+ zap.Error(guaranteedErr))
+ if quotaErr != nil {
Review Comment:
I think we should use quotaErr here, because we only need to return for both quotaErr != nil and guaranteedErr != nil
--
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
[GitHub] [yunikorn-core] manirajv06 commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "manirajv06 (via GitHub)" <gi...@apache.org>.
manirajv06 commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1257863189
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
+ zap.Stringer("maxResource", quotaRes))
+ quotaRes = nil // Skip setting quota if it has a value <= 0
+ }
}
- if !resources.StrictlyGreaterThanZero(res) {
- log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
- zap.Stringer("maxResource", res))
+
+ // need to set guaranteed resource: convert json string to resource
+ if guaranteed != "" {
+ guaranteedRes, guaranteedErr = resources.NewResourceFromString(guaranteed)
+ if guaranteedErr != nil {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource conversion failure",
+ zap.String("json guaranteed string", guaranteed),
+ zap.Error(guaranteedErr))
+ } else if !resources.StrictlyGreaterThanZero(guaranteedRes) {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource has at least one 0 value: cannot set queue guaranteed resource",
+ zap.Stringer("guaranteedResource", guaranteedRes))
+ guaranteedRes = nil // Skip setting guaranteed resource if it has a value <= 0
+ }
+ }
+
+ if quotaErr != nil && guaranteedErr != nil {
Review Comment:
Can we do this "return" earlier while logging itself?
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
+ zap.Stringer("maxResource", quotaRes))
Review Comment:
Error message bit misleading. Many be "Max resource quantities should be greater than zero" ?
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
Review Comment:
Is guaranteed mandatory? or Use only when annotation has it?
--
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
[GitHub] [yunikorn-core] zhuqi-lucas commented on a diff in pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#discussion_r1258230294
##########
pkg/scheduler/objects/queue.go:
##########
@@ -663,29 +663,61 @@ func (sq *Queue) AddApplication(app *Application) {
// YUNIKORN-199: update the quota from the namespace
// get the tag with the quota
quota := app.GetTag(common.AppTagNamespaceResourceQuota)
- if quota == "" {
+ // get the tag with the guaranteed resource
+ guaranteed := app.GetTag(common.AppTagNamespaceResourceGuaranteed)
+ if quota == "" && guaranteed == "" {
return
}
+
+ var quotaRes, guaranteedRes *resources.Resource
+ var quotaErr, guaranteedErr error
+
// need to set a quota: convert json string to resource
- res, err := resources.NewResourceFromString(quota)
- if err != nil {
- log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
- zap.String("json quota string", quota),
- zap.Error(err))
- return
+ if quota != "" {
+ quotaRes, quotaErr = resources.NewResourceFromString(quota)
+ if quotaErr != nil {
+ log.Log(log.SchedQueue).Warn("application resource quota conversion failure",
+ zap.String("json quota string", quota),
+ zap.Error(quotaErr))
+ } else if !resources.StrictlyGreaterThanZero(quotaRes) {
+ log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
+ zap.Stringer("maxResource", quotaRes))
+ quotaRes = nil // Skip setting quota if it has a value <= 0
+ }
}
- if !resources.StrictlyGreaterThanZero(res) {
- log.Log(log.SchedQueue).Warn("application resource quota has at least one 0 value: cannot set queue limit",
- zap.Stringer("maxResource", res))
+
+ // need to set guaranteed resource: convert json string to resource
+ if guaranteed != "" {
+ guaranteedRes, guaranteedErr = resources.NewResourceFromString(guaranteed)
+ if guaranteedErr != nil {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource conversion failure",
+ zap.String("json guaranteed string", guaranteed),
+ zap.Error(guaranteedErr))
+ } else if !resources.StrictlyGreaterThanZero(guaranteedRes) {
+ log.Log(log.SchedQueue).Warn("application guaranteed resource has at least one 0 value: cannot set queue guaranteed resource",
+ zap.Stringer("guaranteedResource", guaranteedRes))
+ guaranteedRes = nil // Skip setting guaranteed resource if it has a value <= 0
+ }
+ }
+
+ if quotaErr != nil && guaranteedErr != nil {
Review Comment:
I think we will return when both quotaErr and guaranteedErr, for example:
When quotaErr happen but guaranteedErr not happen, we can set for the guranteed resource seperately, every annotaion can set independently
So we only return here when both quotaErr and guaranteedErr happend.
--
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
[GitHub] [yunikorn-core] manirajv06 commented on pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "manirajv06 (via GitHub)" <gi...@apache.org>.
manirajv06 commented on PR #569:
URL: https://github.com/apache/yunikorn-core/pull/569#issuecomment-1632222969
looks good. See if you can increase the test coverage.
--
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
[GitHub] [yunikorn-core] manirajv06 closed pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
Posted by "manirajv06 (via GitHub)" <gi...@apache.org>.
manirajv06 closed pull request #569: [YUNIKORN-1575] [core] Ability to specify guaranteedResources via namespace annotations
URL: https://github.com/apache/yunikorn-core/pull/569
--
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