[GitHub] [spark] holdenk commented on a change in pull request #29846: [SPARK-32971][K8S] Support dynamic PVC creation/deletion for K8s executors

[GitBox <gi...@apache.org>]

holdenk commented on a change in pull request #29846:
URL: https://github.com/apache/spark/pull/29846#discussion_r493778976



##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/KubernetesUtils.scala
##########
@@ -323,4 +323,22 @@ private[spark] object KubernetesUtils extends Logging {
         .build()
     }
   }
+
+  // Add a OwnerReference to the given resources making the pod an owner of them so when
+  // the pod is deleted, the resources are garbage collected.
+  def addOwnerReference(pod: Pod, resources: Seq[HasMetadata]): Unit = {

Review comment:
       I like this :)
   Initially I was thinking about asking about re-using the PVCs because I know that in some systems PVC creating is slow (for dynamic scaling), but it's probably better to return the resources and not do some weird storage pool like I was thinking of.

##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/MountVolumesFeatureStep.scala
##########
@@ -84,4 +112,15 @@ private[spark] class MountVolumesFeatureStep(conf: KubernetesConf)
       (volumeMount, volume)
     }
   }
+
+  override def getAdditionalKubernetesResources(): Seq[HasMetadata] = {
+    additionalResources
+  }
+}
+
+private[spark] object MountVolumesFeatureStep {
+  val PVC_ON_DEMAND = "OnDemand"
+  val PVC = "PersistentVolumeClaim"
+  val PVC_POSTFIX = "-pvc"
+  val PVC_ACCESS_MODE = "ReadWriteOnce"

Review comment:
       Would it make sense to have the OnDemand, postfix, and access mode configurable?

##########
File path: resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala
##########
@@ -163,22 +165,6 @@ private[spark] class Client(
     }
   }
 
-  // Add a OwnerReference to the given resources making the driver pod an owner of them so when
-  // the driver pod is deleted, the resources are garbage collected.
-  private def addDriverOwnerReference(driverPod: Pod, resources: Seq[HasMetadata]): Unit = {
-    val driverPodOwnerReference = new OwnerReferenceBuilder()
-      .withName(driverPod.getMetadata.getName)
-      .withApiVersion(driverPod.getApiVersion)
-      .withUid(driverPod.getMetadata.getUid)
-      .withKind(driverPod.getKind)
-      .withController(true)
-      .build()
-    resources.foreach { resource =>
-      val originalMetadata = resource.getMetadata
-      originalMetadata.setOwnerReferences(Collections.singletonList(driverPodOwnerReference))
-    }
-  }
-

Review comment:
       Thank you for unifying this code :)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



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