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/02/08 22:47:10 UTC
[GitHub] [incubator-yunikorn-k8shim] craigcondit opened a new pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
craigcondit opened a new pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365
### What is this PR for?
Fix a major memory leak in scheduler cache that causes pods to be stored forever.
### What type of PR is it?
* [x] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-876
### How should this be tested?
Local unit tests pass ; also verified via debugger that removed pods are properly removed from cache.
### 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] [incubator-yunikorn-k8shim] craigcondit commented on a change in pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r804384195
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -255,26 +246,25 @@ func (cache *SchedulerCache) UpdatePod(oldPod, newPod *v1.Pod) error {
defer cache.lock.Unlock()
currState, ok := cache.podsMap[key]
- switch {
- // An assumed pod won't have Update/Remove event. It needs to have Add event
- // before Update event, in which case the state would change from Assumed to Added.
- case ok && !cache.isAssumedPod(key):
- if currState.Spec.NodeName != newPod.Spec.NodeName {
- log.Logger().Error("pod updated on a different node than previously added to", zap.String("pod", key))
- log.Logger().Error("scheduler cache is corrupted and can badly affect scheduling decisions")
- }
- if err = cache.updatePod(oldPod, newPod); err != nil {
- return err
+ if ok {
+ // pod exists and is assumed
+ if !cache.isAssumedPod(key) && currState.Spec.NodeName != newPod.Spec.NodeName {
Review comment:
See previous reply. Caches need to stay in sync with their original data.
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (12b0a81) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.97%`.
> The diff coverage is `79.51%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.39% +1.97%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3960 +120
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `57.62% <76.71%> (+26.70%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...12b0a81](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e69de9) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.98%`.
> The diff coverage is `80.00%`.
> :exclamation: Current head 5e69de9 differs from pull request most recent head 8b6f047. Consider uploading reports for the commit 8b6f047 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.40% +1.98%
==========================================
Files 41 41
Lines 6152 6153 +1
==========================================
+ Hits 3840 3963 +123
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `58.08% <77.33%> (+27.17%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...8b6f047](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9c8d88b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.97%`.
> The diff coverage is `79.51%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.39% +1.97%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3960 +120
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `57.62% <76.71%> (+26.70%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...9c8d88b](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e69de9) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.98%`.
> The diff coverage is `80.00%`.
> :exclamation: Current head 5e69de9 differs from pull request most recent head 9c8d88b. Consider uploading reports for the commit 9c8d88b to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.40% +1.98%
==========================================
Files 41 41
Lines 6152 6153 +1
==========================================
+ Hits 3840 3963 +123
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `58.08% <77.33%> (+27.17%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...9c8d88b](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 commented on a change in pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r802131105
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -283,34 +281,46 @@ func (cache *SchedulerCache) addPod(pod *v1.Pod) {
}
n.AddPod(pod)
}
+ cache.podsMap[key] = pod
}
-func (cache *SchedulerCache) updatePod(oldPod, newPod *v1.Pod) error {
- if err := cache.removePod(oldPod); err != nil {
- return err
+func (cache *SchedulerCache) updatePod(oldPod, newPod *v1.Pod, key string) error {
+ var result error = nil
+ // remove old version from cache
+ if err := cache.removePod(oldPod, false); err != nil {
+ result = err
}
- cache.addPod(newPod)
- return nil
+ // add new version to cache
+ cache.addPod(newPod, key)
+ return result
}
func (cache *SchedulerCache) RemovePod(pod *v1.Pod) error {
cache.lock.Lock()
defer cache.lock.Unlock()
- return cache.removePod(pod)
+ return cache.removePod(pod, true)
}
-func (cache *SchedulerCache) removePod(pod *v1.Pod) error {
+func (cache *SchedulerCache) removePod(pod *v1.Pod, removeAlloc bool) error {
+ if removeAlloc {
+ // remove pod from any pending or in-progress allocations
+ delete(cache.pendingAllocations, string(pod.UID))
+ delete(cache.inProgressAllocations, string(pod.UID))
+ }
+
+ // remove pod from cache
+ delete(cache.podsMap, string(pod.UID))
+
+ // remove pod from node
Review comment:
Here we make sure to remove from cache regardless so that we don't leak resources.
--
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 #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r802130542
##########
File path: pkg/cache/context.go
##########
@@ -271,13 +284,7 @@ func (ctx *Context) updatePodInCache(oldObj, newObj interface{}) {
func (ctx *Context) filterPods(obj interface{}) bool {
switch obj := obj.(type) {
case *v1.Pod:
- // if a terminated pod is added to cache, it will
- // add requested resource to the cached node, causing
- // the node uses more resources that it actually is,
- // this can only be fixed after the pod is removed.
- // (trigger the delete pod)
- return utils.GeneralPodFilter(obj) &&
- !utils.IsPodTerminated(obj)
+ return utils.GeneralPodFilter(obj)
Review comment:
This logic gets moved to `addPodToCache()` / `updatePodInCache()` as we DO want to allow terminated pods to flow through to `removePodFromCache()`. The old logic filtered these out from all methods, resulting in leaks of terminated pods.
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (12b0a81) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/87050bb340d1f6228c736a1c1d8a41978e30d6dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87050bb) will **increase** coverage by `1.97%`.
> The diff coverage is `79.51%`.
> :exclamation: Current head 12b0a81 differs from pull request most recent head 4e19f33. Consider uploading reports for the commit 4e19f33 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.39% +1.97%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3960 +120
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `57.62% <76.71%> (+26.70%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [87050bb...4e19f33](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9c8d88b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.97%`.
> The diff coverage is `79.51%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.39% +1.97%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3960 +120
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `57.62% <76.71%> (+26.70%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...9c8d88b](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] yangwwei commented on a change in pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r804375007
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -255,26 +246,25 @@ func (cache *SchedulerCache) UpdatePod(oldPod, newPod *v1.Pod) error {
defer cache.lock.Unlock()
currState, ok := cache.podsMap[key]
- switch {
- // An assumed pod won't have Update/Remove event. It needs to have Add event
- // before Update event, in which case the state would change from Assumed to Added.
- case ok && !cache.isAssumedPod(key):
- if currState.Spec.NodeName != newPod.Spec.NodeName {
- log.Logger().Error("pod updated on a different node than previously added to", zap.String("pod", key))
- log.Logger().Error("scheduler cache is corrupted and can badly affect scheduling decisions")
- }
- if err = cache.updatePod(oldPod, newPod); err != nil {
- return err
+ if ok {
+ // pod exists and is assumed
+ if !cache.isAssumedPod(key) && currState.Spec.NodeName != newPod.Spec.NodeName {
Review comment:
Refer to [cache.go#L516-L523](https://github.com/kubernetes/kubernetes/blob/3866cb91f22da6eb49dab10dd5c33385690b57b4/pkg/scheduler/internal/cache/cache.go#L516-L523), the default side, it only updates the pod when the pod is not assumed. But our logic is updating the pod no matter if it is assumed or not.
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -217,30 +217,21 @@ func (cache *SchedulerCache) AddPod(pod *v1.Pod) error {
defer cache.lock.Unlock()
currState, ok := cache.podsMap[key]
- switch {
- case ok && cache.isAssumedPod(key):
- if currState.Spec.NodeName != pod.Spec.NodeName {
- // The pod was added to a different node than it was assumed to.
- log.Logger().Warn("inconsistent pod location",
- zap.String("assumedLocation", pod.Spec.NodeName),
- zap.String("actualLocation", currState.Spec.NodeName))
-
- // Clean this up.
- err = cache.removePod(currState)
- if err != nil {
- log.Logger().Debug("node not in cache",
- zap.Error(err))
+ if ok {
+ // pod exists
+ if cache.isAssumedPod(key) {
Review comment:
Refer to [cache.go#L481-492](https://github.com/kubernetes/kubernetes/blob/3866cb91f22da6eb49dab10dd5c33385690b57b4/pkg/scheduler/internal/cache/cache.go#L481-L492), there is a slight difference:
in the default scheduler side, it only updates the pod when the pod.sepc.NodeName changes and the pod is assumed. In our code, we do the update no matter if the pod is assumed or not.
Another thing, when the pod node name is unchanged, and the pod is assumed, it deletes the pod from the cache. [cache.go#489](https://github.com/kubernetes/kubernetes/blob/3866cb91f22da6eb49dab10dd5c33385690b57b4/pkg/scheduler/internal/cache/cache.go#L489). We no longer have this logic anywhere.
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (296d57d) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `0.86%`.
> The diff coverage is `48.71%`.
> :exclamation: Current head 296d57d differs from pull request most recent head 5e69de9. Consider uploading reports for the commit 5e69de9 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 63.28% +0.86%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3892 +52
+ Misses 2160 2103 -57
- Partials 152 155 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `33.46% <37.50%> (+2.54%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.18% <100.00%> (+6.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...5e69de9](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9c8d88b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.97%`.
> The diff coverage is `79.51%`.
> :exclamation: Current head 9c8d88b differs from pull request most recent head 12b0a81. Consider uploading reports for the commit 12b0a81 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.39% +1.97%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3960 +120
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `57.62% <76.71%> (+26.70%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...12b0a81](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 commented on a change in pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r804383916
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -217,30 +217,21 @@ func (cache *SchedulerCache) AddPod(pod *v1.Pod) error {
defer cache.lock.Unlock()
currState, ok := cache.podsMap[key]
- switch {
- case ok && cache.isAssumedPod(key):
- if currState.Spec.NodeName != pod.Spec.NodeName {
- // The pod was added to a different node than it was assumed to.
- log.Logger().Warn("inconsistent pod location",
- zap.String("assumedLocation", pod.Spec.NodeName),
- zap.String("actualLocation", currState.Spec.NodeName))
-
- // Clean this up.
- err = cache.removePod(currState)
- if err != nil {
- log.Logger().Debug("node not in cache",
- zap.Error(err))
+ if ok {
+ // pod exists
+ if cache.isAssumedPod(key) {
Review comment:
> in the default scheduler side, it only updates the pod when the pod.sepc.NodeName changes and the pod is assumed. In our code, we do the update no matter if the pod is assumed or not.
Original code then gets out of sync whenever a pod is assumed. There is no reason to do this, and in the case where the pod gets I assumed we are now stale. We should keep the new code.
> Another thing, when the pod node name is unchanged, and the pod is assumed, it deletes the pod from the cache. [cache.go#489](https://github.com/kubernetes/kubernetes/blob/3866cb91f22da6eb49dab10dd5c33385690b57b4/pkg/scheduler/internal/cache/cache.go#L489). We no longer have this logic anywhere.
This is even worse. Now we lose a pod entirely while we have an active allocation. This also has run-on effects in plug-in mode because we don't explicitly control the bind phase and are reliant on informer updates to keep us synchronized. Again, new code is necessary.
We are not a carbon-copy of upstream K8s and I'm not willing to keep obviously broken code simply because that's how upstream does it. Caches are meant to reflect the things they cache. Any conditional updates de-synchronize it and make it useless. The original code tries to be entirely too clever by half and resulted in a massive memory leak due to that complexity.
--
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 pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1034092400
Added unit tests for both `context` and `scheduler_cache`. Also further simplified some of the error handling in `scheduler_cache`.
--
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 pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1034332867
Rebased on master after YUNIKORN-1074, and fixed breakage affecting plugin mode.
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e19f33) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/87050bb340d1f6228c736a1c1d8a41978e30d6dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87050bb) will **increase** coverage by `1.80%`.
> The diff coverage is `69.01%`.
> :exclamation: Current head 4e19f33 differs from pull request most recent head ae9a0a5. Consider uploading reports for the commit ae9a0a5 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.22% +1.80%
==========================================
Files 41 41
Lines 6152 6138 -14
==========================================
+ Hits 3840 3942 +102
+ Misses 2160 2042 -118
- Partials 152 154 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `53.30% <63.93%> (+22.39%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [87050bb...ae9a0a5](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] commented on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db9f473) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `0.11%`.
> The diff coverage is `30.76%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 62.53% +0.11%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3846 +6
+ Misses 2160 2152 -8
Partials 152 152
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/incubator-yunikorn-k8shim/pull/365/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=) | `34.69% <0.00%> (ø)` | |
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `33.46% <37.50%> (+2.54%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...db9f473](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e69de9) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.98%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.40% +1.98%
==========================================
Files 41 41
Lines 6152 6153 +1
==========================================
+ Hits 3840 3963 +123
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `58.08% <77.33%> (+27.17%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...5e69de9](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e69de9) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.98%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.40% +1.98%
==========================================
Files 41 41
Lines 6152 6153 +1
==========================================
+ Hits 3840 3963 +123
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `58.08% <77.33%> (+27.17%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...5e69de9](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 commented on a change in pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r802131870
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -353,22 +359,15 @@ func (cache *SchedulerCache) ForgetPod(pod *v1.Pod) error {
currState, ok := cache.podsMap[key]
if ok && currState.Spec.NodeName != pod.Spec.NodeName {
- return fmt.Errorf("pod %v was assumed on %v but assigned to %v",
- key, pod.Spec.NodeName, currState.Spec.NodeName)
+ log.Logger().Warn("pod was assumed on one node but found on another",
+ zap.String("pod", key),
+ zap.String("expectedNode", currState.Spec.NodeName),
+ zap.String("actualNode", pod.Spec.NodeName))
}
- switch {
- // Only assumed pod can be forgotten.
- case ok && cache.isAssumedPod(key):
- err = cache.removePod(pod)
- if err != nil {
- return err
- }
- delete(cache.assumedPods, key)
- delete(cache.podsMap, key)
- default:
- return fmt.Errorf("pod %v wasn't assumed so cannot be forgotten", key)
- }
+ delete(cache.assumedPods, key)
+ delete(cache.pendingAllocations, key)
+ delete(cache.inProgressAllocations, key)
Review comment:
Removed most of this logic as it's just horribly broken. We now ensure that we cleanup the cache regardless of its current state, and we log a warning if the pod was assigned to an unexpected node instead of throwing an error. Also, we no longer remove the pod from the cache, we just forget its allocations (which is what this function is meant to do anyway). All removals are now in `removePod()` where they belong.
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (db9f473) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `0.11%`.
> The diff coverage is `30.76%`.
> :exclamation: Current head db9f473 differs from pull request most recent head 296d57d. Consider uploading reports for the commit 296d57d to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 62.53% +0.11%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3846 +6
+ Misses 2160 2152 -8
Partials 152 152
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/incubator-yunikorn-k8shim/pull/365/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=) | `34.69% <0.00%> (ø)` | |
| [pkg/cache/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `33.46% <37.50%> (+2.54%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...296d57d](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (4e19f33) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/87050bb340d1f6228c736a1c1d8a41978e30d6dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87050bb) will **increase** coverage by `1.80%`.
> The diff coverage is `69.01%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.22% +1.80%
==========================================
Files 41 41
Lines 6152 6138 -14
==========================================
+ Hits 3840 3942 +102
+ Misses 2160 2042 -118
- Partials 152 154 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `53.30% <63.93%> (+22.39%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [87050bb...4e19f33](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 commented on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1040524927
> @craigcondit can you please check if we need to do a set node here as well to avoid NPE? It is similar to what you have fixed in the 3rd commit.
We don't need anything here, as we call SetNode() further down, which takes care of the issue.
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9c8d88b) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.97%`.
> The diff coverage is `79.51%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.39% +1.97%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3960 +120
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `57.62% <76.71%> (+26.70%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...9c8d88b](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 removed a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit removed a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1034179427
I get a clean e2e test run locally, and build/unit tests pass locally and here. We seem to have some reliability problems with the e2e test runs. I've observed this at times locally, and it seems to be that the kind cluster doesn't always start up cleanly or in a timely manner, and so the initial development namespace fails to create. The failures are not YK-related.
--
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 #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on a change in pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#discussion_r802131870
##########
File path: pkg/cache/external/scheduler_cache.go
##########
@@ -353,22 +359,15 @@ func (cache *SchedulerCache) ForgetPod(pod *v1.Pod) error {
currState, ok := cache.podsMap[key]
if ok && currState.Spec.NodeName != pod.Spec.NodeName {
- return fmt.Errorf("pod %v was assumed on %v but assigned to %v",
- key, pod.Spec.NodeName, currState.Spec.NodeName)
+ log.Logger().Warn("pod was assumed on one node but found on another",
+ zap.String("pod", key),
+ zap.String("expectedNode", currState.Spec.NodeName),
+ zap.String("actualNode", pod.Spec.NodeName))
}
- switch {
- // Only assumed pod can be forgotten.
- case ok && cache.isAssumedPod(key):
- err = cache.removePod(pod)
- if err != nil {
- return err
- }
- delete(cache.assumedPods, key)
- delete(cache.podsMap, key)
- default:
- return fmt.Errorf("pod %v wasn't assumed so cannot be forgotten", key)
- }
+ delete(cache.assumedPods, key)
+ delete(cache.pendingAllocations, key)
+ delete(cache.inProgressAllocations, key)
Review comment:
Removed most of this logic as it's just horribly broken. We now ensure that we cleanup the cache regardless of its current state, and we log a warning if the pod was assigned to an unexpected node instead of throwing an error.
--
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 pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1034179427
I get a clean e2e test run locally, and build/unit tests pass locally and here. We seem to have some reliability problems with the e2e test runs. I've observed this at times locally, and it seems to be that the kind cluster doesn't always start up cleanly or in a timely manner, and so the initial development namespace fails to create. The failures are not YK-related.
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (12b0a81) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.97%`.
> The diff coverage is `79.51%`.
> :exclamation: Current head 12b0a81 differs from pull request most recent head e78a6a4. Consider uploading reports for the commit e78a6a4 to get more accurate results
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.39% +1.97%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3960 +120
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `57.62% <76.71%> (+26.70%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [87050bb...e78a6a4](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 commented on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1040524927
> @craigcondit can you please check if we need to do a set node here as well to avoid NPE? It is similar to what you have fixed in the 3rd commit.
We don't need anything here, as we call SetNode() further down, which takes care of the issue.
--
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 #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit closed pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365
--
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] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (296d57d) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `0.86%`.
> The diff coverage is `48.71%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 63.28% +0.86%
==========================================
Files 41 41
Lines 6152 6150 -2
==========================================
+ Hits 3840 3892 +52
+ Misses 2160 2103 -57
- Partials 152 155 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `33.46% <37.50%> (+2.54%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.18% <100.00%> (+6.48%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...296d57d](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (5e69de9) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/9256785133622b91f1e4a6f4cac6b580871ead93?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (9256785) will **increase** coverage by `1.98%`.
> The diff coverage is `80.00%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.40% +1.98%
==========================================
Files 41 41
Lines 6152 6153 +1
==========================================
+ Hits 3840 3963 +123
+ Misses 2160 2037 -123
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `58.08% <77.33%> (+27.17%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [9256785...5e69de9](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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] codecov[bot] edited a comment on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1033146171
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 [#365](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (ae9a0a5) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/87050bb340d1f6228c736a1c1d8a41978e30d6dd?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (87050bb) will **increase** coverage by `2.01%`.
> The diff coverage is `83.33%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 #365 +/- ##
==========================================
+ Coverage 62.41% 64.43% +2.01%
==========================================
Files 41 41
Lines 6152 6144 -8
==========================================
+ Hits 3840 3959 +119
+ Misses 2160 2032 -128
- Partials 152 153 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/external/scheduler\_cache.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL2V4dGVybmFsL3NjaGVkdWxlcl9jYWNoZS5nbw==) | `58.55% <80.88%> (+27.63%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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=) | `41.56% <100.00%> (+6.86%)` | :arrow_up: |
| [pkg/cache/node\_coordinator.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365/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-cGtnL2NhY2hlL25vZGVfY29vcmRpbmF0b3IuZ28=) | `66.21% <100.00%> (+2.92%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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/365?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 [87050bb...ae9a0a5](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/365?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 commented on pull request #365: [YUNIKORN-876] Fix pod memory leaks in scheduler cache
Posted by GitBox <gi...@apache.org>.
craigcondit commented on pull request #365:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/365#issuecomment-1036416347
Updated PR with fix for crash discovered by @brickyard, as well as additional unit tests to bring add/update/remove node methods in scheduler_cache to 100% branch coverage, verifying that patch fixes crash.
At this point we have far better tested code for this than upstream. @yangwwei, can you sign off on this? I know you have some concerns about the differences from upstream, but at this point I think they are theoretical. Let's address in a follow-up JIRA if we discover a regression.
--
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