viirya commented on code in PR #828:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/828#discussion_r4018838860
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
Review Comment:
I agree with this finding, but I think it should be addressed before merging
rather than deferred to support for suspending running attempts.
Resource creation and status persistence are separate operations. If the
driver is created but persisting `DriverRequested` fails, the next
reconciliation restores `Submitted` from the status cache. An operator restart
between those operations produces the same discrepancy.
If `suspend` becomes `true` at that point, this branch prevents
initialization recovery. `SparkAppReconciler` selects only validation, cleanup,
and initialization for `Submitted`, so the existing driver is no longer
observed for completion or timeout while the flag remains set. The equivalent
window also exists in `ClusterInitStep` between resource creation and
persisting `RunningHealthy`.
Could we distinguish an initialization that has already started from one
that has not requested resources yet, and allow the former to recover? Please
add a regression test covering successful resource creation, failed status
persistence, and then enabling suspend.
I would avoid fixing this solely with `getDriverPod().isEmpty()`: that
lookup uses the informer cache, and the driver labels do not distinguish
attempts, so a previous attempt's pod could incorrectly bypass the hold.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppInitStep.java:
##########
@@ -67,6 +67,10 @@ public ReconcileProgress reconcile(
return proceed();
}
SparkApplication app = context.getResource();
+ if (app.getSpec().isSuspend()) {
+ log.debug("Application is suspended, driver resources would not be
requested.");
+ return completeAndDefaultRequeue();
Review Comment:
Holding the application here also preserves the initializing state's
timestamp. This affects the existing restart-counter logic:
`ApplicationStatus.calculateCurrentAttemptDuration()` measures from the latest
`Submitted` or `ScheduledToRestart`, and `terminateOrRestart()` applies the
time-based counter reset before checking retry limits.
For example, with `maxRestartAttempts: 1` and `restartCounterResetMillis:
3600000`, hold the permitted retry in `ScheduledToRestart` for over an hour,
then resume it and let it fail immediately. The suspended hour satisfies the
reset threshold, so the counter resets and another retry is allowed instead of
exhausting the configured limit.
This makes time spent waiting for admission count toward the documented
reset for a long-running attempt. Could we exclude suspended time from that
calculation and cover this sequence with both trimmed and untrimmed histories?
Simply changing the initializing timestamp on resume would also change
restart-backoff and history semantics, so those should be preserved when
addressing this.
##########
docs/spark_custom_resources.md:
##########
@@ -525,6 +525,34 @@ Note that `ttlAfterStopMillis` applies to the app as well
as its secondary resou
latter is smaller, then it takes higher precedence: operator would remove all
resources related
to this app after `ttlAfterStopMillis`.
+## Suspend
+
+Both `SparkApplication` and `SparkCluster` support `.spec.suspend`. When it is
set to `true`, the
+operator keeps the resource in its initializing state (`Submitted`, or
`ScheduledToRestart` for an
+application that is scheduled to restart) and does not request the driver pod
or the master / worker
+StatefulSets. Setting it back to `false` resumes the regular lifecycle.
Review Comment:
This distinction is still missing from the documentation at the current
head. A valid resource created with `suspend: true` does not have its initial
status persisted, so `Submitted` is an in-memory state and the `Current State`
column remains blank. An application held in `ScheduledToRestart`, however,
retains the status already written by cleanup.
Deferring a dedicated `Suspended` state seems reasonable for this PR, but
please document the observable behavior explicitly. For example:
> For a valid resource created with `suspend: true`, the initial `Submitted`
status is not persisted to the API server, so `kubectl get` shows an empty
`Current State` until initialization resumes. An application held in
`ScheduledToRestart` retains its previously persisted status.
The PR description's statement that a held resource has no `.status` should
likewise be limited to the initial-submission case.
--
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]
