peter-toth commented on code in PR #866:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/866#discussion_r4079981163
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterUnknownStateStep.java:
##########
@@ -33,11 +33,17 @@ public final class ClusterUnknownStateStep extends
ClusterReconcileStep {
*
* @param context The SparkClusterContext for the cluster.
* @param statusRecorder The SparkClusterStatusRecorder for recording status
updates.
- * @return The ReconcileProgress indicating an immediate re-queue.
+ * @return The ReconcileProgress indicating an immediate re-queue, or no
re-queue if the cluster
+ * is already failed.
*/
@Override
public ReconcileProgress reconcile(
SparkClusterContext context, SparkClusterStatusRecorder statusRecorder) {
+ // Appending Failed again would write a new status on every reconcile,
forever.
+ if
(context.getResource().getStatus().getCurrentState().getCurrentStateSummary()
+ == ClusterStateSummary.Failed) {
+ return ReconcileProgress.completeAndNoRequeue();
Review Comment:
**Finding 1.** This line is what makes `Failed` the terminal state of a
`SparkCluster`, and `docs/architecture.md` says the opposite. The `## Cluster
State Transition` diagram is:
```
Submitted --> RunningHealthy
Submitted --> SchedulingFailure
RunningHealthy --> Failed
RunningHealthy --> ResourceReleased
SchedulingFailure --> ResourceReleased
Failed --> ResourceReleased
ResourceReleased --> [*]
```
Against the code, four of those six edges cannot be taken and one real edge
is missing:
- **`ResourceReleased` is never set.** `grep` for
`ClusterStateSummary.ResourceReleased` finds only the `case` label in
`SparkClusterReconciler.getReconcileSteps`, and `cleanup()` does not set it
either — it runs `ClusterValidateStep` plus `ClusterTerminatedStep` and then
`defaultDelete()`, leaving the secondary resources to owner-reference GC. So
`RunningHealthy --> ResourceReleased`, `SchedulingFailure -->
ResourceReleased`, `Failed --> ResourceReleased` and `ResourceReleased --> [*]`
are all unreachable.
- **`RunningHealthy --> Failed` cannot happen** either: the `RunningHealthy`
case adds no step at all ("There is nothing to do because Spark Cluster is
supposed to run infinitely"), and `ClusterUnknownStateStep` is the only
producer of `Failed`.
- **`SchedulingFailure --> Failed` is missing**, which is the one failure
edge that exists and the one this guard terminates. Your description calls it
"the existing `SchedulingFailure -> Failed` transition", and a reader cannot
find it in the diagram.
The doc was already wrong about `ResourceReleased` before this PR, which is
why I am not filing it as Blocking. What makes it this PR's business is the
direction: the diagram tells a user that a failed cluster proceeds to have its
resources released, and this change is what settles that it never does. The
prose right below it says the same thing in words — "K8s resources created for
a cluster would be deleted as the final stage of the cluster lifecycle by
default" — which is true only via owner-reference GC when the CR itself is
deleted, not via any state transition.
The corrected machine is small enough to state exactly:
```mermaid
stateDiagram-v2
[*] --> Submitted
Submitted --> RunningHealthy
Submitted --> SchedulingFailure
SchedulingFailure --> Failed
RunningHealthy --> [*]
Failed --> [*]
```
with the prose noting that a cluster leaves either state only by being
deleted, at which point owner references garbage-collect its resources, and
that a `Failed` cluster is not reconciled further.
--
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]
