viirya commented on code in PR #877:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/877#discussion_r4089639367
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/kueue/KueueWorkloadUtils.java:
##########
@@ -517,20 +518,35 @@ 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. Without the label, an
operator which does
+ * not watch Workloads may not be granted access to them, and then it has
none to delete, so the
+ * rejection is taken as no Workload. One without Kueue installed is
answered with a 404, which
+ * the deletion ignores.
*
* @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.
* @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)
Review Comment:
[P1] Please do not treat this 403 as proof that no Workload exists. A
Workload may have been created while Kueue access was enabled, then the queue
label and RBAC may have been removed before suspension. In that case, DELETE
returns 403, but this branch lets ClusterSuspendStep report release as complete
while the admitted Workload still holds quota. The cluster can then resume
without the label and skip a new admission. Could we preserve a visible failure
and retry unless absence of the Workload can actually be established? Please
also test an existing Workload whose deletion is denied.
##########
docs/spark_custom_resources.md:
##########
@@ -661,7 +661,9 @@ 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
Review Comment:
[P2] Please qualify ‘queued again when it is resumed.’ This paragraph now
covers a resource whose queue label was removed. If the label is still absent
on resume, holdForKueueAdmission skips Kueue and the resource is not queued.
This sentence should say that requeueing requires the label to remain or be
added back.
--
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]
