HwangDongJun commented on code in PR #58187:
URL: https://github.com/apache/spark/pull/58187#discussion_r4310470893
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/submit/ClientSuite.scala:
##########
@@ -221,14 +306,216 @@ class ClientSuite extends SparkFunSuite with
BeforeAndAfter {
assert(secrets.nonEmpty)
assert(configMaps.nonEmpty)
val configMap = configMaps.head
- assert(configMap.getMetadata.getName ===
- KubernetesClientUtils.configMapNameDriver)
+ // SPARK-38079: configMapNameDriver is a `def` -- see its own comment --
so this cannot
+ // compare against a freshly-generated name (that call would return a
different one); it
+ // can only check the name has the shape configMapNameDriver is documented
to produce.
+ assert(configMap.getMetadata.getName.matches("spark-drv-\\S+-conf-map"))
assert(configMap.getImmutable())
assert(configMap.getData.containsKey(SPARK_CONF_FILE_NAME))
assert(configMap.getData.get(SPARK_CONF_FILE_NAME).contains("conf1key=conf1value"))
assert(configMap.getData.get(SPARK_CONF_FILE_NAME).contains("conf2key=conf2value"))
}
+ test("SPARK-38079: driver pod is created (still scheduling-gated) before its
own config " +
+ "map, and the config map's single create call already carries the owner
reference") {
+ val submissionClient = new Client(
+ kconf,
+ driverBuilder,
+ kubernetesClient,
+ loggingPodStatusWatcher)
+ submissionClient.run()
+ // The pod itself must be created before any pre-resource resourceList()
call, since a
+ // pre-resource's owner reference (set below) needs the pod's UID.
+ assert(gatedPod === fullExpectedPod(gatedConfigMapName))
+ verify(namedPods).create()
+
+ // The (single) resourceList(...) call creating the driver's own config
map must already
+ // carry the owner reference -- there is no separate, later "refresh"
call. The pod stays
+ // scheduling-gated (so kubelet cannot attempt to mount it) for as long as
this has not yet
+ // happened, avoiding the "configmap ... not found" mount race
(SPARK-38079) without ever
+ // creating the config map ownerless.
+ val resourceListCall = createdResourcesArgumentCaptor.getAllValues.get(0)
+ val configMaps = resourceListCall
+ .filter(_.isInstanceOf[ConfigMap]).map(_.asInstanceOf[ConfigMap])
+ assert(configMaps.nonEmpty,
+ "the driver's own config map must be created as a pre-resource")
+ val ownerReferences = configMaps.head.getMetadata.getOwnerReferences
+ assert(ownerReferences.size() === 1)
+ assert(ownerReferences.get(0).getName === POD_NAME)
+ assert(ownerReferences.get(0).getUid === DRIVER_POD_UID)
+
+ // The scheduling gate must be removed only after the config map has been
created.
+
verify(namedPods).edit(ArgumentMatchers.any[java.util.function.UnaryOperator[Pod]]())
+ }
+
+ // SPARK-38079: the three failure branches in run() between pod creation and
gate removal --
+ // covering (a) the pod creation call itself failing, (b) the pre-resource
creation call
+ // failing after the pod exists, and (c) the gate-removal call failing after
pre-resources
+ // exist -- each verified against real-cluster behavior
(KubernetesClientException on
+ // AlreadyExists/Invalid/NotFound) before being written as these mock-based
tests.
+ test("SPARK-38079: pod creation failure propagates without attempting any
pre-resource " +
+ "creation") {
+ val podCreationFailure = new RuntimeException("simulated pod creation
failure")
+ doThrow(podCreationFailure).when(namedPods).create()
+ val submissionClient = new Client(
+ kconf,
+ driverBuilder,
+ kubernetesClient,
+ loggingPodStatusWatcher)
+
+ val thrown = intercept[RuntimeException] {
+ submissionClient.run()
+ }
+
+ assert(thrown eq podCreationFailure)
+ verify(kubernetesClient,
never()).resourceList(ArgumentMatchers.any[Array[HasMetadata]](): _*)
+ }
+
+ test("SPARK-38079: pre-resource creation failure (after the pod exists)
deletes both the " +
+ "pod and the pre-resources, then propagates") {
+ val preResourceFailure = new RuntimeException("simulated pre-resource
creation failure")
+ doThrow(preResourceFailure).when(resourceList).serverSideApply()
+ val submissionClient = new Client(
+ kconf,
+ driverBuilder,
+ kubernetesClient,
+ loggingPodStatusWatcher)
+
+ val thrown = intercept[RuntimeException] {
+ submissionClient.run()
+ }
+
+ assert(thrown eq preResourceFailure)
+ // Not just "delete() was called on some mock" -- pods().resource(...)
must have been
+ // called with the actual created (gated) pod, i.e. the same pod object
this cleanup path
+ // is documented to delete, and each pre-resource must be deleted
individually (not as a
+ // single resourceList(...).delete() call, which stops at the first
failing item).
+ verify(namedPods).create()
+ verify(podOperations).resource(podWithOwnerReference(gatedConfigMapName))
+ verify(namedPods).delete()
+ verify(namedResource).delete()
Review Comment:
Reworked around the smaller flow. Each failure test now asserts which delete
happens: pod creation failing deletes nothing but the pre-resources and never
calls `delete()` on the pod; a post-resource failure deletes only the pod, and
that pod is the one this submission created (its UID is checked). The gate
tests keep a second gate from the template and assert the patch names only
ours, and that the other gate remains.
Making the patch clear every gate fails two tests, and deleting the pod
after a failed create fails one.
--
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]
