From 0b9bef2c086a54b0731fa7aa0a6bd7cb856b48ee Mon Sep 17 00:00:00 2001
From: Tingluo Huang <tingluohuang@github.com>
Date: Sat, 19 Feb 2022 07:22:31 -0500
Subject: [PATCH] Try to unconfig runner before deleting the pod to recreate
 (#1125)

There is a race condition between ARC and GitHub service about deleting runner pod.

- The ARC use REST API to find a particular runner in a pod that is not running any jobs, so it decides to delete the pod.
- A job is queued on the GitHub service side, and it sends the job to this idle runner right before ARC deletes the pod.
- The ARC delete the runner pod which cause the in-progress job to end up canceled.

To avoid this race condition, I am calling `r.unregisterRunner()` before deleting the pod.
- `r.unregisterRunner()` will return 204 to indicate the runner is deleted from the GitHub service, we should be safe to delete the pod.
- `r.unregisterRunner` will return 400 to indicate the runner is still running a job, so we will leave this runner pod as it is.

TODO: I need to do some E2E tests to force the race condition to happen.

Ref #911
---
 controllers/runner_controller.go | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/controllers/runner_controller.go b/controllers/runner_controller.go
index 17976231..1bfa8bcc 100644
--- a/controllers/runner_controller.go
+++ b/controllers/runner_controller.go
@@ -404,14 +404,31 @@ func (r *RunnerReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		return ctrl.Result{}, nil
 	}
 
-	// Delete current pod if recreation is needed
-	if err := r.Delete(ctx, &pod); err != nil {
-		log.Error(err, "Failed to delete pod resource")
-		return ctrl.Result{}, err
+	// Try to delete current pod if recreation is needed
+	safeToDeletePod := false
+	ok, err := r.unregisterRunner(ctx, runner.Spec.Enterprise, runner.Spec.Organization, runner.Spec.Repository, runner.Name)
+	if err != nil {
+		log.Error(err, "Failed to unregister runner before deleting the pod.", "runner", runner.Name)
+	} else {
+		// `r.unregisterRunner()` will returns `false, nil` if the runner is not found on GitHub.
+		if !ok {
+			log.Info("Runner no longer exists on GitHub", "runner", runner.Name)
+		}
+
+		safeToDeletePod = true
 	}
 
-	r.Recorder.Event(&runner, corev1.EventTypeNormal, "PodDeleted", fmt.Sprintf("Deleted pod '%s'", newPod.Name))
-	log.Info("Deleted runner pod", "repository", runner.Spec.Repository)
+	if safeToDeletePod {
+		// Only delete the pod if we successfully unregistered the runner or the runner is already deleted from the service.
+		// This should help us avoid race condition between runner pickup job after we think the runner is not busy.
+		if err := r.Delete(ctx, &pod); err != nil {
+			log.Error(err, "Failed to delete pod resource")
+			return ctrl.Result{}, err
+		}
+
+		r.Recorder.Event(&runner, corev1.EventTypeNormal, "PodDeleted", fmt.Sprintf("Deleted pod '%s'", newPod.Name))
+		log.Info("Deleted runner pod", "repository", runner.Spec.Repository)
+	}
 
 	return ctrl.Result{}, nil
 }