LuciferYang commented on code in PR #58343:
URL: https://github.com/apache/spark/pull/58343#discussion_r4023871056
##########
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:
Added, in `KubernetesExecutorBuilderSuite`: `mockKubernetesClient` gets a
template naming only `serviceAccountName`, with
`spark.kubernetes.authenticate.executor.serviceAccountName` set, and the
finished pod has to name the template's account in both fields. The template
needs a container of its own, otherwise `loadPodFromTemplate` fails before the
step runs. Skipping the write on the template branch leaves it with `null did
not equal "template-name"`.
##########
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:
Added, one row each way, and reading the deprecated field first reddens both.
The warn test gets `serviceAccount: configured-name` with
`serviceAccountName: template-name` under
`executor.serviceAccountName=configured-name`, where comparing against the
wrong field sees the configured account and stays quiet. The silence test gets
the mirror image, `serviceAccount: other-name` with `serviceAccountName:
same-name` under `=same-name`, where the same mistake invents a displacement.
The names are picked so that the assertions already there still apply.
##########
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:
Applied.
--
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]
