HwangDongJun commented on code in PR #58187:
URL: https://github.com/apache/spark/pull/58187#discussion_r4310434030
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -131,48 +147,80 @@ private[spark] class Client(
.withName(configMapName)
.endConfigMap()
.endVolume()
+ .addToNodeSelector(
+ pinnedNodeName.map(nodeName => Map("kubernetes.io/hostname" ->
nodeName))
+ .getOrElse(Map.empty[String, String]).asJava)
+ // SPARK-38079: holds the pod unschedulable -- so kubelet cannot
attempt to mount
+ // anything on it -- until its pre-resources exist (see below). Only
removed once
+ // that is true, right before the watch loop; see the "Remove the
pre-resources
+ // scheduling gate" step below for why this makes an
ownerless-resource window,
+ // and its consequences (a shutdown hook), unnecessary.
+ .addNewSchedulingGate(PRE_RESOURCES_SCHEDULING_GATE)
.endSpec()
.build()
val driverPodName = resolvedDriverPod.getMetadata.getName
- // setup resources before pod creation
- val preKubernetesResources =
resolvedDriverSpec.driverPreKubernetesResources
+ // SPARK-38079: the driver's own base config map (mounted as
SPARK_CONF_VOLUME_DRIVER
+ // above) must also be created before the pod is schedulable, 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)
+
+ var watch: Watch = null
+ val createdDriverPod: Pod =
+ try {
+
kubernetesClient.pods().inNamespace(conf.namespace).resource(resolvedDriverPod).create()
+ } catch {
+ case NonFatal(e) =>
+ logError("Please check \"kubectl auth can-i create pod\" first. It
should be yes.")
+ throw e
+ }
+
+ // SPARK-38079: some of the pre-resources above (e.g. the Kerberos
keytab/delegation token
+ // secrets, the driver Kubernetes credentials secret) carry credentials.
Now that the
+ // driver pod exists, its UID is known, so each pre-resource's owner
reference can be set
+ // before -- and included in -- the single call that creates it. The pod
itself is still
+ // scheduling-gated, so nothing can be scheduled against it (and so
nothing can attempt to
+ // mount these resources) until they exist -- but they are never ownerless
at any point
+ // after this call: either it succeeds and every pre-resource already has
an owner
+ // reference, or it fails and none of them (that made it to the server)
are left
+ // referencing anything, since the driver pod that would have owned them
is deleted in the
+ // catch block below.
try {
+ addOwnerReference(createdDriverPod, preKubernetesResources)
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
- }
-
- 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.")
+ deletePodAndPreResources(createdDriverPod, preKubernetesResources)
throw e
}
- // Refresh all pre-resources' owner references
+ // SPARK-38079: remove the pre-resources scheduling gate now that its
pre-resources exist,
+ // letting the scheduler proceed with this pod. If this process is
terminated abruptly
+ // before this point (e.g. Ctrl-C, SIGTERM, or a fatal JVM error), the
driver pod is left
+ // behind still gated -- inert (kubelet cannot schedule/mount anything on
a gated pod) and
+ // visible as `Pending`/`SchedulingGated` via any standard pod listing.
That is a low-
+ // severity leak, in the same class Spark already accepts elsewhere for a
process killed
+ // right after pod creation (e.g. Ctrl-C before the watch loop below
starts), so no
+ // shutdown hook is registered to guard this window.
try {
- addOwnerReference(createdDriverPod, preKubernetesResources)
- kubernetesClient.resourceList(preKubernetesResources:
_*).forceConflicts().serverSideApply()
+
kubernetesClient.pods().inNamespace(conf.namespace).withName(driverPodName).edit(
+ (currentPod: Pod) => new PodBuilder(currentPod)
+ .editSpec()
+ .removeMatchingFromSchedulingGates(_.getName ==
PRE_RESOURCES_SCHEDULING_GATE)
Review Comment:
Switched to the strategic merge patch you proposed, sent through
`pods().resource(createdDriverPod).patch(...)`, so there is no GET and no
`resourceVersion` in the request.
On a cluster I let another writer touch the pod between our read and our
patch, 5 times each. `edit()` failed with 409 on 5 of 5, and the named patch
succeeded on 5 of 5 and left the other gate in place. The patch is also
idempotent: sending it again returns `no 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
