You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2021/02/19 15:37:59 UTC
[GitHub] [incubator-yunikorn-k8shim] kingamarton opened a new pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
kingamarton opened a new pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232
Filled ownreference field for the placeholder 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.
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 #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (3f35f6c) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.63%`.
> The diff coverage is `57.48%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
- Coverage 59.75% 59.12% -0.64%
==========================================
Files 35 35
Lines 3133 3229 +96
==========================================
+ Hits 1872 1909 +37
- Misses 1180 1237 +57
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `70.40% <ø> (-4.00%)` | :arrow_down: |
| [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `43.33% <8.33%> (-9.73%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `72.84% <64.17%> (-3.90%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `63.15% <80.00%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [b8b95bf...00aeaad](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (3f35f6c) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.63%`.
> The diff coverage is `57.48%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
- Coverage 59.75% 59.12% -0.64%
==========================================
Files 35 35
Lines 3133 3229 +96
==========================================
+ Hits 1872 1909 +37
- Misses 1180 1237 +57
- Partials 81 83 +2
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `70.40% <ø> (-4.00%)` | :arrow_down: |
| [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `43.33% <8.33%> (-9.73%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `72.84% <64.17%> (-3.90%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `63.15% <80.00%> (ø)` | |
| ... and [8 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [b8b95bf...3f35f6c](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (1640c97) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **increase** coverage by `0.24%`.
> The diff coverage is `52.63%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
+ Coverage 59.75% 59.99% +0.24%
==========================================
Files 35 35
Lines 3133 3137 +4
==========================================
+ Hits 1872 1882 +10
+ Misses 1180 1174 -6
Partials 81 81
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `79.36% <ø> (-0.33%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/placeholder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyLmdv) | `97.29% <75.00%> (-2.71%)` | :arrow_down: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `59.74% <100.00%> (+0.26%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.86% <100.00%> (+1.11%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `41.21% <100.00%> (+0.13%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [5dc97fa...3401f41](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#discussion_r595331685
##########
File path: pkg/appmgmt/general/general.go
##########
@@ -123,20 +126,40 @@ func (os *Manager) getAppMetadata(pod *v1.Pod) (interfaces.ApplicationMetadata,
if err != nil {
log.Logger().Error("unable to get taskGroups by given pod", zap.Error(err))
}
+ ownerReferences := getOwnerReferences(pod)
+
placeholderTimeout, err := utils.GetPlaceholderTimeoutParam(pod)
if err != nil {
log.Logger().Warn("unable to get placeholder timeout by given pod.", zap.Error(err))
}
return interfaces.ApplicationMetadata{
- ApplicationID: appId,
- QueueName: utils.GetQueueNameFromPod(pod),
- User: user,
- Tags: tags,
- TaskGroups: taskGroups,
+ ApplicationID: appId,
+ QueueName: utils.GetQueueNameFromPod(pod),
+ User: user,
+ Tags: tags,
+ TaskGroups: taskGroups,
PlaceholderTimeoutInSec: placeholderTimeout,
+ OwnerReferences: ownerReferences,
}, true
}
+func getOwnerReferences(pod *v1.Pod) []metav1.OwnerReference {
Review comment:
pls add comments to this function, to explain how we decide what owner-reference will be set for the placeholders.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] wilfred-s commented on a change in pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
wilfred-s commented on a change in pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#discussion_r585440436
##########
File path: pkg/cache/placeholder.go
##########
@@ -36,6 +36,11 @@ type Placeholder struct {
}
func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1.TaskGroup) *Placeholder {
+ ownerRef := app.placeholderOwnerReferences
Review comment:
nit: this is a list of references, should call this `ownerRefs` plural
##########
File path: pkg/cache/placeholder_manager_test.go
##########
@@ -36,12 +36,69 @@ import (
"github.com/apache/incubator-yunikorn-k8shim/pkg/common/constants"
)
+const (
+ appID = "app01"
+ queue = "root.default"
+ namespace = "test"
+)
+
func TestCreateAppPlaceholders(t *testing.T) {
- const (
- appID = "app01"
- queue = "root.default"
- namespace = "test"
- )
+ app := createAppWIthTaskGroupForTest()
+ mockedAPIProvider := client.NewMockedAPIProvider()
+ createdPods := createAndCheckPlaceholderCreate(mockedAPIProvider, app, t)
+ for _, pod := range createdPods {
+ assert.Assert(t, len(pod.OwnerReferences) == 0, "By default the pod should not have owner reference set")
+ }
+
+ // simulate placeholder creation failures
+ // failed to create one placeholder
+ mockedAPIProvider.MockCreateFn(func(pod *v1.Pod) (*v1.Pod, error) {
+ if pod.Name == "tg-test-group-2-app01-15" {
+ return nil, fmt.Errorf("failed to create pod %s", pod.Name)
+ }
+ return pod, nil
+ })
+ err := placeholderMgr.createAppPlaceholders(app)
+ assert.Error(t, err, "failed to create pod tg-test-group-2-app01-15")
+}
+
+func createAndCheckPlaceholderCreate(mockedAPIProvider *client.MockedAPIProvider, app *Application, t *testing.T) map[string]*v1.Pod {
+ createdPods := make(map[string]*v1.Pod)
+ mockedAPIProvider.MockCreateFn(func(pod *v1.Pod) (*v1.Pod, error) {
+ createdPods[pod.Name] = pod
+ return pod, nil
+ })
+ placeholderMgr = &PlaceholderManager{
+ clients: mockedAPIProvider.GetAPIs(),
+ RWMutex: sync.RWMutex{},
+ }
+
+ err := placeholderMgr.createAppPlaceholders(app)
+ assert.NilError(t, err, "create app placeholders should be successful")
+ assert.Equal(t, len(createdPods), 30)
+ return createdPods
+}
+
+func TestCreateAppPlaceholdersWithOwnReference(t *testing.T) {
+ app := createAppWIthTaskGroupForTest()
+ controller := true
+ ownRef := apis.OwnerReference{
+ Name: "JobId",
+ UID: "JobUid",
+ Controller: &controller,
+ }
+ app.setOwnReference([]apis.OwnerReference{ownRef})
+ mockedAPIProvider := client.NewMockedAPIProvider()
+ pods := createAndCheckPlaceholderCreate(mockedAPIProvider, app, t)
+ for _, pod := range pods {
+ assert.Assert(t, len(pod.OwnerReferences) == 1, "The pod should have exactly one owner reference set")
+ assert.Assert(t, *pod.OwnerReferences[0].Controller == false, "The owner reference should not be a controller")
+ assert.Assert(t, pod.OwnerReferences[0].Name == ownRef.Name, "The owner reference name does not match")
+ assert.Assert(t, pod.OwnerReferences[0].UID == ownRef.UID, "The owner reference UID does not match")
Review comment:
nit should we not use `assert.Equals()` all comparisons are simple values?
##########
File path: pkg/cache/application.go
##########
@@ -208,6 +210,12 @@ func (app *Application) getTaskGroups() []v1alpha1.TaskGroup {
return app.taskGroups
}
+func (app *Application) setOwnReference(ref []metav1.OwnerReference) {
Review comment:
nit: can we name this `setOwnerReference()`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (3401f41) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **increase** coverage by `0.27%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
+ Coverage 59.75% 60.02% +0.27%
==========================================
Files 35 35
Lines 3133 3137 +4
==========================================
+ Hits 1872 1883 +11
+ Misses 1180 1173 -7
Partials 81 81
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `79.36% <ø> (-0.33%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `59.74% <100.00%> (+0.26%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.86% <100.00%> (+1.11%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `41.21% <100.00%> (+0.13%)` | :arrow_up: |
| [pkg/cache/placeholder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyLmdv) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [b8b95bf...3f35f6c](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei merged pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
yangwwei merged pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#discussion_r595313058
##########
File path: pkg/cache/placeholder.go
##########
@@ -46,6 +46,11 @@ type Placeholder struct {
}
func newPlaceholder(placeholderName string, app *Application, taskGroup v1alpha1.TaskGroup) *Placeholder {
+ ownerRefs := app.placeholderOwnerReferences
+ controller := false
+ for _, r := range ownerRefs {
+ *r.Controller = controller
+ }
Review comment:
we are setting the controller field to false all the time.
could you pls add some more comments why we are doing this?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] commented on pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] commented on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (1640c97) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **increase** coverage by `0.24%`.
> The diff coverage is `52.63%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
+ Coverage 59.75% 59.99% +0.24%
==========================================
Files 35 35
Lines 3133 3137 +4
==========================================
+ Hits 1872 1882 +10
+ Misses 1180 1174 -6
Partials 81 81
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `79.36% <ø> (-0.33%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/placeholder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyLmdv) | `97.29% <75.00%> (-2.71%)` | :arrow_down: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `59.74% <100.00%> (+0.26%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.86% <100.00%> (+1.11%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `41.21% <100.00%> (+0.13%)` | :arrow_up: |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [5dc97fa...1640c97](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (3401f41) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **increase** coverage by `0.27%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
+ Coverage 59.75% 60.02% +0.27%
==========================================
Files 35 35
Lines 3133 3137 +4
==========================================
+ Hits 1872 1883 +11
+ Misses 1180 1173 -7
Partials 81 81
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `79.36% <ø> (-0.33%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `59.74% <100.00%> (+0.26%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.86% <100.00%> (+1.11%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `41.21% <100.00%> (+0.13%)` | :arrow_up: |
| [pkg/cache/placeholder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyLmdv) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [5dc97fa...3401f41](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] yangwwei commented on a change in pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
yangwwei commented on a change in pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#discussion_r579607351
##########
File path: pkg/appmgmt/general/general.go
##########
@@ -130,6 +130,7 @@ func (os *Manager) getAppMetadata(pod *v1.Pod) (interfaces.ApplicationMetadata,
User: user,
Tags: tags,
TaskGroups: taskGroups,
+ PlaceholderOwnerReferences: pod.OwnerReferences,
Review comment:
I think we can just call this owner reference since this is a general K8s concept.
It doesn't mean only placeholder pod has that.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (3401f41) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **increase** coverage by `0.27%`.
> The diff coverage is `66.66%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
+ Coverage 59.75% 60.02% +0.27%
==========================================
Files 35 35
Lines 3133 3137 +4
==========================================
+ Hits 1872 1883 +11
+ Misses 1180 1173 -7
Partials 81 81
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/cache/placeholder\_manager.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyX21hbmFnZXIuZ28=) | `79.36% <ø> (-0.33%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/appmgmt/general/general.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `59.74% <100.00%> (+0.26%)` | :arrow_up: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `77.86% <100.00%> (+1.11%)` | :arrow_up: |
| [pkg/cache/context.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2NvbnRleHQuZ28=) | `41.21% <100.00%> (+0.13%)` | :arrow_up: |
| [pkg/cache/placeholder.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3BsYWNlaG9sZGVyLmdv) | `100.00% <100.00%> (ø)` | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [5dc97fa...a2899e2](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-k8shim] codecov[bot] edited a comment on pull request #232: [YUNIKORN-521] Placeholder pods are not cleaned when the job is deleted
Posted by GitBox <gi...@apache.org>.
codecov[bot] edited a comment on pull request #232:
URL: https://github.com/apache/incubator-yunikorn-k8shim/pull/232#issuecomment-782158226
# [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=h1) Report
> Merging [#232](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=desc) (00aeaad) into [master](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/commit/c47ed51f075c5af5910f71da40e7e68699a9abae?el=desc) (c47ed51) will **decrease** coverage by `0.32%`.
> The diff coverage is `57.48%`.
[](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #232 +/- ##
==========================================
- Coverage 59.75% 59.43% -0.33%
==========================================
Files 35 35
Lines 3133 3229 +96
==========================================
+ Hits 1872 1919 +47
- Misses 1180 1228 +48
- Partials 81 82 +1
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [pkg/appmgmt/appmgmt\_recovery.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2FwcG1nbXQvYXBwbWdtdF9yZWNvdmVyeS5nbw==) | `67.50% <0.00%> (-8.18%)` | :arrow_down: |
| [pkg/cache/amprotocol\_mock.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FtcHJvdG9jb2xfbW9jay5nbw==) | `0.00% <0.00%> (ø)` | |
| [pkg/cache/task.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL3Rhc2suZ28=) | `74.40% <ø> (ø)` | |
| [pkg/common/resource.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9yZXNvdXJjZS5nbw==) | `90.72% <0.00%> (-9.28%)` | :arrow_down: |
| [pkg/common/utils/gang\_utils.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi91dGlscy9nYW5nX3V0aWxzLmdv) | `67.94% <0.00%> (-13.59%)` | :arrow_down: |
| [pkg/controller/application/app\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbnRyb2xsZXIvYXBwbGljYXRpb24vYXBwX2NvbnRyb2xsZXIuZ28=) | `71.05% <ø> (-0.26%)` | :arrow_down: |
| [...missioncontrollers/webhook/admission\_controller.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL3BsdWdpbi9hZG1pc3Npb25jb250cm9sbGVycy93ZWJob29rL2FkbWlzc2lvbl9jb250cm9sbGVyLmdv) | `33.74% <0.00%> (+1.00%)` | :arrow_up: |
| [pkg/cache/application\_events.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uX2V2ZW50cy5nbw==) | `43.33% <8.33%> (-9.73%)` | :arrow_down: |
| [pkg/cache/application.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NhY2hlL2FwcGxpY2F0aW9uLmdv) | `72.84% <64.17%> (-3.90%)` | :arrow_down: |
| [pkg/common/si\_helper.go](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree#diff-cGtnL2NvbW1vbi9zaV9oZWxwZXIuZ28=) | `63.15% <80.00%> (ø)` | |
| ... and [7 more](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=footer). Last update [b8b95bf...00aeaad](https://codecov.io/gh/apache/incubator-yunikorn-k8shim/pull/232?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org