chore: bump chart (#1138)

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

charts/actions-runner-controller/Chart.yaml

@@ -15,10 +15,10 @@ type: application
 # This is the chart version. This version number should be incremented each time you make changes
 # to the chart and its templates, including the app version.
 # Versions are expected to follow Semantic Versioning (https://semver.org/)
-version: 0.16.0
+version: 0.16.1
 
 # Used as the default manager tag value when no tag property is provided in the values.yaml
-appVersion: 0.21.0
+appVersion: 0.21.1
 
 home: https://github.com/actions-runner-controller/actions-runner-controller
 

cmd/githubwebhookserver/main.go

@@ -144,6 +144,8 @@ func main() {
 		}
 	})
 
+	ctrl.SetLogger(logger)
+
 	// In order to support runner groups with custom visibility (selected repositories), we need to perform some GitHub API calls.
 	// Let the user define if they want to opt-in supporting this option by providing the proper GitHub authentication parameters
 	// Without an opt-in, runner groups with custom visibility won't be supported to save API calls
@@ -160,8 +162,6 @@ func main() {
 		setupLog.Info("GitHub client is not initialized. Runner groups with custom visibility are not supported. If needed, please provide GitHub authentication. This will incur in extra GitHub API calls")
 	}
 
-	ctrl.SetLogger(logger)
-
 	mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
 		Scheme:             scheme,
 		SyncPeriod:         &syncPeriod,

controllers/horizontal_runner_autoscaler_webhook.go

@@ -614,11 +614,7 @@ func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) getManagedRunnerGroup
 			return nil, fmt.Errorf("unsupported scale target kind: %v", kind)
 		}
 
-		if g == "" {
-			continue
-		}
-
-		if e == "" && o == "" {
+		if g != "" && e == "" && o == "" {
 			autoscaler.Log.V(1).Info(
 				"invalid runner group config in scale target: spec.group must be set along with either spec.enterprise or spec.organization",
 				"scaleTargetKind", kind,
@@ -631,6 +627,16 @@ func (autoscaler *HorizontalRunnerAutoscalerGitHubWebhook) getManagedRunnerGroup
 		}
 
 		if e != enterprise && o != org {
+			autoscaler.Log.V(1).Info(
+				"Skipped scale target irrelevant to event",
+				"eventOrganization", org,
+				"eventEnterprise", enterprise,
+				"scaleTargetKind", kind,
+				"scaleTargetGroup", g,
+				"scaleTargetEnterprise", e,
+				"scaleTargetOrganization", o,
+			)
+
 			continue
 		}
 

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
 }
@@ -531,32 +548,15 @@ func (r *RunnerReconciler) processRunnerCreation(ctx context.Context, runner v1a
 	return ctrl.Result{}, nil
 }
 
+// unregisterRunner unregisters the runner from GitHub Actions by name.
+//
+// This function returns:
+// - (true, nil) when it has successfully unregistered the runner.
+// - (false, nil) when the runner has been already unregistered.
+// - (false, err) when it postponed unregistration due to the runner being busy, or it tried to unregister the runner but failed due to
+//   an error returned by GitHub API.
 func (r *RunnerReconciler) unregisterRunner(ctx context.Context, enterprise, org, repo, name string) (bool, error) {
-	runners, err := r.GitHubClient.ListRunners(ctx, enterprise, org, repo)
-	if err != nil {
-		return false, err
-	}
-
-	id := int64(0)
-	for _, runner := range runners {
-		if runner.GetName() == name {
-			if runner.GetBusy() {
-				return false, fmt.Errorf("runner is busy")
-			}
-			id = runner.GetID()
-			break
-		}
-	}
-
-	if id == int64(0) {
-		return false, nil
-	}
-
-	if err := r.GitHubClient.RemoveRunner(ctx, enterprise, org, repo, id); err != nil {
-		return false, err
-	}
-
-	return true, nil
+	return unregisterRunner(ctx, r.GitHubClient, enterprise, org, repo, name)
 }
 
 func (r *RunnerReconciler) updateRegistrationToken(ctx context.Context, runner v1alpha1.Runner) (bool, error) {
@@ -626,6 +626,11 @@ func (r *RunnerReconciler) newPod(runner v1alpha1.Runner) (corev1.Pod, error) {
 		runner.ObjectMeta.Annotations,
 		runner.Spec,
 		r.GitHubClient.GithubBaseURL,
+		// Token change should trigger replacement.
+		// We need to include this explicitly here because
+		// runner.Spec does not contain the possibly updated token stored in the
+		// runner status yet.
+		runner.Status.Registration.Token,
 	)
 
 	objectMeta := metav1.ObjectMeta{

controllers/runner_pod_controller.go

@@ -378,42 +378,7 @@ func (r *RunnerPodReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 }
 
 func (r *RunnerPodReconciler) unregisterRunner(ctx context.Context, enterprise, org, repo, name string) (bool, error) {
-	runners, err := r.GitHubClient.ListRunners(ctx, enterprise, org, repo)
-	if err != nil {
-		return false, err
-	}
-
-	var busy bool
-
-	id := int64(0)
-	for _, runner := range runners {
-		if runner.GetName() == name {
-			// Sometimes a runner can stuck "busy" even though it is already "offline".
-			// Thus removing the condition on status can block the runner pod from being terminated forever.
-			busy = runner.GetBusy()
-			if runner.GetStatus() != "offline" && busy {
-				r.Log.Info("This runner will delay the runner pod deletion and the runner deregistration until it becomes either offline or non-busy", "name", runner.GetName(), "status", runner.GetStatus(), "busy", runner.GetBusy())
-				return false, fmt.Errorf("runner is busy")
-			}
-			id = runner.GetID()
-			break
-		}
-	}
-
-	if id == int64(0) {
-		return false, nil
-	}
-
-	// Sometimes a runner can stuck "busy" even though it is already "offline".
-	// Trying to remove the offline but busy runner can result in errors like the following:
-	//    failed to remove runner: DELETE https://api.github.com/repos/actions-runner-controller/mumoshu-actions-test/actions/runners/47: 422 Bad request - Runner \"example-runnerset-0\" is still running a job\" []
-	if !busy {
-		if err := r.GitHubClient.RemoveRunner(ctx, enterprise, org, repo, id); err != nil {
-			return false, err
-		}
-	}
-
-	return true, nil
+	return unregisterRunner(ctx, r.GitHubClient, enterprise, org, repo, name)
 }
 
 func (r *RunnerPodReconciler) SetupWithManager(mgr ctrl.Manager) error {

controllers/unregister.go

@@ -0,0 +1,49 @@
+package controllers
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/actions-runner-controller/actions-runner-controller/github"
+)
+
+// unregisterRunner unregisters the runner from GitHub Actions by name.
+//
+// This function returns:
+// - (true, nil) when it has successfully unregistered the runner.
+// - (false, nil) when the runner has been already unregistered.
+// - (false, err) when it postponed unregistration due to the runner being busy, or it tried to unregister the runner but failed due to
+//   an error returned by GitHub API.
+func unregisterRunner(ctx context.Context, client *github.Client, enterprise, org, repo, name string) (bool, error) {
+	runners, err := client.ListRunners(ctx, enterprise, org, repo)
+	if err != nil {
+		return false, err
+	}
+
+	id := int64(0)
+	for _, runner := range runners {
+		if runner.GetName() == name {
+			// Note that sometimes a runner can stuck "busy" even though it is already "offline".
+			// But we assume that it's not actually offline and still running a job.
+			if runner.GetBusy() {
+				return false, fmt.Errorf("runner is busy")
+			}
+			id = runner.GetID()
+			break
+		}
+	}
+
+	if id == int64(0) {
+		return false, nil
+	}
+
+	// Trying to remove a busy runner can result in errors like the following:
+	//    failed to remove runner: DELETE https://api.github.com/repos/actions-runner-controller/mumoshu-actions-test/actions/runners/47: 422 Bad request - Runner \"example-runnerset-0\" is still running a job\" []
+	//
+	// TODO: Probably we can just remove the runner by ID without seeing if the runner is busy, by treating it as busy when a remove-runner call failed with 422?
+	if err := client.RemoveRunner(ctx, enterprise, org, repo, id); err != nil {
+		return false, err
+	}
+
+	return true, nil
+}

simulator/runnergroups.go

@@ -3,6 +3,7 @@ package simulator
 import (
 	"fmt"
 	"sort"
+	"strings"
 
 	"github.com/google/go-github/v39/github"
 )
@@ -96,6 +97,10 @@ type RunnerGroup struct {
 	Name  string
 }
 
+func (r RunnerGroup) String() string {
+	return fmt.Sprintf("RunnerGroup{Scope:%s, Kind:%s, Name:%s}", r.Scope, r.Kind, r.Name)
+}
+
 // VisibleRunnerGroups is a set of enterprise and organization runner groups
 // that are visible to a GitHub repository.
 // GitHub Actions chooses one of such visible group on which the workflow job is scheduled.
@@ -111,6 +116,15 @@ func NewVisibleRunnerGroups() *VisibleRunnerGroups {
 	return &VisibleRunnerGroups{}
 }
 
+func (g *VisibleRunnerGroups) String() string {
+	var gs []string
+	for _, g := range g.sortedGroups {
+		gs = append(gs, g.String())
+	}
+
+	return strings.Join(gs, ", ")
+}
+
 func (g *VisibleRunnerGroups) IsEmpty() bool {
 	return len(g.sortedGroups) == 0
 }