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/01/10 05:26:33 UTC
[GitHub] [incubator-yunikorn-k8shim] anuraagnalluri opened a new pull request #349: [YUNIKORN-937] Update eventrecorder usage to new K8S API
anuraagnalluri opened a new pull request #349:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/349
### What is this PR for?
Upgrade usage of EventRecorder to report "k8s.io/api/events/v1beta1".Event events instead of "k8s.io/api/core/v1".Event events. More details on deprecation of core API can be found under Rule #1's Note [here](https://kubernetes.io/docs/reference/using-api/deprecation-policy/).
### What type of PR is it?
* [ ] - Bug Fix
* [x] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
### What is the Jira issue?
* https://issues.apache.org/jira/browse/YUNIKORN-937
### How should this be tested?
Existing unit + e2e tests should pass.
### Screenshots (if appropriate)
### Questions:
--
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] [incubator-yunikorn-k8shim] anuraagnalluri commented on a change in pull request #349: [YUNIKORN-937] Update eventrecorder usage to new K8S API
Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #349:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/349#discussion_r780900292
##########
File path: pkg/cache/application.go
##########
@@ -422,7 +422,7 @@ func (app *Application) scheduleTasks(taskScheduleCondition func(t *Task) bool)
log.Logger().Warn("init task failed", zap.Error(err))
}
} else {
- events.GetRecorder().Event(task.GetTaskPod(), v1.EventTypeWarning, "FailedScheduling", err.Error())
+ events.GetRecorder().Eventf(task.GetTaskPod(), nil, v1.EventTypeWarning, "FailedScheduling", "FailedScheduling", err.Error())
Review comment:
@craigcondit Minor difference in new API here: https://pkg.go.dev/k8s.io/client-go@v0.20.11/tools/events#EventRecorder
Using `nil` in place of `related` (2nd param) currently since we did not have concept of secondary object in previous Events API. By same logic, we're duplicating `reason` param to be same as `action` since the latter was also not present in previous API.
Let me know if there's a better structure I can follow to populate these args correctly in `Eventf`.
--
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] [incubator-yunikorn-k8shim] anuraagnalluri commented on a change in pull request #349: [YUNIKORN-937] Update eventrecorder usage to new K8S API
Posted by GitBox <gi...@apache.org>.
anuraagnalluri commented on a change in pull request #349:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/349#discussion_r780900292
##########
File path: pkg/cache/application.go
##########
@@ -422,7 +422,7 @@ func (app *Application) scheduleTasks(taskScheduleCondition func(t *Task) bool)
log.Logger().Warn("init task failed", zap.Error(err))
}
} else {
- events.GetRecorder().Event(task.GetTaskPod(), v1.EventTypeWarning, "FailedScheduling", err.Error())
+ events.GetRecorder().Eventf(task.GetTaskPod(), nil, v1.EventTypeWarning, "FailedScheduling", "FailedScheduling", err.Error())
Review comment:
Minor difference in new API here: https://pkg.go.dev/k8s.io/client-go@v0.20.11/tools/events#EventRecorder
Using `nil` in place of `related` (2nd param) currently since we did not have concept of secondary object in previous Events API. By same logic, we're duplicating `reason` param to be same as `action` since the latter was also not present in previous API.
Let me know if there's a better structure I can follow to populate these args correctly in `Eventf`.
--
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] [incubator-yunikorn-k8shim] craigcondit commented on a change in pull request #349: [YUNIKORN-937] Update eventrecorder usage to new K8S API
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #349:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/349#discussion_r783468798
##########
File path: pkg/cache/application.go
##########
@@ -422,7 +422,7 @@ func (app *Application) scheduleTasks(taskScheduleCondition func(t *Task) bool)
log.Logger().Warn("init task failed", zap.Error(err))
}
} else {
- events.GetRecorder().Event(task.GetTaskPod(), v1.EventTypeWarning, "FailedScheduling", err.Error())
+ events.GetRecorder().Eventf(task.GetTaskPod(), nil, v1.EventTypeWarning, "FailedScheduling", "FailedScheduling", err.Error())
Review comment:
This looks fine to me.
--
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] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #349: [YUNIKORN-937] Update eventrecorder usage to new K8S API
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #349:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/349#issuecomment-1008613080
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349?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 [#349](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (97fd947) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (c47ed51) will **increase** coverage by `2.22%`.
> The diff coverage is `62.85%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
```diff
@@ Coverage Diff @@
## master #349 +/- ##
==========================================
+ Coverage 59.75% 61.97% +2.22%
==========================================
Files 35 39 +4
Lines 3133 5131 +1998
==========================================
+ Hits 1872 3180 +1308
- Misses 1180 1845 +665
- Partials 81 106 +25
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349?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/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/context\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL2NhY2hlL2NvbnRleHRfcmVjb3ZlcnkuZ28=) | `40.67% <0.00%> (-5.67%)` | :arrow_down: |
| [pkg/common/events/recorder\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXJfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/common/events/states.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL2NvbW1vbi9ldmVudHMvc3RhdGVzLmdv) | `0.00% <0.00%> (ø)` | |
| [pkg/shim/main.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL3NoaW0vbWFpbi5nbw==) | `0.00% <0.00%> (ø)` | |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `34.43% <5.71%> (+1.69%)` | :arrow_up: |
| [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.92% <16.66%> (-7.76%)` | :arrow_down: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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=) | `37.82% <23.88%> (-3.26%)` | :arrow_down: |
| [pkg/common/events/recorder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL2NvbW1vbi9ldmVudHMvcmVjb3JkZXIuZ28=) | `42.10% <25.00%> (+12.10%)` | :arrow_up: |
| [pkg/cache/nodes.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/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-cGtnL2NhY2hlL25vZGVzLmdv) | `78.28% <46.15%> (-1.52%)` | :arrow_down: |
| ... and [55 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349?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/incubator-yunikorn-k8shim/pull/349?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 [1e45e53...97fd947](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/349?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] [incubator-yunikorn-k8shim] craigcondit closed pull request #349: [YUNIKORN-937] Update eventrecorder usage to new K8S API
Posted by GitBox <gi...@apache.org>.
craigcondit closed pull request #349:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/349
--
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