dongjoon-hyun commented on code in PR #980: URL: https://github.com/apache/spark-kubernetes-operator/pull/980#discussion_r4350510031
########## spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/ClusterCleanUpStep.java: ########## @@ -0,0 +1,121 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.spark.k8s.operator.reconciler.reconcilesteps; + +import static org.apache.spark.k8s.operator.Constants.CLUSTER_RELEASED_AFTER_SCHEDULING_FAILURE_MESSAGE; +import static org.apache.spark.k8s.operator.SparkClusterResourceSpec.getMasterServiceName; +import static org.apache.spark.k8s.operator.SparkClusterResourceSpec.getWorkerNetworkPolicyName; +import static org.apache.spark.k8s.operator.SparkClusterResourceSpec.getWorkerServiceName; +import static org.apache.spark.k8s.operator.reconciler.ReconcileProgress.completeAndDefaultRequeue; +import static org.apache.spark.k8s.operator.reconciler.ReconcileProgress.proceed; + +import java.util.List; +import java.util.Optional; + +import io.fabric8.kubernetes.api.model.Pod; +import io.fabric8.kubernetes.client.KubernetesClient; +import io.fabric8.kubernetes.client.KubernetesClientException; +import lombok.extern.slf4j.Slf4j; + +import org.apache.spark.k8s.operator.SparkCluster; +import org.apache.spark.k8s.operator.context.SparkClusterContext; +import org.apache.spark.k8s.operator.kueue.KueueWorkloadUtils; +import org.apache.spark.k8s.operator.reconciler.ReconcileProgress; +import org.apache.spark.k8s.operator.status.ClusterState; +import org.apache.spark.k8s.operator.status.ClusterStateSummary; +import org.apache.spark.k8s.operator.utils.EventUtils; +import org.apache.spark.k8s.operator.utils.ReconcilerUtils; +import org.apache.spark.k8s.operator.utils.SparkClusterStatusRecorder; + +/** + * Releases the resources of a cluster which failed with SchedulingFailure, and then terminates it + * with ResourceReleased. Nothing requests them again, so the master and workers which were + * requested before the failure would otherwise keep running, and its Kueue Workload would keep its + * quota, until the cluster is deleted. + */ +@Slf4j +public final class ClusterCleanUpStep extends ClusterReconcileStep { + /** + * Releases the resources of a cluster in SchedulingFailure, which {@link ClusterInitStep} records + * with the cause before anything is released, so that the release, which lasts until the pods + * are gone, has a state to continue in. Like {@link ClusterSuspendStep}, everything is deleted by + * name, since the spec may not build, which may be the failure itself, and the resources may be + * left from before the cluster was suspended. Unlike a suspended cluster, the Services and the + * NetworkPolicy are deleted as well, since nothing applies them again. + * + * <p>The Workload is released only after the master and worker pods are gone, or are still + * terminating past their timeout, see {@link ClusterSuspendStep#waitForPods}. A pod stuck in + * terminating does not hold the cluster beyond that, since no pod of its name is created again. + * Everything here is idempotent, so a release that failed is simply retried. A failure which is + * not expected to clear on its own is reported, since SchedulingFailure reports the failure to + * request the resources, not the one to release them. + * + * @param context The SparkClusterContext for the cluster. + * @param statusRecorder The SparkClusterStatusRecorder for recording status updates. + * @return The ReconcileProgress indicating the next step. + */ + @Override + public ReconcileProgress reconcile( + SparkClusterContext context, SparkClusterStatusRecorder statusRecorder) { + SparkCluster cluster = context.getResource(); + if (cluster.getStatus().getCurrentState().getCurrentStateSummary() + != ClusterStateSummary.SchedulingFailure) { + return proceed(); + } + String namespace = cluster.getMetadata().getNamespace(); + String name = cluster.getMetadata().getName(); + KubernetesClient client = context.getClient(); + try { + List<Pod> pods = ClusterSuspendStep.deleteMasterAndWorkers(client, cluster); + client.services().inNamespace(namespace).withName(getMasterServiceName(name)).delete(); + client.services().inNamespace(namespace).withName(getWorkerServiceName(name)).delete(); + client + .network() + .networkPolicies() + .inNamespace(namespace) + .withName(getWorkerNetworkPolicyName(name)) + .delete(); + Optional<ReconcileProgress> waiting = ClusterSuspendStep.waitForPods(pods); + if (waiting.isPresent()) { + log.debug("Waiting for the pods of the failed cluster to be deleted."); + return waiting.get(); + } + // A Workload admitted before the queue label was removed is released as well. + KueueWorkloadUtils.deleteWorkloadOf(client, cluster); Review Comment: Thank you, @peter-toth. You're right. Addressed in 1d17d70. Like `AppCleanUpStep`, `ClusterCleanUpStep` now deletes the Services and the NetworkPolicy only after the `Workload` is released, so a failure to delete them no longer holds the quota. I added `failedReleaseOfResourceWithoutQuotaDoesNotHoldKueueWorkload` to `ClusterCleanUpStepTest`, which fails without this change, and a sentence to `spark_custom_resources.md`. I'll handle the HPA and PDB deletes in `deleteMasterAndWorkers`, which have the same shape, in a follow-up PR, since the helper is shared with the suspend path. -- 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]
