peter-toth commented on code in PR #980: URL: https://github.com/apache/spark-kubernetes-operator/pull/980#discussion_r4349288708
########## 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: **Finding 1.** The two Service deletes and the NetworkPolicy delete run before `waitForPods` and the `Workload` release. A persistent failure on any of them lands in the `catch`, so the `Workload` is never released. None of these resources holds any quota. One way to get there is an operator which may not access NetworkPolicies. The apply in `ClusterInitStep` then fails with 403, and the cluster moves to `SchedulingFailure`. The delete here gets the same 403, since RBAC is checked before the object is looked up. The Helm chart grants the access, so this needs a custom role, or an admission policy which denies `DELETE`. I checked it with a throwaway test on the mock server. It used a `SchedulingFailure` cluster with a `Workload`, no pods, and `network()` throwing 403. After 5 reconciles the `Workload` was still there, and the cluster stayed in `SchedulingFailure`. `AppCleanUpStep.releaseResources` avoids this on purpose ([L239-L243](https://github.com/apache/spark-kubernetes-operator/blob/9322a03fa370fb48a0f1ebe9499eb7f49ec655af/spark-operator/src/main/java/org/apache/spark/k8s/operator/reconciler/reconcilesteps/AppCleanUpStep.java#L239-L243)). It reports a failed delete only after the `Workload` is released, so that such a resource "does not hold the quota once no pod uses it". Moving these deletes after the release fixes it. With the change below, the same probe releases the `Workload` on the first reconcile. The cluster still stays in `SchedulingFailure`, and retries the NetworkPolicy delete with the `ClusterReleaseFailed` event. `ClusterCleanUpStepTest`, `ClusterInitStepTest` and `ClusterSuspendStepTest` still pass with it. ```suggestion 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); 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(); ``` The HPA and PDB deletes in `deleteMasterAndWorkers` have the same shape. That helper is shared with the suspend path, so I left them out. -- 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]
