From 791634fb128b871999486aa3ffb2acbb90cd4efb Mon Sep 17 00:00:00 2001 From: Yusuke Kuoka Date: Sun, 13 Mar 2022 07:24:35 +0000 Subject: [PATCH] Fix static runners not scaling up It turned out that #1179 broke static runners in a way it is no longer able to scale up at all when the desired replicas is updated. This fixes that by correcting a certain short-circuit that is intended only for ephemeral runners to not mistakenly triggered for static runners. --- controllers/runner_pod_owner.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/controllers/runner_pod_owner.go b/controllers/runner_pod_owner.go index 6cf1ca23..9e150fed 100644 --- a/controllers/runner_pod_owner.go +++ b/controllers/runner_pod_owner.go @@ -329,7 +329,7 @@ func syncRunnerPodsOwners(ctx context.Context, c client.Client, log logr.Logger, maybeRunning := pending + running wantMoreRunners := newDesiredReplicas > maybeRunning - alreadySyncedAfterEffectiveTime := lastSyncTime != nil && effectiveTime != nil && lastSyncTime.After(effectiveTime.Time) + alreadySyncedAfterEffectiveTime := ephemeral && lastSyncTime != nil && effectiveTime != nil && lastSyncTime.After(effectiveTime.Time) runnerPodRecreationDelayAfterWebhookScale := lastSyncTime != nil && time.Now().Before(lastSyncTime.Add(DefaultRunnerPodRecreationDelayAfterWebhookScale)) log = log.WithValues( @@ -344,6 +344,16 @@ func syncRunnerPodsOwners(ctx context.Context, c client.Client, log logr.Logger, ) if wantMoreRunners && alreadySyncedAfterEffectiveTime && runnerPodRecreationDelayAfterWebhookScale { + // This is our special handling of the situation for ephemeral runners only. + // + // Handling static runners this way results in scale-up to not work at all, + // because then any scale up attempts for static runenrs fall within this condition, for two reasons. + // First, static(persistent) runners will never restart on their own. + // Second, we don't update EffectiveTime for static runners. + // + // We do need to skip this condition for static runners, and that's why we take the `ephemeral` flag into account when + // computing `alreadySyncedAfterEffectiveTime``. + log.V(2).Info( "Detected that some ephemeral runners have disappeared. " + "Usually this is due to that ephemeral runner completions " +