dongjoon-hyun commented on PR #58163:
URL: https://github.com/apache/spark/pull/58163#issuecomment-5357222960
Thank you for working on this, @LuciferYang. I reviewed the change and
verified a few of the claims independently. The problem is real and the
implementation is correct; my comments are mostly about size and style.
**What I verified**
- Behavior is unchanged. `shouldMountSecret` still keys off the same four
configs, just via `submittedCredentialConfs.nonEmpty` instead of the
`isDefined` disjunction.
- The field precedence matches Kubernetes. `SetDefaults_PodSpec` in
`pkg/apis/core/v1/defaults.go` does `if len(obj.ServiceAccountName) == 0 {
obj.ServiceAccountName = obj.DeprecatedServiceAccount };
obj.DeprecatedServiceAccount = obj.ServiceAccountName`, so `serviceAccountName`
winning and an empty one falling back to the alias is exactly right. And since
`KubernetesUtils.loadPodFromTemplate` deserializes client-side with no
API-server defaulting, the "a template can leave the other field null"
reasoning holds.
- `CONFIGS` is in the right sorted position (`CONFIG5` < `CONFIGS` <
`CONFIG_DEPRECATION_MESSAGE`), which is all `LogKeysSuite` checks, and the
plural-key-with-joined-value pattern already has precedent in `PATHS`
(`KubernetesClientUtils`) and `EXECUTOR_IDS` (`ExecutorAllocationManager`). No
concern here.
- CI is effectively green. The only failure is
`StateDataSourceTransformWithStateSuiteCheckpointV2` ("There are 2 possibly
leaked file streams") in `sql - extended tests`, which is unrelated flakiness.
The `kubernetes` module and the K8s integration tests passed.
**Comments**
1. The 14-line comment block in `configurePod` reads like the PR description
rather than a code comment. Lines such as `// Warn rather than fail: these
submissions are accepted today, and rejecting them needs a release note.` and
the `caCertFile` explanation belong in the JIRA/PR, not in the source. Three or
four lines is enough here, e.g.
```scala
// The credentials secret takes precedence over the driver service
account: this branch never
// applies the account, so warn that the pod keeps whatever its spec
names, or the namespace
// default. Stay quiet when the spec already names the same account,
reading both fields with
// `serviceAccountName` winning, matching Kubernetes' SetDefaults_PodSpec.
```
The test comments have the same issue; several of the multi-line ones
could be a single line.
2. The message hardcodes `spark.kubernetes.authenticate.driver.mounted.*`
while `KUBERNETES_AUTH_DRIVER_MOUNTED_CONF_PREFIX` is already imported and used
a few lines above (and the test uses the constant). Please use
`s"$KUBERNETES_AUTH_DRIVER_MOUNTED_CONF_PREFIX.*"`.
3. The warning computes `podServiceAccount` but does not use it. When the
pod spec does name an account, saying which one would be much more useful than
"the service account its spec already names, or the namespace's default".
4. In `running-on-kubernetes.md`, "Spark logs a warning about that, unless
the pod spec already names this same account" documents the suppression
heuristic. I would keep the behavior and the `mounted.*` escape hatch and drop
the subordinate clause. The two edits to the overwritten-pod-spec table are
accurate and worth keeping.
5. `withLogAppenderReturning[T]` discards the result at three of its six
call sites. The plain `var pod: SparkPod = _` pattern used elsewhere in the
tree would do, or keep the helper only where the value is actually needed.
6. Minor, and only if it is cheap to check: the operator reaches this code
through `SparkAppContext.getSecondaryResourceSpec()`, which caches per
reconcile context, so the warning may repeat across reconciliations. Not an
issue for `spark-submit`, where it fires once.
**Out of scope, but noticed while reading**
`ExecutorKubernetesCredentialsFeatureStep.configurePod` tests only
`Option(pod.pod.getSpec.getServiceAccount).isEmpty`, i.e. the deprecated alias.
An executor pod template that names only `serviceAccountName` is therefore
treated as unset and gets overwritten. Once the driver side reads both fields
correctly, the two steps disagree. Not for this PR, but it may deserve a
separate JIRA.
**warn vs. require**
You asked for an opinion, and I think `logWarning` is the right call. The
docs have said "cannot" since 2.3.0, so a `require` is the more honest reading,
but rejecting submissions that are accepted today does not belong in a minor
release. If we want enforcement later, it should be config-gated in its own
ticket.
With items 1 and 2 addressed I am happy with this.
--
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]
