From 4d2d7a851c1d711aaf5b6b15e7f135c5b7d985b1 Mon Sep 17 00:00:00 2001 From: Zain Rizvi Date: Tue, 15 Oct 2024 18:15:25 -0500 Subject: [PATCH 1/5] Enable non-org autoscaler to read scale-config from any repo --- .../runners/lambdas/runners/src/scale-runners/config.ts | 2 +- .../lambdas/runners/src/scale-runners/scale-down.ts | 2 +- .../runners/lambdas/runners/src/scale-runners/scale-up.ts | 2 +- terraform-aws-github-runner/modules/runners/variables.tf | 8 ++++++++ terraform-aws-github-runner/variables.tf | 4 ++-- 5 files changed, 13 insertions(+), 5 deletions(-) diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.ts index aa98bf706e..b1354f178d 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.ts @@ -97,7 +97,7 @@ export class Config { this.runnerGroupName = process.env.RUNNER_GROUP_NAME; this.runnersExtraLabels = process.env.RUNNER_EXTRA_LABELS; /* istanbul ignore next */ - this.scaleConfigRepo = process.env.SCALE_CONFIG_REPO || 'test-infra'; + this.scaleConfigRepo = process.env.SCALE_CONFIG_REPO || ''; this.scaleConfigRepoPath = process.env.SCALE_CONFIG_REPO_PATH || '.github/scale-config.yml'; this.secretsManagerSecretsId = process.env.SECRETSMANAGER_SECRETS_ID; this.sSMParamCleanupAgeDays = Number(process.env.SSM_PARAM_CLEANUP_AGE_DAYS || '7'); 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 8d5c6a7410..d3aa16fcc0 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 @@ -373,7 +373,7 @@ export async function isEphemeralRunner(ec2runner: RunnerInfo, metrics: ScaleDow } const repo: Repo = (() => { - if (Config.Instance.enableOrganizationRunners) { + if (Config.Instance.scaleConfigRepo) { return { owner: ec2runner.org !== undefined ? (ec2runner.org as string) : getRepo(ec2runner.repo as string).owner, repo: Config.Instance.scaleConfigRepo, diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts index 132c46bbf4..6aade4be30 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts @@ -60,7 +60,7 @@ export async function scaleUp( const runnerTypes = await getRunnerTypes( { owner: repo.owner, - repo: Config.Instance.enableOrganizationRunners ? Config.Instance.scaleConfigRepo : repo.repo, + repo: Config.Instance.scaleConfigRepo || repo.repo, }, metrics, ); diff --git a/terraform-aws-github-runner/modules/runners/variables.tf b/terraform-aws-github-runner/modules/runners/variables.tf index 1387af2a47..f16c59ef93 100644 --- a/terraform-aws-github-runner/modules/runners/variables.tf +++ b/terraform-aws-github-runner/modules/runners/variables.tf @@ -289,6 +289,14 @@ variable "scale_config_repo" { description = "Repository to fetch scale config from." default = "" type = string + + validation { + # Enforce that a value for scale_config_repo is set when enable_organization_runners is set to true. + # Setting the value is optional when not using organization runners since we'll default to the + # job's repository. + condition = var.enable_organization_runners == true ? var.scale_config_repo != "" : true + error_message = "scale_config_repo is required when enable_organization_runners is set to true" + } } variable "scale_config_repo_path" { diff --git a/terraform-aws-github-runner/variables.tf b/terraform-aws-github-runner/variables.tf index 80f9c64d39..b99af920cf 100644 --- a/terraform-aws-github-runner/variables.tf +++ b/terraform-aws-github-runner/variables.tf @@ -346,8 +346,8 @@ variable "cant_have_issues_labels" { } variable "scale_config_repo" { - description = "Repository to fetch scale config from if `enable_organization_runners` is set to true. Otherwise the job's repo will be used" - default = "" # Internally defaults to 'test-infra' + description = "Repository to fetch scale config from. Optional if `enable_organization_runners` is set to false, in which case the job's repo will be used" + default = "" type = string } From 3abbb0117478d6654559f2e182fba9f28fb3225f Mon Sep 17 00:00:00 2001 From: Zain Rizvi Date: Wed, 16 Oct 2024 11:31:22 -0500 Subject: [PATCH 2/5] fix validation --- .../lambdas/runners/src/scale-runners/scale-up.ts | 2 +- .../modules/runners/scale-up.tf | 10 ++++++++++ .../modules/runners/variables.tf | 8 -------- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts index 6aade4be30..ae91c8ba5b 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-up.ts @@ -60,7 +60,7 @@ export async function scaleUp( const runnerTypes = await getRunnerTypes( { owner: repo.owner, - repo: Config.Instance.scaleConfigRepo || repo.repo, + repo: Config.Instance.scaleConfigRepo || repo.repo, }, metrics, ); diff --git a/terraform-aws-github-runner/modules/runners/scale-up.tf b/terraform-aws-github-runner/modules/runners/scale-up.tf index 4deb756295..8a47122534 100644 --- a/terraform-aws-github-runner/modules/runners/scale-up.tf +++ b/terraform-aws-github-runner/modules/runners/scale-up.tf @@ -28,6 +28,16 @@ resource "aws_lambda_function" "scale_up" { memory_size = 2048 publish = true + lifecycle { + precondition { + # Enforce that a value for scale_config_repo is set when enable_organization_runners is set to true. + # Setting the value is optional when not using organization runners since we'll default to the + # job's repository. + condition = var.enable_organization_runners == true ? var.scale_config_repo != "" : true + error_message = "scale_config_repo is required when enable_organization_runners is set to true" + } + } + environment { variables = { CANT_HAVE_ISSUES_LABELS = join(",", var.cant_have_issues_labels) diff --git a/terraform-aws-github-runner/modules/runners/variables.tf b/terraform-aws-github-runner/modules/runners/variables.tf index f16c59ef93..1387af2a47 100644 --- a/terraform-aws-github-runner/modules/runners/variables.tf +++ b/terraform-aws-github-runner/modules/runners/variables.tf @@ -289,14 +289,6 @@ variable "scale_config_repo" { description = "Repository to fetch scale config from." default = "" type = string - - validation { - # Enforce that a value for scale_config_repo is set when enable_organization_runners is set to true. - # Setting the value is optional when not using organization runners since we'll default to the - # job's repository. - condition = var.enable_organization_runners == true ? var.scale_config_repo != "" : true - error_message = "scale_config_repo is required when enable_organization_runners is set to true" - } } variable "scale_config_repo_path" { From 8be26843a4abfbce2384f0b039d3e98399cb822a Mon Sep 17 00:00:00 2001 From: Zain Rizvi Date: Wed, 16 Oct 2024 11:32:11 -0500 Subject: [PATCH 3/5] fix validation --- .../modules/runners/scale-down.tf | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/terraform-aws-github-runner/modules/runners/scale-down.tf b/terraform-aws-github-runner/modules/runners/scale-down.tf index 76d4bad47c..9daa764b2e 100644 --- a/terraform-aws-github-runner/modules/runners/scale-down.tf +++ b/terraform-aws-github-runner/modules/runners/scale-down.tf @@ -26,6 +26,16 @@ resource "aws_lambda_function" "scale_down" { tags = local.tags memory_size = 2048 + lifecycle { + precondition { + # Enforce that a value for scale_config_repo is set when enable_organization_runners is set to true. + # Setting the value is optional when not using organization runners since we'll default to the + # job's repository. + condition = var.enable_organization_runners == true ? var.scale_config_repo != "" : true + error_message = "scale_config_repo is required when enable_organization_runners is set to true" + } + } + environment { variables = { AWS_REGION_INSTANCES = join(",", var.aws_region_instances) From badbb8cf1541b09306530466dead9a01c4633b5c Mon Sep 17 00:00:00 2001 From: Zain Rizvi Date: Wed, 16 Oct 2024 12:14:04 -0500 Subject: [PATCH 4/5] fix test --- .../runners/lambdas/runners/src/scale-runners/config.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.test.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.test.ts index 761ed42bbf..34d9dd4b71 100644 --- a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.test.ts +++ b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/config.test.ts @@ -193,7 +193,7 @@ describe('Config', () => { expect(Config.Instance.mustHaveIssuesLabels).toEqual([]); expect(Config.Instance.runnerGroupName).toBeUndefined(); expect(Config.Instance.runnersExtraLabels).toBeUndefined(); - expect(Config.Instance.scaleConfigRepo).toEqual('test-infra'); + expect(Config.Instance.scaleConfigRepo).toEqual(''); expect(Config.Instance.scaleConfigRepoPath).toEqual('.github/scale-config.yml'); expect(Config.Instance.secretsManagerSecretsId).toBeUndefined(); expect(Config.Instance.shuffledAwsRegionInstances).toEqual([]); From 6586a36ff396877023a63153b994715fa3358bfc Mon Sep 17 00:00:00 2001 From: Zain Rizvi Date: Wed, 16 Oct 2024 13:15:58 -0500 Subject: [PATCH 5/5] Add scale down tests --- .../src/scale-runners/scale-down.test.ts | 129 +++++++++++++----- 1 file changed, 95 insertions(+), 34 deletions(-) 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 7288a17869..4361ebb022 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 @@ -1256,57 +1256,118 @@ describe('scale-down', () => { describe('repo runners', () => { const runnerType = 'runnerTypeDef'; const owner = 'the-org'; - const repo: Repo = { + const runnerRepo: Repo = { owner: owner, repo: 'a-repo', }; - const repoKey = `${owner}/a-repo`; + const runnerRepoKey = `${owner}/a-repo`; - beforeEach(() => { - jest.spyOn(Config, 'Instance', 'get').mockImplementation( - () => - ({ - ...baseConfig, - enableOrganizationRunners: false, - } as unknown as Config), - ); - }); + describe('When no scaleConfigRepo is set, the runner repo is used as the source for scale config', () => { - it('is_ephemeral === undefined', async () => { - const mockedGetRunnerTypes = mocked(getRunnerTypes); + beforeEach(() => { + jest.spyOn(Config, 'Instance', 'get').mockImplementation( + () => + ({ + ...baseConfig, + enableOrganizationRunners: false, + } as unknown as Config), + ); + }); - mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, {} as RunnerType]])); + it('is_ephemeral === undefined', async () => { + const mockedGetRunnerTypes = mocked(getRunnerTypes); - expect(await isEphemeralRunner({ runnerType: runnerType, repo: repoKey } as RunnerInfo, metrics)).toEqual( - false, - ); + mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, {} as RunnerType]])); - expect(mockedGetRunnerTypes).toBeCalledTimes(1); - expect(mockedGetRunnerTypes).toBeCalledWith(repo, metrics); - }); + expect(await isEphemeralRunner({ runnerType: runnerType, repo: runnerRepoKey } as RunnerInfo, metrics)).toEqual( + false, + ); - it('is_ephemeral === true', async () => { - const mockedGetRunnerTypes = mocked(getRunnerTypes); + expect(mockedGetRunnerTypes).toBeCalledTimes(1); + expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, metrics); + }); - mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, { is_ephemeral: true } as RunnerType]])); + it('is_ephemeral === true', async () => { + const mockedGetRunnerTypes = mocked(getRunnerTypes); - expect(await isEphemeralRunner({ runnerType: runnerType, repo: repoKey } as RunnerInfo, metrics)).toEqual(true); + mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, { is_ephemeral: true } as RunnerType]])); - expect(mockedGetRunnerTypes).toBeCalledTimes(1); - expect(mockedGetRunnerTypes).toBeCalledWith(repo, metrics); + expect(await isEphemeralRunner({ runnerType: runnerType, repo: runnerRepoKey } as RunnerInfo, metrics)).toEqual(true); + + expect(mockedGetRunnerTypes).toBeCalledTimes(1); + expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, metrics); + }); + + it('is_ephemeral === false', async () => { + const mockedGetRunnerTypes = mocked(getRunnerTypes); + + mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, { is_ephemeral: false } as RunnerType]])); + + expect(await isEphemeralRunner({ runnerType: runnerType, repo: runnerRepoKey } as RunnerInfo, metrics)).toEqual( + false, + ); + + expect(mockedGetRunnerTypes).toBeCalledTimes(1); + expect(mockedGetRunnerTypes).toBeCalledWith(runnerRepo, metrics); + }); }); - it('is_ephemeral === false', async () => { - const mockedGetRunnerTypes = mocked(getRunnerTypes); + describe("When a scaleConfigRepo is set, it's used as the source for scale config", () => { + const centralRepoName = 'central-repo'; // to be the test-infra equivalent + const centralRepo: Repo = { + owner: owner, + repo: centralRepoName, + }; - mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, { is_ephemeral: false } as RunnerType]])); + beforeEach(() => { + jest.spyOn(Config, 'Instance', 'get').mockImplementation( + () => + ({ + ...baseConfig, + enableOrganizationRunners: false, + scaleConfigRepo: centralRepoName, + } as unknown as Config), + ); + }); + + it('is_ephemeral === undefined', async () => { + const mockedGetRunnerTypes = mocked(getRunnerTypes); + + mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, {} as RunnerType]])); + + expect(await isEphemeralRunner({ runnerType: runnerType, repo: runnerRepoKey } as RunnerInfo, metrics)).toEqual( + false, + ); + + expect(mockedGetRunnerTypes).toBeCalledTimes(1); + expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, metrics); + }); + + it('is_ephemeral === true', async () => { + const mockedGetRunnerTypes = mocked(getRunnerTypes); + + mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, { is_ephemeral: true } as RunnerType]])); + + expect(await isEphemeralRunner({ runnerType: runnerType, repo: runnerRepoKey } as RunnerInfo, metrics)).toEqual(true); + + expect(mockedGetRunnerTypes).toBeCalledTimes(1); + expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, metrics); + }); + + it('is_ephemeral === false', async () => { + const mockedGetRunnerTypes = mocked(getRunnerTypes); + + mockedGetRunnerTypes.mockResolvedValueOnce(new Map([[runnerType, { is_ephemeral: false } as RunnerType]])); + + expect(await isEphemeralRunner({ runnerType: runnerType, repo: runnerRepoKey } as RunnerInfo, metrics)).toEqual( + false, + ); + + expect(mockedGetRunnerTypes).toBeCalledTimes(1); + expect(mockedGetRunnerTypes).toBeCalledWith(centralRepo, metrics); + }); - expect(await isEphemeralRunner({ runnerType: runnerType, repo: repoKey } as RunnerInfo, metrics)).toEqual( - false, - ); - expect(mockedGetRunnerTypes).toBeCalledTimes(1); - expect(mockedGetRunnerTypes).toBeCalledWith(repo, metrics); }); }); });