peter-toth commented on code in PR #877:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/877#discussion_r4091203521
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -517,20 +518,40 @@ public static boolean releaseWorkload(final
KubernetesClient client, final HasMe
/**
* Deletes the Workload of the given resource and reports whether one was
there to delete, unlike
- * {@link #releaseWorkload} without swallowing the failure.
+ * {@link #releaseWorkload} without swallowing the failure. A Workload
admitted before the queue
+ * label of the owner was removed is deleted as well. One without Kueue
installed is answered with
+ * a 404, which the deletion ignores.
+ *
+ * <p>Without the label, an operator which does not watch Workloads may not
be granted access to
+ * them, e.g. with Kueue installed for other workloads, so its rejected
deletion is taken as no
+ * Workload. The rejection cannot tell such an operator apart from one whose
access was revoked
+ * after it created a Workload, and a read would be rejected as well. Such a
Workload is left
+ * behind, since retrying cannot delete it either, while failing would keep
every suspended
+ * resource of an operator without the access from resuming.
*
* @param client The KubernetesClient.
* @param owner The SparkApplication or SparkCluster owning the Workload.
- * @return True if a Workload was deleted, false if there was none.
+ * @return True if a Workload was deleted, false if there was none or it may
not be deleted.
* @throws KubernetesClientException if the Workload cannot be deleted.
*/
public static boolean deleteWorkloadOf(final KubernetesClient client, final
HasMetadata owner) {
- return !client
- .resources(Workload.class)
- .inNamespace(owner.getMetadata().getNamespace())
- .withName(KueueWorkloadFactory.getWorkloadName(owner))
- .delete()
- .isEmpty();
+ String name = KueueWorkloadFactory.getWorkloadName(owner);
+ try {
+ return !client
+ .resources(Workload.class)
+ .inNamespace(owner.getMetadata().getNamespace())
+ .withName(name)
+ .delete()
+ .isEmpty();
+ } catch (KubernetesClientException e) {
+ if (KueueWorkloadFactory.hasQueueName(owner)
+ || KUEUE_WORKLOAD_INFORMER_ENABLED.getValue()
+ || e.getCode() != HTTP_FORBIDDEN) {
Review Comment:
**Finding 1.** This clause is the one part of the swallow that no test holds
down.
Per-clause mutation on the three rethrow conditions:
| mutation | result |
|---|---|
| drop `hasQueueName(owner) \|\|` | 2 fail:
`forbiddenKueueWorkloadReleaseIsIgnoredOnlyWithoutKueueAccess[true, false]` and
`KueueWorkloadUtilsTest.rejectedAdmissionWithFailedReleaseIsRetried` |
| drop `KUEUE_WORKLOAD_INFORMER_ENABLED.getValue() \|\|` | 1 fails:
`forbiddenKueueWorkloadReleaseIsIgnoredOnlyWithoutKueueAccess[false, true]` |
| replace `e.getCode() != HTTP_FORBIDDEN` with `false` | **the whole
`:spark-operator:test` suite passes** |
The reason the third one is free is that the only non-403 release test,
`ClusterSuspendStepTest.failedKueueWorkloadReleaseIsRetried`, throws its 503 at
a cluster built by `buildKueueCluster`, so `hasQueueName` rethrows first and
the status code never decides anything.
That matters because this clause is what keeps the trade-off you defend at
[r4089724668](https://github.com/apache/spark-kubernetes-operator/pull/877#discussion_r4089724668)
narrow. Without it, a 500 or a 503 on an unlabeled resource would also read as
"no Workload", the suspend would report success, and a resumed cluster would
start over on top of a stale Workload, which is the case `ClusterSuspendStep`
is built to avoid.
Your new parameterized test carries the status code for one more row. I
applied this and measured both arms: it passes here, and with the clause
replaced by `false` it fails on exactly the new `false, false, 500` case.
```java
@ParameterizedTest
@CsvSource({"true, false, 403", "false, true, 403", "false, false, 403",
"false, false, 500"})
@SuppressWarnings("unchecked")
void forbiddenKueueWorkloadReleaseIsIgnoredOnlyWithoutKueueAccess(
boolean labeled, boolean informerEnabled, int code) {
...
when(workload.delete()).thenThrow(new
KubernetesClientException("failed", code, null));
...
if (labeled || informerEnabled || code != 403) {
```
500 rather than 503 keeps the assertion branch as it is, since
`isTransientError` accepts 503 and `ClusterSuspendStep` would then publish no
event.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterSuspendStep.java:
##########
@@ -180,12 +178,8 @@ private boolean releaseResources(SparkClusterContext
context) {
log.debug("Waiting for the pods of the suspended cluster to be
deleted.");
return false;
}
- // A Workload admitted before its queue label was removed is released as
well, whenever the
- // operator may access Workloads, which the Workload informer requires
too.
- if (KueueWorkloadFactory.hasQueueName(cluster)
- || KUEUE_WORKLOAD_INFORMER_ENABLED.getValue()) {
- KueueWorkloadUtils.deleteWorkloadOf(client, cluster);
- }
+ // A Workload admitted before the queue label was removed is released as
well.
+ KueueWorkloadUtils.deleteWorkloadOf(client, cluster);
Review Comment:
**Finding 2.** Removing the label check here leaves two more of the same in
`AppCleanUpStep`.
`AppCleanUpStep:152` releases the `Workload` of a terminating or restarting
attempt only `if (KueueWorkloadFactory.hasQueueName(application))`, and
`finishKueueWorkloadOfRetainedApp` at `:245` records the Kueue `Finished`
condition under the same check. Both carry the leak this PR removes from the
suspension paths.
Measured on a `SparkApplication` reaching `Succeeded`, with
`KueueWorkloadUtils` mocked static:
| owner | `resourceRetainPolicy` | outcome |
|---|---|---|
| labeled | `Never` | `releaseWorkload` called |
| unlabeled | `Never` | neither called |
| labeled | `Always` | `finishWorkload` called |
| unlabeled | `Always` | neither called |
So an application whose label was removed after its `Workload` was admitted
keeps that quota until its custom resource is deleted, which with the default
`ttlAfterStopMillis` of `-1` may be never. That is the same sentence as your
"Why" section, with "on termination" in place of "on suspension".
Dropping both checks is safe after this PR, which is what makes it worth
doing now rather than reinventing the reasoning later: `deleteWorkloadOf`
already answers a 403 with "no Workload" for an unlabeled owner with the
informer off, `releaseWorkload` swallows everything else, and `finishWorkload`
falls back to `releaseWorkload`. The two admission gates, `ClusterInitStep:224`
and `AppInitStep:187`, must keep the check, since an unlabeled resource has no
admission to request.
Out of this PR's stated scope, so I filed
[SPARK-59758](https://issues.apache.org/jira/browse/SPARK-59758) with the
measurement rather than leaving it to you.
##########
docs/spark_custom_resources.md:
##########
@@ -661,7 +661,14 @@ spec:
* `spec.suspend` takes precedence. A suspended resource does not get a
`Workload`, and suspending
a queued resource deletes its `Workload` to release the quota. Suspending a
running
`SparkCluster` deletes its `Workload` only after its master and worker pods
are gone, since
- terminating pods still occupy the quota. The resource is queued again when
it is resumed.
+ terminating pods still occupy the quota. The `Workload` is deleted even if
the
+ `kueue.x-k8s.io/queue-name` label was removed after the admission. The
resource is queued again
+ when it is resumed with the `kueue.x-k8s.io/queue-name` label.
+* Once the access of the operator to `Workload`s is revoked, e.g. by disabling
+ `operatorRbac.kueue.enabled`, it can no longer delete them, and a `Workload`
left behind keeps
+ its quota until its owner is deleted. Delete them before revoking the
access, e.g. with
Review Comment:
**Finding 3.** This describes the swallowed path only. With the queue label
still present the outcome is worse than a leaked quota.
A labeled owner sends the `403` back out of `deleteWorkloadOf`, so
`releaseResources` returns false and `ClusterSuspendStep` returns before the
resume branch. The cluster cannot leave `Suspended` at all, whatever
`spec.suspend` says.
I ran it: a labeled `Suspended` cluster with `spec.suspend=false` and a
`403` on the `Workload` delete persists no status, stays `Suspended`, and
requeues every 2 minutes indefinitely. Only deleting the `Workload` by hand or
restoring the access gets it moving.
```suggestion
its quota until its owner is deleted. A suspended `SparkCluster` which
still carries the label
does not resume either, until its `Workload` is gone or the access is
restored. Delete them
before revoking the access, e.g. with
```
--
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]
