FIX: add back metrics runnerLessMinimumTime and runnerFound to scaleD…

…own (#881)
pytorch · Oct 17, 2022 · 82d970e · 82d970e
1 parent 787aa4c
commit 82d970e
Show file tree

Hide file tree

Showing 3 changed files with 67 additions and 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
@@ -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/...rm-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts b/...rm-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
@@ -56,22 +56,28 @@ export async function scaleDown(): Promise<void> {
         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 {