Skip to content

Commit 1ab93fd

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 1ab93fd

3 files changed

Lines changed: 280 additions & 3 deletions

File tree

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

Lines changed: 77 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,43 @@ 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+
// Defensive: this is only reachable from the deletion path, but never
523+
// force-remove a finalizer from a live object.
524+
if ephemeralRunner.DeletionTimestamp.IsZero() {
525+
return false, nil
526+
}
527+
if time.Since(ephemeralRunner.DeletionTimestamp.Time) < r.registrationFinalizerForceTimeout() {
528+
return false, nil
529+
}
530+
531+
pod := new(corev1.Pod)
532+
err := r.Get(ctx, types.NamespacedName{Namespace: ephemeralRunner.Namespace, Name: ephemeralRunner.Name}, pod)
533+
switch {
534+
case kerrors.IsNotFound(err):
535+
return true, nil
536+
case err != nil:
537+
return false, fmt.Errorf("failed to get runner pod while evaluating finalizer force-removal: %w", err)
538+
case pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed:
539+
log.Info("Runner pod is in a terminal phase and cannot unregister on its own", "phase", pod.Status.Phase)
540+
return true, nil
541+
default:
542+
return false, nil
543+
}
544+
}
545+
472546
func (r *EphemeralRunnerReconciler) cleanupResources(ctx context.Context, ephemeralRunner *v1alpha1.EphemeralRunner, log logr.Logger) error {
473547
log.Info("Cleaning up the runner pod")
474548
pod := new(corev1.Pod)

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

Lines changed: 201 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1285,6 +1285,207 @@ 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+
// The Actions service always reports the job as
1321+
// still running, so unregistration never succeeds.
1322+
scalefake.WithRemoveRunner(scaleset.JobStillRunningError),
1323+
),
1324+
),
1325+
)),
1326+
},
1327+
}
1328+
1329+
err := controller.SetupWithManager(mgr)
1330+
Expect(err).To(BeNil(), "failed to setup controller")
1331+
1332+
ephemeralRunner = newExampleRunner("test-runner", autoscalingNS.Name, configSecret.Name)
1333+
err = k8sClient.Create(ctx, ephemeralRunner)
1334+
Expect(err).To(BeNil(), "failed to create ephemeral runner")
1335+
1336+
startManagers(GinkgoT(), mgr)
1337+
1338+
// Wait for both finalizers to be added (this happens over two
1339+
// reconcile passes) so a subsequent delete exercises the
1340+
// registration finalizer path.
1341+
Eventually(
1342+
func() ([]string, error) {
1343+
er := new(v1alpha1.EphemeralRunner)
1344+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil {
1345+
return nil, err
1346+
}
1347+
n := len(er.Finalizers)
1348+
return er.Finalizers[:n:n], nil
1349+
},
1350+
ephemeralRunnerTimeout,
1351+
ephemeralRunnerInterval,
1352+
).Should(ContainElements(ephemeralRunnerFinalizerName, ephemeralRunnerActionsFinalizerName), "both finalizers should be added")
1353+
1354+
// Wait for the runner pod to be created.
1355+
Eventually(
1356+
func() error {
1357+
return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod))
1358+
},
1359+
ephemeralRunnerTimeout,
1360+
ephemeralRunnerInterval,
1361+
).Should(Succeed(), "runner pod should be created")
1362+
})
1363+
1364+
deleteEphemeralRunnerAndWaitForTerminating := func() {
1365+
err := k8sClient.Delete(ctx, ephemeralRunner)
1366+
Expect(err).To(BeNil(), "failed to delete ephemeral runner")
1367+
1368+
Eventually(
1369+
func() (bool, error) {
1370+
er := new(v1alpha1.EphemeralRunner)
1371+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil {
1372+
return false, err
1373+
}
1374+
return !er.DeletionTimestamp.IsZero(), nil
1375+
},
1376+
ephemeralRunnerTimeout,
1377+
ephemeralRunnerInterval,
1378+
).Should(BeTrue(), "ephemeral runner should be terminating but held by finalizers")
1379+
}
1380+
1381+
Context("with a short force timeout", func() {
1382+
BeforeEach(func() {
1383+
forceTimeout = time.Second
1384+
})
1385+
1386+
It("force-removes the registration finalizer when the pod is gone and deletion exceeded the timeout", func() {
1387+
deleteEphemeralRunnerAndWaitForTerminating()
1388+
1389+
// While the pod exists, the runner must never be force-finalized,
1390+
// even after the timeout has passed.
1391+
Consistently(
1392+
func() error {
1393+
return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1394+
},
1395+
2*time.Second,
1396+
ephemeralRunnerInterval,
1397+
).Should(Succeed(), "ephemeral runner should stay terminating while the pod exists")
1398+
1399+
// Simulate mid-job pod death: the pod disappears while the
1400+
// service still reports the job as running.
1401+
pod := new(corev1.Pod)
1402+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod)
1403+
Expect(err).To(BeNil(), "failed to get runner pod")
1404+
err = k8sClient.Delete(ctx, pod)
1405+
Expect(err).To(BeNil(), "failed to delete runner pod")
1406+
1407+
Eventually(
1408+
func() bool {
1409+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1410+
return kerrors.IsNotFound(err)
1411+
},
1412+
ephemeralRunnerTimeout,
1413+
ephemeralRunnerInterval,
1414+
).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted")
1415+
})
1416+
1417+
It("force-removes the registration finalizer when the pod is in a terminal phase and deletion exceeded the timeout", func() {
1418+
deleteEphemeralRunnerAndWaitForTerminating()
1419+
1420+
// Let the force timeout elapse while the (non-terminal) pod
1421+
// still exists; nothing should be force-removed yet.
1422+
Consistently(
1423+
func() error {
1424+
return k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1425+
},
1426+
2*time.Second,
1427+
ephemeralRunnerInterval,
1428+
).Should(Succeed(), "ephemeral runner should stay terminating while the pod is not terminal")
1429+
1430+
// Simulate an OOM-killed/evicted runner: the pod object remains
1431+
// but is in a terminal phase, so it can never unregister itself.
1432+
pod := new(corev1.Pod)
1433+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod)
1434+
Expect(err).To(BeNil(), "failed to get runner pod")
1435+
pod.Status.Phase = corev1.PodFailed
1436+
err = k8sClient.Status().Update(ctx, pod)
1437+
Expect(err).To(BeNil(), "failed to update pod status")
1438+
1439+
Eventually(
1440+
func() bool {
1441+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(v1alpha1.EphemeralRunner))
1442+
return kerrors.IsNotFound(err)
1443+
},
1444+
ephemeralRunnerTimeout,
1445+
ephemeralRunnerInterval,
1446+
).Should(BeTrue(), "ephemeral runner should be force-finalized and deleted")
1447+
})
1448+
})
1449+
1450+
Context("with a long force timeout", func() {
1451+
BeforeEach(func() {
1452+
forceTimeout = 5 * time.Minute
1453+
})
1454+
1455+
It("does not force-remove the registration finalizer before the timeout even when the pod is gone", func() {
1456+
deleteEphemeralRunnerAndWaitForTerminating()
1457+
1458+
pod := new(corev1.Pod)
1459+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, pod)
1460+
Expect(err).To(BeNil(), "failed to get runner pod")
1461+
err = k8sClient.Delete(ctx, pod)
1462+
Expect(err).To(BeNil(), "failed to delete runner pod")
1463+
1464+
Eventually(
1465+
func() bool {
1466+
err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, new(corev1.Pod))
1467+
return kerrors.IsNotFound(err)
1468+
},
1469+
ephemeralRunnerTimeout,
1470+
ephemeralRunnerInterval,
1471+
).Should(BeTrue(), "runner pod should be gone")
1472+
1473+
Consistently(
1474+
func() ([]string, error) {
1475+
er := new(v1alpha1.EphemeralRunner)
1476+
if err := k8sClient.Get(ctx, client.ObjectKey{Name: ephemeralRunner.Name, Namespace: ephemeralRunner.Namespace}, er); err != nil {
1477+
return nil, err
1478+
}
1479+
n := len(er.Finalizers)
1480+
return er.Finalizers[:n:n], nil
1481+
},
1482+
3*time.Second,
1483+
ephemeralRunnerInterval,
1484+
).Should(ContainElement(ephemeralRunnerActionsFinalizerName), "registration finalizer should be kept before the timeout")
1485+
})
1486+
})
1487+
})
1488+
12881489
Describe("Pod proxy config", func() {
12891490
var ctx context.Context
12901491
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)