HwangDongJun commented on code in PR #58187:
URL: https://github.com/apache/spark/pull/58187#discussion_r4163388586
##########
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()
+ } catch {
+ case NonFatal(e) =>
+ kubernetesClient.pods().resource(createdDriverPod).delete()
+ kubernetesClient.resourceList(preKubernetesResources: _*).delete()
+ throw e
+ }
+ } finally {
+ // Past this point, the pre-resources either have an owner reference
(success) or have
+ // already been explicitly deleted by one of the catch blocks above
(failure) -- either
+ // way, the shutdown hook has nothing left to do, so remove it instead
of leaving a no-op
+ // hook registered for the remaining lifetime of this process.
+ shutdownHookOps.removeHook(cleanupHookRef)
Review Comment:
This whole shutdown-hook mechanism is gone, replaced by
`spec.schedulingGates`:
- The driver pod is created already carrying
`PRE_RESOURCES_SCHEDULING_GATE`, so kubelet cannot schedule/mount anything on
it from the moment it exists.
- The gate is removed via `.edit()` only after the pre-resources have been
created, with owner references already set (see [the owner-reference reply
above](https://github.com/apache/spark/pull/58187#discussion_r4129910475)).
There's no `finally` here anymore, so a dropped connection, a fatal error,
or any other failure in between simply leaves the gate in place rather than
silently discarding cleanup state (the `register on entry, remove in finally`
mechanism and the `AtomicBoolean` flags guarding it are both gone too).
If this process dies before the gate is removed, the pod is left behind
gated. That's harmless: unschedulable, visible as `Pending`/`SchedulingGated`,
and not an ownerless credential-bearing resource. So no hook is needed to guard
that window either.
--
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]
