Skip to content

Commit 3af222a

Browse files
saxocellphonesaxocellphone
committed
Force-remove EphemeralRunner registration finalizer after deletion timeout
When an EphemeralRunner is deleting and the Actions service keeps returning JobStillRunningError, the reconciler requeues forever. If the runner pod is already gone or in a terminal phase and deletion has been pending past a timeout (default 10m, injectable for tests), force-remove the registration finalizer so the CR can finish deleting. Fixes stuck Terminating EphemeralRunners after mid-job pod death (eviction, OOM, node loss). Related: #4155, #4307 Signed-off-by: Victor Nazzaro <nazzav923@gmail.com>
1 parent a035c5a commit 3af222a

3 files changed

Lines changed: 264 additions & 3 deletions

File tree

‎controllers/actions.github.com/ephemeralrunner_controller.go‎

Lines changed: 75 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -46,13 +46,34 @@ const (
4646
ephemeralRunnerActionsFinalizerName = "ephemeralrunner.actions.github.com/runner-registration-finalizer"
4747
)
4848

49+
// DefaultRegistrationFinalizerForceTimeout is how long a deleting
50+
// EphemeralRunner may stay blocked on the Actions service reporting
51+
// JobStillRunning before the registration finalizer is force-removed.
52+
// The force path only applies when the runner pod is gone or in a terminal
53+
// phase. Without this bound, a mid-job pod death (eviction, OOM, node loss)
54+
// can leave the EphemeralRunner Terminating forever.
55+
const DefaultRegistrationFinalizerForceTimeout = 10 * time.Minute
56+
4957
// EphemeralRunnerReconciler reconciles a EphemeralRunner object
5058
type EphemeralRunnerReconciler struct {
5159
client.Client
5260
Log logr.Logger
5361
Scheme *runtime.Scheme
5462
PublishMetrics bool
5563
ResourceBuilder
64+
65+
// RegistrationFinalizerForceTimeout bounds how long the deletion path
66+
// requeues on JobStillRunning while the runner pod is gone or terminal
67+
// before force-removing the registration finalizer. Zero means
68+
// DefaultRegistrationFinalizerForceTimeout.
69+
RegistrationFinalizerForceTimeout time.Duration
70+
}
71+
72+
func (r *EphemeralRunnerReconciler) registrationFinalizerForceTimeout() time.Duration {
73+
if r.RegistrationFinalizerForceTimeout > 0 {
74+
return r.RegistrationFinalizerForceTimeout
75+
}
76+
return DefaultRegistrationFinalizerForceTimeout
5677
}
5778

5879
var ephemeralRunnerPhaseMetrics = struct {
@@ -110,12 +131,28 @@ func (r *EphemeralRunnerReconciler) Reconcile(ctx context.Context, req ctrl.Requ
110131
log.Error(err, "Failed to clean up runner from service")
111132
return ctrl.Result{}, err
112133
}
134+
forced := false
113135
if !ok {
114-
log.Info("Runner is not finished yet, retrying in 30s")
115-
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
136+
force, forceErr := r.shouldForceRegistrationFinalizerRemoval(ctx, &ephemeralRunner, log)
137+
if forceErr != nil {
138+
log.Error(forceErr, "Failed to evaluate registration finalizer force-removal")
139+
return ctrl.Result{}, forceErr
140+
}
141+
if !force {
142+
log.Info("Runner is not finished yet, retrying in 30s")
143+
return ctrl.Result{RequeueAfter: 30 * time.Second}, nil
144+
}
145+
forced = true
146+
log.Info(
147+
"Actions service still reports the job as running, but the runner pod is gone or terminal and deletion exceeded the timeout; force-removing registration finalizer",
148+
"timeout", r.registrationFinalizerForceTimeout(),
149+
"deletionTimestamp", ephemeralRunner.DeletionTimestamp,
150+
)
116151
}
117152

118-
log.Info("Runner is cleaned up from the service, removing finalizer")
153+
if !forced {
154+
log.Info("Runner is cleaned up from the service, removing finalizer")
155+
}
119156
if controllerutil.RemoveFinalizer(&ephemeralRunner, ephemeralRunnerActionsFinalizerName) {
120157
log.Info("Removed finalizer from ephemeral runner")
121158
if err := r.Patch(ctx, &ephemeralRunner, client.MergeFrom(original)); err != nil {
@@ -469,6 +506,41 @@ func (r *EphemeralRunnerReconciler) cleanupRunnerFromService(ctx context.Context
469506
return true, nil
470507
}
471508

509+
// shouldForceRegistrationFinalizerRemoval returns true when it is safe to drop
510+
// the runner-registration-finalizer without a successful RemoveRunner call.
511+
//
512+
// Required:
513+
// - EphemeralRunner is deleting
514+
// - deletionTimestamp is at least registrationFinalizerForceTimeout() old
515+
// - runner pod is NotFound, or exists in a terminal phase
516+
// (Succeeded/Failed), meaning the runner process cannot complete the job
517+
// and unregister on its own
518+
//
519+
// If the pod exists and is not terminal we never force-remove: the runner may
520+
// still finish the job, after which RemoveRunner succeeds normally.
521+
func (r *EphemeralRunnerReconciler) shouldForceRegistrationFinalizerRemoval(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) (bool, error) {
522+
if ephemeralRunner.DeletionTimestamp.IsZero() {
523+
return false, nil
524+
}
525+
if time.Since(ephemeralRunner.DeletionTimestamp.Time) < r.registrationFinalizerForceTimeout() {
526+
return false, nil
527+
}
528+
529+
pod := new(corev1.Pod)
530+
err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunner.Namespace, Name: ephemeralRunner.Name}, pod)
531+
switch {
532+
case kerrors.IsNotFound(err):
533+
return true, nil
534+
case err != nil:
535+
return false, fmt.Errorf("failed to get runner pod while evaluating finalizer force-removal: %w", err)
536+
case pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed:
537+
log.Info("Runner pod is in a terminal phase and cannot unregister on its own", "phase", pod.Status.Phase)
538+
return true, nil
539+
default:
540+
return false, nil
541+
}
542+
}
543+
472544
func (r *EphemeralRunnerReconciler) cleanupResources(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error {
473545
log.Info("Cleaning up the runner pod")
474546
pod := new(corev1.Pod)

‎controllers/actions.github.com/ephemeralrunner_controller_test.go‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1285,6 +1285,193 @@ var _ = Describe("EphemeralRunner", func() {
12851285
})
12861286
})
12871287

1288+
Describe("Registration finalizer force removal", func() {
1289+
var ctx context.Context
1290+
var mgr ctrl.Manager
1291+
var autoscalingNS *corev1.Namespace
1292+
var configSecret *corev1.Secret
1293+
var controller *EphemeralRunnerReconciler
1294+
var ephemeralRunner *v1alpha1.EphemeralRunner
1295+
var forceTimeout time.Duration
1296+
1297+
BeforeEach(func() {
1298+
ctx = context.Background()
1299+
autoscalingNS, mgr = createNamespace(GinkgoT(), k8sClient)
1300+
configSecret = createDefaultSecret(GinkgoT(), k8sClient, autoscalingNS.Name)
1301+
})
1302+
1303+
JustBeforeEach(func() {
1304+
controller = &EphemeralRunnerReconciler{
1305+
Client: mgr.GetClient(),
1306+
Scheme: mgr.GetScheme(),
1307+
Log: logf.Log,
1308+
RegistrationFinalizerForceTimeout: forceTimeout,
1309+
ResourceBuilder: ResourceBuilder{
1310+
SecretResolver: secretresolver.New(mgr.GetClient(), scalefake.NewMultiClient(
1311+
scalefake.WithClient(
1312+
scalefake.NewClient(
1313+
scalefake.WithGenerateJitRunnerConfig(
1314+
&scaleset.RunnerScaleSetJitRunnerConfig{
1315+
Runner: &scaleset.RunnerReference{ID: 1, Name: "test-runner"},
1316+
EncodedJITConfig: "fake-jit-config",
1317+
},
1318+
nil,
1319+
),
1320+
scalefake.WithRemoveRunner(scaleset.JobStillRunningError),
1321+
),
1322+
),
1323+
)),
1324+
},
1325+
}
1326+
1327+
err := controller.SetupWithManager(mgr)
1328+
Expect(err).To(BeNil(), "failed to setup controller")
1329+
1330+
ephemeralRunner = newExampleRunner("test-runner", autoscalingNS.Name, configSecret.Name)
1331+
err = k8sClient.Create(ctx, ephemeralRunner)
1332+
Expect(err).To(BeNil(), "failed to create ephemeral runner")
1333+
1334+
startManagers(GinkgoT(), mgr)
1335+
1336+
Eventually(
1337+
func() ([]string, error) {
1338+
er := new(v1alpha1.EphemeralRunner)
1339+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil {
1340+
return nil, err
1341+
}
1342+
n := len(er.Finalizers)
1343+
return er.Finalizers[:n:n], nil
1344+
},
1345+
ephemeralRunnerTimeout,
1346+
ephemeralRunnerInterval,
1347+
).Should(ContainElements(ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName), "both finalizers should be added")
1348+
1349+
Eventually(
1350+
func() error {
1351+
return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod))
1352+
},
1353+
ephemeralRunnerTimeout,
1354+
ephemeralRunnerInterval,
1355+
).Should(Succeed(), "runner pod should be created")
1356+
})
1357+
1358+
deleteEphemeralRunnerAndWaitForTerminating := func() {
1359+
err := k8sClient.Delete(ctx, ephemeralRunner)
1360+
Expect(err).To(BeNil(), "failed to delete ephemeral runner")
1361+
1362+
Eventually(
1363+
func() (bool, error) {
1364+
er := new(v1alpha1.EphemeralRunner)
1365+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil {
1366+
return false, err
1367+
}
1368+
return !er.DeletionTimestamp.IsZero(), nil
1369+
},
1370+
ephemeralRunnerTimeout,
1371+
ephemeralRunnerInterval,
1372+
).Should(BeTrue(), "ephemeral runner should be terminating but held by finalizers")
1373+
}
1374+
1375+
Context("with a short force timeout", func() {
1376+
BeforeEach(func() {
1377+
forceTimeout = time.Second
1378+
})
1379+
1380+
It("force-removes the registration finalizer when the pod is gone and deletion exceeded the timeout", func() {
1381+
deleteEphemeralRunnerAndWaitForTerminating()
1382+
1383+
Consistently(
1384+
func() error {
1385+
return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1386+
},
1387+
2*time.Second,
1388+
ephemeralRunnerInterval,
1389+
).Should(Succeed(), "ephemeral runner should stay terminating while the pod exists")
1390+
1391+
pod := new(corev1.Pod)
1392+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod)
1393+
Expect(err).To(BeNil(), "failed to get runner pod")
1394+
err = k8sClient.Delete(ctx, pod)
1395+
Expect(err).To(BeNil(), "failed to delete runner pod")
1396+
1397+
Eventually(
1398+
func() bool {
1399+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1400+
return kerrors.IsNotFound(err)
1401+
},
1402+
ephemeralRunnerTimeout,
1403+
ephemeralRunnerInterval,
1404+
).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted")
1405+
})
1406+
1407+
It("force-removes the registration finalizer when the pod is in a terminal phase and deletion exceeded the timeout", func() {
1408+
deleteEphemeralRunnerAndWaitForTerminating()
1409+
1410+
Consistently(
1411+
func() error {
1412+
return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1413+
},
1414+
2*time.Second,
1415+
ephemeralRunnerInterval,
1416+
).Should(Succeed(), "ephemeral runner should stay terminating while the pod is not terminal")
1417+
1418+
pod := new(corev1.Pod)
1419+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod)
1420+
Expect(err).To(BeNil(), "failed to get runner pod")
1421+
pod.Status.Phase = corev1.PodFailed
1422+
err = k8sClient.Status().Update(ctx, pod)
1423+
Expect(err).To(BeNil(), "failed to update pod status")
1424+
1425+
Eventually(
1426+
func() bool {
1427+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1428+
return kerrors.IsNotFound(err)
1429+
},
1430+
ephemeralRunnerTimeout,
1431+
ephemeralRunnerInterval,
1432+
).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted")
1433+
})
1434+
})
1435+
1436+
Context("with a long force timeout", func() {
1437+
BeforeEach(func() {
1438+
forceTimeout = 5 * time.Minute
1439+
})
1440+
1441+
It("does not force-remove the registration finalizer before the timeout even when the pod is gone", func() {
1442+
deleteEphemeralRunnerAndWaitForTerminating()
1443+
1444+
pod := new(corev1.Pod)
1445+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod)
1446+
Expect(err).To(BeNil(), "failed to get runner pod")
1447+
err = k8sClient.Delete(ctx, pod)
1448+
Expect(err).To(BeNil(), "failed to delete runner pod")
1449+
1450+
Eventually(
1451+
func() bool {
1452+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod))
1453+
return kerrors.IsNotFound(err)
1454+
},
1455+
ephemeralRunnerTimeout,
1456+
ephemeralRunnerInterval,
1457+
).Should(BeTrue(), "runner pod should be gone")
1458+
1459+
Consistently(
1460+
func() ([]string, error) {
1461+
er := new(v1alpha1.EphemeralRunner)
1462+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil {
1463+
return nil, err
1464+
}
1465+
n := len(er.Finalizers)
1466+
return er.Finalizers[:n:n], nil
1467+
},
1468+
3*time.Second,
1469+
ephemeralRunnerInterval,
1470+
).Should(ContainElement(ephemeralRunnerActionsFinalizerName), "registration finalizer should be kept before the timeout")
1471+
})
1472+
})
1473+
})
1474+
12881475
Describe("Pod proxy config", func() {
12891476
var ctx context.Context
12901477
var mgr ctrl.Manager

‎main.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,8 @@ func main() {
342342
Scheme: mgr.GetScheme(),
343343
PublishMetrics: metricsAddr != "0",
344344
ResourceBuilder: rb,
345+
346+
RegistrationFinalizerForceTimeout: actionsgithubcom.DefaultRegistrationFinalizerForceTimeout,
345347
}).SetupWithManager(mgr, runnerOpts...); err != nil {
346348
log.Error(err, "unable to create controller", "controller", "EphemeralRunner")
347349
os.Exit(1)

0 commit comments

Comments
 (0)