You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/02/15 18:20:35 UTC
[GitHub] [incubator-yunikorn-k8shim] kingamarton opened a new pull request #231: [YUNIKORN-406] Handle placeholder timeout
kingamarton opened a new pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#discussion_r589358708
##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -139,6 +150,11 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
log.Logger().Error("failed to delete application", zap.Error(err))
}
} else {
+ if updated.State == events.States().Application.Killed {
+ //TODO: implement the killed event
+ ev := cache.NewFailApplicationEvent(updated.ApplicationID)
+ dispatcher.Dispatch(ev)
+ }
Review comment:
As we discussed, we will fail the application, also the placeholders will be removed. Since the app will be failed, it will be skipped from the next scheduling cycles.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-782023392
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=h1) Report
> Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=desc) (84bf01f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.26%`.
> The diff coverage is `43.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #231 +/- ##
==========================================
- Coverage 59.75% 59.48% -0.27%
==========================================
Files 35 35
Lines 3133 3172 +39
==========================================
+ Hits 1872 1887 +15
- Misses 1180 1202 +22
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `32.54% <0.00%> (-1.62%)` | :arrow_down: |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `74.40% <ø> (ø)` | |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.66% <12.50%> (-0.42%)` | :arrow_down: |
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `76.47% <62.50%> (-3.22%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.81% <95.45%> (+1.07%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `60.50% <100.00%> (+1.03%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=footer). Last update [b48c35c...f358a19](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei merged pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-782023392
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=h1) Report
> Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=desc) (84bf01f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.26%`.
> The diff coverage is `43.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #231 +/- ##
==========================================
- Coverage 59.75% 59.48% -0.27%
==========================================
Files 35 35
Lines 3133 3172 +39
==========================================
+ Hits 1872 1887 +15
- Misses 1180 1202 +22
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `32.54% <0.00%> (-1.62%)` | :arrow_down: |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `74.40% <ø> (ø)` | |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.66% <12.50%> (-0.42%)` | :arrow_down: |
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `76.47% <62.50%> (-3.22%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.81% <95.45%> (+1.07%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `60.50% <100.00%> (+1.03%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=footer). Last update [6513242...f8ca96f](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-793142873
Overall the changes looked good, +1.
For the remaining review comments, I have created several follow up JIRAs under https://issues.apache.org/jira/browse/YUNIKORN-553.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-782023392
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=h1) Report
> Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=desc) (84bf01f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.26%`.
> The diff coverage is `43.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #231 +/- ##
==========================================
- Coverage 59.75% 59.48% -0.27%
==========================================
Files 35 35
Lines 3133 3172 +39
==========================================
+ Hits 1872 1887 +15
- Misses 1180 1202 +22
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `32.54% <0.00%> (-1.62%)` | :arrow_down: |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `74.40% <ø> (ø)` | |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.66% <12.50%> (-0.42%)` | :arrow_down: |
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `76.47% <62.50%> (-3.22%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.81% <95.45%> (+1.07%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `60.50% <100.00%> (+1.03%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=footer). Last update [b48c35c...8e31d83](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on a change in pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
kingamarton commented on a change in pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#discussion_r589355547
##########
File path: pkg/common/constants/constants.go
##########
@@ -50,11 +50,14 @@ const SchedulerName = "yunikorn"
// Application crd
const AppManagerHandlerName = "yunikorn-app"
-const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
-const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
-const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
// Gang scheduling
const PlaceholderContainerImage = "k8s.gcr.io/pause"
const PlaceholderContainerName = "pause"
const PlaceholderPodRestartPolicy = "Never"
+const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
+const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
+const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
+const AnnotationSchedulingPolicyParam = "yunikorn.apache.org/schedulingPolicyParameters"
+const SchedulingPolicyTimeoutParam = "timeout"
Review comment:
I renamed it to placeholderTimeout
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-782023392
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=h1) Report
> Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=desc) (84bf01f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.26%`.
> The diff coverage is `43.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #231 +/- ##
==========================================
- Coverage 59.75% 59.48% -0.27%
==========================================
Files 35 35
Lines 3133 3172 +39
==========================================
+ Hits 1872 1887 +15
- Misses 1180 1202 +22
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `32.54% <0.00%> (-1.62%)` | :arrow_down: |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `74.40% <ø> (ø)` | |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.66% <12.50%> (-0.42%)` | :arrow_down: |
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `76.47% <62.50%> (-3.22%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.81% <95.45%> (+1.07%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `60.50% <100.00%> (+1.03%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=footer). Last update [5dc97fa...84bf01f](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] kingamarton commented on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
kingamarton commented on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-782060114
I still have to cover the changes with unit tests. Before I start to write them, @wilfred-s , @yangwwei can you please check briefly the patch if you agree with this approach?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-782023392
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=h1) Report
> Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=desc) (8e31d83) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `1.05%`.
> The diff coverage is `42.27%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #231 +/- ##
==========================================
- Coverage 59.75% 58.69% -1.06%
==========================================
Files 35 35
Lines 3133 3196 +63
==========================================
+ Hits 1872 1876 +4
- Misses 1180 1237 +57
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `70.40% <ø> (-4.00%)` | :arrow_down: |
| [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `43.33% <8.33%> (-9.73%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `72.57% <62.50%> (-4.17%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `63.15% <80.00%> (ø)` | |
| ... and [5 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=footer). Last update [b48c35c...8e31d83](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#issuecomment-782023392
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=h1) Report
> Merging [#231](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=desc) (84bf01f) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.26%`.
> The diff coverage is `43.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #231 +/- ##
==========================================
- Coverage 59.75% 59.48% -0.27%
==========================================
Files 35 35
Lines 3133 3172 +39
==========================================
+ Hits 1872 1887 +15
- Misses 1180 1202 +22
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `32.54% <0.00%> (-1.62%)` | :arrow_down: |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `74.40% <ø> (ø)` | |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `40.66% <12.50%> (-0.42%)` | :arrow_down: |
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `76.47% <62.50%> (-3.22%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.81% <95.45%> (+1.07%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `60.50% <100.00%> (+1.03%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=footer). Last update [6513242...6f404df](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/231?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#discussion_r584462823
##########
File path: pkg/common/utils/gang_utils.go
##########
@@ -117,6 +118,28 @@ func GetTaskGroupsFromAnnotation(pod *v1.Pod) ([]v1alpha1.TaskGroup, error) {
return taskGroups, nil
}
+func GetPlaceholderTimeoutParam(pod *v1.Pod) (int64, error) {
+ param, ok := pod.Annotations[constants.AnnotationSchedulingPolicyParam]
+ if !ok {
+ return 0, fmt.Errorf("no scheduling policy parameters defined for the pod")
+ }
+ params := strings.Split(param, constants.SchedulingPolicyParamDelimiter)
+ for _, p := range params {
+ if strings.HasPrefix(p, constants.SchedulingPolicyTimeoutParam) {
+ timeoutParam := strings.Split(p, "=")
+ if len(timeoutParam) != 2 {
+ return 0, fmt.Errorf("unable to parse timeout value from annotation")
+ }
+ timeout, err := strconv.ParseInt(timeoutParam[1], 10, 64)
+ if err != nil {
+ return 0, fmt.Errorf("failed to parse timeout value: %s", timeoutParam[1])
+ }
+ return timeout, nil
+ }
+ }
Review comment:
Can we do the split first and then find the exact match for SchedulingPolicyTimeoutParam?
Since the parameters could be arbitrary key-value pairs, just in case there could be other parameters that starts with "timeout".
##########
File path: pkg/common/constants/constants.go
##########
@@ -50,11 +50,14 @@ const SchedulerName = "yunikorn"
// Application crd
const AppManagerHandlerName = "yunikorn-app"
-const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
-const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
-const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
// Gang scheduling
const PlaceholderContainerImage = "k8s.gcr.io/pause"
const PlaceholderContainerName = "pause"
const PlaceholderPodRestartPolicy = "Never"
+const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
+const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
+const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
+const AnnotationSchedulingPolicyParam = "yunikorn.apache.org/schedulingPolicyParameters"
+const SchedulingPolicyTimeoutParam = "timeout"
Review comment:
We need to do a second think about the name.
"timeout" is not very descriptive to me and may cause confusion.
Maybe we can explicitly call it "reservationTimeout"
##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -139,6 +150,11 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
log.Logger().Error("failed to delete application", zap.Error(err))
}
} else {
+ if updated.State == events.States().Application.Killed {
+ //TODO: implement the killed event
+ ev := cache.NewFailApplicationEvent(updated.ApplicationID)
+ dispatcher.Dispatch(ev)
+ }
Review comment:
@kingamarton how can we support a "Kill" application event?
I am not fully understanding what will happen behind this.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #231: [YUNIKORN-460] Handle placeholder timeout
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #231:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/231#discussion_r589638890
##########
File path: pkg/callback/scheduler_callback.go
##########
@@ -139,6 +150,10 @@ func (callback *AsyncRMCallback) RecvUpdateResponse(response *si.UpdateResponse)
log.Logger().Error("failed to delete application", zap.Error(err))
}
} else {
+ if updated.State == events.States().Application.Killed {
+ ev := cache.NewFailApplicationEvent(updated.ApplicationID)
+ dispatcher.Dispatch(ev)
+ }
Review comment:
When we fail an application, we need to expose some pod level events to indicate this issue.
we can do this in a follow up JIRA.
##########
File path: pkg/common/constants/constants.go
##########
@@ -50,11 +50,14 @@ const SchedulerName = "yunikorn"
// Application crd
const AppManagerHandlerName = "yunikorn-app"
-const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
-const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
-const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
// Gang scheduling
const PlaceholderContainerImage = "k8s.gcr.io/pause"
const PlaceholderContainerName = "pause"
const PlaceholderPodRestartPolicy = "Never"
+const AnnotationPlaceholderFlag = "yunikorn.apache.org/placeholder"
+const AnnotationTaskGroupName = "yunikorn.apache.org/task-group-name"
+const AnnotationTaskGroups = "yunikorn.apache.org/task-groups"
+const AnnotationSchedulingPolicyParam = "yunikorn.apache.org/schedulingPolicyParameters"
+const SchedulingPolicyTimeoutParam = "placeholderTimeout"
Review comment:
Looks like it implies the unit for `placeholderTimeout` is seconds, I think we should have this declared explicitly in the parameter, otherwise, we will need additional docs to explain the format. Suggest to rename this to `placeholderTimeoutInSeconds`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org