From 8e1690cea5f0b5cd36b06c361a95c8de058f1668 Mon Sep 17 00:00:00 2001 From: Vishal Wadhera <30888002+wadherv@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:54:51 +0530 Subject: [PATCH 1/2] fix(perf): filter EC2 runner tags client-side instead of via DescribeInstances --- .../aws/ec2/src/control-plane/runners.ts | 58 +++++++++++-------- 1 file changed, 35 insertions(+), 23 deletions(-) diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts index 050804cce1..b8825bf89a 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.ts @@ -6,6 +6,7 @@ import { DeleteTagsCommand, DescribeInstancesCommand, DescribeInstancesResult, + type Instance, RunInstancesCommand, type RunInstancesCommandInput, RunInstancesCommandOutput, @@ -35,40 +36,30 @@ interface Ec2Filter { type FleetError = NonNullable[number]; export async function listEC2Runners(filters: Ec2ListRunnerFilters | undefined = undefined): Promise { - const ec2Filters = constructFilters(filters); - const runners: RunnerInfo[] = []; - for (const filter of ec2Filters) { - runners.push(...(await getRunners(filter))); - } - return runners; + const ec2Statuses = filters?.statuses ? filters.statuses : ['running', 'pending']; + const stateFilter: Ec2Filter[] = [{ Name: 'instance-state-name', Values: ec2Statuses }]; + const tagFilters = constructTagFilters(filters); + return await getRunners(stateFilter, tagFilters); } -function constructFilters(filters?: Ec2ListRunnerFilters): Ec2Filter[][] { - const ec2Statuses = filters?.statuses ? filters.statuses : ['running', 'pending']; - const ec2Filters: Ec2Filter[][] = []; - const ec2FiltersBase = [{ Name: 'instance-state-name', Values: ec2Statuses }]; +function constructTagFilters(filters?: Ec2ListRunnerFilters): Ec2Filter[] { + const tagFilters: Ec2Filter[] = [{ Name: 'tag:ghr:Application', Values: ['github-action-runner'] }]; if (filters) { if (filters.environment !== undefined) { - ec2FiltersBase.push({ Name: 'tag:ghr:environment', Values: [filters.environment] }); + tagFilters.push({ Name: 'tag:ghr:environment', Values: [filters.environment] }); } if (filters.runnerType && filters.runnerOwner) { - ec2FiltersBase.push({ Name: `tag:ghr:Type`, Values: [filters.runnerType] }); - ec2FiltersBase.push({ Name: `tag:ghr:Owner`, Values: [filters.runnerOwner] }); + tagFilters.push({ Name: `tag:ghr:Type`, Values: [filters.runnerType] }); + tagFilters.push({ Name: `tag:ghr:Owner`, Values: [filters.runnerOwner] }); } if (filters.orphan) { - ec2FiltersBase.push({ Name: 'tag:ghr:orphan', Values: ['true'] }); + tagFilters.push({ Name: 'tag:ghr:orphan', Values: ['true'] }); } } - - for (const key of ['tag:ghr:Application']) { - const filter = [...ec2FiltersBase]; - filter.push({ Name: key, Values: ['github-action-runner'] }); - ec2Filters.push(filter); - } - return ec2Filters; + return tagFilters; } -async function getRunners(ec2Filters: Ec2Filter[]): Promise { +async function getRunners(ec2Filters: Ec2Filter[], tagFilters: Ec2Filter[]): Promise { const ec2 = getTracedAWSV3Client(new EC2Client({ region: process.env.AWS_REGION })); const runners: RunnerInfo[] = []; let nextToken; @@ -79,11 +70,32 @@ async function getRunners(ec2Filters: Ec2Filter[]): Promise { ); hasNext = instances.NextToken ? true : false; nextToken = instances.NextToken; - runners.push(...getRunnerInfo(instances)); + runners.push(...getRunnerInfo(filterInstancesByTags(instances, tagFilters))); } return runners; } +function matchesTagFilters(instance: Instance, tagFilters: Ec2Filter[]): boolean { + return tagFilters.every((filter) => { + const tagKey = filter.Name.replace(/^tag:/, ''); + const tagValue = instance.Tags?.find((t) => t.Key === tagKey)?.Value; + return tagValue !== undefined && filter.Values.includes(tagValue); + }); +} + +function filterInstancesByTags(result: DescribeInstancesResult, tagFilters: Ec2Filter[]): DescribeInstancesResult { + if (!result.Reservations) { + return result; + } + return { + ...result, + Reservations: result.Reservations.map((reservation) => ({ + ...reservation, + Instances: reservation.Instances?.filter((instance) => matchesTagFilters(instance, tagFilters)), + })), + }; +} + function getRunnerInfo(runningInstances: DescribeInstancesResult) { const runners: RunnerInfo[] = []; if (runningInstances.Reservations) { From ba95db04e28da6857beed268428149ff9ef5df39 Mon Sep 17 00:00:00 2001 From: Vishal Wadhera <30888002+wadherv@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:55:15 +0530 Subject: [PATCH 2/2] Update runners.test.ts --- .../aws/ec2/src/control-plane/runners.test.ts | 124 ++++++++++-------- 1 file changed, 70 insertions(+), 54 deletions(-) diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.test.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.test.ts index 738c6da13d..97e8db56f0 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.test.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runners.test.ts @@ -139,67 +139,75 @@ describe('list instances', () => { expect(mockEC2Client).toHaveReceivedCommand(DescribeInstancesCommand); }); - it('filters instances on repo name', async () => { + it('sends only the instance-state-name filter to EC2 (tags are filtered client-side)', async () => { mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances); await listEC2Runners({ runnerType: 'Repo', runnerOwner: REPO_NAME, - environment: undefined, + environment: ENVIRONMENT, + orphan: true, }); expect(mockEC2Client).toHaveReceivedCommandWith(DescribeInstancesCommand, { - Filters: [ - { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:ghr:Type', Values: ['Repo'] }, - { Name: 'tag:ghr:Owner', Values: [REPO_NAME] }, - { Name: 'tag:ghr:Application', Values: ['github-action-runner'] }, - ], + Filters: [{ Name: 'instance-state-name', Values: ['running', 'pending'] }], }); }); - it('filters instances on org name', async () => { - mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances); - await listEC2Runners({ + it('filters instances on repo name (client-side)', async () => { + const instances = buildInstancesResult([ + { 'ghr:Application': 'github-action-runner', 'ghr:Type': 'Repo', 'ghr:Owner': REPO_NAME }, + { 'ghr:Application': 'github-action-runner', 'ghr:Type': 'Org', 'ghr:Owner': ORG_NAME }, + ]); + mockEC2Client.on(DescribeInstancesCommand).resolves(instances); + const resp = await listEC2Runners({ + runnerType: 'Repo', + runnerOwner: REPO_NAME, + environment: undefined, + }); + expect(resp.length).toBe(1); + expect(resp[0].owner).toBe(REPO_NAME); + }); + + it('filters instances on org name (client-side)', async () => { + const instances = buildInstancesResult([ + { 'ghr:Application': 'github-action-runner', 'ghr:Type': 'Org', 'ghr:Owner': ORG_NAME }, + { 'ghr:Application': 'github-action-runner', 'ghr:Type': 'Repo', 'ghr:Owner': REPO_NAME }, + ]); + mockEC2Client.on(DescribeInstancesCommand).resolves(instances); + const resp = await listEC2Runners({ runnerType: 'Org', runnerOwner: ORG_NAME, environment: undefined, }); - expect(mockEC2Client).toHaveReceivedCommandWith(DescribeInstancesCommand, { - Filters: [ - { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:ghr:Type', Values: ['Org'] }, - { Name: 'tag:ghr:Owner', Values: [ORG_NAME] }, - { Name: 'tag:ghr:Application', Values: ['github-action-runner'] }, - ], - }); + expect(resp.length).toBe(1); + expect(resp[0].owner).toBe(ORG_NAME); }); - it('filters instances on environment', async () => { - mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances); - await listEC2Runners({ environment: ENVIRONMENT }); - expect(mockEC2Client).toHaveReceivedCommandWith(DescribeInstancesCommand, { - Filters: [ - { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:ghr:environment', Values: [ENVIRONMENT] }, - { Name: 'tag:ghr:Application', Values: ['github-action-runner'] }, - ], - }); + it('filters instances on environment (client-side)', async () => { + const instances = buildInstancesResult([ + { 'ghr:Application': 'github-action-runner', 'ghr:environment': ENVIRONMENT }, + { 'ghr:Application': 'github-action-runner', 'ghr:environment': 'some-other-environment' }, + ]); + mockEC2Client.on(DescribeInstancesCommand).resolves(instances); + const resp = await listEC2Runners({ environment: ENVIRONMENT }); + expect(resp.length).toBe(1); }); - it('filters instances on environment and orphan', async () => { - mockRunningInstances.Reservations![0].Instances![0].Tags!.push({ - Key: 'ghr:orphan', - Value: 'true', - }); - mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances); - await listEC2Runners({ environment: ENVIRONMENT, orphan: true }); - expect(mockEC2Client).toHaveReceivedCommandWith(DescribeInstancesCommand, { - Filters: [ - { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:ghr:environment', Values: [ENVIRONMENT] }, - { Name: 'tag:ghr:orphan', Values: ['true'] }, - { Name: 'tag:ghr:Application', Values: ['github-action-runner'] }, - ], - }); + it('filters instances on environment and orphan (client-side)', async () => { + const instances = buildInstancesResult([ + { 'ghr:Application': 'github-action-runner', 'ghr:environment': ENVIRONMENT, 'ghr:orphan': 'true' }, + { 'ghr:Application': 'github-action-runner', 'ghr:environment': ENVIRONMENT }, + { 'ghr:Application': 'github-action-runner', 'ghr:environment': 'some-other-environment', 'ghr:orphan': 'true' }, + ]); + mockEC2Client.on(DescribeInstancesCommand).resolves(instances); + const resp = await listEC2Runners({ environment: ENVIRONMENT, orphan: true }); + expect(resp.length).toBe(1); + }); + + it('excludes instances missing the Application tag (client-side)', async () => { + const instances = buildInstancesResult([{ 'ghr:Type': 'Org', 'ghr:Owner': ORG_NAME }]); + mockEC2Client.on(DescribeInstancesCommand).resolves(instances); + const resp = await listEC2Runners(); + expect(resp.length).toBe(0); }); it('No instances, undefined reservations list.', async () => { @@ -211,7 +219,7 @@ describe('list instances', () => { expect(resp.length).toBe(0); }); - it('Instances with no tags.', async () => { + it('Instances with no tags are excluded (no Application tag match).', async () => { const noInstances: DescribeInstancesResult = { Reservations: [ { @@ -227,17 +235,14 @@ describe('list instances', () => { }; mockEC2Client.on(DescribeInstancesCommand).resolves(noInstances); const resp = await listEC2Runners(); - expect(resp.length).toBe(1); + expect(resp.length).toBe(0); }); it('Filter instances for state running.', async () => { mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances); await listEC2Runners({ statuses: ['running'] }); expect(mockEC2Client).toHaveReceivedCommandWith(DescribeInstancesCommand, { - Filters: [ - { Name: 'instance-state-name', Values: ['running'] }, - { Name: 'tag:ghr:Application', Values: ['github-action-runner'] }, - ], + Filters: [{ Name: 'instance-state-name', Values: ['running'] }], }); }); @@ -245,14 +250,25 @@ describe('list instances', () => { mockEC2Client.on(DescribeInstancesCommand).resolves(mockRunningInstances); await listEC2Runners({ statuses: undefined }); expect(mockEC2Client).toHaveReceivedCommandWith(DescribeInstancesCommand, { - Filters: [ - { Name: 'instance-state-name', Values: ['running', 'pending'] }, - { Name: 'tag:ghr:Application', Values: ['github-action-runner'] }, - ], + Filters: [{ Name: 'instance-state-name', Values: ['running', 'pending'] }], }); }); }); +function buildInstancesResult(tagSets: Record[]): DescribeInstancesResult { + return { + Reservations: [ + { + Instances: tagSets.map((tags, index) => ({ + LaunchTime: new Date('2020-10-10T14:48:00.000+09:00'), + InstanceId: `i-${index}`, + Tags: Object.entries(tags).map(([Key, Value]) => ({ Key, Value })), + })), + }, + ], + }; +} + describe('terminate runner', () => { beforeEach(() => { vi.clearAllMocks();