peter-toth commented on code in PR #867:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/867#discussion_r4080143178
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/observers/BaseAppDriverObserver.java:
##########
@@ -101,6 +99,11 @@ protected Optional<ApplicationState>
observeDriverTermination(
return Optional.of(new ApplicationState(Succeeded,
DRIVER_COMPLETED_MESSAGE));
}
+ if (status.getContainerStatuses() == null ||
status.getContainerStatuses().isEmpty()) {
Review Comment:
**Finding 1.** Everything below this guard is untested. I deleted the whole
span from `initContainerStatusList` at `:107` through the `isDriverRestarted`
block at `:139` — the init-container failure, `terminatedCriticalContainers`
and driver-restarted branches, all three at once — and `./gradlew
:spark-operator:test` reported `BUILD SUCCESSFUL`, 401 tests, nothing failed.
The reason the existing coverage does not reach them is the short-circuit
this PR just moved. `AppResourceObserveStepTest.observeTerminatedDriver` builds
its pod with `withPhase("Failed")` / `withPhase("Succeeded")` plus a terminated
container status, so both of its cases return at `:89` or `:98` and never get
past this guard. So `DRIVER_FAILED_INIT_CONTAINERS_MESSAGE`,
`DRIVER_SUCCEEDED_MESSAGE` and `DRIVER_RESTARTED_MESSAGE` are produced by no
test.
Not a defect in the reorder, which is scoped correctly and is itself pinned
— reverting it fails three of your four new tests. What makes it worth raising
here is that this PR adds the first test file the `observers` package has ever
had, and it reorders the method directly above these branches, so a later
regression in them would be silent. Each needs a pod whose phase is *not*
terminal, which is what keeps them reachable:
```java
private Pod runningPodWithDriverContainer(ContainerStatus driverContainer)
{
return new PodBuilder()
.withNewStatus()
.withPhase("Running")
.withContainerStatuses(driverContainer)
.endStatus()
.build();
}
@Test
void terminatedDriverContainerWithNonZeroExitIsFailed() { /* exitCode 1 ->
Failed */ }
@Test
void terminatedDriverContainerWithZeroExitIsSucceeded() { /* exitCode 0 ->
Succeeded */ }
@Test
void restartedDriverContainerIsFailed() { /* restartCount 1, running state
-> Failed */ }
@Test
void failedInitContainerIsFailed() { /* withInitContainerStatuses(failed),
main container waiting */ }
```
The container name has to be the driver main container that
`ModelUtils.findDriverMainContainerStatus` selects, and unlike your four new
tests these need a real `ApplicationSpec` rather than `new ApplicationSpec()`,
because `spec` is only read past this guard.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/observers/BaseAppDriverObserver.java:
##########
@@ -101,6 +99,11 @@ protected Optional<ApplicationState>
observeDriverTermination(
return Optional.of(new ApplicationState(Succeeded,
DRIVER_COMPLETED_MESSAGE));
}
+ if (status.getContainerStatuses() == null ||
status.getContainerStatuses().isEmpty()) {
+ log.debug("Cannot determine driver pod status, the pod may in pending
state.");
Review Comment:
**Finding 2.** This is now byte-identical to the message at `:85`, and the
two guards mean different things: `:85` is "there is no status at all", this
one is "there is a status, the phase is not terminal, and no container has
reported yet". With one shared string a debug log cannot tell you which branch
returned, on the method whose whole bug was calling a terminal pod "pending".
#863's proposed patch split them, using `"Cannot determine driver pod
status, status is null."` for the first. Worth taking that, and dropping "may
in pending state" from the `status == null` branch where it is not even true:
```suggestion
if (status.getContainerStatuses() == null ||
status.getContainerStatuses().isEmpty()) {
log.debug("No container has reported yet, the driver pod may be in
pending state.");
return Optional.empty();
```
with `:85` becoming `log.debug("Cannot determine driver pod status, the
status is null.");`.
--
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]
