dongjoon-hyun commented on code in PR #58343:
URL: https://github.com/apache/spark/pull/58343#discussion_r4017631650
##########
docs/core-migration-guide.md:
##########
@@ -42,6 +42,8 @@ license: |
- Since Spark 4.3, a positive `spark.executor.pyspark.memory` allocation that
is too small to give each concurrent task slot at least 1 MiB fails the Python
task with an error instead of silently running the workers without any memory
limit. Setting `spark.executor.pyspark.memory=0` still disables the limit. To
restore a working memory limit, increase `spark.executor.pyspark.memory` or
reduce the executor's concurrent task capacity.
+- Since Spark 4.3, an executor pod template that names a service account in
`serviceAccountName` keeps it: Spark applies
`spark.kubernetes.authenticate.executor.serviceAccountName`, or the driver's
account as a fallback, only when the template names no account in either
`serviceAccount` or `serviceAccountName`. Earlier versions decided by reading
the deprecated `serviceAccount` field alone, so a template that named the
account in `serviceAccountName` had it overwritten. Spark logs a warning when
`spark.kubernetes.authenticate.executor.serviceAccountName` named an account
the template displaced.
Review Comment:
Two wording notes on this entry, separate from the version question.
1. The empty-value case is a user-facing change of its own, but this text
does not show it. A template with `serviceAccount: ""` and no
`serviceAccountName` used to be returned as-is, so the executors ended up on
the namespace's `default` account. Now `podServiceAccount` treats `""` as
unset, so they run as
`spark.kubernetes.authenticate.executor.serviceAccountName` or, failing that,
the driver's account. The driver's account is usually the one our RBAC example
binds to `edit` (`kubectl create clusterrolebinding spark-role
--clusterrole=edit ...` in `running-on-kubernetes.md`), so this can widen what
executors are allowed to do. The PR description lists it as the second change,
but "names no account" does not tell a reader that an empty value counts. Could
we spell it out here?
2. "had it overwritten" holds only when
`spark.kubernetes.authenticate.executor.serviceAccountName` or
`spark.kubernetes.authenticate.driver.serviceAccountName` was set. With
neither, `buildPodWithServiceAccount(None, pod)` returned the pod unchanged, so
the template's `serviceAccountName` was already kept. Stating that condition
would spare users who set neither from auditing their templates.
##########
resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/features/ExecutorKubernetesCredentialsFeatureStep.scala:
##########
@@ -18,24 +18,60 @@ package org.apache.spark.deploy.k8s.features
import org.apache.spark.deploy.k8s.{KubernetesConf, SparkPod}
import
org.apache.spark.deploy.k8s.Config.{KUBERNETES_DRIVER_SERVICE_ACCOUNT_NAME,
KUBERNETES_EXECUTOR_SERVICE_ACCOUNT_NAME}
-import org.apache.spark.deploy.k8s.KubernetesUtils.buildPodWithServiceAccount
+import
org.apache.spark.deploy.k8s.KubernetesUtils.{buildPodWithServiceAccount,
podServiceAccount}
+import org.apache.spark.internal.Logging
+import org.apache.spark.internal.LogKeys.{CONFIG, SERVICE_ACCOUNT_NAME, VALUE}
private[spark] class ExecutorKubernetesCredentialsFeatureStep(kubernetesConf:
KubernetesConf)
- extends KubernetesFeatureConfigStep {
+ extends KubernetesFeatureConfigStep with Logging {
private lazy val driverServiceAccount =
kubernetesConf.get(KUBERNETES_DRIVER_SERVICE_ACCOUNT_NAME)
private lazy val executorServiceAccount =
kubernetesConf.get(KUBERNETES_EXECUTOR_SERVICE_ACCOUNT_NAME)
override def configurePod(pod: SparkPod): SparkPod = {
- pod.copy(
- // if not setup by the pod template, fallback to the executor's sa,
- // if executor's sa is not setup, the last option is driver's sa.
- pod = if (Option(pod.pod.getSpec.getServiceAccount).isEmpty) {
- buildPodWithServiceAccount(executorServiceAccount
- .orElse(driverServiceAccount), pod).getOrElse(pod.pod)
- } else {
- pod.pod
- })
+ podServiceAccount(pod) match {
+ // The pod template's account takes precedence, so the pod goes back as
it came.
+ case Some(templateAccount) =>
+ reportAccountNotApplied(templateAccount)
+ pod
Review Comment:
Returning the pod untouched leaves whichever field the template did not set
as `null` for the rest of the chain. User feature steps from
`spark.kubernetes.executor.pod.featureSteps` run after this step in
`KubernetesExecutorBuilder`, so with a template naming only
`serviceAccountName` and a configured account, such a step reading
`getServiceAccount` used to see the configured account and now sees `null`.
The API server's `SetDefaults_PodSpec` writes the same value into both
fields anyway, so how about mirroring that here?
```scala
case Some(templateAccount) =>
reportAccountNotApplied(templateAccount)
pod.copy(pod = buildPodWithServiceAccount(Some(templateAccount),
pod).getOrElse(pod.pod))
```
That keeps the spec handed to later steps consistent with what the API
server stores. The first new test would then assert the account in both fields
instead of `null`. Take or leave.
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/ExecutorKubernetesCredentialsFeatureStepSuite.scala:
##########
@@ -52,18 +57,155 @@ class ExecutorKubernetesCredentialsFeatureStepSuite
extends SparkFunSuite with B
assertSAName("executor-name", spec)
}
+ test("SPARK-58910: keep the service account named by the executor pod
template") {
Review Comment:
All the new tests feed a `PodBuilder`-built pod straight into the step, so
the path the bug actually takes (template -> `KubernetesExecutorBuilder` ->
`BasicExecutorFeatureStep` -> this step) is not pinned. The template in
`PodBuilderSuite.podWithSupportedFeatures` still uses only the deprecated
`withServiceAccount`, with no account configured, so the builder suites pass on
`master` and here alike. Since `mockKubernetesClient(pod)` already takes a
custom pod, one case in `KubernetesExecutorBuilderSuite` with a template naming
only `serviceAccountName` and
`spark.kubernetes.authenticate.executor.serviceAccountName` set would cover it
end to end.
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/ExecutorKubernetesCredentialsFeatureStepSuite.scala:
##########
@@ -52,18 +57,155 @@ class ExecutorKubernetesCredentialsFeatureStepSuite
extends SparkFunSuite with B
assertSAName("executor-name", spec)
}
+ test("SPARK-58910: keep the service account named by the executor pod
template") {
+ // Either spelling means the template already picked an account, so the
configured one must not
+ // replace it. The field the template left alone stays null: the step
hands the pod back as it
+ // came rather than mirroring the account into both fields. Varying the
configuration alongside
+ // the spelling keeps the driver fallback covered here too.
+ Seq(
+ (EXECUTOR_SA_CONF, podWithAccount(serviceAccountName =
Some("template-name")),
+ "template-name", null),
+ (DRIVER_SA_CONF, podWithAccount(serviceAccount = Some("template-name")),
+ null, "template-name")
+ ).foreach { case (conf, templatePod, expectedName, expectedAlias) =>
+ val spec = evaluateStep(templatePod, new SparkConf(false).set(conf,
"configured-name"))
+ assert(spec.getServiceAccountName === expectedName, s"via $conf")
+ assert(spec.getServiceAccount === expectedAlias, s"via $conf")
+ }
+ }
+
+ test("SPARK-58910: an empty service account name in the template counts as
unset") {
+ baseConf.set(KUBERNETES_EXECUTOR_SERVICE_ACCOUNT_NAME, "executor-name")
+ // SetDefaults_PodSpec keys off the name being empty rather than null, and
an empty alias copies
+ // up as an empty name, so neither leaves the pod with an account.
+ Seq(
+ podWithAccount(serviceAccountName = Some("")),
+ podWithAccount(serviceAccount = Some(""))
+ ).foreach(templatePod => assertSAName("executor-name",
evaluateStep(templatePod)))
+ }
+
+ test("SPARK-58910: warn when the template displaces the executor
configuration") {
Review Comment:
Minor: none of the shapes in the new tests names different accounts in the
two fields, which is the one case where the step has to use the right field
before comparing. For example, `executor.serviceAccountName=a` with a template
of `serviceAccount: a`, `serviceAccountName: b` should warn once naming `b`,
while `executor.serviceAccountName=b` with the same template should stay
silent. The driver suite pins `serviceAccountName` winning, but only through
the driver step's message, so a regression in this step's comparison would stay
green here.
##########
resource-managers/kubernetes/core/src/test/scala/org/apache/spark/deploy/k8s/features/ExecutorKubernetesCredentialsFeatureStepSuite.scala:
##########
@@ -52,18 +57,155 @@ class ExecutorKubernetesCredentialsFeatureStepSuite
extends SparkFunSuite with B
assertSAName("executor-name", spec)
}
+ test("SPARK-58910: keep the service account named by the executor pod
template") {
+ // Either spelling means the template already picked an account, so the
configured one must not
+ // replace it. The field the template left alone stays null: the step
hands the pod back as it
+ // came rather than mirroring the account into both fields. Varying the
configuration alongside
+ // the spelling keeps the driver fallback covered here too.
+ Seq(
+ (EXECUTOR_SA_CONF, podWithAccount(serviceAccountName =
Some("template-name")),
+ "template-name", null),
+ (DRIVER_SA_CONF, podWithAccount(serviceAccount = Some("template-name")),
+ null, "template-name")
+ ).foreach { case (conf, templatePod, expectedName, expectedAlias) =>
+ val spec = evaluateStep(templatePod, new SparkConf(false).set(conf,
"configured-name"))
+ assert(spec.getServiceAccountName === expectedName, s"via $conf")
+ assert(spec.getServiceAccount === expectedAlias, s"via $conf")
+ }
+ }
+
+ test("SPARK-58910: an empty service account name in the template counts as
unset") {
+ baseConf.set(KUBERNETES_EXECUTOR_SERVICE_ACCOUNT_NAME, "executor-name")
+ // SetDefaults_PodSpec keys off the name being empty rather than null, and
an empty alias copies
+ // up as an empty name, so neither leaves the pod with an account.
+ Seq(
+ podWithAccount(serviceAccountName = Some("")),
+ podWithAccount(serviceAccount = Some(""))
+ ).foreach(templatePod => assertSAName("executor-name",
evaluateStep(templatePod)))
+ }
+
+ test("SPARK-58910: warn when the template displaces the executor
configuration") {
+ // Both spellings have to warn, since the account can be named in either
field. With both
+ // configurations set the message must still name the executor one,
because that is the account
+ // the step would otherwise have applied.
+ Seq(
+ Seq(EXECUTOR_SA_CONF -> "configured-name") ->
+ podWithAccount(serviceAccountName = Some("template-name")),
+ Seq(EXECUTOR_SA_CONF -> "configured-name") ->
+ podWithAccount(serviceAccount = Some("template-name")),
+ Seq(EXECUTOR_SA_CONF -> "configured-name", DRIVER_SA_CONF ->
"driver-name") ->
+ podWithAccount(serviceAccountName = Some("template-name"))
+ ).foreach { case (confs, templatePod) =>
+ val appender = runWith(confs, templatePod)
+ val warnings = messagesAt(appender, Level.WARN)
+ assert(warnings.size === 1, s"expected one warning for $confs, got:
$warnings")
+ // And nothing besides: splitting the report into two independent
statements would add an
+ // INFO about the driver fallback for anyone who set both configurations.
Review Comment:
This still describes the driver fallback as INFO, but it is `logDebug` now.
```suggestion
// And nothing besides: splitting the report into two independent
statements would add a
// DEBUG line about the driver fallback for anyone who set both
configurations.
```
--
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]
