dongjoon-hyun commented on code in PR #885:
URL:
https://github.com/apache/spark-kubernetes-operator/pull/885#discussion_r4100732742
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the
application, once its
+ * driver and executor pods are gone. Kueue would admit another workload
into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by
the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue
label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed
per application and
+ * the next attempt would otherwise run on this admission. A restarted
attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released,
unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be
deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod
uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no
pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
+ KubernetesClientException deleteFailure = null;
+ try {
Review Comment:
Thank you. You're right. Updated to catch per resource and keep the first
failure with the others suppressed, so the driver pod is always asked to go.
`everyResourceIsDeletedEvenIfAnotherFails` covers it.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the
application, once its
+ * driver and executor pods are gone. Kueue would admit another workload
into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by
the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue
label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed
per application and
+ * the next attempt would otherwise run on this admission. A restarted
attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released,
unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be
deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod
uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no
pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
+ KubernetesClientException deleteFailure = null;
+ try {
+ for (HasMetadata resource : resources) {
+ ReconcilerUtils.deleteResourceIfExists(context.getClient(), resource,
forceDelete);
+ }
+ } catch (KubernetesClientException e) {
+ deleteFailure = e;
+ }
+ if (isWaitingForPods(context, application)) {
+ if (deleteFailure != null) {
+ log.warn("Failed to delete the resources of the application, will
retry.", deleteFailure);
+ }
+ return false;
+ }
+ KueueWorkloadUtils.releaseWorkload(context.getClient(), application);
Review Comment:
Good catch. Updated to call `deleteWorkloadOf` and keep the application in
its state on a failure, so the next pass retries it before a restart is
scheduled or the application terminates. Like a suspended `SparkCluster`, a
persistent failure, e.g. a revoked access, holds the application until the
access is restored, which the documentation now mentions.
`failedWorkloadReleaseIsRetriedBeforeRestart` covers it.
##########
spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java:
##########
@@ -217,6 +225,117 @@ public ReconcileProgress reconcile(
}
}
+ /**
+ * Deletes the given resources and then releases the Kueue Workload of the
application, once its
+ * driver and executor pods are gone. Kueue would admit another workload
into the quota which the
+ * terminating pods still occupy. The deletion of each pod is observed by
the pod informer, which
+ * reconciles again.
+ *
+ * <p>The Workload is released even if it was admitted before the queue
label was removed. It is
+ * released before a restart is scheduled, since the Workload name is fixed
per application and
+ * the next attempt would otherwise run on this admission. A restarted
attempt is queued again
+ * with a new Workload only if the label is present.
+ *
+ * <p>A failed deletion is reported only after the Workload is released,
unless pods remain, in
+ * which case it is retried with them. So a resource which cannot be
deleted, e.g. a ConfigMap
+ * the operator may no longer delete, does not hold the quota once no pod
uses it.
+ *
+ * @param context The SparkAppContext for the application.
+ * @param application The SparkApplication.
+ * @param resources The resources to delete.
+ * @return True if the Workload was released, false while the pods remain.
+ * @throws KubernetesClientException if a resource cannot be deleted and no
pod remains.
+ */
+ private boolean releaseResources(
+ final SparkAppContext context,
+ final SparkApplication application,
+ final List<HasMetadata> resources) {
+ boolean forceDelete = enableForceDelete(application);
Review Comment:
Yes, the kubelet stops a force-deleted container within seconds, so I think
the gap is acceptable. Changing when the driver is force deleted is out of the
scope of this PR. I added a sentence to the javadoc of `getWaitForPodsDeadline`
as you suggested.
--
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]
