You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by "zhuqi-lucas (via GitHub)" <gi...@apache.org> on 2023/05/03 12:42:26 UTC
[GitHub] [yunikorn-k8shim] zhuqi-lucas opened a new pull request, #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
zhuqi-lucas opened a new pull request, #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584
### What is this PR for?
In AWS EMR on EKS service, the driver real pod's ownerReference is configmap.
And placeholder's ownerReference is also the driver configmap.
When user cancels emr-containers job, the job-submitter is terminated,
but the placeholder still remains in pending state.
https://docs.aws.amazon.com/emr/latest/EMR-on-EKS-DevelopmentGuide/emr-eks.html
Make sure configmap owner reference is not returned for pod, it will return the real driver pod itself
because when the placeholder pod set ownerreference to configmap, the configmap will not be deleted for placeholder pod
to GC, we need set the placeholder pod's ownerreference to the real driver pod, when the real driver pod is deleted, the
placeholder will be deleted as well.
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - Improvement
* [ ] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
* Open an issue on Jira https://issues.apache.org/jira/browse/YUNIKORN/
* Put link here, and add [YUNIKORN-*Jira number*] in PR title, eg. `[YUNIKORN-2] Gang scheduling interface parameters`
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on a diff in pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#discussion_r1193853822
##########
pkg/appmgmt/general/general_test.go:
##########
@@ -208,10 +210,12 @@ func TestOriginatorPod(t *testing.T) {
}
am.AddPod(&pod1)
assert.Equal(t, len(app.GetNewTasks()), 2)
- task, err = app.GetTask("UID-POD-00002")
+ task, err = app.GetTask("UID-POD-00001")
Review Comment:
Also fixed the e2e test about the originator change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] codecov[bot] commented on pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#issuecomment-1532983329
## [Codecov](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/584?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 [#584](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/584?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a347cac) into [master](https://app.codecov.io/gh/apache/yunikorn-k8shim/commit/523b6f023d8da78b8b5f2d604efe605a130cbaba?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (523b6f0) will **increase** coverage by `0.07%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head a347cac differs from pull request most recent head 9060530. Consider uploading reports for the commit 9060530 to get more accurate results
```diff
@@ Coverage Diff @@
## master #584 +/- ##
==========================================
+ Coverage 69.97% 70.04% +0.07%
==========================================
Files 47 47
Lines 7943 7956 +13
==========================================
+ Hits 5558 5573 +15
+ Misses 2177 2175 -2
Partials 208 208
```
| [Impacted Files](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/584?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/appmgmt/general/general.go](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/584?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGtnL2FwcG1nbXQvZ2VuZXJhbC9nZW5lcmFsLmdv) | `68.58% <100.00%> (+2.29%)` | :arrow_up: |
... and [1 file with indirect coverage changes](https://app.codecov.io/gh/apache/yunikorn-k8shim/pull/584/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#issuecomment-1541400027
Thank you @wilfred-s , i will check it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on a diff in pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#discussion_r1193398156
##########
pkg/appmgmt/general/general.go:
##########
@@ -104,9 +104,15 @@ func isStateAwareDisabled(pod *v1.Pod) bool {
return result
}
-func getOwnerReferences(pod *v1.Pod) []metav1.OwnerReference {
+func getOwnerReference(pod *v1.Pod) []metav1.OwnerReference {
if len(pod.OwnerReferences) > 0 {
- return pod.OwnerReferences
+ // Filter the pod kind owner reference
+ // keep this because we need to find the original pod
+ for _, ref := range pod.OwnerReferences {
+ if ref.Kind == reflect.TypeOf(v1.Pod{}).Name() {
+ return []metav1.OwnerReference{ref}
+ }
+ }
Review Comment:
Thank you @wilfred-s for review, addressed in latest PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#issuecomment-1541388045
See comments in the jira, different approach is needed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s commented on a diff in pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s commented on code in PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#discussion_r1193286464
##########
pkg/appmgmt/general/general_test.go:
##########
@@ -167,6 +167,7 @@ func TestOriginatorPod(t *testing.T) {
owner := apis.OwnerReference{
APIVersion: "v1",
UID: "UID-POD-00002",
+ Kind: "Pod",
}
refer := []apis.OwnerReference{
owner,
Review Comment:
use getOwnerReference to build this
##########
pkg/appmgmt/general/general.go:
##########
@@ -104,9 +104,15 @@ func isStateAwareDisabled(pod *v1.Pod) bool {
return result
}
-func getOwnerReferences(pod *v1.Pod) []metav1.OwnerReference {
+func getOwnerReference(pod *v1.Pod) []metav1.OwnerReference {
if len(pod.OwnerReferences) > 0 {
- return pod.OwnerReferences
+ // Filter the pod kind owner reference
+ // keep this because we need to find the original pod
+ for _, ref := range pod.OwnerReferences {
+ if ref.Kind == reflect.TypeOf(v1.Pod{}).Name() {
+ return []metav1.OwnerReference{ref}
+ }
+ }
Review Comment:
This is not needed, the call should only ever be made with the originator or owner pod.
Just always build a new reference without doing anything else.
NIT simplification:
`v1.SchemeGroupVersion.String()` is always "v1" unless we change the signature.
`reflect.TypeOf(v1.Pod{}).Name()` is always "Pod" nothing else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on a diff in pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#discussion_r1193398156
##########
pkg/appmgmt/general/general.go:
##########
@@ -104,9 +104,15 @@ func isStateAwareDisabled(pod *v1.Pod) bool {
return result
}
-func getOwnerReferences(pod *v1.Pod) []metav1.OwnerReference {
+func getOwnerReference(pod *v1.Pod) []metav1.OwnerReference {
if len(pod.OwnerReferences) > 0 {
- return pod.OwnerReferences
+ // Filter the pod kind owner reference
+ // keep this because we need to find the original pod
+ for _, ref := range pod.OwnerReferences {
+ if ref.Kind == reflect.TypeOf(v1.Pod{}).Name() {
+ return []metav1.OwnerReference{ref}
+ }
+ }
Review Comment:
Thank you @wilfred-s for review, addesseed in latest PR!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on a diff in pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#discussion_r1193400578
##########
pkg/appmgmt/general/general_test.go:
##########
@@ -208,10 +210,12 @@ func TestOriginatorPod(t *testing.T) {
}
am.AddPod(&pod1)
assert.Equal(t, len(app.GetNewTasks()), 2)
- task, err = app.GetTask("UID-POD-00002")
+ task, err = app.GetTask("UID-POD-00001")
Review Comment:
Hi @wilfred-s , if we always return the pod, the test here the originator will be the pod1 who first add into cluster.
I think that's right, because we only assign the ownerreference to placeholders, the placeholders only have the originator pod as the owerreference!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] wilfred-s closed pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "wilfred-s (via GitHub)" <gi...@apache.org>.
wilfred-s closed pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
URL: https://github.com/apache/yunikorn-k8shim/pull/584
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-k8shim] zhuqi-lucas commented on a diff in pull request #584: [YUNIKORN-1708] Filtered owner references for placeholder pods.
Posted by "zhuqi-lucas (via GitHub)" <gi...@apache.org>.
zhuqi-lucas commented on code in PR #584:
URL: https://github.com/apache/yunikorn-k8shim/pull/584#discussion_r1193400578
##########
pkg/appmgmt/general/general_test.go:
##########
@@ -208,10 +210,12 @@ func TestOriginatorPod(t *testing.T) {
}
am.AddPod(&pod1)
assert.Equal(t, len(app.GetNewTasks()), 2)
- task, err = app.GetTask("UID-POD-00002")
+ task, err = app.GetTask("UID-POD-00001")
Review Comment:
Hi @wilfred-s , if we always return the pod, the test here the originator will be the pod1 who first add into cluster.
--
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