You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "pbacsko (via GitHub)" <gi...@apache.org> on 2023/05/31 08:41:41 UTC
[GitHub] [yunikorn-k8shim] pbacsko opened a new pull request, #605: [YUNIKORN-1744] Enable recovery for MockScheduler
pbacsko opened a new pull request, #605:
URL: https://github.com/apache/yunikorn-k8shim/pull/605
### What is this PR for?
Remove `IsTestingMode()` calls to make recovery possible when using `MockScheduler`.
Add methods so we can simulate existing nodes and pods.
### What type of PR is it?
* [ ] - Bug Fix
* [x] - 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-k8shim] pbacsko commented on a diff in pull request #605: [YUNIKORN-1744] Enable recovery for MockScheduler
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #605:
URL: https://github.com/apache/yunikorn-k8shim/pull/605#discussion_r1212159512
##########
pkg/cache/context_recovery.go:
##########
@@ -80,86 +73,77 @@ func (ctx *Context) recover(mgr []interfaces.Recoverable, due time.Duration) err
ctx.nodes.addAndReportNode(node, false)
}
- // current, disable getting pods for a node during test,
- // because in the tests, we don't really send existing allocations
- // we simply simulate to accept or reject nodes on conditions.
- if !ctx.apiProvider.IsTestingMode() {
- var podList *corev1.PodList
- podList, err = ctx.apiProvider.GetAPIs().KubeClient.GetClientSet().
- CoreV1().Pods("").
- List(context.Background(), metav1.ListOptions{})
- if err != nil {
- return err
- }
+ pods, err := ctx.apiProvider.GetAPIs().PodInformer.Lister().List(labels.Everything())
Review Comment:
I rewrote this to `PodInformer.Lister().List()`, documentation says it's faster.
Although at this point, we already have the list of pods from a previous listing, so we can even skip this (perhaps in a separate JIRA?).
--
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 #605: [YUNIKORN-1744] Enable recovery for MockScheduler
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #605:
URL: https://github.com/apache/yunikorn-k8shim/pull/605#discussion_r1218125895
##########
pkg/appmgmt/appmgmt.go:
##########
@@ -56,21 +56,14 @@ func NewAMService(amProtocol interfaces.ApplicationManagementProtocol,
}
log.Logger().Info("Initializing new AppMgmt service")
-
- if !apiProvider.IsTestingMode() {
- log.Logger().Info("Registering Spark operator with the AppMgmt service")
Review Comment:
That line does not add too much information, we always register the Spark operator.
--
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] zhuqi-lucas commented on a diff in pull request #605: [YUNIKORN-1744] Enable recovery for MockScheduler
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #605:
URL: https://github.com/apache/yunikorn-k8shim/pull/605#discussion_r1212505208
##########
pkg/appmgmt/appmgmt.go:
##########
@@ -56,21 +56,14 @@ func NewAMService(amProtocol interfaces.ApplicationManagementProtocol,
}
log.Logger().Info("Initializing new AppMgmt service")
-
- if !apiProvider.IsTestingMode() {
- log.Logger().Info("Registering Spark operator with the AppMgmt service")
Review Comment:
log.Logger().Info("Registering Spark operator with the AppMgmt service")
If this log don't need for new code?
--
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] craigcondit closed pull request #605: [YUNIKORN-1744] Enable recovery for MockScheduler
Posted by "craigcondit (via GitHub)" <gi...@apache.org>.
craigcondit closed pull request #605: [YUNIKORN-1744] Enable recovery for MockScheduler
URL: https://github.com/apache/yunikorn-k8shim/pull/605
--
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 #605: [YUNIKORN-1744] Enable recovery for MockScheduler
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #605:
URL: https://github.com/apache/yunikorn-k8shim/pull/605#issuecomment-1569776785
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#605](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (bc30aa0) into [master](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/73e0e826c1149e6089257563b56d44aabbeb4f0a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (73e0e82) will **increase** coverage by `0.21%`.
> The diff coverage is `13.33%`.
```diff
@@ Coverage Diff @@
## master #605 +/- ##
==========================================
+ Coverage 70.63% 70.84% +0.21%
==========================================
Files 47 47
Lines 7989 7972 -17
==========================================
+ Hits 5643 5648 +5
+ Misses 2141 2119 -22
Partials 205 205
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [pkg/appmgmt/appmgmt\_recovery.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `73.13% <0.00%> (+1.07%)` | :arrow_up: |
| [pkg/cache/context.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `43.86% <0.00%> (+0.05%)` | :arrow_up: |
| [pkg/cache/context\_recovery.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2NhY2hlL2NvbnRleHRfcmVjb3ZlcnkuZ28=) | `43.38% <3.63%> (+4.07%)` | :arrow_up: |
| [pkg/appmgmt/appmgmt.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGtnL2FwcG1nbXQvYXBwbWdtdC5nbw==) | `49.35% <100.00%> (+9.59%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/605/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
: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-k8shim] pbacsko commented on a diff in pull request #605: [YUNIKORN-1744] Enable recovery for MockScheduler
Posted by "pbacsko (via GitHub)" <gi...@apache.org>.
pbacsko commented on code in PR #605:
URL: https://github.com/apache/yunikorn-k8shim/pull/605#discussion_r1212159512
##########
pkg/cache/context_recovery.go:
##########
@@ -80,86 +73,77 @@ func (ctx *Context) recover(mgr []interfaces.Recoverable, due time.Duration) err
ctx.nodes.addAndReportNode(node, false)
}
- // current, disable getting pods for a node during test,
- // because in the tests, we don't really send existing allocations
- // we simply simulate to accept or reject nodes on conditions.
- if !ctx.apiProvider.IsTestingMode() {
- var podList *corev1.PodList
- podList, err = ctx.apiProvider.GetAPIs().KubeClient.GetClientSet().
- CoreV1().Pods("").
- List(context.Background(), metav1.ListOptions{})
- if err != nil {
- return err
- }
+ pods, err := ctx.apiProvider.GetAPIs().PodInformer.Lister().List(labels.Everything())
Review Comment:
I rewrote this to `PodInformer.Lister().List()`, documentation says it's faster.
Although at this point, we already have the list of pods from a previous listing, so we can even skip this (perhaps implement this in a separate JIRA?).
--
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