Remove unused VPC and SecGroup after layout migration (#1185)

pytorch · Nov 25, 2022 · a418230 · a418230
1 parent 7c855b5
commit a418230
Show file tree

Hide file tree

Showing 12 changed files with 31 additions and 52 deletions.
diff --git a/terraform-aws-github-runner/main.tf b/terraform-aws-github-runner/main.tf
@@ -64,8 +64,6 @@ module "runners" {
 
   aws_region           = var.aws_region
   aws_region_instances = var.aws_region_instances
-  // TODO remove-me
-  vpc_id               = var.vpc_id
   vpc_ids              = var.vpc_ids
   vpc_sgs              = var.vpc_sgs
   subnet_vpc_ids       = var.subnet_vpc_ids

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
@@ -17,6 +17,7 @@ describe('Config', () => {
       'AWS_REGION|VPC_1,AWS_REGION|VPC_2,AWS_REGION_INSTANCES_1|VPC_3,' +
       'AWS_REGION_INSTANCES_2|VPC_4,AWS_REGION_INSTANCES_2|VPC_5';
     process.env.CANT_HAVE_ISSUES_LABELS = 'label 1,label 2';
+    process.env.DATETIME_DEPLOY = '19050202000000';
     process.env.ENABLE_ORGANIZATION_RUNNERS = 'YES';
     process.env.ENVIRONMENT = 'ENVIRONMENT';
     process.env.GHES_URL = 'GHES_URL';
@@ -52,6 +53,7 @@ describe('Config', () => {
       ]),
     );
     expect(Config.Instance.cantHaveIssuesLabels).toEqual(['label 1', 'label 2']);
+    expect(Config.Instance.datetimeDeploy).toBe('19050202000000');
     expect(Config.Instance.environment).toBe('ENVIRONMENT');
     expect(Config.Instance.ghesUrl).toBe('GHES_URL');
     expect(Config.Instance.ghesUrlApi).toBe('GHES_URL/api/v3');
@@ -124,6 +126,7 @@ describe('Config', () => {
     delete process.env.AWS_REGION;
     delete process.env.AWS_REGIONS_TO_VPC_IDS;
     delete process.env.CANT_HAVE_ISSUES_LABELS;
+    delete process.env.DATETIME_DEPLOY;
     delete process.env.ENVIRONMENT;
     delete process.env.GHES_URL;
     delete process.env.GITHUB_APP_CLIENT_ID;
@@ -150,6 +153,7 @@ describe('Config', () => {
 
     expect(Config.Instance.awsRegion).toBe('us-east-1');
     expect(Config.Instance.cantHaveIssuesLabels).toEqual([]);
+    expect(Config.Instance.datetimeDeploy).toBeUndefined();
     expect(Config.Instance.environment).toBe('gh-ci');
     expect(Config.Instance.ghesUrl).toBeUndefined();
     expect(Config.Instance.ghesUrlApi).toBeUndefined();

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
@@ -7,6 +7,7 @@ export class Config {
   readonly awsRegionInstances: string[];
   readonly awsRegionsToVpcIds: Map<string, Array<string>>;
   readonly cantHaveIssuesLabels: string[];
+  readonly datetimeDeploy: string | undefined;
   readonly enableOrganizationRunners: boolean;
   readonly environment: string;
   readonly ghesUrl: undefined | string;
@@ -39,6 +40,8 @@ export class Config {
     this.awsRegionsToVpcIds = this.getMapFromFlatEnv(process.env.AWS_REGIONS_TO_VPC_IDS);
     /* istanbul ignore next */
     this.cantHaveIssuesLabels = process.env.CANT_HAVE_ISSUES_LABELS?.split(',').filter((w) => w.length > 0) || [];
+    /* istanbul ignore next */
+    this.datetimeDeploy = process.env.DATETIME_DEPLOY ? process.env.DATETIME_DEPLOY : undefined;
     this.enableOrganizationRunners = getBoolean(process.env.ENABLE_ORGANIZATION_RUNNERS);
     this.environment = process.env.ENVIRONMENT || 'gh-ci';
     /* istanbul ignore next */

diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts
@@ -100,14 +100,15 @@ export async function listRunners(
           /* istanbul ignore next */
           return (
             reservation.Instances?.map((instance) => ({
+              applicationDeployDatetime: instance.Tags?.find((e) => e.Key === 'ApplicationDeployDatetime')?.Value,
               awsRegion: itm.awsRegion,
+              environment: instance.Tags?.find((e) => e.Key === 'Environment')?.Value,
+              ghRunnerId: instance.Tags?.find((e) => e.Key === 'GithubRunnerID')?.Value,
               instanceId: instance.InstanceId as string,
               launchTime: instance.LaunchTime,
-              repo: instance.Tags?.find((e) => e.Key === 'Repo')?.Value,
               org: instance.Tags?.find((e) => e.Key === 'Org')?.Value,
+              repo: instance.Tags?.find((e) => e.Key === 'Repo')?.Value,
               runnerType: instance.Tags?.find((e) => e.Key === 'RunnerType')?.Value,
-              ghRunnerId: instance.Tags?.find((e) => e.Key === 'GithubRunnerID')?.Value,
-              environment: instance.Tags?.find((e) => e.Key === 'Environment')?.Value,
             })) ?? []
           );
         }) ?? []
@@ -290,6 +291,9 @@ export async function createRunner(runnerParameters: RunnerInputParameters, metr
       { Key: 'Application', Value: 'github-action-runner' },
       { Key: 'RunnerType', Value: runnerParameters.runnerType.runnerTypeName },
     ];
+    if (Config.Instance.datetimeDeploy) {
+      tags.push({ Key: 'ApplicationDeployDatetime', Value: Config.Instance.datetimeDeploy });
+    }
     if (runnerParameters.repoName !== undefined) {
       tags.push({
         Key: 'Repo',

diff --git a/...rm-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts b/...rm-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/scale-down.test.ts
@@ -423,7 +423,7 @@ describe('scale-down', () => {
       expect(mockedListGithubRunnersOrg).toBeCalledTimes(15);
       expect(mockedListGithubRunnersOrg).toBeCalledWith(theOrg, metrics);
 
-      expect(mockedGetRunnerTypes).toBeCalledTimes(3);
+      expect(mockedGetRunnerTypes).toBeCalledTimes(4);
       expect(mockedGetRunnerTypes).toBeCalledWith({ owner: theOrg, repo: scaleConfigRepo }, metrics);
 
       expect(mockedRemoveGithubRunnerOrg).toBeCalledTimes(3);
@@ -731,7 +731,7 @@ describe('scale-down', () => {
       expect(mockedListGithubRunnersRepo).toBeCalledTimes(15);
       expect(mockedListGithubRunnersRepo).toBeCalledWith(repo, metrics);
 
-      expect(mockedGetRunnerTypes).toBeCalledTimes(3);
+      expect(mockedGetRunnerTypes).toBeCalledTimes(4);
       expect(mockedGetRunnerTypes).toBeCalledWith(repo, metrics);
 
       expect(mockedRemoveGithubRunnerRepo).toBeCalledTimes(3);

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
@@ -91,9 +91,10 @@ export async function scaleDown(): Promise<void> {
         if (
           ghRunnersRemovable.length - removedRunners <= Config.Instance.minAvailableRunners &&
           ghRunner !== undefined &&
+          ec2runner.applicationDeployDatetime == Config.Instance.datetimeDeploy &&
           !(await isEphemeralRunner(ec2runner, metrics))
         ) {
-          break;
+          continue;
         }
 
         let shouldRemoveEC2 = true;

diff --git a/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/utils.ts b/terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/utils.ts
@@ -4,14 +4,15 @@ export interface Repo {
 }
 
 export interface RunnerInfo {
+  applicationDeployDatetime?: string;
   awsRegion: string;
+  environment?: string;
+  ghRunnerId?: string;
   instanceId: string;
   launchTime?: Date;
-  repo?: string;
   org?: string;
+  repo?: string;
   runnerType?: string;
-  ghRunnerId?: string;
-  environment?: string;
 }
 
 export function getRepoKey(repo: Repo): string {

diff --git a/terraform-aws-github-runner/modules/runners/main.tf b/terraform-aws-github-runner/modules/runners/main.tf
@@ -16,16 +16,17 @@ locals {
     var.tags,
   )
 
-  name_sg                                = var.overrides["name_sg"] == "" ? local.tags["Name"] : var.overrides["name_sg"]
-  name_runner                            = var.overrides["name_runner"] == "" ? local.tags["Name"] : var.overrides["name_runner"]
-  role_path                              = var.role_path == null ? "/${var.environment}/" : var.role_path
+  datetime_deploy                        = formatdate("YYYYMMDDhhmmss", timestamp())
   instance_profile_path                  = var.instance_profile_path == null ? "/${var.environment}/" : var.instance_profile_path
   lambda_zip                             = var.lambda_zip == null ? "${path.module}/lambdas/runners/runners.zip" : var.lambda_zip
-  userdata_template                      = var.userdata_template == null ? "${path.module}/templates/user-data.sh" : var.userdata_template
-  userdata_template_windows              = "${path.module}/templates/user-data.ps1"
+  name_runner                            = var.overrides["name_runner"] == "" ? local.tags["Name"] : var.overrides["name_runner"]
+  name_sg                                = var.overrides["name_sg"] == "" ? local.tags["Name"] : var.overrides["name_sg"]
+  role_path                              = var.role_path == null ? "/${var.environment}/" : var.role_path
   userdata_arm_patch                     = "${path.module}/templates/arm-runner-patch.tpl"
   userdata_install_config_runner_linux   = "${path.module}/templates/install-config-runner.sh"
   userdata_install_config_runner_windows = "${path.module}/templates/install-config-runner.ps1"
+  userdata_template                      = var.userdata_template == null ? "${path.module}/templates/user-data.sh" : var.userdata_template
+  userdata_template_windows              = "${path.module}/templates/user-data.ps1"
   vpc_id_to_idx                          = {for idx, vpc in var.vpc_ids: vpc.vpc => idx}
 }
 
@@ -226,29 +227,6 @@ locals {
   })
 }
 
-// TODO runner_sg is here to allow apply only module.canary_runners, once changes are rolled out
-// this module will be removed as it have no use. The problem comes as the target determinator
-// on terraform mistenkely assumes this target as being a dependency on module.canary_runners
-// because both have a common root
-resource "aws_security_group" "runner_sg" {
-  name_prefix = "${var.environment}-github-actions-runner-sg"
-  description = "Github Actions Runner security group"
-  vpc_id      = var.vpc_id
-
-  egress {
-    from_port   = 0
-    to_port     = 0
-    protocol    = "-1"
-    cidr_blocks = ["0.0.0.0/0"]
-  }
-  tags = merge(
-    local.tags,
-    {
-      "Name" = format("%s", local.name_sg)
-    },
-  )
-}
-
 resource "aws_security_group" "runners_sg" {
   count       = length(var.vpc_ids)
   name_prefix = "${var.environment}-github-actions-runner-sg-${count.index}"

diff --git a/terraform-aws-github-runner/modules/runners/scale-down.tf b/terraform-aws-github-runner/modules/runners/scale-down.tf
@@ -28,6 +28,8 @@ resource "aws_lambda_function" "scale_down" {
 
   environment {
     variables = {
+      AWS_REGION_INSTANCES            = join(",", var.aws_region_instances)
+      DATETIME_DEPLOY                 = local.datetime_deploy
       ENABLE_ORGANIZATION_RUNNERS     = var.enable_organization_runners
       ENVIRONMENT                     = var.environment
       GHES_URL                        = var.ghes_url
@@ -40,7 +42,6 @@ resource "aws_lambda_function" "scale_down" {
       MINIMUM_RUNNING_TIME_IN_MINUTES = var.minimum_running_time_in_minutes
       SCALE_DOWN_CONFIG               = jsonencode(var.idle_config)
       SECRETSMANAGER_SECRETS_ID       = var.secretsmanager_secrets_id
-      AWS_REGION_INSTANCES            = join(",", var.aws_region_instances)
     }
   }
 

diff --git a/terraform-aws-github-runner/modules/runners/scale-up.tf b/terraform-aws-github-runner/modules/runners/scale-up.tf
@@ -31,6 +31,7 @@ resource "aws_lambda_function" "scale_up" {
   environment {
     variables = {
       CANT_HAVE_ISSUES_LABELS               = join(",", var.cant_have_issues_labels)
+      DATETIME_DEPLOY                       = local.datetime_deploy
       ENABLE_ORGANIZATION_RUNNERS           = var.enable_organization_runners
       ENVIRONMENT                           = var.environment
       GITHUB_APP_CLIENT_ID                  = var.github_app.client_id

diff --git a/terraform-aws-github-runner/modules/runners/variables.tf b/terraform-aws-github-runner/modules/runners/variables.tf
@@ -9,12 +9,6 @@ variable "aws_region_instances" {
   type        = list(string)
 }
 
-// TODO remove-me
-variable "vpc_id" {
-  description = "The old VPC id."
-  type        = string
-}
-
 variable "vpc_ids" {
   description = "The list of vpc_id for aws_region. keys; 'vpc' 'region'"
   type        = list(map(string))

diff --git a/terraform-aws-github-runner/variables.tf b/terraform-aws-github-runner/variables.tf
@@ -9,12 +9,6 @@ variable "aws_region_instances" {
   type        = list(string)
 }
 
-// TODO remove-me
-variable "vpc_id" {
-  description = "Old vpc id."
-  type        = string
-}
-
 variable "vpc_ids" {
   description = "The list of vpc_id for aws_region. keys; 'vpc' 'region'"
   type        = list(map(string))