sparkconnect: SIGTERM the foreground JVM in preStop instead of calling stop-connect-server.sh

[SAY-5]

Description

The SparkConnect server container is started with SPARK_NO_DAEMONIZE=true (foreground mode), so spark-daemon.sh does not write the PID file that ${SPARK_HOME}/sbin/stop-connect-server.sh uses to locate the process. The preStop hook therefore always fails with:

no org.apache.spark.sql.connect.service.SparkConnectServer to stop

The JVM is left to be SIGKILLed at the end of the termination grace period, event logs never finalize from .inprogress, the run shows as incomplete in Spark History Server, and any other shutdown hooks only run partially.

Fix

Replace the stop-connect-server.sh invocation with a targeted pkill -TERM against the server class. That delivers SIGTERM to the foreground JVM, which triggers Spark's normal shutdown hook so event logs finalize and the listener bus drains before the container exits.

Fixes #2903.

Test

• go build ./internal/controller/sparkconnect/... green.
• go test ./internal/controller/sparkconnect/... failure is pre-existing (envtest required) and unchanged by this PR (confirmed by stashing the diff and rerunning).

Signed-off-by: SAY-5 SAY-5@users.noreply.github.com

[google-oss-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign andreyvelich for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[github-actions]

🎉 Welcome to the Kubeflow Spark Operator! 🎉

Thanks for opening your first PR! We're happy to have you as part of our community 🚀

Here's what happens next:

• If you haven't already, please check out our Contributing Guide for repo-specific guidelines and the Kubeflow Contributor Guide for general community standards.
• Our team will review your PR soon!

Join the community:

• Slack: Join our #kubeflow-spark-on-kubernetes Slack channel.

Feel free to ask questions in the comments if you need any help or clarification!
Thanks again for contributing to Kubeflow! 🙏

[Copilot]

Pull request overview

Updates Spark Connect server pod shutdown behavior so the foreground JVM receives SIGTERM and can run Spark’s normal shutdown hooks (finalizing event logs, draining listeners), addressing #2903.

Changes:

• Replace stop-connect-server.sh in the pod preStop hook with a pkill -TERM targeting SparkConnectServer.
• Add detailed inline rationale explaining why daemon-style stop scripts fail with SPARK_NO_DAEMONIZE=true.

Comments suppressed due to low confidence (1)

internal/controller/sparkconnect/reconciler.go:436

• There are existing unit tests for reconciler.go behavior (e.g., mutateServerService), but this change to mutateServerPod isn’t covered. Adding a test that asserts the generated container lifecycle preStop command matches the expected shutdown behavior would help prevent regressions (especially around the exact signal command/regex).

		container.Lifecycle = &corev1.Lifecycle{
			PreStop: &corev1.LifecycleHandler{
				Exec: &corev1.ExecAction{
					Command: []string{
						"bash",
						"-c",
						"pkill -TERM -f org.apache.spark.sql.connect.service.SparkConnectServer || true",
					},
				},
			},
		}

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The preStop hook sends SIGTERM and exits immediately. Kubernetes runs preStop before sending SIGTERM to PID 1, so right after this hook completes kubelet will SIGTERM the container’s PID 1 (currently bash -c ...), which can terminate the container while the JVM is still shutting down and prevent the graceful shutdown you’re trying to enable. Consider having preStop wait/poll until the SparkConnectServer process exits (bounded by the termination grace period), or restructure the container entrypoint so the JVM becomes PID 1 and receives kubelet’s SIGTERM directly.

[Copilot]

pkill -f treats the pattern as a regular expression; the unescaped dots in org.apache.spark.sql.connect.service.SparkConnectServer will match any character, which can unintentionally match and signal unrelated processes. Escape the dots (or otherwise anchor/constrain the regex) so it only targets the intended JVM command line.

Suggested change

	"pkill -TERM -f org.apache.spark.sql.connect.service.SparkConnectServer || true",
	`pkill -TERM -f 'org\.apache\.spark\.sql\.connect\.service\.SparkConnectServer' || true`,

[Copilot]

|| true will also mask cases where pkill is missing from the image (exit 127), leading to silent non-graceful shutdowns. Consider guarding with a command -v pkill check (and/or only tolerating the "no process matched" exit code) so missing tooling is surfaced in logs.

Suggested change

	"pkill -TERM -f org.apache.spark.sql.connect.service.SparkConnectServer || true",
	"if ! command -v pkill >/dev/null 2>&1; then echo 'pkill not found; cannot gracefully stop Spark Connect server' >&2; exit 127; fi; pkill -TERM -f org.apache.spark.sql.connect.service.SparkConnectServer; status=$?; if [ \"$status\" -ne 0 ] && [ \"$status\" -ne 1 ]; then exit \"$status\"; fi",