feat: mark orphan runners before removing them

Author: npalm

lambdas/functions/control-plane/jest.config.ts

@@ -6,10 +6,10 @@ const config: Config = {
   ...defaultConfig,
   coverageThreshold: {
     global: {
-      statements: 97.79,
-      branches: 96.13,
-      functions: 95.4,
-      lines: 98.06,
+      statements: 97.99,
+      branches: 96.04,
+      functions: 97.53,
+      lines: 98.27,
     },
   },
 };

lambdas/functions/control-plane/src/aws/runners.d.ts

@@ -9,6 +9,7 @@ export interface RunnerList {
   type?: string;
   repo?: string;
   org?: string;
+  orphan?: boolean;
 }
 
 export interface RunnerInfo {

lambdas/functions/control-plane/src/aws/runners.test.ts

@@ -68,6 +68,23 @@ describe('list instances', () => {
       launchTime: new Date('2020-10-10T14:48:00.000+09:00'),
       type: 'Org',
       owner: 'CoderToCat',
+      orphan: false,
+    });
+  });
+
+  it('check orphan tag.', async () => {
+    const instances: DescribeInstancesResult = mockRunningInstances;
+    instances.Reservations![0].Instances![0].Tags!.push({ Key: 'ghr:orphan', Value: 'true' });
+    mockEC2Client.on(DescribeInstancesCommand).resolves(instances);
+
+    const resp = await listEC2Runners();
+    expect(resp.length).toBe(1);
+    expect(resp).toContainEqual({
+      instanceId: instances.Reservations![0].Instances![0].InstanceId!,
+      launchTime: instances.Reservations![0].Instances![0].LaunchTime!,
+      type: 'Org',
+      owner: 'CoderToCat',
+      orphan: true,
     });
   });
 
@@ -115,6 +132,23 @@ describe('list instances', () => {
     });
   });
 
+  it('filters instances on environment and orphan', async () => {
+    mockRunningInstances.Reservations![0].Instances![0].Tags!.push({
+      Key: 'ghr:orphan',
+      Value: 'true',
+    });
+    mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances);
+    await listEC2Runners({ environment: ENVIRONMENT, orphan: true });
+    expect(mockEC2Client).toHaveReceivedCommandWith(DescribeInstancesCommand, {
+      Filters: [
+        { Name: 'instance-state-name', Values: ['running', 'pending'] },
+        { Name: 'tag:ghr:environment', Values: [ENVIRONMENT] },
+        { Name: 'tag:ghr:orphan', Values: ['true'] },
+        { Name: 'tag:ghr:Application', Values: ['github-action-runner'] },
+      ],
+    });
+  });
+
   it('No instances, undefined reservations list.', async () => {
     const noInstances: DescribeInstancesResult = {
       Reservations: undefined,

lambdas/functions/control-plane/src/aws/runners.ts

@@ -90,6 +90,7 @@ function getRunnerInfo(runningInstances: DescribeInstancesResult) {
             type: i.Tags?.find((e) => e.Key === 'ghr:Type')?.Value as string,
             repo: i.Tags?.find((e) => e.Key === 'ghr:Repo')?.Value as string,
             org: i.Tags?.find((e) => e.Key === 'ghr:Org')?.Value as string,
+            orphan: i.Tags?.find((e) => e.Key === 'ghr:orphan')?.Value === 'true',
           });
         }
       }

lambdas/functions/control-plane/src/scale-runners/scale-down.test.ts

@@ -5,7 +5,7 @@ import nock from 'nock';
 
 import { RunnerInfo, RunnerList } from '../aws/runners.d';
 import * as ghAuth from '../gh-auth/gh-auth';
-import { listEC2Runners, terminateRunner } from './../aws/runners';
+import { listEC2Runners, terminateRunner, tag } from './../aws/runners';
 import { githubCache } from './cache';
 import { newestFirstStrategy, oldestFirstStrategy, scaleDown } from './scale-down';
 
@@ -42,6 +42,7 @@ const mockedAppAuth = mocked(ghAuth.createGithubAppAuth, { shallow: false });
 const mockedInstallationAuth = mocked(ghAuth.createGithubInstallationAuth, { shallow: false });
 const mockCreateClient = mocked(ghAuth.createOctoClient, { shallow: false });
 const mockListRunners = mocked(listEC2Runners);
+const mockTagRunners = mocked(tag);
 const mockTerminateRunners = mocked(terminateRunner);
 
 export interface TestData {
@@ -172,7 +173,7 @@ describe('Scale down runners', () => {
     describe.each(runnerTypes)('For %s runners.', (type) => {
       it('Should not call terminate when no runners online.', async () => {
         // setup
-        mockListRunners.mockResolvedValue([]);
+        mockAwsRunners([]);
 
         // act
         await scaleDown();
@@ -197,7 +198,8 @@ describe('Scale down runners', () => {
 
         mockGitHubRunners(runners);
         mockListRunners.mockResolvedValue(runners);
-        // act
+        mockAwsRunners(runners);
+
         await scaleDown();
 
         // assert
@@ -220,7 +222,7 @@ describe('Scale down runners', () => {
         const runners = [createRunnerTestData('idle-1', type, MINIMUM_TIME_RUNNING_IN_MINUTES - 1, true, false, false)];
 
         mockGitHubRunners(runners);
-        mockListRunners.mockResolvedValue(runners);
+        mockAwsRunners(runners);
 
         // act
         await scaleDown();
@@ -235,7 +237,7 @@ describe('Scale down runners', () => {
         const runners = [createRunnerTestData('booting-1', type, MINIMUM_BOOT_TIME - 1, false, false, false)];
 
         mockGitHubRunners(runners);
-        mockListRunners.mockResolvedValue(runners);
+        mockAwsRunners(runners);
 
         // act
         await scaleDown();
@@ -250,7 +252,7 @@ describe('Scale down runners', () => {
         const runners = [createRunnerTestData('busy-1', type, MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)];
 
         mockGitHubRunners(runners);
-        mockListRunners.mockResolvedValue(runners);
+        mockAwsRunners(runners);
 
         // act
         await scaleDown();
@@ -274,7 +276,7 @@ describe('Scale down runners', () => {
         ];
 
         mockGitHubRunners(runners);
-        mockListRunners.mockResolvedValue(runners);
+        mockAwsRunners(runners);
         mockOctokit.actions.deleteSelfHostedRunnerFromRepo.mockImplementation(() => {
           return { status: 500 };
         });
@@ -293,10 +295,31 @@ describe('Scale down runners', () => {
 
       it(`Should terminate orphan.`, async () => {
         // setup
-        const runners = [createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, true, true)];
+        const orphanRunner = createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, false, false);
+        const idleRunner = createRunnerTestData('idle-1', type, MINIMUM_BOOT_TIME + 1, true, false, false);
+        const runners = [orphanRunner, idleRunner];
 
-        mockGitHubRunners([]);
-        mockListRunners.mockResolvedValue(runners);
+        mockGitHubRunners([idleRunner]);
+        mockAwsRunners(runners);
+
+        // act
+        await scaleDown();
+
+        // assert
+        checkTerminated(runners);
+        checkNonTerminated(runners);
+
+        expect(mockTagRunners).toHaveBeenCalledWith(orphanRunner.instanceId, [
+          {
+            Key: 'orphan',
+            Value: 'true',
+          },
+        ]);
+        expect(mockTagRunners).not.toHaveBeenCalledWith(idleRunner.instanceId, expect.anything());
+
+        // next cycle, update test data set orphan to true and terminate should be true
+        orphanRunner.orphan = true;
+        orphanRunner.shouldBeTerminated = true;
 
         // act
         await scaleDown();
@@ -306,21 +329,60 @@ describe('Scale down runners', () => {
         checkNonTerminated(runners);
       });
 
-      it(`Should orphan termination failure is not resulting in an exception..`, async () => {
+      it(`Should ignore errors when termination orphan fails.`, async () => {
         // setup
-        const runners = [createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, true, true)];
+        const orphanRunner = createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, true, true);
+        const runners = [orphanRunner];
 
         mockGitHubRunners([]);
-        mockListRunners.mockResolvedValue(runners);
-        mockTerminateRunners.mockRejectedValue(new Error('Termination failed'));
+        mockAwsRunners(runners);
+        mockTerminateRunners.mockImplementation(() => {
+          throw new Error('Failed to terminate');
+        });
 
-        // act and ensure no exception is thrown
-        await expect(scaleDown()).resolves.not.toThrow();
+        // act
+        await scaleDown();
+
+        // assert
+        checkTerminated(runners);
+        checkNonTerminated(runners);
+      });
+
+      describe('When orphan termination fails', () => {
+        it(`Should not throw in case of list runner exception.`, async () => {
+          // setup
+          const runners = [createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, true, true)];
+
+          mockGitHubRunners([]);
+          mockListRunners.mockRejectedValueOnce(new Error('Failed to list runners'));
+          mockAwsRunners(runners);
+
+          // ac
+          await scaleDown();
+
+          // assert
+          checkNonTerminated(runners);
+        });
+
+        it(`Should not throw in case of terminate runner exception.`, async () => {
+          // setup
+          const runners = [createRunnerTestData('orphan-1', type, MINIMUM_BOOT_TIME + 1, false, true, true)];
+
+          mockGitHubRunners([]);
+          mockAwsRunners(runners);
+          mockTerminateRunners.mockRejectedValue(new Error('Failed to terminate'));
+
+          // act and ensure no exception is thrown
+          await scaleDown();
+
+          // assert
+          checkNonTerminated(runners);
+        });
       });
 
       it(`Should not terminate instance in case de-register fails.`, async () => {
         // setup
-        const runners = [createRunnerTestData('idle-1', type, MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, true, false)];
+        const runners = [createRunnerTestData('idle-1', type, MINIMUM_TIME_RUNNING_IN_MINUTES + 1, true, false, false)];
 
         mockOctokit.actions.deleteSelfHostedRunnerFromOrg.mockImplementation(() => {
           return { status: 500 };
@@ -330,7 +392,7 @@ describe('Scale down runners', () => {
         });
 
         mockGitHubRunners(runners);
-        mockListRunners.mockResolvedValue(runners);
+        mockAwsRunners(runners);
 
         // act and should resolve
         await expect(scaleDown()).resolves.not.toThrow();
@@ -352,7 +414,7 @@ describe('Scale down runners', () => {
         });
 
         mockGitHubRunners(runners);
-        mockListRunners.mockResolvedValue(runners);
+        mockAwsRunners(runners);
 
         // act
         await expect(scaleDown()).resolves.not.toThrow();
@@ -383,7 +445,7 @@ describe('Scale down runners', () => {
           ];
 
           mockGitHubRunners(runners);
-          mockListRunners.mockResolvedValue(runners);
+          mockAwsRunners(runners);
 
           // act
           await scaleDown();
@@ -502,6 +564,12 @@ describe('Scale down runners', () => {
   });
 });
 
+function mockAwsRunners(runners: RunnerTestItem[]) {
+  mockListRunners.mockImplementation(async (filter) => {
+    return runners.filter((r) => !filter?.orphan || filter?.orphan === r.orphan);
+  });
+}
+
 function checkNonTerminated(runners: RunnerTestItem[]) {
   const notTerminated = runners.filter((r) => !r.shouldBeTerminated);
   for (const toTerminate of notTerminated) {

lambdas/functions/control-plane/src/scale-runners/scale-down.ts

@@ -3,7 +3,7 @@ import { createChildLogger } from '@terraform-aws-github-runner/aws-powertools-u
 import moment from 'moment';
 
 import { createGithubAppAuth, createGithubInstallationAuth, createOctoClient } from '../gh-auth/gh-auth';
-import { bootTimeExceeded, listEC2Runners, terminateRunner } from './../aws/runners';
+import { bootTimeExceeded, listEC2Runners, tag, terminateRunner } from './../aws/runners';
 import { RunnerInfo, RunnerList } from './../aws/runners.d';
 import { GhRunners, githubCache } from './cache';
 import { ScalingDownConfig, getEvictionStrategy, getIdleRunnerCount } from './scale-down-config';
@@ -184,8 +184,7 @@ async function evaluateAndRemoveRunners(
         }
       } else {
         if (bootTimeExceeded(ec2Runner)) {
-          logger.info(`Runner '${ec2Runner.instanceId}' is orphaned and will be removed.`);
-          terminateOrphan(ec2Runner.instanceId);
+          markOrphan(ec2Runner.instanceId);
         } else {
           logger.debug(`Runner ${ec2Runner.instanceId} has not yet booted.`);
         }
@@ -194,11 +193,26 @@ async function evaluateAndRemoveRunners(
   }
 }
 
-async function terminateOrphan(instanceId: string): Promise<void> {
+async function markOrphan(instanceId: string): Promise<void> {
   try {
-    await terminateRunner(instanceId);
+    await tag(instanceId, [{ Key: 'orphan', Value: 'true' }]);
+    logger.info(`Runner '${instanceId}' marked as orphan.`);
   } catch (e) {
-    logger.debug(`Orphan runner '${instanceId}' cannot be removed.`);
+    logger.warn(`Orphan runner '${instanceId}' cannot be marked.`);
+  }
+}
+
+async function terminateOrphan(environment: string): Promise<void> {
+  try {
+    const orphanRunners = await listEC2Runners({ environment, orphan: true });
+
+    for (const runner of orphanRunners) {
+      await terminateRunner(runner.instanceId).catch((e) => {
+        logger.error(`Failed to terminate orphan runner '${runner.instanceId}'`, { error: e });
+      });
+    }
+  } catch (e) {
+    logger.warn(`Failure during orphan runner termination.`, { error: e });
   }
 }
 
@@ -221,14 +235,18 @@ async function listRunners(environment: string) {
 }
 
 function filterRunners(ec2runners: RunnerList[]): RunnerInfo[] {
-  return ec2runners.filter((ec2Runner) => ec2Runner.type) as RunnerInfo[];
+  return ec2runners.filter((ec2Runner) => ec2Runner.type && !ec2Runner.orphan) as RunnerInfo[];
 }
 
 export async function scaleDown(): Promise<void> {
   githubCache.reset();
-  const scaleDownConfigs = JSON.parse(process.env.SCALE_DOWN_CONFIG) as [ScalingDownConfig];
   const environment = process.env.ENVIRONMENT;
+  const scaleDownConfigs = JSON.parse(process.env.SCALE_DOWN_CONFIG) as [ScalingDownConfig];
+
+  // first runners marked to be orphan.
+  await terminateOrphan(environment);
 
+  // next scale down idle runners with respect to config and mark potential orphans
   const ec2Runners = await listRunners(environment);
   const activeEc2RunnersCount = ec2Runners.length;
   logger.info(`Found: '${activeEc2RunnersCount}' active GitHub EC2 runner instances before clean-up.`);