dongjoon-hyun commented on code in PR #877:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/877#discussion_r4089724668
##########
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:
Thank you for the review, @viirya. The scenario is valid, but I'd like to
keep the release non-blocking. A 403 cannot tell an operator whose Kueue access
was revoked from one that never had it, and a GET is denied as well, so the
absence cannot be established. Retrying cannot succeed without a manual fix,
and blocking would keep every unlabeled suspended resource from resuming
whenever Kueue is installed but the operator is not granted access to it, which
is the Helm default (`operatorRbac.kueue.enabled=false`). This case also
behaves the same as before this PR, which skipped the deletion without the
label and the informer. Instead, I fixed the overstated comment, documented
that leftover `Workload`s should be deleted before revoking the operator's
Kueue access, and extended the test to 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:
Thanks. I qualified it as "queued again when it is resumed with the
`kueue.x-k8s.io/queue-name` label."
--
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]
