peter-toth commented on code in PR #857:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/857#discussion_r4061719940
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -194,14 +222,16 @@ private Optional<ReconcileProgress> holdForKueueAdmission(
* Checks whether the driver pod of the current attempt has already been
requested. This covers
* the case where the driver was created but the status update to
DriverRequested failed, so that
* a suspended application still completes its initialization instead of
being held with a live
- * driver. See {@link SparkAppContext#getCurrentAttemptDriverPod()} for how
a pod left from a
- * previous attempt is told apart.
+ * driver. See {@link SparkAppContext#getCurrentAttemptDriverPodStrictly()}
for how a pod left
+ * from a previous attempt is told apart.
*
* @param context The SparkAppContext for the application.
* @return True if the driver pod of the current attempt exists, false
otherwise.
+ * @throws KubernetesClientException if the lookup fails, so that a running
driver is not
+ * mistaken for one that was never requested.
*/
private boolean isDriverRequested(SparkAppContext context) {
- return context.getCurrentAttemptDriverPod().isPresent();
+ return context.getCurrentAttemptDriverPodStrictly().isPresent();
Review Comment:
**Finding 5.** Nothing pins the throw this line depends on. I replaced the
body of `getCurrentAttemptDriverPodStrictly` with a version that catches
`KubernetesClientException` and returns `Optional.empty()` — the lenient
behaviour this PR exists to get off the app path — and the entire
`:spark-operator` suite passed.
Why the five new tests do not cover it: they all stub a
`mock(SparkAppContext.class)`, so
`when(mockContext.getCurrentAttemptDriverPodStrictly()).thenThrow(...)` tests
`AppInitStep`'s reaction to a throwing context, never that the real method
throws. And every one of the seven `SparkAppContextTest` cases calls the
lenient `getCurrentAttemptDriverPod()`, including
`apiErrorDuringVerificationIsTreatedAsAbsent`, which asserts the *opposite*
contract. So the strict variant has no caller in any test.
The cluster side does not have this hole: `isMasterRequested` is real code
in `ClusterInitStep`, and `suspendedClusterWithUnverifiableMasterIsNotHeld`
makes the client itself throw, so the read and the decision are covered
together.
This is new rather than something I missed last round — `e90b0c6` had
`SparkAppContextTest.apiErrorDuringVerificationIsPropagated` doing exactly
this, and the rebase onto #854 replaced it with the lenient-wrapper version.
Minimum fix, which I ran both ways (fails on the swallowing mutant, passes
on `3ab7498`):
```java
@Test
void apiErrorDuringVerificationIsPropagatedStrictly() {
SparkAppContext context = buildContext(List.of(driverPodSpec), null);
when(context.getClient().pods().inNamespace("default").withName(anyString()).get())
.thenThrow(new KubernetesClientException("boom", 500, null));
Assertions.assertThrows(
KubernetesClientException.class,
context::getCurrentAttemptDriverPodStrictly);
}
```
Dropping the lenient wrapper instead gets this for free: the six behaviour
cases retarget onto the strict method, and
`apiErrorDuringVerificationIsTreatedAsAbsent` becomes the assertion above.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -183,9 +184,36 @@ public ReconcileProgress reconcile(
*/
private Optional<ReconcileProgress> holdForKueueAdmission(
SparkAppContext context, SparkApplication app) {
- if (!KueueWorkloadFactory.hasQueueName(app) || isDriverRequested(context))
{
+ if (!KueueWorkloadFactory.hasQueueName(app)) {
return Optional.empty();
}
+ try {
+ if (isDriverRequested(context)) {
+ return Optional.empty();
+ }
+ } catch (KubernetesClientException e) {
+ // Requesting the admission of a driver which is already running would
be wrong, so the
+ // lookup is retried rather than failing the application with the
terminal
+ // SchedulingFailure. Like a failed admission request, a transport level
failure is not
+ // published, since writing an event would only add load to an API
server that is often the
+ // cause of the failure, and it goes away on its own, so it keeps the
short interval.
+ log.warn("Failed to check whether the driver exists before requesting
admission.", e);
+ if (ReconcilerUtils.isTransientError(e)) {
Review Comment:
**Finding 6.** This block, `ClusterInitStep.java:194-215` and
`KueueWorkloadUtils.java:184-201` are now the same shape three times:
`log.warn`, `isTransientError` to the short interval with no event, otherwise a
`KueueAdmissionRequestFailed` warning and the default interval. Round 1 had one
copy of it; this PR adds the second and third, which is the point at which it
is worth naming.
The risk is drift rather than volume. A future change to the classification
— adding 429 to `isTransientError`, changing the persistent interval, adding a
metric — has to be made in three places, and two of them are in files whose
tests would not notice.
Since all three live in front of `holdForAdmission` and share its reason,
the helper belongs next to it:
```java
/**
* Reports a failed Kueue admission request, or a failed read that has to
happen before one, and
* returns the progress to retry it with. A transport level failure is not
published, since
* writing an event would only add load to an API server that is often the
cause of it, and it
* goes away on its own, so it keeps the short interval. Anything else,
such as a missing RBAC
* rule, is retried with the default interval so that its event is not
rewritten every few
* seconds until a user fixes the cause.
*/
static ReconcileProgress retryAfterRequestFailure(
final BaseContext<?> context, final KubernetesClientException e, final
String what) {
log.warn("{}, will retry.", what, e);
if (ReconcilerUtils.isTransientError(e)) {
return
ReconcileProgress.completeAndRequeueAfter(STALE_WORKLOAD_REQUEUE_INTERVAL);
}
EventUtils.warn(
context.getEventRecorder(),
EventUtils.REASON_KUEUE_ADMISSION_REQUEST_FAILED,
what + ", will retry. " + EventUtils.describe(e));
return ReconcileProgress.completeAndDefaultRequeue();
}
```
Each catch then reads `return
Optional.of(KueueWorkloadUtils.retryAfterRequestFailure(context, e, "Failed to
check whether the driver exists before requesting Kueue admission"));`, and
`holdForAdmission`'s own catch passes `"Failed to request Kueue admission"`.
The five new tests already assert both arms for both steps, so they pin the
extraction as it stands.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterInitStep.java:
##########
@@ -65,8 +67,20 @@ public ReconcileProgress reconcile(
SparkCluster cluster = context.getResource();
// A cluster whose master StatefulSet already exists has been requested
before (e.g. the status
// update to RunningHealthy failed), so let it complete its initialization
even if suspended.
- if (cluster.getSpec().isSuspend() && !isMasterRequested(context)) {
- return SuspendUtils.holdForSuspend(context, "master and workers");
+ if (cluster.getSpec().isSuspend()) {
+ final boolean masterRequested;
+ try {
+ masterRequested = isMasterRequested(context);
+ } catch (KubernetesClientException e) {
+ // Whether the master is live is unknown, not answered. Holding would
claim in an event
+ // that none was requested, and would release the Kueue quota of a
running master, so
+ // look again with the steady-state interval instead.
+ log.warn("Failed to check whether the master of a suspended cluster
exists.", e);
+ return completeAndDefaultRequeue();
Review Comment:
**Finding 7.** Not publishing `SuspendHeld` here is right, since it would
claim no master was requested. But publishing nothing leaves a suspended
cluster with no signal at all when the failure is persistent, and
`suspendedClusterWithUnverifiableMasterIsNotHeld` pins that silence with
`verifyNoInteractions(eventRecorder)`.
Three facts stack up. A suspended resource never gets its `Submitted` status
persisted, which `docs/spark_custom_resources.md` states outright. This branch
returns before `SuspendUtils.holdForSuspend`, so there is no event. And
`verifyNoInteractions(recorder)` in that test confirms no status write. So for
a 403 on `StatefulSet` reads the user sees an empty `Current State`, no events,
and a 120-second loop that never resolves — while the operator cannot decide
whether the master is live.
For a 503 silence is the right answer, and for the same reason the admission
branch gives: do not add event writes to an API server that is already failing.
It is the persistent half that is asymmetric, because the admission branch
fourteen lines down classifies the identical exception and reports it.
`KueueAdmissionRequestFailed` does not fit, since no admission is involved,
so this needs its own reason:
```java
} catch (KubernetesClientException e) {
log.warn("Failed to check whether the master of a suspended cluster
exists.", e);
if (!ReconcilerUtils.isTransientError(e)) {
EventUtils.warn(
context.getEventRecorder(),
EventUtils.REASON_SUSPEND_CHECK_FAILED,
"Cannot tell whether the master was requested before, so the
suspend hold is "
+ "deferred. " + EventUtils.describe(e));
}
return completeAndDefaultRequeue();
}
```
plus the `REASON_SUSPEND_CHECK_FAILED` constant and a row in the `Warning`
table of `docs/configuration.md`. `AppInitStep.java:78-84` has the same gap and
came in with #854, so the same helper covers both. If you would rather not add
a reason in this PR, a follow-up is fine — the cluster half is what this PR
adds, so I am raising it here rather than against #854.
--
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]
