dongjoon-hyun commented on code in PR #877:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/877#discussion_r4091229590
##########
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:
Thank you for the mutation check, @peter-toth. I added the `false, false,
500` row, so that only a `403` of an unlabeled owner without the informer is
taken as no `Workload`.
##########
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:
Thank you for filing SPARK-59758. I'll handle both release paths of
`AppCleanUpStep` there, keeping the two admission gates.
##########
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:
Thanks. Since the bullet was rewritten for r4090106589, I merged your
sentence into the new text: a suspended `SparkCluster` which still carries the
label does not resume until its `Workload` is gone or the access is restored.
--
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]
