[SPARK-58910][K8S] Keep the executor pod template's service account - #58343
[SPARK-58910][K8S] Keep the executor pod template's service account#58343LuciferYang wants to merge 2 commits into
Conversation
The guard read only the deprecated `serviceAccount` alias, so a template that named `serviceAccountName` looked like it had no account and the configured one overwrote it.
The two rows claimed executor pods are unaffected, which no release matched. Also spells out on the executor config entry that a template naming an account wins, and pins the test's null assertion in a comment.
|
Thanks for the fix, @LuciferYang. I went through the diagnosis and ran the suite locally. Verification
I also reproduced the mutation you described. Reverting the guard to master's The root cause checks out: Two things worth discussing
Related: it is worth deciding explicitly how far this should be backported. Executor pods that used to run as the configured account will now run as the one the template names, and without the warning above users have nothing pointing at the change. Minor, take or leave
|
What changes were proposed in this pull request?
ExecutorKubernetesCredentialsFeatureStepasks whether the executor pod template already names a service account by readingspec.serviceAccount, the field Kubernetes deprecated in favor ofspec.serviceAccountName. A template is deserialized client side byKubernetesUtils.loadPodFromTemplate, with no API-server defaulting, so a template that namesserviceAccountNamearrives withserviceAccountnull and the guard concludes the template named nothing.buildPodWithServiceAccountthen writes both fields withspark.kubernetes.authenticate.executor.serviceAccountName, or withspark.kubernetes.authenticate.driver.serviceAccountNamewhen that one is unset.The guard now reads both fields, with an empty value counting as unset, which is what
SetDefaults_PodSpecdoes:That second line is also why leaving
serviceAccountnull in process is harmless: once the pod reaches the API server the two fields end up in sync either way. The read prefersserviceAccountNamefor the same reason the snippet does, though the preference has no effect in this step, which only asks whether either field names something.SPARK-58872 added the same two-field read on the driver side, for the warning it logs there.
I also reindented the method body from six spaces to four, which is why lines I did not otherwise touch show up in the diff.
The pod template table in
running-on-kubernetes.mdalso gets corrected. ItsserviceAccountandserviceAccountNamerows ended with "Executor pods will remain unaffected", which no release ever matched: the rows arrived with pod templates in SPARK-24434 and the executor step landed in the same 3.0.0 release. The step overrides both fields on executor pods whenever the template names no account and one of the two configurations is set, today usingspark.kubernetes.authenticate.executor.serviceAccountNameand falling back tospark.kubernetes.authenticate.driver.serviceAccountName. The rows now state the driver rule and the executor rule separately, and the entry for the executor config says a template that names an account wins.Stating both rules in one place makes the asymmetry visible: a template-named account loses on the driver and wins on the executor. That asymmetry is long-standing, not new here. The driver applies its configured account unconditionally when it mounts no credentials secret, and the rows have said Spark overrides the template's account on driver pods since pod templates arrived in SPARK-24434, while the executor step has always described its configured account as a fallback for a template that names none.
Why are the changes needed?
The step's own comment states the contract it does not keep: the configured account is a fallback "if not setup by the pod template". For a template that names an account, that holds only when the deprecated
spec.serviceAccountfield is present, since the guard's null check never looks atspec.serviceAccountName.The executors instead run as the configured account, so the RBAC bound to the account the template names does not apply.
The guard has read only the alias since SPARK-27872 (
7912ab85a6f) in 3.0.0. The executor-side account came later, in SPARK-30122 (f9f06eee985, 3.1.0), so before that the account that won was the driver's configured one.Does this PR introduce any user-facing change?
Yes, in two ways. First, an executor pod template that names an account in
spec.serviceAccountNameand carries nospec.serviceAccountfield at all: with a service account configured through eitherspark.kubernetes.authenticate.executor.serviceAccountNameorspark.kubernetes.authenticate.driver.serviceAccountName, those executor pods used to run as the configured account and now run as the one the template names. Second, a template whosespec.serviceAccountis present but empty, withspec.serviceAccountNameempty or absent: that used to count as naming an account, which dropped the configured one and left the pod on the namespace default, and now counts as unset, so the configured account applies. Every other template behaves as before, including one that names a real account inspec.serviceAccountNamealongside a presentspec.serviceAccount, which the old guard kept too because it only asked whetherspec.serviceAccountwas null.One thing this PR deliberately does not do: when the template wins, the executor config is dropped without a word in the log. SPARK-58872 added that WARN on the driver side, for the same user-visible situation: a configuration set, and nothing saying why it did nothing. It is an improvement rather than part of this fix, so it should go in a separate change that does not travel to the maintenance branches. Filed as SPARK-59053.
How was this patch tested?
Two tests in
ExecutorKubernetesCredentialsFeatureStepSuite.SPARK-58910: keep the service account named by the executor pod templateconfigures the executor account, then asserts that a template namingserviceAccountNamecomes out with that name and a null alias, and that a template naming the deprecatedserviceAccountcomes out with that alias and a null name. Asserting the two fields separately instead of going through the suite'sassertSANameis deliberate: the step returns the pod untouched in this branch, so whichever field the template left alone has to stay null.SPARK-58910: an empty service account name in the template counts as unsetcovers the other half. An empty value in either field, with the other absent, means the pod has no account, so the configured one applies.Every clause of the new expression has a mutation that turns one of the two tests red, and I ran all of them:
master's original guard, which reads the deprecated alias only and without the emptiness filter, and fails with"[executor]-name" did not equal "[template]-name"; reading onlyserviceAccountName; dropping thenonEmptyfilter onserviceAccountName; dropping it on the alias, which goes red through a null dereference insideassertSANamerather than an equality failure.build/sbt -Pkubernetes kubernetes/test: 392 tests across 42 suites, all passing.kubernetes/scalastyleandkubernetes/Test/scalastylereport no issues.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code (Opus 5)