diff --git a/charts/gha-runner-scale-set-controller-experimental/templates/manager_listener_role.yaml b/charts/gha-runner-scale-set-controller-experimental/templates/manager_listener_role.yaml index 38108298..40febda5 100644 --- a/charts/gha-runner-scale-set-controller-experimental/templates/manager_listener_role.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/templates/manager_listener_role.yaml @@ -12,6 +12,7 @@ rules: - create - delete - get + - patch - apiGroups: - "" resources: diff --git a/charts/gha-runner-scale-set-controller-experimental/tests/controller_rbac_listener_role_test.yaml b/charts/gha-runner-scale-set-controller-experimental/tests/controller_rbac_listener_role_test.yaml index b54c834e..4170fd2a 100644 --- a/charts/gha-runner-scale-set-controller-experimental/tests/controller_rbac_listener_role_test.yaml +++ b/charts/gha-runner-scale-set-controller-experimental/tests/controller_rbac_listener_role_test.yaml @@ -24,6 +24,14 @@ tests: path: rules[0].resources[0] value: "pods" template: manager_listener_role.yaml + - equal: + path: rules[0].verbs + value: + - create + - delete + - get + - patch + template: manager_listener_role.yaml - equal: path: rules[1].resources[0] value: "pods/status" diff --git a/charts/gha-runner-scale-set-controller/templates/manager_listener_role.yaml b/charts/gha-runner-scale-set-controller/templates/manager_listener_role.yaml index a238d5fc..8fc7207e 100644 --- a/charts/gha-runner-scale-set-controller/templates/manager_listener_role.yaml +++ b/charts/gha-runner-scale-set-controller/templates/manager_listener_role.yaml @@ -12,6 +12,7 @@ rules: - create - delete - get + - patch - apiGroups: - "" resources: diff --git a/charts/gha-runner-scale-set-controller/tests/template_test.go b/charts/gha-runner-scale-set-controller/tests/template_test.go index a2a34aa0..1022eae3 100644 --- a/charts/gha-runner-scale-set-controller/tests/template_test.go +++ b/charts/gha-runner-scale-set-controller/tests/template_test.go @@ -248,6 +248,7 @@ func TestTemplate_CreateManagerListenerRole(t *testing.T) { assert.Equal(t, "test-arc-gha-rs-controller-listener", managerListenerRole.Name) assert.Equal(t, 4, len(managerListenerRole.Rules)) assert.Equal(t, "pods", managerListenerRole.Rules[0].Resources[0]) + assert.ElementsMatch(t, []string{"create", "delete", "get", "patch"}, managerListenerRole.Rules[0].Verbs) assert.Equal(t, "pods/status", managerListenerRole.Rules[1].Resources[0]) assert.Equal(t, "secrets", managerListenerRole.Rules[2].Resources[0]) assert.Equal(t, "serviceaccounts", managerListenerRole.Rules[3].Resources[0]) diff --git a/controllers/actions.github.com/autoscalinglistener_controller.go b/controllers/actions.github.com/autoscalinglistener_controller.go index 4764c014..c9d2d83e 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller.go +++ b/controllers/actions.github.com/autoscalinglistener_controller.go @@ -524,6 +524,15 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. return ctrl.Result{}, nil } + if listenerPodIsDead(&listenerPod) { + logDeadListenerPod(&listenerPod, log) + return ctrl.Result{}, r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log) + } + + if !listenerPod.DeletionTimestamp.IsZero() { + return ctrl.Result{}, nil + } + desiredLabels := r.filterAndMergeLabels(listenerPod.Labels, desiredPod.Labels) labelsModified := !maps.Equal(listenerPod.Labels, desiredLabels) desiredAnnotations := r.mergeAnnotations(listenerPod.Annotations, desiredPod.Annotations) @@ -577,30 +586,9 @@ func (r *AutoscalingListenerReconciler) Reconcile(ctx context.Context, req ctrl. cs := listenerContainerStatus(&listenerPod) switch { - case listenerPod.Status.Reason == "Evicted": - log.Info( - "Listener pod is evicted", - "phase", listenerPod.Status.Phase, - "reason", listenerPod.Status.Reason, - "message", listenerPod.Status.Message, - ) - - return ctrl.Result{}, r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log) - case cs == nil: log.Info("Listener pod is not ready", "namespace", listenerPod.Namespace, "name", listenerPod.Name) return ctrl.Result{}, nil - case cs.State.Terminated != nil: - log.Info( - "Listener pod is terminated", - "namespace", listenerPod.Namespace, - "name", listenerPod.Name, - "reason", cs.State.Terminated.Reason, - "message", cs.State.Terminated.Message, - ) - - return ctrl.Result{}, r.deleteListenerPod(ctx, &autoscalingListener, &listenerPod, log) - case cs.State.Running != nil: if err := r.publishRunningListener(&autoscalingListener, true); err != nil { log.Error(err, "Unable to publish running listener", "namespace", listenerPod.Namespace, "name", listenerPod.Name) @@ -986,3 +974,33 @@ func listenerContainerStatus(pod *corev1.Pod) *corev1.ContainerStatus { } return nil } + +func listenerPodIsDead(pod *corev1.Pod) bool { + if pod.Status.Reason == "Evicted" { + return true + } + + cs := listenerContainerStatus(pod) + return cs != nil && cs.State.Terminated != nil +} + +func logDeadListenerPod(pod *corev1.Pod, log logr.Logger) { + if pod.Status.Reason == "Evicted" { + log.Info( + "Listener pod is evicted", + "phase", pod.Status.Phase, + "reason", pod.Status.Reason, + "message", pod.Status.Message, + ) + return + } + + cs := listenerContainerStatus(pod) + log.Info( + "Listener pod is terminated", + "namespace", pod.Namespace, + "name", pod.Name, + "reason", cs.State.Terminated.Reason, + "message", cs.State.Terminated.Message, + ) +} diff --git a/controllers/actions.github.com/autoscalinglistener_controller_test.go b/controllers/actions.github.com/autoscalinglistener_controller_test.go index 14e32555..97948914 100644 --- a/controllers/actions.github.com/autoscalinglistener_controller_test.go +++ b/controllers/actions.github.com/autoscalinglistener_controller_test.go @@ -6,12 +6,14 @@ import ( "fmt" "os" "path/filepath" + "sync/atomic" "time" corev1 "k8s.io/api/core/v1" rbacv1 "k8s.io/api/rbac/v1" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" logf "sigs.k8s.io/controller-runtime/pkg/log" ghalistenerconfig "github.com/actions/actions-runner-controller/cmd/ghalistener/config" @@ -21,6 +23,7 @@ import ( . "github.com/onsi/gomega" kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "github.com/actions/actions-runner-controller/apis/actions.github.com/v1alpha1" @@ -1039,6 +1042,203 @@ var _ = Describe("Test AutoScalingListener customization", func() { }) }) +var _ = Describe("AutoscalingListener dead pod recovery", func() { + var ( + ctx context.Context + mgr ctrl.Manager + autoscalingNS *corev1.Namespace + autoscalingRunnerSet *v1alpha1.AutoscalingRunnerSet + autoscalingListener *v1alpha1.AutoscalingListener + configSecret *corev1.Secret + reconcileRequest ctrl.Request + newListenerReconciler func(client.Client) *AutoscalingListenerReconciler + ) + + BeforeEach(func() { + ctx = context.Background() + autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient) + configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name) + + min := 1 + max := 10 + autoscalingRunnerSet = &v1alpha1.AutoscalingRunnerSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-asrs", + Namespace: autoscalingNS.Name, + }, + Spec: v1alpha1.AutoscalingRunnerSetSpec{ + GitHubConfigUrl: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + MaxRunners: &max, + MinRunners: &min, + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "runner", + Image: "ghcr.io/actions/runner", + }, + }, + }, + }, + }, + } + Expect(k8sClient.Create(ctx, autoscalingRunnerSet)).To(Succeed()) + + autoscalingListener = &v1alpha1.AutoscalingListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-asl", + Namespace: autoscalingNS.Name, + Labels: map[string]string{ + "arc.test/listener-label": "desired", + }, + }, + Spec: v1alpha1.AutoscalingListenerSpec{ + GitHubConfigURL: "https://github.com/owner/repo", + GitHubConfigSecret: configSecret.Name, + RunnerScaleSetID: 1, + AutoscalingRunnerSetNamespace: autoscalingRunnerSet.Namespace, + AutoscalingRunnerSetName: autoscalingRunnerSet.Name, + EphemeralRunnerSetName: "test-ers", + MaxRunners: 10, + MinRunners: 1, + Image: "ghcr.io/owner/repo", + }, + } + Expect(k8sClient.Create(ctx, autoscalingListener)).To(Succeed()) + + reconcileRequest = ctrl.Request{NamespacedName: client.ObjectKeyFromObject(autoscalingListener)} + newListenerReconciler = func(c client.Client) *AutoscalingListenerReconciler { + return &AutoscalingListenerReconciler{ + Client: c, + Scheme: mgr.GetScheme(), + Log: logf.Log, + ResourceBuilder: ResourceBuilder{ + ResourceCache: newTestResourceCache(), + SecretResolver: secretresolver.New(c, scalefake.NewMultiClient()), + Scheme: mgr.GetScheme(), + }, + } + } + }) + + waitForListenerPod := func(reconciler *AutoscalingListenerReconciler) *corev1.Pod { + Eventually( + func() error { + _, err := reconciler.Reconcile(ctx, reconcileRequest) + if err != nil { + return err + } + + return k8sClient.Get(ctx, reconcileRequest.NamespacedName, new(corev1.Pod)) + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(Succeed()) + + pod := new(corev1.Pod) + Expect(k8sClient.Get(ctx, reconcileRequest.NamespacedName, pod)).To(Succeed()) + return pod + } + + markPodDeadWithMetadataDrift := func(pod *corev1.Pod, evicted bool) { + original := pod.DeepCopy() + pod.Labels["arc.test/listener-label"] = "stale" + pod.Annotations[AnnotationKeyListenerConfigResourceVersion] = "stale" + Expect(k8sClient.Patch(ctx, pod, client.MergeFrom(original))).To(Succeed()) + + Expect(k8sClient.Get(ctx, reconcileRequest.NamespacedName, pod)).To(Succeed()) + if evicted { + pod.Status.Reason = "Evicted" + } else { + pod.Status.ContainerStatuses = []corev1.ContainerStatus{ + { + Name: autoscalingListenerContainerName, + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ExitCode: 1}, + }, + }, + } + } + Expect(k8sClient.Status().Update(ctx, pod)).To(Succeed()) + } + + recoverDeadPod := func(evicted, forbidPodPatch bool) { + bootstrapReconciler := newListenerReconciler(k8sClient) + pod := waitForListenerPod(bootstrapReconciler) + oldPodUID := pod.UID + markPodDeadWithMetadataDrift(pod, evicted) + + reconcilerClient := client.Client(k8sClient) + var podPatchAttempts atomic.Int32 + if forbidPodPatch { + watchClient, err := client.NewWithWatch(cfg, client.Options{Scheme: mgr.GetScheme()}) + Expect(err).NotTo(HaveOccurred()) + reconcilerClient = interceptor.NewClient(watchClient, interceptor.Funcs{ + Patch: func(ctx context.Context, c client.WithWatch, obj client.Object, patch client.Patch, opts ...client.PatchOption) error { + if _, ok := obj.(*corev1.Pod); ok { + podPatchAttempts.Add(1) + return kerrors.NewForbidden(schema.GroupResource{Resource: "pods"}, obj.GetName(), fmt.Errorf("pod patches are forbidden")) + } + return c.Patch(ctx, obj, patch, opts...) + }, + }) + } + + reconciler := newListenerReconciler(reconcilerClient) + _, err := reconciler.Reconcile(ctx, reconcileRequest) + Expect(err).NotTo(HaveOccurred()) + Expect(podPatchAttempts.Load()).To(BeZero(), "dead listener pods must be deleted before metadata is patched") + + Eventually( + func() error { + _, err := reconciler.Reconcile(ctx, reconcileRequest) + if err != nil { + return err + } + + pod := new(corev1.Pod) + err = k8sClient.Get(ctx, reconcileRequest.NamespacedName, pod) + if kerrors.IsNotFound(err) { + return err + } + if err != nil { + return err + } + if !pod.DeletionTimestamp.IsZero() { + if err := k8sClient.Delete(ctx, pod, client.GracePeriodSeconds(0)); err != nil { + return err + } + return fmt.Errorf("listener pod is terminating") + } + if pod.UID == oldPodUID { + return fmt.Errorf("listener pod has not been recreated") + } + if pod.Labels["arc.test/listener-label"] != "desired" || + pod.Annotations[AnnotationKeyListenerConfigResourceVersion] == "stale" { + return fmt.Errorf("replacement listener pod has stale metadata") + } + return nil + }, + autoscalingListenerTestTimeout, + autoscalingListenerTestInterval, + ).Should(Succeed()) + Expect(podPatchAttempts.Load()).To(BeZero(), "the replacement listener pod should not need a metadata patch") + } + + It("recreates an evicted listener pod with metadata drift when pod patches are forbidden", func() { + recoverDeadPod(true, true) + }) + + It("recreates a terminated listener pod with metadata drift when pod patches are forbidden", func() { + recoverDeadPod(false, true) + }) + + It("recreates a terminated listener pod with metadata drift without a patch failure", func() { + recoverDeadPod(false, false) + }) +}) + var _ = Describe("Test AutoScalingListener controller with proxy", func() { var ctx context.Context var mgr ctrl.Manager