dongjoon-hyun commented on PR #58343: URL: https://github.com/apache/spark/pull/58343#issuecomment-5667898692
Thanks for the update, @LuciferYang. I went through the second revision. **Verified** The helper extraction is a faithful move: `podServiceAccount` is the same `serviceAccountName`-wins, empty-counts-as-unset read the driver step carried inline, and `DriverKubernetesCredentialsFeatureStepSuite` still covers all three shapes. The `SetDefaults_PodSpec` attribution matches the current `pkg/apis/core/v1/defaults.go`. `LogKeys` usage mirrors the driver step, `withLogAppender` captures INFO for the named logger, and `kubernetes` is fine under MiMa with a `private[spark]` member. I found no correctness issue in the guard itself. **Two things I would like settled before merging** 1. Empty configuration values are now handled two different ways in the same PR. The template side treats `""` as unset (new), but the write path at `ExecutorKubernetesCredentialsFeatureStep.scala:39` still treats `executor.serviceAccountName=""` as set, shadows a non-empty driver fallback, and writes `""` into both pod fields, while the docs (`Config.scala:386-388`, the pod template table) say the driver account is the fallback "if this parameter is not setup". The new `nonEmpty` guard, the "long-standing" comment at line 62, and the three `EXECUTOR_SA_CONF -> ""` test rows pin that quirk as intended. The driver step's SPARK-58872 warning does not filter empty values either, so the two steps' reporting rules disagree. I would rather not codify this. Either align both paths with `.filter(_.nonEmpty)` at the `lazy val`s (small user-visible change: an empty executor value falls back to the driver account instead of writing `""`), or leave the write path alone, drop the three test ro ws and the comment, and file a follow-up. I lean toward the follow-up given the backport. 2. On the per-pod INFO, I will take you up on the offer. It fires on the configuration our own RBAC docs recommend (`driver.serviceAccountName` set, template naming an executor account), once per pod for the life of the application, and it reports nothing wrong. Please either lower it to `logDebug` or emit it once per application; `ExecutorPodsAllocator.recoveryModeCpusWarned` is the existing once-only pattern in this module, and `KubernetesClusterManager` already loads the template once at startup if you prefer to report there. The WARN can stay per pod; each line does correspond to a pod launched with a different account than configured. **Minor** - `configurePod` reads `templateServiceAccount` twice and the `else` branch is `pod.copy(pod = pod.pod)`, an identity copy. Since the body was rewritten anyway, a single `match` on `podServiceAccount(pod)` reads better: `case Some(account) => reportAccountNotApplied(account); pod` and `case None => pod.copy(pod = buildPodWithServiceAccount(...).getOrElse(pod.pod))`. - The suite has three copies of the `loggingEvents.filter(level).map(_.getMessage.getFormattedMessage).toSeq` chain (`warningsFrom`, `allOutput`, and the inline INFO extraction in the INFO test). One `messagesAt(appender, level)` covers all three. `podWithAccount` and `warningsFrom` are also byte-for-byte the driver suite's; sharing them is optional. - In the pod template table, the executor paragraph is still verbatim in both rows, and "see `spark.kubernetes.authenticate.executor.serviceAccountName` for which account that is" restates the "Modified value" cell. Dropping that sentence is a safe trim. - Should `docs/core-migration-guide.md` get an entry? The "Upgrading from Core 4.2 to 4.3" section already records K8s behavior changes without a restore knob, and the RBAC flip is exactly the kind of thing a user searches there for. The backport makes the "Since Spark 4.3" wording awkward, so I am asking rather than requesting. -- 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]
