peter-toth commented on code in PR #890:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/890#discussion_r4110661982
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -320,10 +322,12 @@ private static StatefulSet buildWorkerStatefulSet(
.endMetadata()
.editOrNewSpec()
.withSchedulerName(scheduler)
- .withTerminationGracePeriodSeconds(0L)
.withNewDnsConfig()
Review Comment:
**Finding 1.** The same bug this PR fixes lives on this line, and here it
drops user config rather than a default.
`withNewDnsConfig()` starts a fresh nested builder, so whatever the user put
in `spec.workerSpec.statefulSetSpec.template.spec.dnsConfig` is thrown away.
`editOrNewDnsConfig()` is the variant that seeds from the existing value.
Measured with a probe that sets `nameservers: [10.0.0.10]` and `searches:
[my.search.domain]` on the template used for both roles:
```
master dnsConfig=PodDNSConfig(nameservers=[10.0.0.10], options=[],
searches=[my.search.domain])
worker dnsConfig=PodDNSConfig(nameservers=[], options=[],
searches=[cluster-name-worker-svc.my-namespace.svc.cluster.local])
```
So the master keeps it, because its builder never touches `dnsConfig`, and
the worker loses the nameservers and the search domain. On a cluster with
custom DNS that breaks name resolution for the workers, silently.
This one has no alternative channel, which is what separates it from
`schedulerName`: there is no Spark conf or spec field that adds a search
domain, so the pod template is the only place a user can put it.
```java
.editOrNewDnsConfig()
.addToSearches(String.format("%s-worker-svc.%s.svc.cluster.local", name,
namespace))
.endDnsConfig();
```
I applied that and re-ran the probe:
```
worker dnsConfig=PodDNSConfig(nameservers=[10.0.0.10], options=[],
searches=[my.search.domain,
cluster-name-worker-svc.my-namespace.svc.cluster.local])
```
`SparkClusterResourceSpecTest` stays green, and `spotlessCheck`,
`checkstyleMain`, `pmdMain` and `spotbugsMain` pass. `addToSearches` appends,
so the user's entries are tried first; `addToSearches(0, ...)` would put the
worker domain first if you prefer that.
Worth knowing before you touch it: nothing in the repo asserts the worker's
search domain today. I grepped `spark-submission-worker/src/test`,
`spark-operator/src/test` and `tests/` for `svc.cluster.local` and got no hits,
so the only `dnsConfig` reference in `SparkClusterResourceSpecTest` was my own
probe. A fix here would be carrying the first test for it, which is a good
reason to keep it in a follow-up rather than tacking it on.
##########
spark-submission-worker/src/main/java/org/apache/spark/k8s/operator/SparkClusterResourceSpec.java:
##########
@@ -237,8 +237,10 @@ private static StatefulSet buildMasterStatefulSet(
.addToLabels(LABEL_SPARK_VERSION_NAME, version)
.endMetadata()
.editOrNewSpec()
- .withSchedulerName(scheduler)
- .withTerminationGracePeriodSeconds(0L);
+ .withSchedulerName(scheduler);
Review Comment:
**Finding 2.** This line has the same shape as the one being fixed: an
unconditional overwrite whose value is a default.
`scheduler` is `conf.get(Config.KUBERNETES_SCHEDULER_NAME().key(),
"default-scheduler")` at `SparkClusterResourceSpec.java:70`, so when the conf
is unset it is the literal string `default-scheduler`, not an operator
invariant. Measured with a template that sets `schedulerName: yunikorn` and no
Spark conf:
```
master schedulerName=default-scheduler
worker schedulerName=default-scheduler
```
The consequence is quiet rather than loud. With YuniKorn installed the way
the README does it, `--set embedAdmissionController=false`, nothing re-labels
the pods, so they simply go to the default scheduler and the user sees no error.
I am filing this below finding 1 rather than next to it because there *is* a
sanctioned channel here: `examples/cluster-on-yunikorn.yaml:29` sets
`spark.kubernetes.scheduler.name: "yunikorn"` in `sparkConf`, and that path
works. So this is about the pod template silently losing to a default, not
about a user having no way to express what they want.
The same guard as the one in this PR would do it, if you think the template
should win:
```java
if (StringUtils.isNotEmpty(scheduler) &&
!partialStatefulSet.hasSchedulerName()) {
partialStatefulSet = partialStatefulSet.withSchedulerName(scheduler);
}
```
That reverses the precedence though, so the conf would no longer override a
template. Reading `conf.get(key, null)` and only applying a non-null value
keeps the conf authoritative while letting the template through when the conf
is unset, which matches how `terminationGracePeriodSeconds` now behaves. Either
is a judgement call about which of the two the user means, so a follow-up
ticket is the right place for it.
--
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]
