From cd4ab7b0d6fa2e502429528d4c762347aa572cf1 Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Fri, 4 Jun 2021 14:08:57 -0400 Subject: [PATCH 1/7] fix(scale): Refactor Runner Type and Owner --- .../runners/src/scale-runners/runners.test.ts | 69 ++++++++----------- .../runners/src/scale-runners/runners.ts | 21 +++--- .../runners/src/scale-runners/scale-down.ts | 2 +- .../src/scale-runners/scale-up.test.ts | 44 ++++++------ .../runners/src/scale-runners/scale-up.ts | 48 ++++++------- 5 files changed, 86 insertions(+), 98 deletions(-) diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts index d3569cc463..0b500d5a72 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts @@ -1,5 +1,4 @@ -import { listRunners, RunnerInfo, createRunner } from './runners'; -import { EC2, SSM } from 'aws-sdk'; +import { listRunners, createRunner } from './runners'; const mockEC2 = { describeInstances: jest.fn(), runInstances: jest.fn() }; const mockSSM = { putParameter: jest.fn() }; @@ -8,6 +7,10 @@ jest.mock('aws-sdk', () => ({ SSM: jest.fn().mockImplementation(() => mockSSM), })); +const ORG_NAME = 'SomeAwesomeCoder'; +const REPO_NAME = `${ORG_NAME}/some-amazing-library`; +const ENVIRONMENT = 'unit-test-environment'; + describe('list instances', () => { const mockDescribeInstances = { promise: jest.fn() }; beforeEach(() => { @@ -30,8 +33,8 @@ describe('list instances', () => { LaunchTime: new Date('2020-10-11T14:48:00.000+09:00'), InstanceId: 'i-5678', Tags: [ - { Key: 'Repo', Value: 'SomeAwesomeCoder/some-amazing-library' }, - { Key: 'Org', Value: 'SomeAwesomeCoder' }, + { Key: 'Repo', Value: REPO_NAME }, + { Key: 'Org', Value: ORG_NAME }, { Key: 'Application', Value: 'github-action-runner' }, ], }, @@ -54,8 +57,8 @@ describe('list instances', () => { expect(resp).toContainEqual({ instanceId: 'i-5678', launchTime: new Date('2020-10-11T14:48:00.000+09:00'), - repo: 'SomeAwesomeCoder/some-amazing-library', - org: 'SomeAwesomeCoder', + repo: REPO_NAME, + org: ORG_NAME, }); }); @@ -65,46 +68,34 @@ describe('list instances', () => { }); it('filters instances on repo name', async () => { - await listRunners({ repoName: 'SomeAwesomeCoder/some-amazing-library' }); + await listRunners({ runnerType: 'Repo', runnerOwner: REPO_NAME }); expect(mockEC2.describeInstances).toBeCalledWith({ Filters: [ { Name: 'tag:Application', Values: ['github-action-runner'] }, { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:Repo', Values: ['SomeAwesomeCoder/some-amazing-library'] }, + { Name: 'tag:Repo', Values: [REPO_NAME] }, ], }); }); it('filters instances on org name', async () => { - await listRunners({ orgName: 'SomeAwesomeCoder' }); + await listRunners({ runnerType: 'Org', runnerOwner: ORG_NAME }); expect(mockEC2.describeInstances).toBeCalledWith({ Filters: [ { Name: 'tag:Application', Values: ['github-action-runner'] }, { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:Org', Values: ['SomeAwesomeCoder'] }, + { Name: 'tag:Org', Values: [ORG_NAME] }, ], }); }); it('filters instances on org name', async () => { - await listRunners({ environment: 'unit-test-environment' }); - expect(mockEC2.describeInstances).toBeCalledWith({ - Filters: [ - { Name: 'tag:Application', Values: ['github-action-runner'] }, - { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:Environment', Values: ['unit-test-environment'] }, - ], - }); - }); - - it('filters instances on both org name and repo name', async () => { - await listRunners({ orgName: 'SomeAwesomeCoder', repoName: 'SomeAwesomeCoder/some-amazing-library' }); + await listRunners({ environment: ENVIRONMENT }); expect(mockEC2.describeInstances).toBeCalledWith({ Filters: [ { Name: 'tag:Application', Values: ['github-action-runner'] }, { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:Repo', Values: ['SomeAwesomeCoder/some-amazing-library'] }, - { Name: 'tag:Org', Values: ['SomeAwesomeCoder'] }, + { Name: 'tag:Environment', Values: [ENVIRONMENT] }, ], }); }); @@ -132,9 +123,9 @@ describe('create runner', () => { it('calls run instances with the correct config for repo', async () => { await createRunner({ runnerConfig: 'bla', - environment: 'unit-test-env', - repoName: 'SomeAwesomeCoder/some-amazing-library', - orgName: undefined, + environment: ENVIRONMENT, + runnerType: 'Repo', + runnerOwner: REPO_NAME }); expect(mockEC2.runInstances).toBeCalledWith({ MaxCount: 1, @@ -146,7 +137,7 @@ describe('create runner', () => { ResourceType: 'instance', Tags: [ { Key: 'Application', Value: 'github-action-runner' }, - { Key: 'Repo', Value: 'SomeAwesomeCoder/some-amazing-library' }, + { Key: 'Repo', Value: REPO_NAME }, ], }, ], @@ -156,9 +147,9 @@ describe('create runner', () => { it('calls run instances with the correct config for org', async () => { await createRunner({ runnerConfig: 'bla', - environment: 'unit-test-env', - repoName: undefined, - orgName: 'SomeAwesomeCoder', + environment: ENVIRONMENT, + runnerType: 'Org', + runnerOwner: ORG_NAME, }); expect(mockEC2.runInstances).toBeCalledWith({ MaxCount: 1, @@ -170,7 +161,7 @@ describe('create runner', () => { ResourceType: 'instance', Tags: [ { Key: 'Application', Value: 'github-action-runner' }, - { Key: 'Org', Value: 'SomeAwesomeCoder' }, + { Key: 'Org', Value: ORG_NAME }, ], }, ], @@ -180,12 +171,12 @@ describe('create runner', () => { it('creates ssm parameters for each created instance', async () => { await createRunner({ runnerConfig: 'bla', - environment: 'unit-test-env', - repoName: undefined, - orgName: 'SomeAwesomeCoder', + environment: ENVIRONMENT, + runnerType: 'Org', + runnerOwner: ORG_NAME, }); expect(mockSSM.putParameter).toBeCalledWith({ - Name: 'unit-test-env-i-1234', + Name: `${ENVIRONMENT}-i-1234`, Value: 'bla', Type: 'SecureString', }); @@ -197,9 +188,9 @@ describe('create runner', () => { }); await createRunner({ runnerConfig: 'bla', - environment: 'unit-test-env', - repoName: undefined, - orgName: 'SomeAwesomeCoder', + environment: ENVIRONMENT, + runnerType: 'Org', + runnerOwner: ORG_NAME, }); expect(mockSSM.putParameter).not.toBeCalled(); }); diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.ts index d2a5c4b92d..f84bf97290 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/runners.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/runners.ts @@ -8,9 +8,9 @@ export interface RunnerInfo { } export interface ListRunnerFilters { - repoName?: string; - orgName?: string; - environment?: string; + runnerType?: 'Org' | 'Repo'; + runnerOwner?: string; + environment?: string | undefined; } export async function listRunners(filters: ListRunnerFilters | undefined = undefined): Promise { @@ -23,11 +23,8 @@ export async function listRunners(filters: ListRunnerFilters | undefined = undef if (filters.environment !== undefined) { ec2Filters.push({ Name: 'tag:Environment', Values: [filters.environment] }); } - if (filters.repoName !== undefined) { - ec2Filters.push({ Name: 'tag:Repo', Values: [filters.repoName] }); - } - if (filters.orgName !== undefined) { - ec2Filters.push({ Name: 'tag:Org', Values: [filters.orgName] }); + if (filters.runnerType && filters.runnerOwner) { + ec2Filters.push({ Name: `tag:${filters.runnerType}`, Values: [filters.runnerOwner] }); } } const runningInstances = await ec2.describeInstances({ Filters: ec2Filters }).promise(); @@ -52,8 +49,8 @@ export async function listRunners(filters: ListRunnerFilters | undefined = undef export interface RunnerInputParameters { runnerConfig: string; environment: string; - repoName?: string; - orgName?: string; + runnerType: 'Org' | 'Repo'; + runnerOwner: string; } export async function terminateRunner(runner: RunnerInfo): Promise { @@ -89,8 +86,8 @@ export async function createRunner(runnerParameters: RunnerInputParameters): Pro Tags: [ { Key: 'Application', Value: 'github-action-runner' }, { - Key: runnerParameters.orgName ? 'Org' : 'Repo', - Value: runnerParameters.orgName ? runnerParameters.orgName : runnerParameters.repoName, + Key: runnerParameters.runnerType, + Value: runnerParameters.runnerOwner }, ], }, diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts index c79bbc2b44..7d8a98a4a2 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-down.ts @@ -139,7 +139,7 @@ export async function scaleDown(): Promise { // list and sort runners, newest first. This ensure we keep the newest runners longer. const runners = ( await listRunners({ - environment: environment, + environment }) ).sort((a, b): number => { if (a.launchTime === undefined) return 1; diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts index f3bc78b633..e2f6640ad7 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts @@ -126,7 +126,8 @@ describe('scaleUp with GHES', () => { await scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', - repoName: undefined, + runnerType: 'Org', + runnerOwner: TEST_DATA.repositoryOwner }); }); @@ -174,8 +175,8 @@ describe('scaleUp with GHES', () => { expect(createRunner).toBeCalledWith({ environment: 'unit-test-environment', runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} --token 1234abcd `, - orgName: TEST_DATA.repositoryOwner, - repoName: undefined, + runnerType: 'Org', + runnerOwner: TEST_DATA.repositoryOwner, }); }); @@ -187,8 +188,8 @@ describe('scaleUp with GHES', () => { environment: 'unit-test-environment', runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} ` + `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`, - orgName: TEST_DATA.repositoryOwner, - repoName: undefined, + runnerType: 'Org', + runnerOwner: TEST_DATA.repositoryOwner, }); }); }); @@ -202,7 +203,8 @@ describe('scaleUp with GHES', () => { await scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', - repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, + runnerType: 'Repo', + runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, }); }); @@ -253,8 +255,8 @@ describe('scaleUp with GHES', () => { runnerConfig: `--url ` + `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + `--token 1234abcd --labels label1,label2`, - orgName: undefined, - repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, + runnerType: 'Repo', + runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, }); }); @@ -267,8 +269,8 @@ describe('scaleUp with GHES', () => { runnerConfig: `--url ` + `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + `--token 1234abcd --labels label1,label2`, - orgName: undefined, - repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, + runnerType: 'Repo', + runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, }); }); }); @@ -322,7 +324,8 @@ describe('scaleUp with public GH', () => { await scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', - repoName: undefined, + runnerType: 'Org', + runnerOwner: TEST_DATA.repositoryOwner }); }); @@ -345,8 +348,8 @@ describe('scaleUp with public GH', () => { expect(createRunner).toBeCalledWith({ environment: 'unit-test-environment', runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} --token 1234abcd `, - orgName: TEST_DATA.repositoryOwner, - repoName: undefined, + runnerType: 'Org', + runnerOwner: TEST_DATA.repositoryOwner }); }); @@ -358,8 +361,8 @@ describe('scaleUp with public GH', () => { environment: 'unit-test-environment', runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} ` + `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`, - orgName: TEST_DATA.repositoryOwner, - repoName: undefined, + runnerType: 'Org', + runnerOwner: TEST_DATA.repositoryOwner }); }); }); @@ -373,7 +376,8 @@ describe('scaleUp with public GH', () => { await scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', - repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, + runnerType: 'Repo', + runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, }); }); @@ -414,8 +418,8 @@ describe('scaleUp with public GH', () => { environment: 'unit-test-environment', runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + `--token 1234abcd --labels label1,label2`, - orgName: undefined, - repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, + runnerType: 'Repo', + runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, }); }); @@ -427,8 +431,8 @@ describe('scaleUp with public GH', () => { environment: 'unit-test-environment', runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + `--token 1234abcd --labels label1,label2`, - orgName: undefined, - repoName: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, + runnerType: 'Repo', + runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, }); }); }); diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts index 2cfed5bdb2..dfbbf5d10c 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts @@ -30,16 +30,16 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage const githubClient = await createOctoClient(ghAuth.token, ghesApiUrl); installationId = enableOrgLevel ? ( - await githubClient.apps.getOrgInstallation({ - org: payload.repositoryOwner, - }) - ).data.id + await githubClient.apps.getOrgInstallation({ + org: payload.repositoryOwner, + }) + ).data.id : ( - await githubClient.apps.getRepoInstallation({ - owner: payload.repositoryOwner, - repo: payload.repositoryName, - }) - ).data.id; + await githubClient.apps.getRepoInstallation({ + owner: payload.repositoryOwner, + repo: payload.repositoryName, + }) + ).data.id; } const ghAuth = await createGithubAuth(installationId, 'installation', ghesApiUrl); @@ -51,20 +51,17 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage repo: payload.repositoryName, }); - const repoName = enableOrgLevel ? undefined : `${payload.repositoryOwner}/${payload.repositoryName}`; - const orgName = enableOrgLevel ? payload.repositoryOwner : undefined; + const runnerType = enableOrgLevel ? 'Org' : 'Repo'; + const runnerOwner = enableOrgLevel ? payload.repositoryOwner : `${payload.repositoryOwner}/${payload.repositoryName}`; if (checkRun.data.status === 'queued') { const currentRunners = await listRunners({ - environment: environment, - repoName: repoName, + environment, + runnerType, + runnerOwner }); console.info( - `${ - enableOrgLevel - ? `Organization ${payload.repositoryOwner}` - : `Repo ${payload.repositoryOwner}/${payload.repositoryName}` - } has ${currentRunners.length}/${maximumRunners} runners`, + `${runnerType} ${runnerOwner} has ${currentRunners.length}/${maximumRunners} runners`, ); if (currentRunners.length < maximumRunners) { @@ -72,9 +69,9 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage const registrationToken = enableOrgLevel ? await githubInstallationClient.actions.createRegistrationTokenForOrg({ org: payload.repositoryOwner }) : await githubInstallationClient.actions.createRegistrationTokenForRepo({ - owner: payload.repositoryOwner, - repo: payload.repositoryName, - }); + owner: payload.repositoryOwner, + repo: payload.repositoryName, + }); const token = registrationToken.data.token; const labelsArgument = runnerExtraLabels !== undefined ? `--labels ${runnerExtraLabels}` : ''; @@ -83,11 +80,10 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage await createRunner({ environment: environment, runnerConfig: enableOrgLevel - ? `--url ${configBaseUrl}/${payload.repositoryOwner} --token ${token} ${labelsArgument}${runnerGroupArgument}` - : `--url ${configBaseUrl}/${payload.repositoryOwner}/${payload.repositoryName} ` + - `--token ${token} ${labelsArgument}`, - orgName: orgName, - repoName: repoName, + ? `--url ${configBaseUrl}/${runnerOwner} --token ${token} ${labelsArgument}${runnerGroupArgument}` + : `--url ${configBaseUrl}/${runnerOwner} --token ${token} ${labelsArgument}`, + runnerType, + runnerOwner, }); } else { console.info('No runner will be created, maximum number of runners reached.'); From 80ebe92667d25a2be854f82b82784ae2d974397b Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Fri, 4 Jun 2021 14:14:49 -0400 Subject: [PATCH 2/7] `environment` should not be optional --- .../runners/lambdas/runners/src/scale-runners/runners.test.ts | 4 ++-- modules/runners/lambdas/runners/src/scale-runners/runners.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts index 0b500d5a72..0c3cab19c8 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts @@ -68,7 +68,7 @@ describe('list instances', () => { }); it('filters instances on repo name', async () => { - await listRunners({ runnerType: 'Repo', runnerOwner: REPO_NAME }); + await listRunners({ runnerType: 'Repo', runnerOwner: REPO_NAME, environment: undefined }); expect(mockEC2.describeInstances).toBeCalledWith({ Filters: [ { Name: 'tag:Application', Values: ['github-action-runner'] }, @@ -79,7 +79,7 @@ describe('list instances', () => { }); it('filters instances on org name', async () => { - await listRunners({ runnerType: 'Org', runnerOwner: ORG_NAME }); + await listRunners({ runnerType: 'Org', runnerOwner: ORG_NAME, environment: undefined }); expect(mockEC2.describeInstances).toBeCalledWith({ Filters: [ { Name: 'tag:Application', Values: ['github-action-runner'] }, diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.ts index f84bf97290..e5e9b3f544 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/runners.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/runners.ts @@ -10,7 +10,7 @@ export interface RunnerInfo { export interface ListRunnerFilters { runnerType?: 'Org' | 'Repo'; runnerOwner?: string; - environment?: string | undefined; + environment: string | undefined; } export async function listRunners(filters: ListRunnerFilters | undefined = undefined): Promise { From b9c4600d181f436f9467c88499beec0a414046fd Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Wed, 9 Jun 2021 14:33:27 -0400 Subject: [PATCH 3/7] feat(runners): Support Multiple Instance Types --- main.tf | 1 + .../runners/src/scale-runners/runners.test.ts | 23 +++-- .../runners/src/scale-runners/runners.ts | 78 +++++++++-------- .../src/scale-runners/scale-up.test.ts | 84 ++++++++++--------- .../runners/src/scale-runners/scale-up.ts | 31 +++++-- modules/runners/main.tf | 10 ++- modules/runners/scale-up.tf | 3 +- modules/runners/variables.tf | 6 ++ outputs.tf | 6 +- variables.tf | 6 ++ 10 files changed, 147 insertions(+), 101 deletions(-) diff --git a/main.tf b/main.tf index 5668117757..4d14d75c8a 100644 --- a/main.tf +++ b/main.tf @@ -68,6 +68,7 @@ module "runners" { s3_location_runner_binaries = local.s3_action_runner_url instance_type = var.instance_type + instance_types = var.instance_types market_options = var.market_options block_device_mappings = var.block_device_mappings diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts index 0c3cab19c8..4436a1b63f 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts @@ -7,6 +7,7 @@ jest.mock('aws-sdk', () => ({ SSM: jest.fn().mockImplementation(() => mockSSM), })); +const LAUNCH_TEMPLATE = 'lt-1'; const ORG_NAME = 'SomeAwesomeCoder'; const REPO_NAME = `${ORG_NAME}/some-amazing-library`; const ENVIRONMENT = 'unit-test-environment'; @@ -115,22 +116,20 @@ describe('create runner', () => { ], }); mockSSM.putParameter.mockImplementation(() => mockPutParameter); - process.env.LAUNCH_TEMPLATE_NAME = 'launch-template-name'; - process.env.LAUNCH_TEMPLATE_VERSION = '1'; process.env.SUBNET_IDS = 'sub-1234'; }); it('calls run instances with the correct config for repo', async () => { await createRunner({ - runnerConfig: 'bla', + runnerServiceConfig: 'bla', environment: ENVIRONMENT, runnerType: 'Repo', runnerOwner: REPO_NAME - }); + }, LAUNCH_TEMPLATE); expect(mockEC2.runInstances).toBeCalledWith({ MaxCount: 1, MinCount: 1, - LaunchTemplate: { LaunchTemplateName: 'launch-template-name', Version: '1' }, + LaunchTemplate: { LaunchTemplateName: LAUNCH_TEMPLATE, Version: '$Default' }, SubnetId: 'sub-1234', TagSpecifications: [ { @@ -146,15 +145,15 @@ describe('create runner', () => { it('calls run instances with the correct config for org', async () => { await createRunner({ - runnerConfig: 'bla', + runnerServiceConfig: 'bla', environment: ENVIRONMENT, runnerType: 'Org', runnerOwner: ORG_NAME, - }); + }, LAUNCH_TEMPLATE); expect(mockEC2.runInstances).toBeCalledWith({ MaxCount: 1, MinCount: 1, - LaunchTemplate: { LaunchTemplateName: 'launch-template-name', Version: '1' }, + LaunchTemplate: { LaunchTemplateName: LAUNCH_TEMPLATE, Version: '$Default' }, SubnetId: 'sub-1234', TagSpecifications: [ { @@ -170,11 +169,11 @@ describe('create runner', () => { it('creates ssm parameters for each created instance', async () => { await createRunner({ - runnerConfig: 'bla', + runnerServiceConfig: 'bla', environment: ENVIRONMENT, runnerType: 'Org', runnerOwner: ORG_NAME, - }); + }, LAUNCH_TEMPLATE); expect(mockSSM.putParameter).toBeCalledWith({ Name: `${ENVIRONMENT}-i-1234`, Value: 'bla', @@ -187,11 +186,11 @@ describe('create runner', () => { Instances: [], }); await createRunner({ - runnerConfig: 'bla', + runnerServiceConfig: 'bla', environment: ENVIRONMENT, runnerType: 'Org', runnerOwner: ORG_NAME, - }); + }, LAUNCH_TEMPLATE); expect(mockSSM.putParameter).not.toBeCalled(); }); }); diff --git a/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/modules/runners/lambdas/runners/src/scale-runners/runners.ts index e5e9b3f544..11b1058919 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/runners.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/runners.ts @@ -13,6 +13,13 @@ export interface ListRunnerFilters { environment: string | undefined; } +export interface RunnerInputParameters { + runnerServiceConfig: string; + environment: string; + runnerType: 'Org' | 'Repo'; + runnerOwner: string; +} + export async function listRunners(filters: ListRunnerFilters | undefined = undefined): Promise { const ec2 = new EC2(); const ec2Filters = [ @@ -46,13 +53,6 @@ export async function listRunners(filters: ListRunnerFilters | undefined = undef return runners; } -export interface RunnerInputParameters { - runnerConfig: string; - environment: string; - runnerType: 'Org' | 'Repo'; - runnerOwner: string; -} - export async function terminateRunner(runner: RunnerInfo): Promise { const ec2 = new EC2(); await ec2 @@ -63,47 +63,53 @@ export async function terminateRunner(runner: RunnerInfo): Promise { console.debug('Runner terminated.' + runner.instanceId); } -export async function createRunner(runnerParameters: RunnerInputParameters): Promise { - const launchTemplateName = process.env.LAUNCH_TEMPLATE_NAME as string; - const launchTemplateVersion = process.env.LAUNCH_TEMPLATE_VERSION as string; - - const subnets = (process.env.SUBNET_IDS as string).split(','); - const randomSubnet = subnets[Math.floor(Math.random() * subnets.length)]; +export async function createRunner(runnerParameters: RunnerInputParameters, launchTemplateName: string): Promise { console.debug('Runner configuration: ' + JSON.stringify(runnerParameters)); const ec2 = new EC2(); const runInstancesResponse = await ec2 - .runInstances({ - MaxCount: 1, - MinCount: 1, - LaunchTemplate: { - LaunchTemplateName: launchTemplateName, - Version: launchTemplateVersion, - }, - SubnetId: randomSubnet, - TagSpecifications: [ - { - ResourceType: 'instance', - Tags: [ - { Key: 'Application', Value: 'github-action-runner' }, - { - Key: runnerParameters.runnerType, - Value: runnerParameters.runnerOwner - }, - ], - }, - ], - }) + .runInstances(getInstanceParams(launchTemplateName, runnerParameters)) .promise(); console.info('Created instance(s): ', runInstancesResponse.Instances?.map((i) => i.InstanceId).join(',')); - const ssm = new SSM(); runInstancesResponse.Instances?.forEach(async (i: EC2.Instance) => { await ssm .putParameter({ Name: runnerParameters.environment + '-' + (i.InstanceId as string), - Value: runnerParameters.runnerConfig, + Value: runnerParameters.runnerServiceConfig, Type: 'SecureString', }) .promise(); }); } + +function getInstanceParams( + launchTemplateName: string, + runnerParameters: RunnerInputParameters +): EC2.RunInstancesRequest { + return { + MaxCount: 1, + MinCount: 1, + LaunchTemplate: { + LaunchTemplateName: launchTemplateName, + Version: '$Default', + }, + SubnetId: getSubnet(), + TagSpecifications: [ + { + ResourceType: 'instance', + Tags: [ + { Key: 'Application', Value: 'github-action-runner' }, + { + Key: runnerParameters.runnerType, + Value: runnerParameters.runnerOwner + }, + ], + }, + ], + }; +} + +function getSubnet(): string { + const subnets = (process.env.SUBNET_IDS as string).split(','); + return subnets[Math.floor(Math.random() * subnets.length)]; +} diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts index e2f6640ad7..26f7d83e5c 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts @@ -1,6 +1,7 @@ import { mocked } from 'ts-jest/utils'; -import { ActionRequestMessage, scaleUp } from './scale-up'; -import { listRunners, createRunner } from './runners'; +import { ActionRequestMessage, scaleUp, createRunnerLoop } from './scale-up'; +import * as scaleUpModule from './scale-up'; +import { listRunners, createRunner, RunnerInputParameters } from './runners'; import * as ghAuth from './gh-auth'; import nock from 'nock'; @@ -40,8 +41,17 @@ const TEST_DATA_WITHOUT_INSTALL_ID: ActionRequestMessage = { installationId: 0, }; +const LAUNCH_TEMPLATE = 'lt-1'; + const cleanEnv = process.env; +const expectedRunnerParams: RunnerInputParameters = { + environment: 'unit-test-environment', + runnerServiceConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} --token 1234abcd `, + runnerType: 'Org', + runnerOwner: TEST_DATA.repositoryOwner +}; + beforeEach(() => { nock.disableNetConnect(); jest.resetModules(); @@ -53,6 +63,7 @@ beforeEach(() => { process.env.GITHUB_APP_CLIENT_SECRET = 'TEST_CLIENT_SECRET'; process.env.RUNNERS_MAXIMUM_COUNT = '3'; process.env.ENVIRONMENT = 'unit-test-environment'; + process.env.LAUNCH_TEMPLATE_NAME = 'lt-1,lt-2'; mockOctokit.checks.get.mockImplementation(() => ({ data: { @@ -171,26 +182,26 @@ describe('scaleUp with GHES', () => { }); it('creates a runner with correct config', async () => { + //const createRunnerLoopSpy = jest.spyOn(scaleUpModule, 'createRunnerLoop'); await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} --token 1234abcd `, - runnerType: 'Org', - runnerOwner: TEST_DATA.repositoryOwner, - }); + //expect(createRunnerLoopSpy).toBeCalledWith(expectedRunnerParams); + expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); }); it('creates a runner with labels in s specific group', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; process.env.RUNNER_GROUP_NAME = 'TEST_GROUP'; await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} ` + - `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`, - runnerType: 'Org', - runnerOwner: TEST_DATA.repositoryOwner, - }); + expectedRunnerParams.runnerServiceConfig = `--url ` + + `https://github.enterprise.something/${TEST_DATA.repositoryOwner} ` + + `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`; + expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); + }); + + it('attempts next launch template if first fails', async () => { + const createRunnerLoopSpy = jest.spyOn(scaleUpModule, 'createRunnerLoop'); + await scaleUp('aws:sqs', TEST_DATA); + expect(createRunnerLoopSpy).toBeCalledWith(expectedRunnerParams); }); }); @@ -250,28 +261,23 @@ describe('scaleUp with GHES', () => { it('creates a runner with correct config and labels', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerConfig: `--url ` + - `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + - `--token 1234abcd --labels label1,label2`, - runnerType: 'Repo', - runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, - }); + expectedRunnerParams.runnerServiceConfig = `--url ` + + `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + + `--token 1234abcd --labels label1,label2`; + expectedRunnerParams.runnerType = 'Repo'; + expectedRunnerParams.runnerOwner = `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`; + expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); }); it('creates a runner and ensure the group argument is ignored', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; process.env.RUNNER_GROUP_NAME = 'TEST_GROUP_IGNORED'; await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerConfig: `--url ` + - `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + - `--token 1234abcd --labels label1,label2`, - runnerType: 'Repo', - runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, - }); + expectedRunnerParams.runnerServiceConfig = `--url ` + + `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + + `--token 1234abcd --labels label1,label2`; + expectedRunnerParams.runnerOwner = `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`; + expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); }); }); }); @@ -347,10 +353,10 @@ describe('scaleUp with public GH', () => { await scaleUp('aws:sqs', TEST_DATA); expect(createRunner).toBeCalledWith({ environment: 'unit-test-environment', - runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} --token 1234abcd `, + runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} --token 1234abcd `, runnerType: 'Org', runnerOwner: TEST_DATA.repositoryOwner - }); + }, LAUNCH_TEMPLATE); }); it('creates a runner with labels in s specific group', async () => { @@ -359,11 +365,11 @@ describe('scaleUp with public GH', () => { await scaleUp('aws:sqs', TEST_DATA); expect(createRunner).toBeCalledWith({ environment: 'unit-test-environment', - runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} ` + + runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} ` + `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`, runnerType: 'Org', runnerOwner: TEST_DATA.repositoryOwner - }); + }, LAUNCH_TEMPLATE); }); }); @@ -416,11 +422,11 @@ describe('scaleUp with public GH', () => { await scaleUp('aws:sqs', TEST_DATA); expect(createRunner).toBeCalledWith({ environment: 'unit-test-environment', - runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + + runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + `--token 1234abcd --labels label1,label2`, runnerType: 'Repo', runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, - }); + }, LAUNCH_TEMPLATE); }); it('creates a runner and ensure the group argument is ignored', async () => { @@ -429,11 +435,11 @@ describe('scaleUp with public GH', () => { await scaleUp('aws:sqs', TEST_DATA); expect(createRunner).toBeCalledWith({ environment: 'unit-test-environment', - runnerConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + + runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + `--token 1234abcd --labels label1,label2`, runnerType: 'Repo', runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, - }); + }, LAUNCH_TEMPLATE); }); }); }); diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts index dfbbf5d10c..f9c16db2bc 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts @@ -1,4 +1,4 @@ -import { listRunners, createRunner } from './runners'; +import { listRunners, createRunner, RunnerInputParameters } from './runners'; import { createOctoClient, createGithubAuth } from './gh-auth'; import yn from 'yn'; @@ -77,16 +77,33 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage const labelsArgument = runnerExtraLabels !== undefined ? `--labels ${runnerExtraLabels}` : ''; const runnerGroupArgument = runnerGroup !== undefined ? ` --runnergroup ${runnerGroup}` : ''; const configBaseUrl = ghesBaseUrl ? ghesBaseUrl : 'https://github.com'; - await createRunner({ - environment: environment, - runnerConfig: enableOrgLevel - ? `--url ${configBaseUrl}/${runnerOwner} --token ${token} ${labelsArgument}${runnerGroupArgument}` - : `--url ${configBaseUrl}/${runnerOwner} --token ${token} ${labelsArgument}`, - runnerType, + + await createRunnerLoop({ + environment, + runnerServiceConfig: enableOrgLevel + ? `--url ${configBaseUrl}/${payload.repositoryOwner} --token ${token} ${labelsArgument}${runnerGroupArgument}` + : `--url ${configBaseUrl}/${payload.repositoryOwner}/${payload.repositoryName} ` + + `--token ${token} ${labelsArgument}`, runnerOwner, + runnerType + }); } else { console.info('No runner will be created, maximum number of runners reached.'); } } }; + +export async function createRunnerLoop(runnerParameters: RunnerInputParameters): Promise { + const launchTemplateNames = process.env.LAUNCH_TEMPLATE_NAME?.split(',') as string[]; + for (const launchTemplateName of launchTemplateNames) { + console.info(`Attempting to launch instance using ${launchTemplateName}.`); + try { + await createRunner(runnerParameters, launchTemplateName); + break; + } catch (error) { + console.error(error.message); + throw error; + } + } +} diff --git a/modules/runners/main.tf b/modules/runners/main.tf index 353870141c..ac742d8d4b 100644 --- a/modules/runners/main.tf +++ b/modules/runners/main.tf @@ -17,6 +17,8 @@ locals { userdata_template = var.userdata_template == null ? "${path.module}/templates/user-data.sh" : var.userdata_template userdata_arm_patch = "${path.module}/templates/arm-runner-patch.tpl" userdata_install_config_runner = "${path.module}/templates/install-config-runner.sh" + + instance_types = var.instance_types == null ? [var.instance_type] : var.instance_types } data "aws_ami" "runner" { @@ -34,7 +36,9 @@ data "aws_ami" "runner" { } resource "aws_launch_template" "runner" { - name = "${var.environment}-action-runner" + for_each = local.instance_types + + name = "${var.environment}-action-runner-${each.value}" dynamic "block_device_mappings" { for_each = [var.block_device_mappings] @@ -62,7 +66,7 @@ resource "aws_launch_template" "runner" { } image_id = data.aws_ami.runner.id - instance_type = var.instance_type + instance_type = each.value key_name = var.key_name vpc_security_group_ids = compact(concat( @@ -102,6 +106,8 @@ resource "aws_launch_template" "runner" { })) tags = local.tags + + update_default_version = true } locals { diff --git a/modules/runners/scale-up.tf b/modules/runners/scale-up.tf index 5418880662..37c40d7d56 100644 --- a/modules/runners/scale-up.tf +++ b/modules/runners/scale-up.tf @@ -39,8 +39,7 @@ resource "aws_lambda_function" "scale_up" { RUNNER_EXTRA_LABELS = var.runner_extra_labels RUNNER_GROUP_NAME = var.runner_group_name RUNNERS_MAXIMUM_COUNT = var.runners_maximum_count - LAUNCH_TEMPLATE_NAME = aws_launch_template.runner.name - LAUNCH_TEMPLATE_VERSION = aws_launch_template.runner.latest_version + LAUNCH_TEMPLATE_NAME = join(",", [for template in aws_launch_template.runner : template.name]) SUBNET_IDS = join(",", var.subnet_ids) } } diff --git a/modules/runners/variables.tf b/modules/runners/variables.tf index f1ca68aa17..c72a847916 100644 --- a/modules/runners/variables.tf +++ b/modules/runners/variables.tf @@ -63,6 +63,12 @@ variable "instance_type" { default = "m5.large" } +variable "instance_types" { + description = "List of instance types for the action runner." + type = set(string) + default = null +} + variable "ami_filter" { description = "List of maps used to create the AMI filter for the action runner AMI." type = map(list(string)) diff --git a/outputs.tf b/outputs.tf index 7b39a8d8b3..3cb4684a37 100644 --- a/outputs.tf +++ b/outputs.tf @@ -1,8 +1,8 @@ output "runners" { value = { - launch_template_name = module.runners.launch_template.name - launch_template_id = module.runners.launch_template.id - launch_template_version = module.runners.launch_template.latest_version + launch_template_name = [for template in module.runners.launch_template : template.name] + launch_template_id = [for template in module.runners.launch_template : template.id] + launch_template_version = [for template in module.runners.launch_template : template.latest_version] lambda_up = module.runners.lambda_scale_up lambda_down = module.runners.lambda_scale_down role_runner = module.runners.role_runner diff --git a/variables.tf b/variables.tf index 4f455467fb..a857054501 100644 --- a/variables.tf +++ b/variables.tf @@ -354,3 +354,9 @@ variable "volume_size" { type = number default = 30 } + +variable "instance_types" { + description = "List of instance types for the action runner." + type = set(string) + default = null +} From e2008c0e68aa4ea3a432a901eb766f954b14a31a Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Mon, 14 Jun 2021 12:24:24 -0400 Subject: [PATCH 4/7] Correcting failed launch logic --- .../runners/lambdas/runners/src/scale-runners/scale-up.ts | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts index f9c16db2bc..07f9f05029 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts @@ -96,14 +96,18 @@ export const scaleUp = async (eventSource: string, payload: ActionRequestMessage export async function createRunnerLoop(runnerParameters: RunnerInputParameters): Promise { const launchTemplateNames = process.env.LAUNCH_TEMPLATE_NAME?.split(',') as string[]; + let launched = false; for (const launchTemplateName of launchTemplateNames) { console.info(`Attempting to launch instance using ${launchTemplateName}.`); try { await createRunner(runnerParameters, launchTemplateName); + launched = true; break; } catch (error) { - console.error(error.message); - throw error; + console.error(error); } } + if (launched == false) { + throw Error('All launch templates failed'); + } } From c8633f8215655f1199869d4538869f676e707c40 Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Mon, 14 Jun 2021 12:24:51 -0400 Subject: [PATCH 5/7] Updating tests --- .../src/scale-runners/scale-up.test.ts | 176 ++++++++++-------- 1 file changed, 99 insertions(+), 77 deletions(-) diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts index 26f7d83e5c..5525201dd6 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts @@ -1,5 +1,4 @@ import { mocked } from 'ts-jest/utils'; -import { ActionRequestMessage, scaleUp, createRunnerLoop } from './scale-up'; import * as scaleUpModule from './scale-up'; import { listRunners, createRunner, RunnerInputParameters } from './runners'; import * as ghAuth from './gh-auth'; @@ -25,7 +24,7 @@ jest.mock('@octokit/rest', () => ({ jest.mock('./runners'); -const TEST_DATA: ActionRequestMessage = { +const TEST_DATA: scaleUpModule.ActionRequestMessage = { id: 1, eventType: 'check_run', repositoryName: 'hello-world', @@ -33,7 +32,7 @@ const TEST_DATA: ActionRequestMessage = { installationId: 2, }; -const TEST_DATA_WITHOUT_INSTALL_ID: ActionRequestMessage = { +const TEST_DATA_WITHOUT_INSTALL_ID: scaleUpModule.ActionRequestMessage = { id: 3, eventType: 'check_run', repositoryName: 'hello-world', @@ -45,12 +44,13 @@ const LAUNCH_TEMPLATE = 'lt-1'; const cleanEnv = process.env; -const expectedRunnerParams: RunnerInputParameters = { +const EXPECTED_RUNNER_PARAMS: RunnerInputParameters = { environment: 'unit-test-environment', runnerServiceConfig: `--url https://github.enterprise.something/${TEST_DATA.repositoryOwner} --token 1234abcd `, runnerType: 'Org', runnerOwner: TEST_DATA.repositoryOwner }; +let expectedRunnerParams: RunnerInputParameters; beforeEach(() => { nock.disableNetConnect(); @@ -108,11 +108,11 @@ describe('scaleUp with GHES', () => { it('ignores non-sqs events', async () => { expect.assertions(1); - expect(scaleUp('aws:s3', TEST_DATA)).rejects.toEqual(Error('Cannot handle non-SQS events!')); + expect(scaleUpModule.scaleUp('aws:s3', TEST_DATA)).rejects.toEqual(Error('Cannot handle non-SQS events!')); }); it('checks queued workflows', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.checks.get).toBeCalledWith({ check_run_id: TEST_DATA.id, owner: TEST_DATA.repositoryOwner, @@ -124,17 +124,18 @@ describe('scaleUp with GHES', () => { mockOctokit.checks.get.mockImplementation(() => ({ data: { total_count: 0, runners: [] }, })); - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(listRunners).not.toBeCalled(); }); describe('on org level', () => { beforeEach(() => { process.env.ENABLE_ORGANIZATION_RUNNERS = 'true'; + expectedRunnerParams = { ...EXPECTED_RUNNER_PARAMS }; }); it('gets the current org level runners', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', runnerType: 'Org', @@ -144,12 +145,12 @@ describe('scaleUp with GHES', () => { it('does not create a token when maximum runners has been reached', async () => { process.env.RUNNERS_MAXIMUM_COUNT = '1'; - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForOrg).not.toBeCalled(); }); it('creates a token when maximum runners has not been reached', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForOrg).toBeCalled(); expect(mockOctokit.actions.createRegistrationTokenForOrg).toBeCalledWith({ org: TEST_DATA.repositoryOwner, @@ -158,7 +159,7 @@ describe('scaleUp with GHES', () => { it('does not retrieve installation id if already set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.apps.getOrgInstallation).not.toBeCalled(); expect(mockOctokit.apps.getRepoInstallation).not.toBeCalled(); expect(spy).toBeCalledWith( @@ -170,7 +171,7 @@ describe('scaleUp with GHES', () => { it('retrieves installation id if not set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); expect(mockOctokit.apps.getRepoInstallation).not.toBeCalled(); expect(spy).toHaveBeenNthCalledWith(1, undefined, 'app', "https://github.enterprise.something/api/v3"); expect(spy).toHaveBeenNthCalledWith( @@ -182,36 +183,42 @@ describe('scaleUp with GHES', () => { }); it('creates a runner with correct config', async () => { - //const createRunnerLoopSpy = jest.spyOn(scaleUpModule, 'createRunnerLoop'); - await scaleUp('aws:sqs', TEST_DATA); - //expect(createRunnerLoopSpy).toBeCalledWith(expectedRunnerParams); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); }); - it('creates a runner with labels in s specific group', async () => { + it('creates a runner with labels in a specific group', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; process.env.RUNNER_GROUP_NAME = 'TEST_GROUP'; - await scaleUp('aws:sqs', TEST_DATA); - expectedRunnerParams.runnerServiceConfig = `--url ` + - `https://github.enterprise.something/${TEST_DATA.repositoryOwner} ` + - `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`; + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expectedRunnerParams.runnerServiceConfig = expectedRunnerParams.runnerServiceConfig + + `--labels label1,label2 --runnergroup TEST_GROUP`; expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); }); it('attempts next launch template if first fails', async () => { - const createRunnerLoopSpy = jest.spyOn(scaleUpModule, 'createRunnerLoop'); - await scaleUp('aws:sqs', TEST_DATA); - expect(createRunnerLoopSpy).toBeCalledWith(expectedRunnerParams); + const mockCreateRunners = mocked(createRunner); + mockCreateRunners.mockRejectedValueOnce(new Error('no capactiy')); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expect(createRunner).toBeCalledTimes(2); + expect(createRunner).toHaveBeenNthCalledWith(1, expectedRunnerParams, 'lt-1'); + expect(createRunner).toHaveBeenNthCalledWith(2, expectedRunnerParams, 'lt-2'); }); }); describe('on repo level', () => { beforeEach(() => { process.env.ENABLE_ORGANIZATION_RUNNERS = 'false'; + expectedRunnerParams = { ...EXPECTED_RUNNER_PARAMS }; + expectedRunnerParams.runnerType = 'Repo'; + expectedRunnerParams.runnerOwner = `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`; + expectedRunnerParams.runnerServiceConfig = `--url ` + + `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + + `--token 1234abcd `; }); it('gets the current repo level runners', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', runnerType: 'Repo', @@ -221,12 +228,12 @@ describe('scaleUp with GHES', () => { it('does not create a token when maximum runners has been reached', async () => { process.env.RUNNERS_MAXIMUM_COUNT = '1'; - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForRepo).not.toBeCalled(); }); it('creates a token when maximum runners has not been reached', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForRepo).toBeCalledWith({ owner: TEST_DATA.repositoryOwner, repo: TEST_DATA.repositoryName, @@ -235,7 +242,7 @@ describe('scaleUp with GHES', () => { it('does not retrieve installation id if already set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.apps.getOrgInstallation).not.toBeCalled(); expect(mockOctokit.apps.getRepoInstallation).not.toBeCalled(); expect(spy).toBeCalledWith( @@ -247,7 +254,7 @@ describe('scaleUp with GHES', () => { it('retrieves installation id if not set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); expect(mockOctokit.apps.getOrgInstallation).not.toBeCalled(); expect(spy).toHaveBeenNthCalledWith(1, undefined, 'app', "https://github.enterprise.something/api/v3"); expect(spy).toHaveBeenNthCalledWith( @@ -260,10 +267,8 @@ describe('scaleUp with GHES', () => { it('creates a runner with correct config and labels', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; - await scaleUp('aws:sqs', TEST_DATA); - expectedRunnerParams.runnerServiceConfig = `--url ` + - `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + - `--token 1234abcd --labels label1,label2`; + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expectedRunnerParams.runnerServiceConfig = expectedRunnerParams.runnerServiceConfig + `--labels label1,label2`; expectedRunnerParams.runnerType = 'Repo'; expectedRunnerParams.runnerOwner = `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`; expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); @@ -272,24 +277,33 @@ describe('scaleUp with GHES', () => { it('creates a runner and ensure the group argument is ignored', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; process.env.RUNNER_GROUP_NAME = 'TEST_GROUP_IGNORED'; - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expectedRunnerParams.runnerServiceConfig = `--url ` + `https://github.enterprise.something/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + `--token 1234abcd --labels label1,label2`; expectedRunnerParams.runnerOwner = `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`; expect(createRunner).toBeCalledWith(expectedRunnerParams, 'lt-1'); }); + + it('attempts next launch template if first fails', async () => { + const mockCreateRunners = mocked(createRunner); + mockCreateRunners.mockRejectedValueOnce(new Error('no capactiy')); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expect(createRunner).toBeCalledTimes(2); + expect(createRunner).toHaveBeenNthCalledWith(1, expectedRunnerParams, 'lt-1'); + expect(createRunner).toHaveBeenNthCalledWith(2, expectedRunnerParams, 'lt-2'); + }); }); }); describe('scaleUp with public GH', () => { it('ignores non-sqs events', async () => { expect.assertions(1); - expect(scaleUp('aws:s3', TEST_DATA)).rejects.toEqual(Error('Cannot handle non-SQS events!')); + expect(scaleUpModule.scaleUp('aws:s3', TEST_DATA)).rejects.toEqual(Error('Cannot handle non-SQS events!')); }); it('checks queued workflows', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.checks.get).toBeCalledWith({ check_run_id: TEST_DATA.id, owner: TEST_DATA.repositoryOwner, @@ -299,7 +313,7 @@ describe('scaleUp with public GH', () => { it('does not retrieve installation id if already set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.apps.getOrgInstallation).not.toBeCalled(); expect(mockOctokit.apps.getRepoInstallation).not.toBeCalled(); expect(spy).toBeCalledWith(TEST_DATA.installationId, 'installation', ""); @@ -307,7 +321,7 @@ describe('scaleUp with public GH', () => { it('retrieves installation id if not set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); expect(mockOctokit.apps.getRepoInstallation).not.toBeCalled(); expect(spy).toHaveBeenNthCalledWith(1, undefined, 'app', ""); expect(spy).toHaveBeenNthCalledWith(2, TEST_DATA.installationId, 'installation', ""); @@ -317,17 +331,20 @@ describe('scaleUp with public GH', () => { mockOctokit.checks.get.mockImplementation(() => ({ data: { status: 'completed' }, })); - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(listRunners).not.toBeCalled(); }); describe('on org level', () => { beforeEach(() => { process.env.ENABLE_ORGANIZATION_RUNNERS = 'true'; + expectedRunnerParams = { ...EXPECTED_RUNNER_PARAMS }; + expectedRunnerParams.runnerServiceConfig = + `--url https://github.com/${TEST_DATA.repositoryOwner} --token 1234abcd `; }); it('gets the current org level runners', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', runnerType: 'Org', @@ -337,12 +354,12 @@ describe('scaleUp with public GH', () => { it('does not create a token when maximum runners has been reached', async () => { process.env.RUNNERS_MAXIMUM_COUNT = '1'; - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForOrg).not.toBeCalled(); }); it('creates a token when maximum runners has not been reached', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForOrg).toBeCalled(); expect(mockOctokit.actions.createRegistrationTokenForOrg).toBeCalledWith({ org: TEST_DATA.repositoryOwner, @@ -350,36 +367,42 @@ describe('scaleUp with public GH', () => { }); it('creates a runner with correct config', async () => { - await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} --token 1234abcd `, - runnerType: 'Org', - runnerOwner: TEST_DATA.repositoryOwner - }, LAUNCH_TEMPLATE); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expect(createRunner).toBeCalledWith(expectedRunnerParams, LAUNCH_TEMPLATE); }); it('creates a runner with labels in s specific group', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; process.env.RUNNER_GROUP_NAME = 'TEST_GROUP'; - await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner} ` + - `--token 1234abcd --labels label1,label2 --runnergroup TEST_GROUP`, - runnerType: 'Org', - runnerOwner: TEST_DATA.repositoryOwner - }, LAUNCH_TEMPLATE); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expectedRunnerParams.runnerServiceConfig = + expectedRunnerParams.runnerServiceConfig + `--labels label1,label2 --runnergroup TEST_GROUP`; + expect(createRunner).toBeCalledWith(expectedRunnerParams, LAUNCH_TEMPLATE); + }); + + it('attempts next launch template if first fails', async () => { + const mockCreateRunners = mocked(createRunner); + mockCreateRunners.mockRejectedValueOnce(new Error('no capactiy')); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expect(createRunner).toBeCalledTimes(2); + expect(createRunner).toHaveBeenNthCalledWith(1, expectedRunnerParams, 'lt-1'); + expect(createRunner).toHaveBeenNthCalledWith(2, expectedRunnerParams, 'lt-2'); }); }); describe('on repo level', () => { beforeEach(() => { process.env.ENABLE_ORGANIZATION_RUNNERS = 'false'; + expectedRunnerParams = { ...EXPECTED_RUNNER_PARAMS }; + expectedRunnerParams.runnerType = 'Repo'; + expectedRunnerParams.runnerOwner = `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`; + expectedRunnerParams.runnerServiceConfig = `--url ` + + `https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + + `--token 1234abcd `; }); it('gets the current repo level runners', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(listRunners).toBeCalledWith({ environment: 'unit-test-environment', runnerType: 'Repo', @@ -389,12 +412,12 @@ describe('scaleUp with public GH', () => { it('does not create a token when maximum runners has been reached', async () => { process.env.RUNNERS_MAXIMUM_COUNT = '1'; - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForRepo).not.toBeCalled(); }); it('creates a token when maximum runners has not been reached', async () => { - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.actions.createRegistrationTokenForRepo).toBeCalledWith({ owner: TEST_DATA.repositoryOwner, repo: TEST_DATA.repositoryName, @@ -403,7 +426,7 @@ describe('scaleUp with public GH', () => { it('does not retrieve installation id if already set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); expect(mockOctokit.apps.getOrgInstallation).not.toBeCalled(); expect(mockOctokit.apps.getRepoInstallation).not.toBeCalled(); expect(spy).toBeCalledWith(TEST_DATA.installationId, 'installation', ""); @@ -411,7 +434,7 @@ describe('scaleUp with public GH', () => { it('retrieves installation id if not set', async () => { const spy = jest.spyOn(ghAuth, 'createGithubAuth'); - await scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA_WITHOUT_INSTALL_ID); expect(mockOctokit.apps.getOrgInstallation).not.toBeCalled(); expect(spy).toHaveBeenNthCalledWith(1, undefined, 'app', ""); expect(spy).toHaveBeenNthCalledWith(2, TEST_DATA.installationId, 'installation', ""); @@ -419,27 +442,26 @@ describe('scaleUp with public GH', () => { it('creates a runner with correct config and labels', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; - await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + - `--token 1234abcd --labels label1,label2`, - runnerType: 'Repo', - runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, - }, LAUNCH_TEMPLATE); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expectedRunnerParams.runnerServiceConfig = expectedRunnerParams.runnerServiceConfig + `--labels label1,label2`; + expect(createRunner).toBeCalledWith(expectedRunnerParams, LAUNCH_TEMPLATE); }); it('creates a runner and ensure the group argument is ignored', async () => { process.env.RUNNER_EXTRA_LABELS = 'label1,label2'; process.env.RUNNER_GROUP_NAME = 'TEST_GROUP_IGNORED'; - await scaleUp('aws:sqs', TEST_DATA); - expect(createRunner).toBeCalledWith({ - environment: 'unit-test-environment', - runnerServiceConfig: `--url https://github.com/${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName} ` + - `--token 1234abcd --labels label1,label2`, - runnerType: 'Repo', - runnerOwner: `${TEST_DATA.repositoryOwner}/${TEST_DATA.repositoryName}`, - }, LAUNCH_TEMPLATE); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expectedRunnerParams.runnerServiceConfig = expectedRunnerParams.runnerServiceConfig + `--labels label1,label2`; + expect(createRunner).toBeCalledWith(expectedRunnerParams, LAUNCH_TEMPLATE); + }); + + it('attempts next launch template if first fails', async () => { + const mockCreateRunners = mocked(createRunner); + mockCreateRunners.mockRejectedValueOnce(new Error('no capactiy')); + await scaleUpModule.scaleUp('aws:sqs', TEST_DATA); + expect(createRunner).toBeCalledTimes(2); + expect(createRunner).toHaveBeenNthCalledWith(1, expectedRunnerParams, 'lt-1'); + expect(createRunner).toHaveBeenNthCalledWith(2, expectedRunnerParams, 'lt-2'); }); }); }); From 3a7a57f7e09c36c80c9e54e4a9282705a1452c37 Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Mon, 14 Jun 2021 14:34:29 -0400 Subject: [PATCH 6/7] Test for all launch templates failing --- .../runners/src/scale-runners/scale-up.test.ts | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts index 5525201dd6..8e157f9084 100644 --- a/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts +++ b/modules/runners/lambdas/runners/src/scale-runners/scale-up.test.ts @@ -203,6 +203,17 @@ describe('scaleUp with GHES', () => { expect(createRunner).toBeCalledTimes(2); expect(createRunner).toHaveBeenNthCalledWith(1, expectedRunnerParams, 'lt-1'); expect(createRunner).toHaveBeenNthCalledWith(2, expectedRunnerParams, 'lt-2'); + mockCreateRunners.mockReset(); + }); + + it('all launch templates fail', async () => { + const mockCreateRunners = mocked(createRunner); + mockCreateRunners.mockRejectedValue(new Error('All launch templates failed')); + await expect(scaleUpModule.scaleUp('aws:sqs', TEST_DATA)).rejects.toThrow('All launch templates failed'); + expect(createRunner).toBeCalledTimes(2); + expect(createRunner).toHaveBeenNthCalledWith(1, expectedRunnerParams, 'lt-1'); + expect(createRunner).toHaveBeenNthCalledWith(2, expectedRunnerParams, 'lt-2'); + mockCreateRunners.mockReset(); }); }); From f3deee5e1fb0c02befa96a6ab78df6828c66d398 Mon Sep 17 00:00:00 2001 From: Nathaniel McAuliffe Date: Wed, 16 Jun 2021 11:27:07 -0400 Subject: [PATCH 7/7] Marking `instance_type` as deprecated --- modules/runners/variables.tf | 2 +- variables.tf | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/runners/variables.tf b/modules/runners/variables.tf index c72a847916..0516f0b1e4 100644 --- a/modules/runners/variables.tf +++ b/modules/runners/variables.tf @@ -58,7 +58,7 @@ variable "market_options" { } variable "instance_type" { - description = "Default instance type for the action runner." + description = "[DEPRECATED] See instance_types." type = string default = "m5.large" } diff --git a/variables.tf b/variables.tf index a857054501..21773e9f25 100644 --- a/variables.tf +++ b/variables.tf @@ -126,7 +126,7 @@ variable "instance_profile_path" { } variable "instance_type" { - description = "Instance type for the action runner." + description = "[DEPRECATED] See instance_types." type = string default = "m5.large" }