From f9907202865e5e52a0aa4ba495d3a3847e7ff6f8 Mon Sep 17 00:00:00 2001 From: edersonbrilhante Date: Thu, 27 Aug 2026 21:20:16 +0200 Subject: [PATCH] refactor(ec2): decouple runner creation results --- .../src/control-plane/create-result.test.ts | 56 +++++ .../ec2/src/control-plane/create-result.ts | 56 +++++ .../ec2/src/control-plane/runner-creation.ts | 7 +- .../ec2/src/control-plane/scale-up.test.ts | 20 +- .../aws/ec2/src/runner-create-result.ts | 11 + .../aws/ec2/src/runners.d.ts | 1 - .../aws/ec2/src/runners.test.ts | 199 ++++++++++-------- .../compute-providers/aws/ec2/src/runners.ts | 188 +++++++---------- 8 files changed, 324 insertions(+), 214 deletions(-) create mode 100644 lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.test.ts create mode 100644 lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.ts create mode 100644 lambdas/libs/compute-providers/aws/ec2/src/runner-create-result.ts diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.test.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.test.ts new file mode 100644 index 0000000000..069e388d70 --- /dev/null +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.test.ts @@ -0,0 +1,56 @@ +import { describe, expect, it } from 'vitest'; + +import type { Ec2RunnerFailureCode } from '../runner-create-result'; +import { toControlPlaneCreateRunnerResult } from './create-result'; + +function result(failureCodes: Ec2RunnerFailureCode[] = []) { + return { + instances: [], + failedInstanceCount: 2, + failureCodes, + }; +} + +describe('control-plane EC2 create result', () => { + it.each([ + ['configured AWS name', ['aws-name:InsufficientInstanceCapacity'], ['InsufficientInstanceCapacity']], + ['built-in AWS name', ['aws-name:ThrottlingException'], []], + ['AWS server fault', ['aws-fault:server'], []], + ['HTTP throttling status', ['aws-http:429'], []], + ['HTTP server status', ['aws-http:503'], []], + ['network code', ['aws-code:ECONNRESET'], []], + ] as const)('classifies %s as retryable', (_name, failureCodes, configuredErrors) => { + expect(toControlPlaneCreateRunnerResult(result([...failureCodes]), configuredErrors)).toEqual({ + instances: [], + retryableErrorCount: 2, + nonRetryableErrorCount: 0, + }); + }); + + it('classifies missing capacity without retry evidence as non-retryable', () => { + expect(toControlPlaneCreateRunnerResult(result(['aws-name:InvalidParameterValue']), [])).toEqual({ + instances: [], + retryableErrorCount: 0, + nonRetryableErrorCount: 2, + }); + }); + + it('classifies every missing instance as retryable when any Fleet code is retryable', () => { + expect( + toControlPlaneCreateRunnerResult(result(['aws-name:InvalidParameterValue', 'aws-name:InternalError']), []), + ).toEqual({ instances: [], retryableErrorCount: 2, nonRetryableErrorCount: 0 }); + }); + + it('preserves created instances while converting failed instances', () => { + expect( + toControlPlaneCreateRunnerResult( + { + instances: ['i-created'], + failedInstanceCount: 1, + failureCodes: ['aws-name:InvalidParameterValue'], + }, + [], + ), + ).toEqual({ instances: ['i-created'], retryableErrorCount: 0, nonRetryableErrorCount: 1 }); + }); +}); diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.ts new file mode 100644 index 0000000000..a78734ab57 --- /dev/null +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/create-result.ts @@ -0,0 +1,56 @@ +import type { CreateRunnerResult } from '../../../../core'; + +import type { Ec2RunnerCreateResult, Ec2RunnerFailureCode } from '../runner-create-result'; + +const RETRYABLE_AWS_ERROR_NAMES = new Set([ + 'EC2ThrottledException', + 'InternalError', + 'RequestLimitExceeded', + 'RequestTimeout', + 'RequestTimeoutException', + 'ServiceUnavailable', + 'Throttling', + 'ThrottlingException', +]); + +const RETRYABLE_NETWORK_ERROR_CODES = new Set([ + 'EAI_AGAIN', + 'ECONNREFUSED', + 'ECONNRESET', + 'ENETUNREACH', + 'ENOTFOUND', + 'ETIMEDOUT', +]); + +function failureCodeValue(failureCode: Ec2RunnerFailureCode, prefix: string): string | undefined { + return failureCode.startsWith(prefix) ? failureCode.slice(prefix.length) : undefined; +} + +function triggersControlPlaneRetry(failureCode: Ec2RunnerFailureCode, configuredErrors: Set): boolean { + const errorName = failureCodeValue(failureCode, 'aws-name:'); + if (errorName !== undefined) { + return configuredErrors.has(errorName) || RETRYABLE_AWS_ERROR_NAMES.has(errorName); + } + + const errorCode = failureCodeValue(failureCode, 'aws-code:'); + if (errorCode !== undefined) return RETRYABLE_NETWORK_ERROR_CODES.has(errorCode); + if (failureCode === 'aws-fault:server') return true; + + const httpStatus = failureCodeValue(failureCode, 'aws-http:'); + if (httpStatus === undefined) return false; + const status = Number(httpStatus); + return status === 429 || status >= 500; +} + +export function toControlPlaneCreateRunnerResult( + result: Ec2RunnerCreateResult, + configuredRetryableErrors: readonly string[], +): CreateRunnerResult { + const configuredErrors = new Set(configuredRetryableErrors); + const retryable = result.failureCodes.some((failureCode) => triggersControlPlaneRetry(failureCode, configuredErrors)); + return { + instances: result.instances, + retryableErrorCount: retryable ? result.failedInstanceCount : 0, + nonRetryableErrorCount: retryable ? 0 : result.failedInstanceCount, + }; +} diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runner-creation.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runner-creation.ts index d3863db8ad..06a6bb430b 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runner-creation.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/runner-creation.ts @@ -13,6 +13,7 @@ import yn from 'yn'; import type { Ec2RunnerResourceOperations } from '../runners'; import type { RunnerInputParameters } from '../runners.d'; +import { toControlPlaneCreateRunnerResult } from './create-result'; const logger = createChildLogger('ec2-runners'); const RUNNER_LABELS_TAG_KEY = 'ghr:runner_labels'; @@ -71,13 +72,15 @@ export async function createRunners( ): Promise { let result: CreateRunnerResult; try { - result = await ec2Operations.create({ + const { scaleErrors, ...ec2CreateConfig } = ec2RunnerConfig; + const ec2Result = await ec2Operations.create({ + ...ec2CreateConfig, runnerType: githubRunnerConfig.runnerType, runnerOwner: githubRunnerConfig.runnerOwner, numberOfRunners, source, - ...ec2RunnerConfig, }); + result = toControlPlaneCreateRunnerResult(ec2Result, scaleErrors); } catch (error) { logger.error('Unexpected error while creating EC2 runner instances.', { error, diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.test.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.test.ts index 7a25762397..22785e0268 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.test.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.test.ts @@ -37,8 +37,8 @@ interface CreateProviderRunnersOptions { githubRunnerConfig?: Partial; } -function createRunnerResult(instances: string[], retryableErrorCount = 0, nonRetryableErrorCount = 0) { - return { instances, retryableErrorCount, nonRetryableErrorCount }; +function createRunnerResult(instances: string[], failedInstanceCount = 0, failureCodes: string[] = []) { + return { instances, failedInstanceCount, failureCodes }; } function runnerConfig(overrides: Partial = {}): CreateGitHubRunnerConfig { @@ -77,7 +77,6 @@ function expectedRunnerParams( subnets: ['subnet-123'], tracingEnabled: false, onDemandFailoverOnError: [], - scaleErrors: ['UnfulfillableCapacity', 'MaxSpotInstanceCountExceeded', 'TargetCapacityLimitExceededException'], source: 'scale-up-lambda', useDedicatedHost: false, ec2OverrideConfig: undefined, @@ -532,14 +531,15 @@ describe('scaleUp with public GH', () => { ); }); - it('creates a runner with correct config and labels and custom scale errors enabled.', async () => { + it('converts configured EC2 failures to the control-plane create result', async () => { process.env.SCALE_ERRORS = '["RequestLimitExceeded"]'; - await createProviderRunners({ - githubRunnerConfig: { runnerType: 'Repo', runnerOwner: repositoryRunnerOwner }, - }); - expect(mockCreateRunner).toHaveBeenCalledWith( - expectedRunnerParams('Repo', repositoryRunnerOwner, { scaleErrors: ['RequestLimitExceeded'] }), - ); + mockCreateRunner.mockResolvedValueOnce(createRunnerResult([], 1, ['aws-name:RequestLimitExceeded'])); + await expect( + createProviderRunners({ + githubRunnerConfig: { runnerType: 'Repo', runnerOwner: repositoryRunnerOwner }, + }), + ).resolves.toEqual({ instances: [], retryableErrorCount: 1, nonRetryableErrorCount: 0 }); + expect(mockCreateRunner).toHaveBeenCalledWith(expectedRunnerParams('Repo', repositoryRunnerOwner)); }); }); }); diff --git a/lambdas/libs/compute-providers/aws/ec2/src/runner-create-result.ts b/lambdas/libs/compute-providers/aws/ec2/src/runner-create-result.ts new file mode 100644 index 0000000000..c4d56e0997 --- /dev/null +++ b/lambdas/libs/compute-providers/aws/ec2/src/runner-create-result.ts @@ -0,0 +1,11 @@ +export type Ec2RunnerFailureCode = + | `aws-name:${string}` + | `aws-code:${string}` + | `aws-fault:${'client' | 'server'}` + | `aws-http:${number}`; + +export interface Ec2RunnerCreateResult { + instances: string[]; + failedInstanceCount: number; + failureCodes: Ec2RunnerFailureCode[]; +} diff --git a/lambdas/libs/compute-providers/aws/ec2/src/runners.d.ts b/lambdas/libs/compute-providers/aws/ec2/src/runners.d.ts index dcec4b9b62..e711ed5318 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/runners.d.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/runners.d.ts @@ -46,6 +46,5 @@ export interface RunnerInputParameters { amiIdSsmParameterName?: string; tracingEnabled?: boolean; onDemandFailoverOnError?: string[]; - scaleErrors: string[]; useDedicatedHost?: boolean; } diff --git a/lambdas/libs/compute-providers/aws/ec2/src/runners.test.ts b/lambdas/libs/compute-providers/aws/ec2/src/runners.test.ts index 4cc39e9f91..dd80e7b4dd 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/runners.test.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/runners.test.ts @@ -355,7 +355,6 @@ describe('create runner', () => { allocationStrategy: SpotAllocationStrategy.CAPACITY_OPTIMIZED, capacityType: 'spot', type: 'Org', - scaleErrors: ['UnfulfillableCapacity', 'MaxSpotInstanceCountExceeded'], source: 'scale-up-lambda', }; @@ -508,8 +507,8 @@ describe('create runner', () => { mockEC2Client.on(CreateFleetCommand).resolves({ Instances: [] }); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: [], }); expect(mockSSMClient).not.toHaveReceivedCommand(PutParameterCommand); }); @@ -540,12 +539,13 @@ describe('create runner', () => { it('keeps cancellation request-scoped and rejects before calling AWS', async () => { const abortController = new AbortController(); const abortReason = new Error('service stopping'); - const ec2Operations = createEc2RunnerClient(new EC2Client({})).forRequest({ + const cancelledOperations = createEc2RunnerClient(new EC2Client({})).forRequest({ signal: abortController.signal, }); abortController.abort(abortReason); - await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).rejects.toThrow('service stopping'); + const runnerParameters = createRunnerConfig(defaultRunnerConfig); + await expect(cancelledOperations.create(runnerParameters)).rejects.toThrow('service stopping'); expect(mockEC2Client).not.toHaveReceivedCommand(CreateFleetCommand); expect(mockSSMClient).not.toHaveReceivedCommand(GetParameterCommand); }); @@ -813,7 +813,6 @@ describe('create runner with errors', () => { allocationStrategy: SpotAllocationStrategy.CAPACITY_OPTIMIZED, capacityType: 'spot', type: 'Repo', - scaleErrors: ['UnfulfillableCapacity', 'MaxSpotInstanceCountExceeded'], source: 'scale-up-lambda', }; const defaultExpectedFleetRequestValues: ExpectedFleetRequestValues = { @@ -833,13 +832,13 @@ describe('create runner with errors', () => { mockEC2Client.on(CreateFleetCommand).resolves({ Instances: [] }); }); - it('returns one retryable error.', async () => { + it('returns the Fleet failure code', async () => { createFleetMockWithErrors(['UnfulfillableCapacity']); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 1, - nonRetryableErrorCount: 0, + failedInstanceCount: 1, + failureCodes: ['aws-name:UnfulfillableCapacity'], }); expect(mockEC2Client).toHaveReceivedCommandWith( CreateFleetCommand, @@ -848,25 +847,29 @@ describe('create runner with errors', () => { expect(mockSSMClient).not.toHaveReceivedCommand(PutParameterCommand); }); - it('returns a retryable error for a transient fleet result error without explicit configuration.', async () => { + it('returns a transient Fleet failure code without classifying it', async () => { createFleetMockWithErrors(['InternalError']); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 1, - nonRetryableErrorCount: 0, + failedInstanceCount: 1, + failureCodes: ['aws-name:InternalError'], }); }); - it('retries every missing instance when Fleet reports any retryable error.', async () => { + it('reports every missing instance and all Fleet failure codes', async () => { createFleetMockWithErrors(['UnfulfillableCapacity', 'MaxSpotInstanceCountExceeded', 'NotMappedError']); await expect( ec2Operations.create({ ...createRunnerConfig(defaultRunnerConfig), numberOfRunners: 3 }), ).resolves.toEqual({ instances: [], - retryableErrorCount: 3, - nonRetryableErrorCount: 0, + failedInstanceCount: 3, + failureCodes: [ + 'aws-name:UnfulfillableCapacity', + 'aws-name:MaxSpotInstanceCountExceeded', + 'aws-name:NotMappedError', + ], }); expect(mockEC2Client).toHaveReceivedCommandWith( CreateFleetCommand, @@ -880,26 +883,23 @@ describe('create runner with errors', () => { await expect( ec2Operations.create({ - ...createRunnerConfig({ - ...defaultRunnerConfig, - scaleErrors: [...defaultRunnerConfig.scaleErrors, 'InsufficientFreeAddressesInSubnet'], - }), + ...createRunnerConfig(defaultRunnerConfig), numberOfRunners: 35, }), ).resolves.toEqual({ instances: [], - retryableErrorCount: 35, - nonRetryableErrorCount: 0, + failedInstanceCount: 35, + failureCodes: ['aws-name:InsufficientFreeAddressesInSubnet'], }); }); - it('returns a non-retryable error count for an unmapped error', async () => { + it('returns the failure code for an unmapped Fleet error', async () => { createFleetMockWithErrors(['NonMappedError']); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: ['aws-name:NonMappedError'], }); expect(mockEC2Client).toHaveReceivedCommandWith( CreateFleetCommand, @@ -913,8 +913,8 @@ describe('create runner with errors', () => { await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: ['i-123'], - retryableErrorCount: 0, - nonRetryableErrorCount: 0, + failedInstanceCount: 0, + failureCodes: [], }); expect(mockEC2Client).toHaveReceivedCommandWith( CreateFleetCommand, @@ -922,13 +922,13 @@ describe('create runner with errors', () => { ); }); - it('returns a non-retryable error count when the create fleet request fails with an unknown exception.', async () => { + it('returns the failure code when the CreateFleet request fails with an unknown exception', async () => { mockEC2Client.on(CreateFleetCommand).rejects(new Error('Some error')); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: ['aws-name:Error'], }); expect(mockEC2Client).toHaveReceivedCommandWith( CreateFleetCommand, @@ -937,31 +937,31 @@ describe('create runner with errors', () => { expect(mockSSMClient).not.toHaveReceivedCommand(PutParameterCommand); }); - it('returns a non-retryable error count when the create fleet request fails with a permanent AWS error.', async () => { + it('returns the failure code when the CreateFleet request fails with a permanent AWS error', async () => { const error = Object.assign(new Error('Not authorized'), { name: 'UnauthorizedOperation' }); mockEC2Client.on(CreateFleetCommand).rejects(error); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: ['aws-name:UnauthorizedOperation'], }); }); it.each(['InvalidAMIID.NotFound', 'InvalidParameterValue', 'InvalidIamInstanceProfile.NotFound'])( - 'returns a non-retryable error count when CreateFleet fails with %s', + 'returns the failure code when CreateFleet fails with %s', async (errorName) => { mockEC2Client.on(CreateFleetCommand).rejects(Object.assign(new Error(errorName), { name: errorName })); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: [`aws-name:${errorName}`], }); }, ); - it('returns a retryable error count when the create fleet request fails with an AWS server error.', async () => { + it('returns all failure identifiers when the CreateFleet request fails with an AWS server error', async () => { const error = Object.assign(new Error('Service unavailable'), { name: 'UnknownServiceError', $fault: 'server', @@ -971,12 +971,12 @@ describe('create runner with errors', () => { await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 1, - nonRetryableErrorCount: 0, + failedInstanceCount: 1, + failureCodes: ['aws-name:UnknownServiceError', 'aws-fault:server', 'aws-http:503'], }); }); - it('returns a non-retryable error count when an AMI parameter is missing', async () => { + it('returns the failure chain when an AMI parameter is missing', async () => { mockSSMClient .on(GetParameterCommand) .rejects(Object.assign(new Error('Parameter does not exist'), { name: 'ParameterNotFound' })); @@ -988,12 +988,16 @@ describe('create runner with errors', () => { amiIdSsmParameterName: 'missing-ami-id-param', }), ), - ).resolves.toEqual({ instances: [], retryableErrorCount: 0, nonRetryableErrorCount: 1 }); + ).resolves.toEqual({ + instances: [], + failedInstanceCount: 1, + failureCodes: ['aws-name:GetParameterError', 'aws-name:ParameterNotFound'], + }); expect(mockEC2Client).not.toHaveReceivedCommand(CreateFleetCommand); expect(mockSSMClient).not.toHaveReceivedCommand(PutParameterCommand); }); - it('returns a retryable error count when the AMI lookup has a transient failure', async () => { + it('returns all failure identifiers when the AMI lookup has a transient failure', async () => { mockSSMClient.on(GetParameterCommand).rejects( Object.assign(new Error('Service unavailable'), { name: 'InternalServerError', @@ -1009,12 +1013,16 @@ describe('create runner with errors', () => { amiIdSsmParameterName: 'my-ami-id-param', }), ), - ).resolves.toEqual({ instances: [], retryableErrorCount: 1, nonRetryableErrorCount: 0 }); + ).resolves.toEqual({ + instances: [], + failedInstanceCount: 1, + failureCodes: ['aws-name:GetParameterError', 'aws-name:InternalServerError', 'aws-fault:server', 'aws-http:503'], + }); expect(mockEC2Client).not.toHaveReceivedCommand(CreateFleetCommand); expect(mockSSMClient).not.toHaveReceivedCommand(PutParameterCommand); }); - it('returns a non-retryable error count when the AMI lookup fails with an unknown exception', async () => { + it('returns the failure chain when the AMI lookup fails with an unknown exception', async () => { mockSSMClient.on(GetParameterCommand).rejects(new Error('Some error')); await expect( @@ -1024,21 +1032,25 @@ describe('create runner with errors', () => { amiIdSsmParameterName: 'my-ami-id-param', }), ), - ).resolves.toEqual({ instances: [], retryableErrorCount: 0, nonRetryableErrorCount: 1 }); + ).resolves.toEqual({ + instances: [], + failedInstanceCount: 1, + failureCodes: ['aws-name:GetParameterError', 'aws-name:Error'], + }); expect(mockEC2Client).not.toHaveReceivedCommand(CreateFleetCommand); expect(mockSSMClient).not.toHaveReceivedCommand(PutParameterCommand); }); - it('returns a non-scale error count with undefined Instances and Errors.', async () => { + it('reports a missing instance when Fleet omits Instances and Errors', async () => { mockEC2Client.on(CreateFleetCommand).resolvesOnce({ Instances: undefined, Errors: undefined }); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: [], }); }); - it('returns a non-scale error count with undefined InstanceIds and ErrorCode.', async () => { + it('reports a missing instance when Fleet omits InstanceIds and ErrorCode', async () => { mockEC2Client.on(CreateFleetCommand).resolvesOnce({ Instances: [{ InstanceIds: undefined }], Errors: [ @@ -1049,8 +1061,8 @@ describe('create runner with errors', () => { }); await expect(ec2Operations.create(createRunnerConfig(defaultRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: [], }); }); }); @@ -1061,7 +1073,6 @@ describe('create runner with errors fail over to OnDemand', () => { capacityType: 'spot', type: 'Repo', onDemandFailoverOnError: ['InsufficientInstanceCapacity'], - scaleErrors: ['UnfulfillableCapacity', 'MaxSpotInstanceCountExceeded'], source: 'scale-up-lambda', }; const defaultExpectedFleetRequestValues: ExpectedFleetRequestValues = { @@ -1088,8 +1099,8 @@ describe('create runner with errors fail over to OnDemand', () => { const instancesResult = await ec2Operations.create(createRunnerConfig(defaultRunnerConfig)); expect(instancesResult).toEqual({ instances: instancesIds, - retryableErrorCount: 0, - nonRetryableErrorCount: 0, + failedInstanceCount: 0, + failureCodes: [], }); expect(mockEC2Client).toHaveReceivedCommandTimes(CreateFleetCommand, 2); @@ -1115,6 +1126,8 @@ describe('create runner with errors fail over to OnDemand', () => { }); it('test InsufficientInstanceCapacity no fallback.', async () => { + createFleetMockWithErrors(['InsufficientInstanceCapacity']); + await expect( ec2Operations.create( createRunnerConfig({ @@ -1122,7 +1135,11 @@ describe('create runner with errors fail over to OnDemand', () => { onDemandFailoverOnError: [], }), ), - ).resolves.toEqual({ instances: [], retryableErrorCount: 0, nonRetryableErrorCount: 1 }); + ).resolves.toEqual({ + instances: [], + failedInstanceCount: 1, + failureCodes: ['aws-name:InsufficientInstanceCapacity'], + }); }); it('test InsufficientInstanceCapacity with multiple instances and fallback to on demand .', async () => { @@ -1135,8 +1152,8 @@ describe('create runner with errors fail over to OnDemand', () => { }); expect(instancesResult).toEqual({ instances: instancesIds, - retryableErrorCount: 0, - nonRetryableErrorCount: 0, + failedInstanceCount: 0, + failureCodes: [], }); expect(mockEC2Client).toHaveReceivedCommandTimes(CreateFleetCommand, 2); @@ -1161,7 +1178,7 @@ describe('create runner with errors fail over to OnDemand', () => { }); }); - it('returns created instances and retryable failures without fallback to on demand.', async () => { + it('returns created instances and failure details without fallback to on demand', async () => { const instancesIds = ['i-123', 'i-456']; // fallback to on demand for UnfulfillableCapacity but InsufficientInstanceCapacity is thrown createFleetMockWithWithOnDemandFallback(['UnfulfillableCapacity'], instancesIds); @@ -1171,7 +1188,11 @@ describe('create runner with errors fail over to OnDemand', () => { ...createRunnerConfig(defaultRunnerConfig), numberOfRunners: 2, }), - ).resolves.toEqual({ instances: ['i-123'], retryableErrorCount: 1, nonRetryableErrorCount: 0 }); + ).resolves.toEqual({ + instances: ['i-123'], + failedInstanceCount: 1, + failureCodes: ['aws-name:UnfulfillableCapacity'], + }); expect(mockEC2Client).toHaveReceivedCommandTimes(CreateFleetCommand, 1); @@ -1232,7 +1253,6 @@ interface RunnerConfig { amiIdSsmParameterName?: string; tracingEnabled?: boolean; onDemandFailoverOnError?: string[]; - scaleErrors: string[]; source: RunnerSource; useDedicatedHost?: boolean; ec2OverrideConfig?: Ec2OverrideConfig; @@ -1256,7 +1276,6 @@ function createRunnerConfig(runnerConfig: RunnerConfig): RunnerInputParameters { amiIdSsmParameterName: runnerConfig.amiIdSsmParameterName, tracingEnabled: runnerConfig.tracingEnabled, onDemandFailoverOnError: runnerConfig.onDemandFailoverOnError, - scaleErrors: runnerConfig.scaleErrors, source: runnerConfig.source, useDedicatedHost: runnerConfig.useDedicatedHost, ec2OverrideConfig: runnerConfig.ec2OverrideConfig, @@ -1383,7 +1402,6 @@ describe('create runner with useDedicatedHost', () => { capacityType: 'on-demand', source: 'scale-up-lambda', type: 'Org', - scaleErrors: [], useDedicatedHost: true, }; @@ -1403,8 +1421,8 @@ describe('create runner with useDedicatedHost', () => { expect(result).toEqual({ instances: ['i-dedicated-1'], - retryableErrorCount: 0, - nonRetryableErrorCount: 0, + failedInstanceCount: 0, + failureCodes: [], }); expect(mockEC2Client).toHaveReceivedCommand(RunInstancesCommand); expect(mockEC2Client).not.toHaveReceivedCommand(CreateFleetCommand); @@ -1420,7 +1438,7 @@ describe('create runner with useDedicatedHost', () => { }), ); - expect(result).toEqual({ instances: ['i-fleet-1'], retryableErrorCount: 0, nonRetryableErrorCount: 0 }); + expect(result).toEqual({ instances: ['i-fleet-1'], failedInstanceCount: 0, failureCodes: [] }); expect(mockEC2Client).toHaveReceivedCommand(CreateFleetCommand); expect(mockEC2Client).not.toHaveReceivedCommand(RunInstancesCommand); }); @@ -1435,7 +1453,7 @@ describe('create runner with useDedicatedHost', () => { }), ); - expect(result).toEqual({ instances: ['i-fleet-1'], retryableErrorCount: 0, nonRetryableErrorCount: 0 }); + expect(result).toEqual({ instances: ['i-fleet-1'], failedInstanceCount: 0, failureCodes: [] }); expect(mockEC2Client).toHaveReceivedCommand(CreateFleetCommand); expect(mockEC2Client).not.toHaveReceivedCommand(RunInstancesCommand); }); @@ -1488,8 +1506,8 @@ describe('create runner with useDedicatedHost', () => { expect(result).toEqual({ instances: ['i-dedicated-1', 'i-dedicated-2'], - retryableErrorCount: 0, - nonRetryableErrorCount: 0, + failedInstanceCount: 0, + failureCodes: [], }); expect(mockEC2Client).toHaveReceivedCommandWith(RunInstancesCommand, { LaunchTemplate: { @@ -1523,7 +1541,7 @@ describe('create runner with useDedicatedHost', () => { }); }); - it('returns a non-retryable failure when spot is used with dedicated host', async () => { + it('returns a neutral failure when spot is used with a dedicated host', async () => { await expect( ec2Operations.create( createRunnerConfig({ @@ -1531,68 +1549,69 @@ describe('create runner with useDedicatedHost', () => { capacityType: 'spot', }), ), - ).resolves.toEqual({ instances: [], retryableErrorCount: 0, nonRetryableErrorCount: 1 }); + ).resolves.toEqual({ instances: [], failedInstanceCount: 1, failureCodes: [] }); expect(mockEC2Client).not.toHaveReceivedCommand(RunInstancesCommand); }); - it('returns a non-retryable failure when RunInstances returns no instances', async () => { + it('reports a missing instance when RunInstances returns no instances', async () => { mockEC2Client.on(RunInstancesCommand).resolves({ Instances: [] }); await expect(ec2Operations.create(createRunnerConfig(dedicatedHostRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: [], }); }); - it('returns a non-retryable failure when RunInstances fails with an unknown exception', async () => { + it('returns the failure code when RunInstances fails with an unknown exception', async () => { mockEC2Client.on(RunInstancesCommand).rejects(new Error('EC2 error')); await expect(ec2Operations.create(createRunnerConfig(dedicatedHostRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: ['aws-name:Error'], }); }); - it('returns a non-retryable failure when RunInstances fails with a permanent AWS error', async () => { + it('returns the failure code when RunInstances fails with a permanent AWS error', async () => { const error = Object.assign(new Error('Invalid subnet'), { name: 'InvalidSubnetID.NotFound' }); mockEC2Client.on(RunInstancesCommand).rejects(error); await expect(ec2Operations.create(createRunnerConfig(dedicatedHostRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: ['aws-name:InvalidSubnetID.NotFound'], }); }); - it('returns retryable failures when RunInstances fails with a network error', async () => { + it('returns all failure identifiers when RunInstances fails with a network error', async () => { const error = Object.assign(new Error('Connection reset'), { code: 'ECONNRESET' }); mockEC2Client.on(RunInstancesCommand).rejects(error); await expect(ec2Operations.create(createRunnerConfig(dedicatedHostRunnerConfig))).resolves.toEqual({ instances: [], - retryableErrorCount: 1, - nonRetryableErrorCount: 0, + failedInstanceCount: 1, + failureCodes: ['aws-name:Error', 'aws-code:ECONNRESET'], }); }); - it('returns retryable failures when RunInstances fails with a configured retryable error', async () => { + it('returns the AWS failure code when RunInstances fails', async () => { const error = Object.assign(new Error('Insufficient capacity'), { name: 'InsufficientInstanceCapacity' }); mockEC2Client.on(RunInstancesCommand).rejects(error); await expect( ec2Operations.create({ - ...createRunnerConfig({ - ...dedicatedHostRunnerConfig, - scaleErrors: ['InsufficientInstanceCapacity'], - }), + ...createRunnerConfig(dedicatedHostRunnerConfig), numberOfRunners: 2, }), - ).resolves.toEqual({ instances: [], retryableErrorCount: 2, nonRetryableErrorCount: 0 }); + ).resolves.toEqual({ + instances: [], + failedInstanceCount: 2, + failureCodes: ['aws-name:InsufficientInstanceCapacity'], + }); }); - it('returns created instances and a non-retryable failure when RunInstances returns fewer instances', async () => { + it('returns created instances and the missing instance count when RunInstances returns fewer instances', async () => { mockEC2Client.on(RunInstancesCommand).resolves({ Instances: [{ InstanceId: 'i-dedicated-1' }], }); @@ -1604,8 +1623,8 @@ describe('create runner with useDedicatedHost', () => { }), ).resolves.toEqual({ instances: ['i-dedicated-1'], - retryableErrorCount: 0, - nonRetryableErrorCount: 1, + failedInstanceCount: 1, + failureCodes: [], }); }); diff --git a/lambdas/libs/compute-providers/aws/ec2/src/runners.ts b/lambdas/libs/compute-providers/aws/ec2/src/runners.ts index f1df6a19cd..d746888a6f 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/runners.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/runners.ts @@ -21,8 +21,9 @@ import { createChildLogger, tracer } from '@aws-github-runner/aws-powertools-uti import { getParameter } from '@aws-github-runner/aws-ssm-util'; import moment from 'moment'; -import type { CreateRunnerResult, RunnerInfo } from '../../../core'; +import type { RunnerInfo } from '../../../core'; import { getDefaultBlockDeviceNameFromLaunchTemplate } from './launch-template'; +import type { Ec2RunnerCreateResult, Ec2RunnerFailureCode } from './runner-create-result'; import type { Ec2ListRunnerFilters, Ec2OverrideConfig, RunnerInputParameters } from './runners.d'; const logger = createChildLogger('runners'); @@ -38,7 +39,7 @@ export interface Ec2RunnerRequestContext { export interface Ec2RunnerResourceOperations { list(filters?: Ec2ListRunnerFilters): Promise; - create(runnerParameters: RunnerInputParameters): Promise; + create(runnerParameters: RunnerInputParameters): Promise; terminate(instanceId: string): Promise; tag(instanceId: string, tags: Tag[]): Promise; untag(instanceId: string, tags: Tag[]): Promise; @@ -210,55 +211,48 @@ interface AwsErrorLike extends Error { }; } -const RETRYABLE_AWS_ERROR_NAMES = new Set([ - 'EC2ThrottledException', - 'InternalError', - 'RequestLimitExceeded', - 'RequestTimeout', - 'RequestTimeoutException', - 'ServiceUnavailable', - 'Throttling', - 'ThrottlingException', -]); - -const RETRYABLE_NETWORK_ERROR_CODES = new Set([ - 'EAI_AGAIN', - 'ECONNREFUSED', - 'ECONNRESET', - 'ENETUNREACH', - 'ENOTFOUND', - 'ETIMEDOUT', -]); - -function isRetryableAwsError(error: unknown, configuredRetryableErrors: string[]): boolean { - if (!(error instanceof Error)) { - return false; - } +const MAX_ERROR_CAUSE_DEPTH = 10; +const SAFE_FAILURE_IDENTIFIER = /^[A-Za-z][A-Za-z0-9._-]{0,127}$/; - const awsError = error as AwsErrorLike; - if (isRetryableAwsErrorName(awsError.name, configuredRetryableErrors)) { - return true; - } +function safeFailureIdentifier(value: unknown): string | undefined { + return typeof value === 'string' && SAFE_FAILURE_IDENTIFIER.test(value) ? value : undefined; +} - const httpStatusCode = awsError.$metadata?.httpStatusCode; - if ( - awsError.$fault === 'server' || - httpStatusCode === 429 || - (httpStatusCode !== undefined && httpStatusCode >= 500) || - (awsError.code !== undefined && RETRYABLE_NETWORK_ERROR_CODES.has(awsError.code)) - ) { - return true; - } +function requestFailureCodes(error: unknown): Ec2RunnerFailureCode[] { + const failureCodes = new Set(); + const visited = new Set(); + let current = error; - if (awsError.cause && awsError.cause !== error) { - return isRetryableAwsError(awsError.cause, configuredRetryableErrors); + for (let depth = 0; depth < MAX_ERROR_CAUSE_DEPTH && current instanceof Error; depth += 1) { + if (visited.has(current)) break; + visited.add(current); + const awsError = current as AwsErrorLike; + const errorName = safeFailureIdentifier(awsError.name); + const errorCode = safeFailureIdentifier(awsError.code); + if (errorName) failureCodes.add(`aws-name:${errorName}`); + if (errorCode) failureCodes.add(`aws-code:${errorCode}`); + if (awsError.$fault === 'client' || awsError.$fault === 'server') { + failureCodes.add(`aws-fault:${awsError.$fault}`); + } + const httpStatusCode = awsError.$metadata?.httpStatusCode; + if (typeof httpStatusCode === 'number' && Number.isSafeInteger(httpStatusCode) && httpStatusCode >= 0) { + failureCodes.add(`aws-http:${httpStatusCode}`); + } + current = awsError.cause; } - return false; + return [...failureCodes]; } -function isRetryableAwsErrorName(errorName: string, configuredRetryableErrors: string[]): boolean { - return configuredRetryableErrors.includes(errorName) || RETRYABLE_AWS_ERROR_NAMES.has(errorName); +function fleetFailureCodes(errors: FleetError[]): Ec2RunnerFailureCode[] { + return [ + ...new Set( + errors.flatMap((error): Ec2RunnerFailureCode[] => { + const errorCode = safeFailureIdentifier(error.ErrorCode); + return errorCode ? [`aws-name:${errorCode}`] : []; + }), + ), + ]; } // The instance_allocation_strategy variable accepts the union of spot and on-demand strategies, @@ -367,7 +361,7 @@ async function createEc2Runner( ec2Client: EC2Client, runnerParameters: RunnerInputParameters, signal: AbortSignal | undefined, -): Promise { +): Promise { logger.debug('Runner configuration.', { runner: { configuration: { @@ -381,13 +375,12 @@ async function createEc2Runner( amiIdOverride = await getAmiIdOverride(runnerParameters); } catch (error) { throwIfAborted(signal, error); - const retryable = isRetryableAwsError(error, runnerParameters.scaleErrors); + const failureCodes = requestFailureCodes(error); logger.warn('Runner creation failed before an EC2 request could be made.', { - error: error as Error, - retryable, failedInstanceCount: runnerParameters.numberOfRunners, + failureCodes, }); - return failedCreateRunnerResult(runnerParameters.numberOfRunners, retryable); + return failedCreateRunnerResult(runnerParameters.numberOfRunners, failureCodes); } // EC2 Fleet (CreateFleet) does not support launching instances onto dedicated hosts @@ -404,9 +397,12 @@ async function createEc2Runner( fleet = await createInstances(runnerParameters, amiIdOverride, ec2Client, signal); } catch (error) { throwIfAborted(signal, error); - const retryable = isRetryableAwsError(error, runnerParameters.scaleErrors); - logger.warn('Create fleet request failed.', { error: error as Error, retryable }); - return failedCreateRunnerResult(runnerParameters.numberOfRunners, retryable); + const failureCodes = requestFailureCodes(error); + logger.warn('Create fleet request failed.', { + failedInstanceCount: runnerParameters.numberOfRunners, + failureCodes, + }); + return failedCreateRunnerResult(runnerParameters.numberOfRunners, failureCodes); } const result = await processFleetResult(fleet, runnerParameters, ec2Client, signal); @@ -421,7 +417,7 @@ async function processFleetResult( runnerParameters: RunnerInputParameters, ec2Client: EC2Client, signal: AbortSignal | undefined, -): Promise { +): Promise { const instances: string[] = fleet.Instances?.flatMap((i) => i.InstanceIds?.flatMap((j) => j) || []) || []; if (instances.length === runnerParameters.numberOfRunners) { @@ -465,57 +461,25 @@ async function processFleetResult( instances.push(...onDemandResult.instances); return { instances, - retryableErrorCount: onDemandResult.retryableErrorCount, - nonRetryableErrorCount: onDemandResult.nonRetryableErrorCount, + failedInstanceCount: onDemandResult.failedInstanceCount, + failureCodes: onDemandResult.failureCodes, }; } - const configuredRetryableErrors = runnerParameters.scaleErrors; - const { fleetErrorsTriggeringRetry, fleetErrorsNotTriggeringRetry } = classifyFleetErrors( - fleet.Errors || [], - configuredRetryableErrors, - ); - const missingInstanceCount = runnerParameters.numberOfRunners - instances.length; - // CreateFleet errors describe failed launch-template overrides, not individual instances. - // A retryable override failure can therefore account for any number of missing instances. - const retryableErrorCount = fleetErrorsTriggeringRetry.length > 0 ? missingInstanceCount : 0; - const nonRetryableErrorCount = missingInstanceCount - retryableErrorCount; + const failureCodes = fleetFailureCodes(fleet.Errors || []); logger.warn('Create fleet did not create every requested instance.', { - data: fleet.Errors, - retryableErrorCount, - nonRetryableErrorCount, - fleetErrorsTriggeringRetry: structuredClone(fleetErrorsTriggeringRetry), - fleetErrorsNotTriggeringRetry: structuredClone(fleetErrorsNotTriggeringRetry), + failedInstanceCount: missingInstanceCount, + failureCodes, }); - return { instances, retryableErrorCount, nonRetryableErrorCount }; -} - -function classifyFleetErrors( - errors: FleetError[], - configuredRetryableErrors: string[], -): { fleetErrorsTriggeringRetry: FleetError[]; fleetErrorsNotTriggeringRetry: FleetError[] } { - return errors.reduce<{ - fleetErrorsTriggeringRetry: FleetError[]; - fleetErrorsNotTriggeringRetry: FleetError[]; - }>( - (classifiedErrors, error) => { - if (isRetryableAwsErrorName(error.ErrorCode || '', configuredRetryableErrors)) { - classifiedErrors.fleetErrorsTriggeringRetry.push(error); - } else { - classifiedErrors.fleetErrorsNotTriggeringRetry.push(error); - } - return classifiedErrors; - }, - { fleetErrorsTriggeringRetry: [], fleetErrorsNotTriggeringRetry: [] }, - ); + return { instances, failedInstanceCount: missingInstanceCount, failureCodes }; } function processRunInstanceResult( result: RunInstancesCommandOutput, runnerParameters: RunnerInputParameters, -): CreateRunnerResult { +): Ec2RunnerCreateResult { const instances = result.Instances?.map((i) => i.InstanceId!).filter(Boolean) || []; if (instances.length === runnerParameters.numberOfRunners) { @@ -529,24 +493,25 @@ function processRunInstanceResult( { data: result }, ); - const nonRetryableErrorCount = runnerParameters.numberOfRunners - instances.length; + const failedInstanceCount = runnerParameters.numberOfRunners - instances.length; logger.warn('RunInstances did not create every requested instance.', { - data: result, - retryable: false, - nonRetryableErrorCount, + failedInstanceCount, }); - return { instances, retryableErrorCount: 0, nonRetryableErrorCount }; + return { instances, failedInstanceCount, failureCodes: [] }; } -function successfulCreateRunnerResult(instances: string[]): CreateRunnerResult { - return { instances, retryableErrorCount: 0, nonRetryableErrorCount: 0 }; +function successfulCreateRunnerResult(instances: string[]): Ec2RunnerCreateResult { + return { instances, failedInstanceCount: 0, failureCodes: [] }; } -function failedCreateRunnerResult(failedInstanceCount: number, isRetryable: boolean): CreateRunnerResult { +function failedCreateRunnerResult( + failedInstanceCount: number, + failureCodes: Ec2RunnerFailureCode[] = [], +): Ec2RunnerCreateResult { return { instances: [], - retryableErrorCount: isRetryable ? failedInstanceCount : 0, - nonRetryableErrorCount: isRetryable ? 0 : failedInstanceCount, + failedInstanceCount, + failureCodes, }; } @@ -568,7 +533,7 @@ async function getAmiIdOverride(runnerParameters: RunnerInputParameters): Promis logger.debug( `Failed to lookup runner AMI ID from SSM parameter: ${runnerParameters.amiIdSsmParameterName}. ` + 'Please ensure that the given parameter exists on this region and contains a valid runner AMI ID', - { error: e }, + { failureCodes: requestFailureCodes(e) }, ); throw e; } @@ -586,7 +551,6 @@ async function createInstances( { Key: 'ghr:Type', Value: runnerParameters.runnerType }, { Key: 'ghr:Owner', Value: runnerParameters.runnerOwner }, ]; - if (runnerParameters.tracingEnabled) { const traceId = tracer.getRootXrayTraceId(); tags.push({ Key: 'ghr:trace_id', Value: traceId! }); @@ -653,7 +617,7 @@ async function createInstances( logger.debug('CreateFleet request payload.', { payload: createFleetCommand.input }); fleet = await ec2Client.send(createFleetCommand, { abortSignal: signal }); } catch (e) { - logger.warn('Create fleet request failed.', { error: e as Error }); + logger.warn('Create fleet request failed.', { failureCodes: requestFailureCodes(e) }); throw e; } return fleet; @@ -664,14 +628,13 @@ async function createInstancesWithRunInstances( amiIdOverride: string | undefined, ec2Client: EC2Client, signal: AbortSignal | undefined, -): Promise { +): Promise { const tags = [ { Key: 'ghr:Application', Value: 'github-action-runner' }, { Key: 'ghr:created_by', Value: runnerParameters.source }, { Key: 'ghr:Type', Value: runnerParameters.runnerType }, { Key: 'ghr:Owner', Value: runnerParameters.runnerOwner }, ]; - if (runnerParameters.tracingEnabled) { const traceId = tracer.getRootXrayTraceId(); tags.push({ Key: 'ghr:trace_id', Value: traceId! }); @@ -681,7 +644,7 @@ async function createInstancesWithRunInstances( logger.warn( 'Spot instances are not supported with RunInstances. Please set targetCapacityType to on-demand for dedicated hosts.', ); - return failedCreateRunnerResult(runnerParameters.numberOfRunners, false); + return failedCreateRunnerResult(runnerParameters.numberOfRunners); } try { @@ -714,9 +677,12 @@ async function createInstancesWithRunInstances( return processRunInstanceResult(result, runnerParameters); } catch (error) { throwIfAborted(signal, error); - const retryable = isRetryableAwsError(error, runnerParameters.scaleErrors); - logger.warn('RunInstances request failed for dedicated host.', { error: error as Error, retryable }); - return failedCreateRunnerResult(runnerParameters.numberOfRunners, retryable); + const failureCodes = requestFailureCodes(error); + logger.warn('RunInstances request failed for dedicated host.', { + failedInstanceCount: runnerParameters.numberOfRunners, + failureCodes, + }); + return failedCreateRunnerResult(runnerParameters.numberOfRunners, failureCodes); } }