From 82d970ee374f5077eea3e34749c1dd1a629eaf23 Mon Sep 17 00:00:00 2001 From: Jean Schmidt <4520845+jeanschmidt@users.noreply.github.com> Date: Mon, 17 Oct 2022 10:42:51 +0200 Subject: [PATCH] FIX: add back metrics runnerLessMinimumTime and runnerFound to scaleDown (#881) --- .../runners/src/scale-runners/metrics.ts | 6 +++ .../src/scale-runners/scale-down.test.ts | 52 ++++++++++++------- .../runners/src/scale-runners/scale-down.ts | 39 +++++++++----- 3 files changed, 67 insertions(+), 30 deletions(-) diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/metrics.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/metrics.ts index 0d4c0b7563..9d8eff82c4 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/metrics.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/metrics.ts @@ -589,6 +589,12 @@ export class ScaleDownMetrics extends Metrics { this.countEntry(`run.ec2runners.${ec2Runner.runnerType}.notMinTime`); } + /* istanbul ignore next */ + runnerIsRemovable(ec2Runner: RunnerInfo) { + this.countEntry(`run.ec2runners.removable`); + this.countEntry(`run.ec2runners.${ec2Runner.runnerType}.removable`); + } + /* istanbul ignore next */ runnerFound(ec2Runner: RunnerInfo) { this.countEntry(`run.ec2runners.total`); diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts index 0f907b9be1..a737742275 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts @@ -888,32 +888,44 @@ describe('scale-down', () => { describe('isRunnerRemovable', () => { describe('ghRunner === undefined', () => { it('launchTime === undefined', () => { - const response = isRunnerRemovable(undefined, { - instanceId: 'AGDGADUWG113', - launchTime: undefined, - }); + const response = isRunnerRemovable( + undefined, + { + instanceId: 'AGDGADUWG113', + launchTime: undefined, + }, + metrics, + ); expect(response).toEqual(false); }); it('exceeded minimum time', () => { - const response = isRunnerRemovable(undefined, { - instanceId: 'AGDGADUWG113', - launchTime: moment(new Date()) - .utc() - .subtract(minimumRunningTimeInMinutes + 5, 'minutes') - .toDate(), - }); + const response = isRunnerRemovable( + undefined, + { + instanceId: 'AGDGADUWG113', + launchTime: moment(new Date()) + .utc() + .subtract(minimumRunningTimeInMinutes + 5, 'minutes') + .toDate(), + }, + metrics, + ); expect(response).toEqual(true); }); it('dont exceeded minimum time', () => { - const response = isRunnerRemovable(undefined, { - instanceId: 'AGDGADUWG113', - launchTime: moment(new Date()) - .utc() - .subtract(minimumRunningTimeInMinutes - 5, 'minutes') - .toDate(), - }); + const response = isRunnerRemovable( + undefined, + { + instanceId: 'AGDGADUWG113', + launchTime: moment(new Date()) + .utc() + .subtract(minimumRunningTimeInMinutes - 5, 'minutes') + .toDate(), + }, + metrics, + ); expect(response).toEqual(false); }); }); @@ -928,6 +940,7 @@ describe('scale-down', () => { instanceId: 'AGDGADUWG113', launchTime: undefined, }, + metrics, ); expect(response).toEqual(false); }); @@ -941,6 +954,7 @@ describe('scale-down', () => { instanceId: 'AGDGADUWG113', launchTime: undefined, }, + metrics, ); expect(response).toEqual(false); }); @@ -957,6 +971,7 @@ describe('scale-down', () => { .subtract(minimumRunningTimeInMinutes + 5, 'minutes') .toDate(), }, + metrics, ); expect(response).toEqual(true); }); @@ -973,6 +988,7 @@ describe('scale-down', () => { .subtract(minimumRunningTimeInMinutes - 5, 'minutes') .toDate(), }, + metrics, ); expect(response).toEqual(false); }); diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts index b7cd950b2c..ed60525978 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts @@ -56,22 +56,28 @@ export async function scaleDown(): Promise { if (ec2runner.repo !== undefined) { const ghRunner = await getGHRunnerRepo(ec2runner, metrics); // if configured to repo, don't mess with organization runners - if (!Config.Instance.enableOrganizationRunners && isRunnerRemovable(ghRunner, ec2runner)) { - if (ghRunner === undefined) { - ghRunnersRemovable.unshift([ec2runner, ghRunner]); - } else { - ghRunnersRemovable.push([ec2runner, ghRunner]); + if (!Config.Instance.enableOrganizationRunners) { + metrics.runnerFound(ec2runner); + if (isRunnerRemovable(ghRunner, ec2runner, metrics)) { + if (ghRunner === undefined) { + ghRunnersRemovable.unshift([ec2runner, ghRunner]); + } else { + ghRunnersRemovable.push([ec2runner, ghRunner]); + } } } // ORG assigned runners } else if (ec2runner.org !== undefined) { const ghRunner = await getGHRunnerOrg(ec2runner, metrics); // if configured to org, don't mess with repo runners - if (Config.Instance.enableOrganizationRunners && isRunnerRemovable(ghRunner, ec2runner)) { - if (ghRunner === undefined) { - ghRunnersRemovable.unshift([ec2runner, ghRunner]); - } else { - ghRunnersRemovable.push([ec2runner, ghRunner]); + if (Config.Instance.enableOrganizationRunners) { + metrics.runnerFound(ec2runner); + if (isRunnerRemovable(ghRunner, ec2runner, metrics)) { + if (ghRunner === undefined) { + ghRunnersRemovable.unshift([ec2runner, ghRunner]); + } else { + ghRunnersRemovable.push([ec2runner, ghRunner]); + } } } } @@ -213,11 +219,20 @@ export async function isEphemeralRunner(ec2runner: RunnerInfo, metrics: ScaleDow return runnerTypes.get(ec2runner.runnerType)?.is_ephemeral ?? false; } -export function isRunnerRemovable(ghRunner: GhRunner | undefined, ec2runner: RunnerInfo): boolean { +export function isRunnerRemovable( + ghRunner: GhRunner | undefined, + ec2runner: RunnerInfo, + metrics: ScaleDownMetrics, +): boolean { if (ghRunner !== undefined && ghRunner.busy) { return false; } - return runnerMinimumTimeExceeded(ec2runner); + if (!runnerMinimumTimeExceeded(ec2runner)) { + metrics.runnerLessMinimumTime(ec2runner); + return false; + } + metrics.runnerIsRemovable(ec2runner); + return true; } export function runnerMinimumTimeExceeded(runner: RunnerInfo): boolean {