Re: [PR] [SPARK-38079][K8S] Create the driver's config maps and mounted secret as pre-resources to avoid a mount race [spark]

["HwangDongJun (via GitHub)" <gi...@apache.org>]

HwangDongJun commented on code in PR #58187:
URL: https://github.com/apache/spark/pull/58187#discussion_r4163358689


##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -136,43 +188,75 @@ private[spark] class Client(
     val driverPodName = resolvedDriverPod.getMetadata.getName
 
     // setup resources before pod creation
-    val preKubernetesResources = resolvedDriverSpec.driverPreKubernetesResources
-    try {
-      kubernetesClient.resourceList(preKubernetesResources: _*).forceConflicts().serverSideApply()
-    } catch {
-      case NonFatal(e) =>
-        logError("Please check \"kubectl auth can-i create [resource]\" first." +
-          " It should be yes. And please also check your feature step implementation.")
-        kubernetesClient.resourceList(preKubernetesResources: _*).delete()
-        throw e
-    }
+    // SPARK-38079: the driver's own base config map (mounted as SPARK_CONF_VOLUME_DRIVER
+    // above) must also be created before the pod itself, to avoid a "configmap ... not
+    // found" mount race between the driver pod and the config map it depends on.
+    val preKubernetesResources = resolvedDriverSpec.driverPreKubernetesResources ++ Seq(configMap)
+
+    // SPARK-38079: some of the pre-resources above (e.g. the Kerberos keytab/delegation token
+    // secrets, the driver Kubernetes credentials secret) carry credentials. They are created
+    // here without an owner reference -- the owner reference is only added once the driver pod
+    // exists (see "Refresh all pre-resources' owner references" below), since Kubernetes owner
+    // references require the owner's UID, which does not exist before the pod is created. If
+    // this process is terminated abruptly in that window (e.g. Ctrl-C, SIGTERM, or a fatal JVM
+    // error), the pre-resources would otherwise be silently orphaned in the namespace forever,
+    // since Kubernetes only garbage-collects via owner references. This shutdown hook makes a
+    // best-effort attempt to delete them (and the driver pod, if this submission created it) in
+    // that case. It is a no-op -- and removed entirely, see the `finally` below -- once the
+    // owner-reference refresh has completed; from that point on the existing owner references
+    // make normal Kubernetes garbage collection sufficient, and e.g. Ctrl-C while waiting for
+    // the application to complete must keep today's behavior of just detaching.
+    val preResourcesApplied = new AtomicBoolean(false)
+    val podCreatedByUs = new AtomicBoolean(false)
+    val cleanupHookRef = shutdownHookOps.addHook(() => cleanupOrphanedPreResources(
+      preKubernetesResources, driverPodName, preResourcesApplied.get(), podCreatedByUs.get()))
 
     var watch: Watch = null
     var createdDriverPod: Pod = null
     try {
-      createdDriverPod =
-        kubernetesClient.pods().inNamespace(conf.namespace).resource(resolvedDriverPod).create()
-    } catch {
-      case NonFatal(e) =>
-        kubernetesClient.resourceList(preKubernetesResources: _*).delete()
-        logError("Please check \"kubectl auth can-i create pod\" first. It should be yes.")
-        throw e
-    }
+      try {
+        kubernetesClient.resourceList(preKubernetesResources: _*).forceConflicts().serverSideApply()
+        preResourcesApplied.set(true)
+      } catch {
+        case NonFatal(e) =>
+          logError("Please check \"kubectl auth can-i create [resource]\" first." +
+            " It should be yes. And please also check your feature step implementation.")
+          kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+          throw e
+      }
 
-    // Refresh all pre-resources' owner references
-    try {
-      addOwnerReference(createdDriverPod, preKubernetesResources)
-      kubernetesClient.resourceList(preKubernetesResources: _*).forceConflicts().serverSideApply()
-    } catch {
-      case NonFatal(e) =>
-        kubernetesClient.pods().resource(createdDriverPod).delete()
-        kubernetesClient.resourceList(preKubernetesResources: _*).delete()
-        throw e
+      try {
+        createdDriverPod =
+          kubernetesClient.pods().inNamespace(conf.namespace).resource(resolvedDriverPod).create()
+        podCreatedByUs.set(true)
+      } catch {
+        case NonFatal(e) =>
+          kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+          logError("Please check \"kubectl auth can-i create pod\" first. It should be yes.")
+          throw e
+      }
+
+      // Refresh all pre-resources' owner references
+      try {
+        addOwnerReference(createdDriverPod, preKubernetesResources)
+        kubernetesClient.resourceList(preKubernetesResources: _*).forceConflicts().serverSideApply()

Review Comment:
   Thanks for the detailed analysis — you're right, and I've reworked this to avoid the UPDATE path entirely.
   
   The driver pod is now created first, gated with `spec.schedulingGates` (`PRE_RESOURCES_SCHEDULING_GATE`) so the scheduler/kubelet cannot act on it yet. Once the pod exists, its UID is known, so every pre-resource's owner reference is set *before* the single call that creates it (`addOwnerReference` followed by one `resourceList(...).serverSideApply()`). That's a CREATE, not an UPDATE, so `OwnerReferencesPermissionEnforcement` never requires `delete` permission here — same as on master today.
   
   The gate is removed (via `.edit()`) only after that pre-resource creation succeeds, so the pod is never schedulable before its pre-resources exist, and no pre-resource is ever ownerless. If pre-resource creation fails, both the pod and whatever pre-resources made it to the server are deleted (see the updated `deletePodAndPreResources`, now per-item so one failure doesn't block the rest — addresses your other comment below too).
   
   Updated `KubernetesClientApplication.scala` (around L140-219) and `ClientSuite.scala` accordingly.



-- 
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@spark.apache.org

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