HwangDongJun commented on code in PR #58187:
URL: https://github.com/apache/spark/pull/58187#discussion_r3858987373
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesClientApplication.scala:
##########
@@ -136,7 +136,10 @@ private[spark] class Client(
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 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)
Review Comment:
Thanks for the detailed writeup, @LuciferYang -- you're right, and I've
implemented the shutdown hook you outlined.
Summary of the change in `Client.run()`:
- A shutdown hook is registered (via `ShutdownHookManager`, per scalastyle
-- not `Runtime.getRuntime` directly) right before the pre-resources are
applied, tracking two flags (`preResourcesApplied`, `podCreatedByUs`) for how
far `run()` got.
- It builds its own fresh `KubernetesClient` rather than reusing the
injected one, since that one may already be closed or concurrently in use by
the time the hook runs, as you noted.
- It deletes the pre-resources (and the pod, only if this submission created
it) only if `preResourcesApplied` is true; otherwise it's a no-op.
- The hook is removed entirely (not left as a no-op) in a `finally`, once
the owner-reference refresh completes or one of the existing catch blocks has
already cleaned up -- so Ctrl-C during `waitAppCompletion` keeps today's detach
behavior.
- Cleanup exceptions are caught and logged, never propagated.
A couple of things worth calling out:
- The fresh client reuses the same request/connection timeouts as the rest
of `run()` (`SparkKubernetesClientFactory`/`ClientType.Submission`), so this
can't hang indefinitely even against a slow API server.
- I intentionally left this without a config flag to disable it -- it's a
no-op outside that specific abrupt-termination window, so there's no
steady-state behavior for a flag to protect against.
Testing: the cleanup logic and the hook registration/removal wiring in
`run()` are both covered by new unit tests with injected fakes (triggering a
real JVM shutdown hook from a test isn't practical). I also reproduced your
exact scenario against a real cluster -- killed the process (SIGTERM) between
the pre-resource apply and the owner-reference refresh, both without and with
this fix -- and confirmed the ConfigMap is left ownerless in the former case
and cleaned up in the latter.
As you noted, SIGKILL and in-flight API calls at the moment of a hard kill
remain uncovered either way.
Let me know if this looks right, or if you'd prefer a different approach.
--
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]
