LuciferYang commented on code in PR #58343:
URL: https://github.com/apache/spark/pull/58343#discussion_r4023869254
##########
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:
Done, it is `Since Spark 4.4` now, in a new `## Upgrading from Core 4.3 to
4.4` section, and the backport stops at `branch-4.x`.
##########
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:
Both taken, and the entry is two bullets now.
The second one is the empty-value case: a template whose deprecated
`serviceAccount` is present but empty, with `serviceAccountName` empty or
absent, used to count as naming an account, so the executor pods ran as the
namespace's default account and now run as the configured one. It says that
this can widen what they may do, since the RBAC setup right above binds the
driver's account to `edit`, and how to keep them where they were: name that
account in `spark.kubernetes.authenticate.executor.serviceAccountName`
explicitly.
The first bullet now says "with either configuration set, a template that
named the account in `serviceAccountName` had it overwritten; with neither set,
the template's account was kept already".
##########
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:
Taken. `configurePod` is one `match` that decides which account wins,
followed by the single write, so both fields carry it on every path:
```scala
val account = podServiceAccount(pod) match {
case Some(templateAccount) =>
reportAccountNotApplied(templateAccount)
Some(templateAccount)
case None => executorServiceAccount.orElse(driverServiceAccount)
}
pod.copy(pod = buildPodWithServiceAccount(account, pod).getOrElse(pod.pod))
```
The first new test goes through the suite's `assertSAName` now, and I added
a row where the template names different accounts in the two fields: both come
out as the `serviceAccountName` one, which is what the API server would have
stored anyway.
--
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]
