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 2022/05/31 07:06:17 UTC
[GitHub] [yunikorn-k8shim] manirajv06 opened a new pull request, #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
manirajv06 opened a new pull request, #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428
### What is this PR for?
Mark the task as "originator" after identify the same as first pod and passes the same info to core via SI message.
### What type of PR is it?
* [ ] - Feature
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1207
### 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-k8shim] pbacsko commented on pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
pbacsko commented on PR #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428#issuecomment-1155188210
+1
--
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-k8shim] pbacsko closed pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
pbacsko closed pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
URL: https://github.com/apache/yunikorn-k8shim/pull/428
--
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-k8shim] pbacsko commented on a diff in pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428#discussion_r888085629
##########
pkg/cache/amprotocol_mock.go:
##########
@@ -71,16 +74,39 @@ func (m *MockedAMProtocol) RemoveApplication(appID string) error {
func (m *MockedAMProtocol) AddTask(request *interfaces.AddTaskRequest) interfaces.ManagedTask {
if app, ok := m.applications[request.Metadata.ApplicationID]; ok {
- if existingTask, err := app.GetTask(request.Metadata.TaskID); err != nil {
- task := NewTask(request.Metadata.TaskID, app, nil, request.Metadata.Pod)
+ existingTask, err := app.GetTask(request.Metadata.TaskID)
+ if err != nil {
+ var originator bool
+ var ownerReferenceUID string
+ // Is this task the originator of the application?
+ // If yes, then make it as "first pod/owner/driver" of the application and set the task as originator
+ for _, ownerReference := range app.getPlaceholderOwnerReferences() {
+ referenceID := string(ownerReference.UID)
+ if request.Metadata.TaskID == referenceID {
+ originator = true
+ ownerReferenceUID = referenceID
+ break
+ }
+ }
+ task := NewFromTaskMeta(request.Metadata.TaskID, app, nil, request.Metadata, originator)
app.addTask(task)
+ log.Logger().Info("task added",
+ zap.String("appID", app.applicationID),
+ zap.String("taskID", task.taskID),
+ zap.String("taskState", task.GetTaskState()))
+ if app.GetOriginatingTask() == nil {
Review Comment:
Just curious... If we've already called `setOriginatingTask()` then this no longer returns `nil`. Also, we set this if `originator == true` and we have only one originator, right? So to me, it's cleaner to read sth like:
```
if originator {
app.setOriginatingTask(task)
log.Logger().Info("app request originating pod added", zap.String("original task", task.GetTaskID()))
}
```
If `originator` is true and `app.GetTask(ownerReferenceUID)` returns an existing task, that means an inconsistent state, isn't it?
If I'm correct, I'd rather simplify this and handle the `task != nil` condition with an error, because it's something that's not supposed to happen:
```
if originator {
if app.GetOriginatingTask() != nil {
log.Logger.Error("Inconsistent state - found another originator task for an application", zap.String("taskId", task.GetTaskID())
}
app.setOriginatingTask(task)
log.Logger().Info("app request originating pod added", zap.String("original task", task.GetTaskID()))
}
```
--
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-k8shim] pbacsko commented on a diff in pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428#discussion_r896802362
##########
pkg/cache/context.go:
##########
@@ -692,22 +692,34 @@ func (ctx *Context) AddTask(request *interfaces.AddTaskRequest) interfaces.Manag
if app, valid := managedApp.(*Application); valid {
existingTask, err := app.GetTask(request.Metadata.TaskID)
if err != nil {
- task := NewFromTaskMeta(request.Metadata.TaskID, app, ctx, request.Metadata)
+ var originator bool
+
+ // Is this task the originator of the application?
+ // If yes, then make it as "first pod/owner/driver" of the application and set the task as originator
+ if app.GetOriginatingTask() == nil {
+ for _, ownerReference := range app.getPlaceholderOwnerReferences() {
+ referenceID := string(ownerReference.UID)
+ if request.Metadata.TaskID == referenceID {
+ originator = true
+ break
Review Comment:
This is new code, but `context_test.go` has not been updated, so codecov complains. Is this legit? I don't think L715-717 is important, but this one + L719-222 are.
--
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-k8shim] pbacsko commented on a diff in pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428#discussion_r888087405
##########
pkg/cache/context.go:
##########
@@ -691,21 +691,30 @@ func (ctx *Context) AddTask(request *interfaces.AddTaskRequest) interfaces.Manag
if app, valid := managedApp.(*Application); valid {
existingTask, err := app.GetTask(request.Metadata.TaskID)
if err != nil {
- task := NewFromTaskMeta(request.Metadata.TaskID, app, ctx, request.Metadata)
+ var originator bool
+ var ownerReferenceUID string
+ // Is this task the originator of the application?
+ // If yes, then make it as "first pod/owner/driver" of the application and set the task as originator
+ for _, ownerReference := range app.getPlaceholderOwnerReferences() {
+ referenceID := string(ownerReference.UID)
+ if request.Metadata.TaskID == referenceID {
+ originator = true
+ ownerReferenceUID = referenceID
+ break
+ }
+ }
+ task := NewFromTaskMeta(request.Metadata.TaskID, app, ctx, request.Metadata, originator)
app.addTask(task)
log.Logger().Info("task added",
zap.String("appID", app.applicationID),
zap.String("taskID", task.taskID),
zap.String("taskState", task.GetTaskState()))
- if app.getOriginatingTask() == nil {
- for _, ownerReference := range app.getPlaceholderOwnerReferences() {
- if task, taskErr := app.GetTask(string(ownerReference.UID)); task != nil && taskErr == nil {
- log.Logger().Info("app request originating pod added",
- zap.String("appID", app.applicationID),
- zap.String("original task", task.GetTaskID()))
- app.setOriginatingTask(task)
- break
- }
+ if app.GetOriginatingTask() == nil {
Review Comment:
Just curious... If we've already called `setOriginatingTask()` then this no longer returns `nil`. Also, we set this if `originator == true` and we have only one originator, right? So to me, it's cleaner to read sth like:
```
if originator {
app.setOriginatingTask(task)
log.Logger().Info("app request originating pod added", zap.String("original task", task.GetTaskID()))
}
```
If `originator` is true and `app.GetTask(ownerReferenceUID)` returns an existing task, that means an inconsistent state, isn't it?
If I'm correct, I'd rather simplify this and handle the `task != nil` condition with an error, because it's something that's not supposed to happen:
```
if originator {
if app.GetOriginatingTask() != nil {
log.Logger.Error("Inconsistent state - found another originator task for an application", zap.String("taskId", task.GetTaskID())
}
app.setOriginatingTask(task)
log.Logger().Info("app request originating pod added", zap.String("original task", task.GetTaskID()))
}
```
--
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-k8shim] codecov[bot] commented on pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on PR #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428#issuecomment-1144967443
# [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#428](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (dd7efce) into [master](https://codecov.io/gh/apache/yunikorn-k8shim/commit/ce79225573b28ec5b050008d6a2f5530afb1c5a0?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ce79225) will **increase** coverage by `0.02%`.
> The diff coverage is `71.87%`.
```diff
@@ Coverage Diff @@
## master #428 +/- ##
==========================================
+ Coverage 67.49% 67.51% +0.02%
==========================================
Files 41 41
Lines 6683 6721 +38
==========================================
+ Hits 4511 4538 +27
- Misses 2007 2015 +8
- Partials 165 168 +3
```
| [Impacted Files](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [pkg/cache/context.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `45.60% <47.05%> (-0.70%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `97.43% <57.14%> (-1.91%)` | :arrow_down: |
| [pkg/cache/task.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `68.54% <69.23%> (-0.50%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `61.25% <92.30%> (+17.70%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `71.78% <100.00%> (ø)` | |
| [pkg/dispatcher/dispatcher.go](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2Rpc3BhdGNoZXIvZGlzcGF0Y2hlci5nbw==) | `76.22% <0.00%> (+1.39%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Last update [ce79225...dd7efce](https://codecov.io/gh/apache/yunikorn-k8shim/pull/428?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation).
--
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-k8shim] pbacsko commented on a diff in pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
pbacsko commented on code in PR #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428#discussion_r888085629
##########
pkg/cache/amprotocol_mock.go:
##########
@@ -71,16 +74,39 @@ func (m *MockedAMProtocol) RemoveApplication(appID string) error {
func (m *MockedAMProtocol) AddTask(request *interfaces.AddTaskRequest) interfaces.ManagedTask {
if app, ok := m.applications[request.Metadata.ApplicationID]; ok {
- if existingTask, err := app.GetTask(request.Metadata.TaskID); err != nil {
- task := NewTask(request.Metadata.TaskID, app, nil, request.Metadata.Pod)
+ existingTask, err := app.GetTask(request.Metadata.TaskID)
+ if err != nil {
+ var originator bool
+ var ownerReferenceUID string
+ // Is this task the originator of the application?
+ // If yes, then make it as "first pod/owner/driver" of the application and set the task as originator
+ for _, ownerReference := range app.getPlaceholderOwnerReferences() {
+ referenceID := string(ownerReference.UID)
+ if request.Metadata.TaskID == referenceID {
+ originator = true
+ ownerReferenceUID = referenceID
+ break
+ }
+ }
+ task := NewFromTaskMeta(request.Metadata.TaskID, app, nil, request.Metadata, originator)
app.addTask(task)
+ log.Logger().Info("task added",
+ zap.String("appID", app.applicationID),
+ zap.String("taskID", task.taskID),
+ zap.String("taskState", task.GetTaskState()))
+ if app.GetOriginatingTask() == nil {
Review Comment:
Ok, this is a mock class, so my comment below might not apply, but still asking the same question here.
--
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-k8shim] manirajv06 commented on a diff in pull request #428: [YUNIKORN-1207] [Shim] preempt pods based on the priorities
Posted by GitBox <gi...@apache.org>.
manirajv06 commented on code in PR #428:
URL: https://github.com/apache/yunikorn-k8shim/pull/428#discussion_r896810997
##########
pkg/cache/context.go:
##########
@@ -692,22 +692,34 @@ func (ctx *Context) AddTask(request *interfaces.AddTaskRequest) interfaces.Manag
if app, valid := managedApp.(*Application); valid {
existingTask, err := app.GetTask(request.Metadata.TaskID)
if err != nil {
- task := NewFromTaskMeta(request.Metadata.TaskID, app, ctx, request.Metadata)
+ var originator bool
+
+ // Is this task the originator of the application?
+ // If yes, then make it as "first pod/owner/driver" of the application and set the task as originator
+ if app.GetOriginatingTask() == nil {
+ for _, ownerReference := range app.getPlaceholderOwnerReferences() {
+ referenceID := string(ownerReference.UID)
+ if request.Metadata.TaskID == referenceID {
+ originator = true
+ break
Review Comment:
Test cases has been already added in general_test.go. I think it is good enough.
--
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