peter-toth commented on code in PR #910:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/910#discussion_r4136720531
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterSuspendStep.java:
##########
@@ -124,7 +124,13 @@ public ReconcileProgress reconcile(
Optional<ReconcileProgress> waiting =
releaseResources(context, statusRecorder, keepWorkload.isEmpty());
if (waiting.isPresent()) {
- return waiting.get();
+ // Pods which are stuck terminating are looked at again only as often
as a suspended
+ // cluster otherwise, while the Workload which they no longer hold is
released once its
+ // backoff elapses, when nothing else reconciles the cluster
+ return keepWorkload
+ .filter(backoff ->
backoff.compareTo(waiting.get().getRequeueAfterDuration()) < 0)
Review Comment:
**Finding 1.** The `backoff < waiting` guard keeps a longer `keepWorkload`
from replacing a shorter retry, but no test pins it. I replaced it with
`.filter(backoff -> true)`, and all 41 tests of `ClusterSuspendStepTest` still
pass. Two cases depend on it:
- A deactivated `Workload` is kept for `suspendHoldRequeueIntervalSeconds`,
so a failed release of its cluster would be retried after 30 minutes instead of
2.
- A long backoff would push back the requeue at the release deadline of a
terminating pod, which may send no more events.
These pass with the guard and fail without it (2m vs 30m, and 30s vs 10m):
```java
@Test
void failedReleaseOfDeactivatedClusterIsRetriedWithTheDefaultInterval() {
SparkCluster cluster = buildEvictedCluster(false);
stubContext(cluster);
createWorkload(cluster, evictedStatus("Deactivated"), false);
KubernetesClient mockClient = mock(KubernetesClient.class);
when(mockClient.apps())
.thenThrow(new KubernetesClientException("Service Unavailable", 503,
null));
when(mockContext.getClient()).thenReturn(mockClient);
Assertions.assertEquals(
ReconcileProgress.completeAndDefaultRequeue(),
new ClusterSuspendStep().reconcile(mockContext, recorder));
}
@Test
void podReleaseDeadlineIsNotPushedBackByALongerBackoff() {
SparkCluster cluster = buildEvictedCluster(false);
KubernetesClient client = spy(kubernetesClient);
stubPodList(
client,
terminatingPod(
"cluster1-master-0", Constants.LABEL_SPARK_ROLE_MASTER_VALUE,
Duration.ofSeconds(270)));
stubContext(cluster, client);
WorkloadStatus status = evictedStatus("PodsReadyTimeout");
status.setRequeueState(
RequeueState.builder()
.count(1)
.requeueAt(Instant.now().plusSeconds(600).toString())
.build());
createWorkload(cluster, status);
ReconcileProgress progress = new
ClusterSuspendStep().reconcile(mockContext, recorder);
Assertions.assertTrue(
progress.getRequeueAfterDuration().compareTo(Duration.ofSeconds(30))
<= 0,
progress.getRequeueAfterDuration().toString());
}
```
##########
docs/spark_custom_resources.md:
##########
@@ -739,7 +739,10 @@ spec:
evicted `Workload` until then, so the operator then deletes it, and the
resource is queued again
with a new `Workload`. Unlike Kueue built-in integrations, which keep the
evicted
`Workload`, the new `Workload` is ordered in its queue like a newly created
one, and it starts
- over the retry counts of its admission checks.
+ over the retry counts of its admission checks and the requeue count of its
`PodsReadyTimeout`s.
+ So their backoff does not grow with each eviction, e.g. it stays at
+ `requeuingStrategy.backoffBaseSeconds` (60 seconds by default) for a
`PodsReadyTimeout`, and a
+ `requeuingStrategy.backoffLimitCount` above 0 never deactivates the
`Workload`.
Review Comment:
**Finding 2.** Starting over with a new `Workload` costs more for a
`PodsReadyTimeout` than for the evictions of SPARK-59803. The growing backoff
and `backoffLimitCount` are the only knobs Kueue has for it, and both act on
the requeue count. With a new `Workload` after every timeout, a cluster whose
workers can never be scheduled is admitted and evicted roughly every `timeout`
+ 60 seconds, forever.
Kueue's built-in integrations keep the `Workload`. Once the pods of the job
are gone, jobframework clears its admission
([`reconciler.go:586-620`](https://github.com/kubernetes-sigs/kueue/blob/v0.19.4/pkg/controller/jobframework/reconciler.go#L586-L620)).
It sets `Requeued` and calls `UnsetQuotaReservationWithCondition`, and Kueue
then requeues the same `Workload` after its backoff with the count kept.
The operator could do the same in `releaseResources` instead of deleting the
evicted `Workload`:
- Clear `status.admission`, and set `QuotaReserved=False` and `Requeued` on
it. Kueue's webhook allows unsetting the admission
([`workload_webhook.go:397-411`](https://github.com/kubernetes-sigs/kueue/blob/v0.19.4/pkg/webhooks/workload_webhook.go#L397-L411)).
- Move to `Submitted`. `requestAdmission` already keeps an evicted
`Workload` without quota as pending (`KueueWorkloadUtils.java:183-186`).
- Drop the `keepKueueWorkload` backoff wait on that path, since Kueue owns
the backoff then.
That would also keep the queue position and the retry counts of the
admission checks, which the doc above lists as lost for every eviction. The
case against: the operator would write more of the status fields Kueue owns
(today it writes only `PodsReady`), and it reworks the release path of
SPARK-59803. A lighter variant copies the old `requeueState.count` onto the new
`Workload`. That keeps the growing backoff and the limit, but not the queue
position.
--
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]
