Remove time.Sleep from run controller delete (#141)

Author: stuartleeks

controllers/run_controller.go

@@ -59,12 +59,18 @@ func (r *RunReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	}
 
 	if instance.IsBeingDeleted() {
-		if err := r.handleFinalizer(instance); err != nil {
+		completed, err := r.handleFinalizer(instance)
+		if err != nil {
 			r.Recorder.Event(instance, corev1.EventTypeWarning, "deleting finalizer", fmt.Sprintf("Failed to delete finalizer: %s", err))
 			return ctrl.Result{}, fmt.Errorf("error when handling finalizer: %v", err)
 		}
-		r.Recorder.Event(instance, corev1.EventTypeNormal, "Deleted", "Object finalizer is deleted")
-		return ctrl.Result{}, nil
+		if completed {
+			r.Recorder.Event(instance, corev1.EventTypeNormal, "Deleted", "Object finalizer is deleted")
+			return ctrl.Result{}, nil
+		}
+		// no error but not completed removing the finalizer - requeue
+		r.Recorder.Event(instance, corev1.EventTypeNormal, "Deleting", "Pending deletion")
+		return ctrl.Result{RequeueAfter: 5 * time.Second}, nil
 	}
 
 	if !instance.HasFinalizer(databricksv1alpha1.RunFinalizerName) {

controllers/run_controller_databricks.go

@@ -21,7 +21,6 @@ import (
 	"fmt"
 	"reflect"
 	"strings"
-	"time"
 
 	databricksv1alpha1 "github.com/microsoft/azure-databricks-operator/api/v1alpha1"
 	"github.com/xinsnake/databricks-sdk-golang/azure"
@@ -106,36 +105,42 @@ func (r *RunReconciler) refresh(instance *databricksv1alpha1.Run) error {
 	return r.Update(context.Background(), instance)
 }
 
-func (r *RunReconciler) delete(instance *databricksv1alpha1.Run) error {
+// delete attempts to cancel and delete a run. Returns bool indicating if complete (safe to retry if not and no error) and an error
+func (r *RunReconciler) delete(instance *databricksv1alpha1.Run) (bool, error) {
 	r.Log.Info(fmt.Sprintf("Deleting run %s", instance.GetName()))
 
 	if instance.Status == nil {
-		return nil
+		return true, nil
 	}
 
 	runID := instance.Status.Metadata.RunID
 
 	// Check if the run exists before trying to delete it
-	if _, err := r.getRun(runID); err != nil {
+	run, err := r.getRun(runID)
+	if err != nil {
 		if strings.Contains(err.Error(), "does not exist") {
-			return nil
+			return true, nil
 		}
-		return err
+		return false, err
 	}
 
-	// We will not check for error when cancelling a job,
-	// if it fails just let it be
-	execution := NewExecution("runs", "cancel")
-	err := r.APIClient.Jobs().RunsCancel(runID)
-	execution.Finish(err)
-
-	// It takes time for DataBricks to cancel a run
-	time.Sleep(15 * time.Second)
-
-	execution = NewExecution("runs", "delete")
+	if run.State.ResultState == nil {
+		// We will not check for error when cancelling a job,
+		// if it fails just let it be
+		execution := NewExecution("runs", "cancel")
+		err := r.APIClient.Jobs().RunsCancel(runID)
+		execution.Finish(err)
+		return false, nil // no error, but indicate not completed to trigger a requeue to delete once cancelled
+	}
+	// job has reached a terminated state
+	execution := NewExecution("runs", "delete")
 	err = r.APIClient.Jobs().RunsDelete(runID)
 	execution.Finish(err)
-	return err
+
+	if err != nil {
+		return false, err
+	}
+	return true, nil
 }
 
 func (r *RunReconciler) runUsingRunNow(instance *databricksv1alpha1.Run) (*dbmodels.Run, bool, error) {

controllers/run_controller_finalizer.go

@@ -27,14 +27,20 @@ func (r *RunReconciler) addFinalizer(instance *databricksv1alpha1.Run) error {
 	return r.Update(context.Background(), instance)
 }
 
-func (r *RunReconciler) handleFinalizer(instance *databricksv1alpha1.Run) error {
+// handleFinalizer returns a bool and an error. If error is set then the attempt failed, otherwise boolean indicates whether it completed
+func (r *RunReconciler) handleFinalizer(instance *databricksv1alpha1.Run) (bool, error) {
 	if !instance.HasFinalizer(databricksv1alpha1.RunFinalizerName) {
-		return nil
+		return true, nil
 	}
 
-	if err := r.delete(instance); err != nil {
-		return err
+	completed, err := r.delete(instance)
+	if err != nil {
+		return false, err
 	}
-	instance.RemoveFinalizer(databricksv1alpha1.RunFinalizerName)
-	return r.Update(context.Background(), instance)
+	if completed {
+		instance.RemoveFinalizer(databricksv1alpha1.RunFinalizerName)
+		err := r.Update(context.Background(), instance)
+		return err != nil, err
+	}
+	return false, nil // no error, but indicate not completed to trigger a requeue to delete once cancelled
 }