LuciferYang opened a new pull request, #57948:
URL: https://github.com/apache/spark/pull/57948
### What changes were proposed in this pull request?
Resolve the pod before deciding whether `spark-submit --kill` found it.
`KillApplication.executeOnPod` null-checks the request handle returned by
`getPod` rather than the pod it resolves to. Call `get()` on the handle and
check that instead, reusing the same handle for the delete.
### Why are the changes needed?
`getPod` returns `client.pods.inNamespace(ns).withName(name)`, a fabric8
`PodResource` request handle. `BaseOperation.withName` never returns null: it
throws `IllegalArgumentException("Name must be provided.")` for a null or empty
name and otherwise constructs a new handle. So `Option(podToDelete).isDefined`
is always true and the `printMessage("Application not found.")` branch is
unreachable.
`PodResource.delete()` on a name the API server does not have sends a real
DELETE. `BaseOperation.deleteAll()` catches the resulting 404 when a name is
set and returns `Collections.emptyList()`, and the caller discards that list.
So `spark-submit --kill <ns>:<wrong-name> --master k8s://...` prints only
the "Submitting a request to kill submission ..." banner and exits 0, which is
the same output a successful kill produces. `--status` on the same submission
ID does report "Application not found.", because `ListStatus.executeOnPod`
calls `get()` before its null check. The two subcommands in the same file
disagree about the same nonexistent pod.
The check has been unreachable since the feature was added in 2019. fabric8
4.x had the same never-null `withName` and also swallowed the 404, returning
`Boolean.FALSE` instead of an empty list, and that value was discarded too.
Three points a reviewer may want to weigh, all deliberate:
Scope. The silence is specific to the API server answering 404. A 403 or 5xx
still propagates a `KubernetesClientException` and a nonzero exit, so this is a
wrong-name or wrong-namespace defect rather than "kill never reports failure".
Standalone mode does report a nonexistent driver ("Driver ... has already
finished or does not exist"), so reporting is the convention this path was
missing.
The GET and DELETE are not atomic. If the pod disappears between them,
fabric8 swallows the 404 and the command exits 0 having printed only the
banner, which is what the old code did for a name that never existed. The race
lands in the previous behavior rather than in a new error, so no retry loop is
needed.
Gating on `delete()`'s returned `List[StatusDetails]` would avoid the extra
GET and close that window, but `Deletable.delete()` documents that "It is not
guaranteed that the returned list will contain all values marked for deletion",
while `Gettable.get()` contracts "the item or null if the item doesn't exist".
Depending on the first would let this bug reappear silently on a future client
bump, with no unit test able to catch it.
Also note that `--kill <name>` without an `ns:` prefix, against a kube
config that supplies no namespace, now throws from `get()` where it previously
issued a silent no-op DELETE against a URL that could never match a pod.
`ListStatus.executeOnPod` already behaves that way on the same input, so the
two subcommands end up equally exposed rather than `--kill` becoming worse than
`--status`.
### Does this PR introduce _any_ user-facing change?
Yes, on one path. `spark-submit --kill` against a driver pod that does not
exist now prints "Application not found." instead of printing nothing. It
previously exited 0 after issuing a DELETE whose 404 was swallowed. The exit
code is unchanged, matching standalone mode, which also exits 0 for a
nonexistent driver. No API or configuration change.
### How was this patch tested?
A test added to `K8sSubmitOpSuite`, asserting both that the message is
printed and that no delete is issued.
The missing pod needs a real `PodResource` mock whose `get` is stubbed to
null. An unstubbed name would make Mockito's `withName` return null, which
sends the buggy check down the false branch for the wrong reason and passes
without the fix.
Confirmed to fail against the unfixed tree:
```
[info] - Kill app that does not exist *** FAILED *** (7 milliseconds)
[info] org.mockito.exceptions.verification.WantedButNotInvoked: Wanted but
not invoked:
[info] err.println("Application not found.");
[info] Actually, there were zero interactions with this mock.
```
(That run predated the JIRA id, so the test name in the captured output
lacked the
`SPARK-58725:` prefix it carries now.)
`build/sbt -Pkubernetes 'kubernetes/testOnly
org.apache.spark.deploy.k8s.submit.K8sSubmitOpSuite'`:
```
[info] - List app status (69 milliseconds)
[info] - List status for multiple apps with glob (3 milliseconds)
[info] - Kill app (11 milliseconds)
[info] - Kill app with gracePeriod (3 milliseconds)
[info] - SPARK-58725: Kill app that does not exist (2 milliseconds)
[info] - Kill multiple apps with glob without gracePeriod (3 milliseconds)
[info] Run completed in 1 second, 722 milliseconds.
[info] Total number of tests run: 6
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 6, failed 0, canceled 0, ignored 0, pending 0
```
`kubernetes/scalastyle` and `kubernetes/Test/scalastyle` report 0 errors.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)
--
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]
