LuciferYang commented on PR #58343: URL: https://github.com/apache/spark/pull/58343#issuecomment-5454143489
Thanks for going through it and running the mutation yourself, @dongjoon-hyun. Both points are in, and the first one changed my mind: SPARK-58872 documented a case that already behaved that way, whereas this PR is what makes `spark.kubernetes.authenticate.executor.serviceAccountName` silently stop applying, so the message belongs with the change that creates the need for it. One refinement while implementing it. Warning on every dropped account turned out to fire on the commonest correct setup: `spark.kubernetes.authenticate.driver.serviceAccountName` is set by nearly every submission, and a template that names its own executor account then displaces a value that was only ever a fallback for executors and still applies to the driver pod. Nothing the user asked for is lost there, so a WARN advising them to delete the account they deliberately put in the template would be a false alarm, once per executor pod. So the WARN is now for the case where it means something, an explicitly set executor account that had no effect, and the driver-fallback case reports the same fact at INFO without the imperative remedy. The flip is still traceable in the driver log, which is where an operator looks. Frequency, since it is a fair question: both lines repeat once per executor pod, because `ExecutorPodsAllocator` rebuilds the feature steps inside its per-pod loop. For the WARN that is bounded by how many pods a misconfigured application launches. The INFO is the one worth arguing about, since it reports a correct configuration; it adds one line to the one `BasicExecutorFeatureStep` already logs per pod. Bounding it needs state outside the step, which is constructed per pod, so the natural home would be `KubernetesClusterManager`, which already loads the executor template once at startup. Say the word and I will move it there. The read is now `KubernetesUtils.podServiceAccount`, called by both steps. I made it `private[spark]` rather than public: every method on that `@DeveloperApi` object carries a `@Since`, and a fix headed for the maintenance branches has no good version to put there. Worth noting the kubernetes module is outside MiMa's scope either way. That also resolves your note about `templateServiceAccount` -- the value is used now, since both messages name the account the pod keeps. The rest: - Shortened the "Modified value" column back to a value and left the executor rule in the description, which now also mentions the warning. - `KUBERNETES_EXECUTOR_SERVICE_ACCOUNT_NAME`'s `.doc(...)` matches the docs, and picks up a missing space it had. - The template test varies the configuration alongside the spelling, so the `orElse` fallback is covered there; the warning test covers both configurations too. - SPARK-59053 is superseded and I will close it. `kubernetes/test` is at 395 tests across 42 suites, all passing, with both scalastyle tasks clean. The new assertions are mutation-checked, including the two that pin the split: splitting the report into two independent statements emits a WARN and an INFO for anyone who set both configurations, and treating an empty executor value as unset would announce a driver account the write path would never have applied. On how far this goes: SPARK-58910 is a Bug affecting 4.0.0 and up, the fix is small, and with the reporting the flip is no longer silent. Which branches would you like it on? -- 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]
