You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by GitBox <gi...@apache.org> on 2020/02/10 10:05:37 UTC

[GitHub] [spark] ScrapCodes opened a new pull request #27520: [SPARK-30771] Avoid failed mount warning from kubernetes and support the optional mount.

ScrapCodes opened a new pull request #27520: [SPARK-30771] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520
 
 
   
   ### What changes were proposed in this pull request?
   
   This PR changes the way we create kubernetes resources, before this PR, we create the driver pod which has reference to various resources including mounted volumes and configmaps. After creating the driver pod, we create other resources. This PR simply reverses the order, it first creates the configmaps and then tries to mount them as k8s volumes.
   
   ```
   kubectl describe po spark-driver
   
   ```
   ### Before this PR
   <img width="1203" alt="Screenshot 2020-02-10 at 3 10 01 PM" src="https://user-images.githubusercontent.com/992952/74140116-7d1af200-4c1a-11ea-9180-1db1fb943094.png">
   
   ### After this PR
   <img width="1203" alt="Screenshot 2020-02-10 at 3 10 16 PM" src="https://user-images.githubusercontent.com/992952/74140123-80ae7900-4c1a-11ea-8e6e-12adba1bfd2b.png">
   
   ### Why are the changes needed?
   https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#configmapvolumesource-v1-core
   Kubernetes allows an optional field indicating, that if the mount for this config map fails, then it is not reattempted nor the pod is declared to be failed.
   In our current code base, we try to mount the volumes and create them later, it works because, kubernetes reattempts failed mounting attempt, because the `optional` field is `false` by default.
   But, if this optional field is set to true, then that mount will not take place at all. Because, when the mount is performed the volume is not created - so mount fails. And this time the mount is not reattempted because the optional field is set as true.
   
   ### Does this PR introduce any user-facing change?
   No
   
   ### How was this patch tested?
   I have tested the patch, by manually running with and without the change and observed the event log in kubectl describe.
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#discussion_r377883931
 
 

 ##########
 File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
 ##########
 @@ -127,15 +127,18 @@ private[spark] class Client(
         .pods()
         .withName(driverPodName)
         .watch(watcher)) { _ =>
-      val createdDriverPod = kubernetesClient.pods().create(resolvedDriverPod)
+      var createdDriverPod: Option[Pod] = None
       try {
         val otherKubernetesResources =
           resolvedDriverSpec.driverKubernetesResources ++ Seq(configMap)
-        addDriverOwnerReference(createdDriverPod, otherKubernetesResources)
         kubernetesClient.resourceList(otherKubernetesResources: _*).createOrReplace()
+        createdDriverPod = Some(kubernetesClient.pods().create(resolvedDriverPod))
+        addDriverOwnerReference(createdDriverPod.get, otherKubernetesResources)
 
 Review comment:
   Basically you will need to update the resources against the API server after the driver pod gets created. However, there's a potential risk here: if the creation of the driver pod failed for whatever reasons, the ownerRefs won't be added so garbage collection won't kick in and consequently the resources won't be properly cleaned up.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] ScrapCodes commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
ScrapCodes commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#discussion_r380498148
 
 

 ##########
 File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
 ##########
 @@ -127,15 +127,18 @@ private[spark] class Client(
         .pods()
         .withName(driverPodName)
         .watch(watcher)) { _ =>
-      val createdDriverPod = kubernetesClient.pods().create(resolvedDriverPod)
+      var createdDriverPod: Option[Pod] = None
       try {
         val otherKubernetesResources =
           resolvedDriverSpec.driverKubernetesResources ++ Seq(configMap)
-        addDriverOwnerReference(createdDriverPod, otherKubernetesResources)
         kubernetesClient.resourceList(otherKubernetesResources: _*).createOrReplace()
+        createdDriverPod = Some(kubernetesClient.pods().create(resolvedDriverPod))
+        addDriverOwnerReference(createdDriverPod.get, otherKubernetesResources)
 
 Review comment:
   @liyinan926 Thanks for helping me think through this, so far whatever solution I could think, I have no way to solve this problem without introducing another problem or risks. I will keep thinking, and if you get any hints, please help me with it. In the meantime, I am closing this 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584053136
 
 
   Merged build finished. Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] SparkQA commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584075499
 
 
   Kubernetes integration test status success
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/22912/
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584075584
 
 
   Merged build finished. Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] SparkQA commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584064556
 
 
   Kubernetes integration test starting
   URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/22912/
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] SparkQA commented on issue #27520: [SPARK-30771] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27520: [SPARK-30771] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584047138
 
 
   **[Test build #118150 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118150/testReport)** for PR 27520 at commit [`514cadd`](https://github.com/apache/spark/commit/514caddf6e022842c1a052c214d4bc78ed606e3d).

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#discussion_r378364228
 
 

 ##########
 File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
 ##########
 @@ -127,15 +127,18 @@ private[spark] class Client(
         .pods()
         .withName(driverPodName)
         .watch(watcher)) { _ =>
-      val createdDriverPod = kubernetesClient.pods().create(resolvedDriverPod)
+      var createdDriverPod: Option[Pod] = None
       try {
         val otherKubernetesResources =
           resolvedDriverSpec.driverKubernetesResources ++ Seq(configMap)
-        addDriverOwnerReference(createdDriverPod, otherKubernetesResources)
         kubernetesClient.resourceList(otherKubernetesResources: _*).createOrReplace()
+        createdDriverPod = Some(kubernetesClient.pods().create(resolvedDriverPod))
+        addDriverOwnerReference(createdDriverPod.get, otherKubernetesResources)
 
 Review comment:
   The idea of using a Job to run the driver has been rejected long time ago.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584053139
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118150/
   Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584075584
 
 
   Merged build finished. Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] ScrapCodes closed pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
ScrapCodes closed pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520
 
 
   

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] SparkQA commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
SparkQA commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584052987
 
 
   **[Test build #118150 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118150/testReport)** for PR 27520 at commit [`514cadd`](https://github.com/apache/spark/commit/514caddf6e022842c1a052c214d4bc78ed606e3d).
    * This patch passes all tests.
    * This patch merges cleanly.
    * This patch adds no public classes.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584053139
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/118150/
   Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] ScrapCodes commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
ScrapCodes commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584067631
 
 
   @liyinan926, Hi Yinan Li, Can you please take a look !

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584053136
 
 
   Merged build finished. Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] SparkQA removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
SparkQA removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584047138
 
 
   **[Test build #118150 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/118150/testReport)** for PR 27520 at commit [`514cadd`](https://github.com/apache/spark/commit/514caddf6e022842c1a052c214d4bc78ed606e3d).

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] ScrapCodes commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
ScrapCodes commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#discussion_r377536379
 
 

 ##########
 File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
 ##########
 @@ -127,15 +127,18 @@ private[spark] class Client(
         .pods()
         .withName(driverPodName)
         .watch(watcher)) { _ =>
-      val createdDriverPod = kubernetesClient.pods().create(resolvedDriverPod)
+      var createdDriverPod: Option[Pod] = None
       try {
         val otherKubernetesResources =
           resolvedDriverSpec.driverKubernetesResources ++ Seq(configMap)
-        addDriverOwnerReference(createdDriverPod, otherKubernetesResources)
         kubernetesClient.resourceList(otherKubernetesResources: _*).createOrReplace()
+        createdDriverPod = Some(kubernetesClient.pods().create(resolvedDriverPod))
+        addDriverOwnerReference(createdDriverPod.get, otherKubernetesResources)
 
 Review comment:
   Hi Yinan Li, thanks a lot for taking the look here. You are right here. But, I was wondering, can I do something to fix it. For example, if I do not create the driver pod before creating resources, the owner reference is not properly set up and if I do the other way, optional mount does not work(this 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#discussion_r377270019
 
 

 ##########
 File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
 ##########
 @@ -127,15 +127,18 @@ private[spark] class Client(
         .pods()
         .withName(driverPodName)
         .watch(watcher)) { _ =>
-      val createdDriverPod = kubernetesClient.pods().create(resolvedDriverPod)
+      var createdDriverPod: Option[Pod] = None
       try {
         val otherKubernetesResources =
           resolvedDriverSpec.driverKubernetesResources ++ Seq(configMap)
-        addDriverOwnerReference(createdDriverPod, otherKubernetesResources)
         kubernetesClient.resourceList(otherKubernetesResources: _*).createOrReplace()
+        createdDriverPod = Some(kubernetesClient.pods().create(resolvedDriverPod))
+        addDriverOwnerReference(createdDriverPod.get, otherKubernetesResources)
 
 Review comment:
   This only adds the `OwnerReference` to the resource metadata to the local copies of the resources in memory, without updating the resources to the API server.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] ScrapCodes commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
ScrapCodes commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#discussion_r378091309
 
 

 ##########
 File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
 ##########
 @@ -127,15 +127,18 @@ private[spark] class Client(
         .pods()
         .withName(driverPodName)
         .watch(watcher)) { _ =>
-      val createdDriverPod = kubernetesClient.pods().create(resolvedDriverPod)
+      var createdDriverPod: Option[Pod] = None
       try {
         val otherKubernetesResources =
           resolvedDriverSpec.driverKubernetesResources ++ Seq(configMap)
-        addDriverOwnerReference(createdDriverPod, otherKubernetesResources)
         kubernetesClient.resourceList(otherKubernetesResources: _*).createOrReplace()
+        createdDriverPod = Some(kubernetesClient.pods().create(resolvedDriverPod))
+        addDriverOwnerReference(createdDriverPod.get, otherKubernetesResources)
 
 Review comment:
   One way to avoid that risk is to have a [job](https://kubernetes.io/docs/concepts/workloads/controllers/jobs-run-to-completion/) and let the job own all the resources including the pod. What do you think?

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins commented on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584075597
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/22912/
   Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
AmplabJenkins removed a comment on issue #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#issuecomment-584075597
 
 
   Test PASSed.
   Refer to this link for build results (access rights to CI server needed): 
   https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder-K8s/22912/
   Test PASSed.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org


[GitHub] [spark] liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.

Posted by GitBox <gi...@apache.org>.
liyinan926 commented on a change in pull request #27520: [SPARK-30771][K8S] Avoid failed mount warning from kubernetes and support the optional mount.
URL: https://github.com/apache/spark/pull/27520#discussion_r377883931
 
 

 ##########
 File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
 ##########
 @@ -127,15 +127,18 @@ private[spark] class Client(
         .pods()
         .withName(driverPodName)
         .watch(watcher)) { _ =>
-      val createdDriverPod = kubernetesClient.pods().create(resolvedDriverPod)
+      var createdDriverPod: Option[Pod] = None
       try {
         val otherKubernetesResources =
           resolvedDriverSpec.driverKubernetesResources ++ Seq(configMap)
-        addDriverOwnerReference(createdDriverPod, otherKubernetesResources)
         kubernetesClient.resourceList(otherKubernetesResources: _*).createOrReplace()
+        createdDriverPod = Some(kubernetesClient.pods().create(resolvedDriverPod))
+        addDriverOwnerReference(createdDriverPod.get, otherKubernetesResources)
 
 Review comment:
   Basically you will need to update the resources against the API server after the driver pod gets created. However, there's a potential risk here: if the creation of the driver pod for whatever reasons, the ownerRefs won't be added so garbage collection won't kick in and consequently the resources won't be properly cleaned up.

----------------------------------------------------------------
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


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org