peter-toth commented on code in PR #981:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/981#discussion_r4351009036
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterSuspendStep.java:
##########
@@ -267,6 +267,7 @@ private Optional<ReconcileProgress> releaseResources(
KubernetesClient client = context.getClient();
try {
List<Pod> pods = deleteMasterAndWorkers(client, cluster);
+ deleteWorkerAutoscalerAndDisruptionBudget(client, cluster);
Review Comment:
**Finding 1.** The suspend path keeps the shape that this PR fixes for a
failed cluster. A persistent failure of the HPA or PDB delete lands in the
`catch`, so `deleteWorkloadOf` never runs. A suspended cluster keeps its quota,
and an evicted one keeps the quota its preemptor waits for.
The operator may lack this access even when its clusters run fine:
- A queued cluster never has an HPA, since
`KueueWorkloadFactory.buildWorkload` rejects `minWorkers < maxWorkers`. So an
operator which runs only queued clusters never applies one.
- A cluster with `minWorkers: 0` has no PDB.
`ClusterInitStep` applies them only when the spec asks for them, so such
clusters reach `RunningHealthy` without the access. The delete here is by name,
so it gets `403` anyway.
I checked it with a throwaway test in `ClusterSuspendStepTest`. It used an
evicted cluster and a `spec.suspend` one, each with a `Workload` and no pods,
and `autoscaling()` or `policy()` throwing `403` on a spy client. After 5
reconciles the `Workload` was still there in all 4 cases.
Nothing needs them gone before the `Workload` is released. A resume still
waits for them, since it needs `releaseResources` to return empty. So the call
can move after the release:
```java
List<Pod> pods = deleteMasterAndWorkers(client, cluster);
Optional<ReconcileProgress> waiting = waitForPods(pods);
if (waiting.isPresent()) {
log.debug("Waiting for the pods of the suspended cluster to be
deleted.");
return waiting;
}
if (releaseWorkload) {
// A Workload admitted before the queue label was removed is
released as well.
KueueWorkloadUtils.deleteWorkloadOf(client, cluster);
}
deleteWorkerAutoscalerAndDisruptionBudget(client, cluster);
```
With it, the same probe releases the `Workload` in all 4 cases, and the
reconcile returns `completeAndDefaultRequeue()` to retry the delete. The 44
existing `ClusterSuspendStepTest` cases and the 9 `ClusterCleanUpStepTest`
cases still pass. The probe could stay as a test next to
`failedReleaseIsRetriedWithoutReleasingKueueWorkload`. The Javadoc of
`releaseResources` and of the new helper would then say the same as
`ClusterCleanUpStep`'s.
--
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]
