LuciferYang commented on PR #58163:
URL: https://github.com/apache/spark/pull/58163#issuecomment-5365610350
Pushed as `3e2480aaf92`. Items 1, 3, 5 and 6 are done as asked. Item 2 takes
a slightly different route to the same end, and item 4 changes the remaining
sentence rather than only deleting the clause; both are explained below.
**1.** Five lines now, close to your version:
```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. Both
spec fields are
// read, `serviceAccountName` winning, matching Kubernetes'
SetDefaults_PodSpec: a pod
// template is deserialized with no API-server defaulting, so it can leave
either one null.
```
The `caCertFile` example and the warn versus require reasoning are in the
JIRA and the PR description. The test comments came down the same way, and the
commit is net negative overall (+37/-41).
**2.** Done, with one deviation. The constant goes in `MDC(PREFIX,
KUBERNETES_AUTH_DRIVER_MOUNTED_CONF_PREFIX)` with the `.*` as literal text,
rather than `MDC(CONFIG2, s"$KUBERNETES_AUTH_DRIVER_MOUNTED_CONF_PREFIX.*")`. I
had the string hardcoded on purpose: all 15 other `MDC(CONFIG2, ...)` sites in
the tree hold a single real config key, so a glob there turns `config2` into a
value nothing can be joined on once structured logging is enabled. `PREFIX`
takes exactly the prefix, which uses the constant and keeps the MDC value
meaningful. Switching to `CONFIG2` is one line if you prefer it.
**3.** Done. The message names the account, and the no-account case now says
what happens instead of offering both:
```
... take precedence: the pod keeps other, named by its spec. To have Spark
apply ...
... take precedence: the pod falls back to the namespace's default account.
To have Spark apply ...
```
Two assertions cover it, each with its own mutation: blanking the name fails
with `did not contain "other"`, blanking the fallback wording fails with `did
not contain "namespace's default"`.
**4.** Clause dropped. I did not leave the rest as "Spark logs a warning
about that", because with the guard in place that sentence is not always true.
It reads "Spark logs a warning when the account is dropped" now, which is
accurate without documenting the heuristic, since in the same-account case
nothing is dropped. The two table rows are untouched.
**5.** Done. It was five call sites with two discarding the value; those two
call `withLogAppender` directly now, and `withLogAppenderReturning` is left
only where the pod is used. The logger name is a suite-level val.
**6.** Checked it. Worth stating the blast radius first: the warning needs
`spark.kubernetes.authenticate.driver.serviceAccountName` set, one of the four
submitted
`spark.kubernetes.authenticate.driver.{oauthToken,caCertFile,clientKeyFile,clientCertFile}`
values rather than the `mounted.*` variants, and a pod spec that does not
already name that same account
(`DriverKubernetesCredentialsFeatureStep.scala:73`, `:91`). So whatever the
cadence, it applies to the misconfiguration the warning is about, not to
applications generally.
It is not per reconcile. Only two steps ever ask for the spec, the result is
memoized per context (`SparkAppContext.java:117-126`), and each of those two
wraps all three getters in a single try/catch that returns
(`AppInitStep.java:98-140`, `AppCleanUpStep.java:140-157`), so a reconcile
builds at most once. `AppInitStep` returns before its getters unless the state
`isInitializing()`, which is exactly `Submitted` or `ScheduledToRestart`
(`:66-67`, `ApplicationStateSummary.java:131-133`). `AppCleanUpStep` reaches
its getters only when the last state observed before termination was
`SchedulingFailure` (`:137`, `:317-321`), one check that covers a live app and
a terminated one; `cleanup()` runs that same step
(`SparkAppReconciler.java:214`) against a context of its own (`:211`), so the
finalizer shares the gate and its build is not deduplicated against the
reconcile path's. `AppResourceObserveStep`, `AppRunningStep`, `AppValidateStep`
and `AppUnknownStateStep` never ask at all, so a r
unning application never rebuilds. That part is measured, not only read:
driving the real reconciler and its real steps with the submission worker
mocked to count `getResourceSpec` calls gives 1 build at `Submitted`, 0 at
`DriverRequested` and `DriverStarted`, 0 across five consecutive
`RunningHealthy` reconciles, 0 for a `Failed` app entering restart, and 1 at
`ScheduledToRestart`, so two builds across ten reconciles for one attempt plus
one restart. A `cleanup()` of a healthy app is 0, and an app in
`SchedulingFailure` is 1 whether reached through `reconcile()` or `cleanup()`.
It does repeat without bound and with no backoff of its own in one failure
mode, which I suspect is the case you had in mind. When `persistStatus` cannot
patch the status it swallows the `KubernetesClientException` and returns false
(`StatusRecorder.java:146-155`), the step then leaves the state where it was
and requeues with no added delay (`AppReconcileStep.java:155-160`,
`ReconcileProgress.java:87-89`), and the cached status is not advanced either,
since `StatusRecorder.java:131` is reached only on success. The next reconcile
is therefore the same state again, and that hits all three building paths:
`Submitted`, `ScheduledToRestart` once its backoff window has elapsed, and the
`SchedulingFailure` release path, which an ordinary reconcile takes as well,
since `AppCleanUpStep` is the second step of every one. This last paragraph is
reasoning from the source rather than measurement, unlike the counts above.
I would still leave the warning as it is. The loop is paced from more than
one direction. Each round sleeps inside `persistStatus`, which attempts the
patch `API_STATUS_PATCH_MAX_ATTEMPTS` times (default 3) and sleeps
`API_RETRY_ATTEMPT_AFTER_SECONDS` (default 1) after each failure including the
last, on the reconcile thread (`StatusRecorder.java:96-129`), so a round costs
at least about three seconds, and considerably more once the fabric8 client's
own retries engage on a 429 or a 5xx. On top of that the operator gives this
controller a JOSDK rate limiter of 5 reconciles per 15 seconds per resource
(`SparkOperator.java:332`, `SparkOperatorConf.java:382-405`). Every round also
already logs an ERROR of the operator's own (`StatusRecorder.java:152`) plus a
WARN from the step (`AppReconcileStep.java:158`, or `:201` on the
append-and-persist paths). A feature-step line riding along is not what to fix
there. The cadence has precedent as well: `KerberosConfDriverFeatureStep` logs
from i
ts class body whenever neither krb5 config is set (`:81-84`), and that step is
constructed on every build (`KubernetesDriverBuilder.scala:86`), before the
excluded-steps filter can drop it. That one is INFO rather than WARN, so an
operator running at WARN would see mine and not it.
On the executor step, I read it the same way, and it is worse there than on
the driver side. The guard is followed by `buildPodWithServiceAccount`, which
writes both fields, so an executor template naming only `serviceAccountName`
does not merely get a stray warning, it runs under a different identity. It
dates from `7912ab85a6f`. Filed as SPARK-58910, with the repro and the reason
`ExecutorKubernetesCredentialsFeatureStepSuite` misses it: all three cases
start from `SparkPod.initialPod()`, so both fields are null and the `isEmpty`
branch always wins.
On warn versus require, agreed, and config-gated enforcement in its own
ticket sounds right if it comes up.
On CI, I read the failure on the commit you reviewed the same way:
`StateDataSourceTransformWithStateSuiteCheckpointV2` twice with "There are 2
possibly leaked file streams", nothing in `kubernetes`. The push above has no
failures so far, with one job still running.
--
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]
