peter-toth commented on code in PR #975:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/975#discussion_r4334882853
##########
spark-operator-api/src/main/java/org/apache/spark/k8s/operator/utils/ModelUtils.java:
##########
@@ -83,15 +92,36 @@ public static List<ContainerStatus>
findDriverMainContainerStatus(
if (appSpec == null) {
return containerStatusList;
}
- Map<String, String> sparkConf = appSpec.getSparkConf();
- String key = sparkConf.get(DRIVER_SPARK_CONTAINER_PROP_KEY);
- if (key == null || key.isEmpty()) {
- return containerStatusList;
+ String mainContainerName = getDriverMainContainerName(appSpec);
+ List<ContainerStatus> mainContainerStatusList =
+ containerStatusList.stream()
+ .filter(c -> mainContainerName.equalsIgnoreCase(c.getName()))
+ .collect(Collectors.toList());
+ return mainContainerStatusList.isEmpty() ? containerStatusList :
mainContainerStatusList;
+ }
+
+ /**
+ * Returns the name of the driver container, which Spark selects from the
pod template like
+ * KubernetesUtils.selectSparkContainer and names like
BasicDriverFeatureStep.
+ */
+ private static String getDriverMainContainerName(final ApplicationSpec
appSpec) {
+ String containerName =
appSpec.getSparkConf().get(DRIVER_SPARK_CONTAINER_PROP_KEY);
+ if (!overrideDriverTemplateEnabled(appSpec)) {
+ return containerName == null || containerName.isEmpty()
Review Comment:
**Finding 1.** Without a pod template in the spec, Spark reads
`spark.kubernetes.driver.podTemplateContainerName` only when it loads
`spark.kubernetes.driver.podTemplateFile`
([`KubernetesDriverBuilder`](https://github.com/apache/spark/blob/v4.2.0/resource-managers/kubernetes/core/src/main/scala/org/apache/spark/deploy/k8s/submit/KubernetesDriverBuilder.scala#L42-L50)).
Otherwise it starts from `SparkPod.initialPod()`, and `BasicDriverFeatureStep`
names the container `spark-kubernetes-driver`.
Here the property wins whenever it is set, even without a template file.
Then no container status has that name, so all containers count again. I ran
the observers at this head with the property set to `driver` and the statuses
`[istio-proxy, spark-kubernetes-driver]`:
- `istio-proxy` exited with 0 or 1 and the driver is running.
`AppDriverReadyObserver` returns `Succeeded` or `Failed`.
- `istio-proxy` is ready and the driver is not. `AppDriverStartObserver`
returns `DriverStarted`.
Using the property only with a template file, like Spark, fixes it (plus the
static import of `DRIVER_SPARK_TEMPLATE_FILE_PROP_KEY`):
```java
if (!overrideDriverTemplateEnabled(appSpec)) {
// Like Spark, the property applies only to a pod template file.
return containerName == null
|| containerName.isEmpty()
||
!appSpec.getSparkConf().containsKey(DRIVER_SPARK_TEMPLATE_FILE_PROP_KEY)
? DEFAULT_DRIVER_CONTAINER_NAME
: containerName;
}
```
With it, all three cases return empty, and the PR's tests pass. The only
test to update is the second half of `testFindDriverMainContainerStatus`, which
needs `spark.kubernetes.driver.podTemplateFile` in its `sparkProps`. The
javadoc sentence starting "Without the pod template in the application
specification" would then name the pod template file too.
##########
docs/migration_guide.md:
##########
@@ -172,6 +172,23 @@ affect building the project, CI, tests or examples.
`driverStartTimeoutMillis` was disabled. Since `DriverStartTimedOut` is an
infrastructure failure
while the others are not, `restartPolicy: OnInfrastructureFailure` no longer
restarts such an
application. Use `OnFailure` to restart it.
+- Since 1.1.0, only the driver container of the driver pod decides whether the
application has
+ started, succeeded or failed. Like Apache Spark, it is the container of the
driver pod template
+ which `spark.kubernetes.driver.podTemplateContainerName` names, or else its
first container, or
+ `spark-kubernetes-driver` without a pod template. 1.0 considered all
containers unless the
+ property was set, so an application moved to `Succeeded` or `Failed` as soon
as a sidecar
+ container, e.g. a log collector or `istio-proxy`, exited, and its driver pod
was deleted while
+ the driver was still running, unless `resourceRetainPolicy` retained it.
Likewise, an
+ application with a sidecar container moves to `DriverStarted` when the
driver container is ready
+ instead of any container, so a driver container whose image cannot be pulled
ends in
+ `DriverStartTimedOut`, which `restartPolicy: OnInfrastructureFailure`
restarts, instead of
+ `DriverReadyTimedOut`. If the driver container is not found, e.g. since the
first container of
Review Comment:
**Finding 2.** One more consequence the guide doesn't cover.
`AppDriverReadyObserver` still moves to `DriverReady` only on the pod's `Ready`
condition, which counts every container. So a regular sidecar container which
exits before the pod is first ready keeps the pod not ready for good, e.g. a
log collector which crashes at start.
- 1.0 moved such an app to `Failed`, or `Succeeded` on exit code 0, right
away.
- Now the driver keeps running, and the app ends as `DriverReadyTimedOut`
after `driverReadyTimeoutMillis`, 5 minutes by default.
I checked it at this head with a `DriverStarted` state 301 seconds old, a
`Ready=False` condition and the statuses `[log-collector (exit 1),
spark-kubernetes-driver (running, ready)]`. `AppDriverReadyObserver` and
`AppDriverRunningObserver` return empty, and `AppDriverTimeoutObserver` returns
`DriverReadyTimedOut`. With `ModelUtils` and `BaseAppDriverObserver` reverted,
both observers return `Failed`.
The new end state is arguably right. The driver service is headless without
`publishNotReadyAddresses`, so executors cannot resolve a driver pod that is
not ready. A sentence after the `DriverStarted` one would cover it, e.g.:
```markdown
Similarly, an application whose sidecar container exits before the driver
pod is ready ends in `DriverReadyTimedOut` after `driverReadyTimeoutMillis`,
instead of `Failed` or `Succeeded` right away.
```
--
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]
