diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts b/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts index b218fe83c6..aa57e9c854 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-up.test.ts @@ -16,6 +16,7 @@ import type { CreateScaleUpRunnersInput, ScaleUpComputeProvider, } from './types'; +import { InvalidRunnerLabelsError } from '@aws-github-runner/compute-providers/core'; import { defaultComputeProvider } from '@aws-github-runner/compute-providers/provider-types'; import { getParameter } from '@aws-github-runner/aws-ssm-util'; import { beforeEach, describe, expect, it, vi } from 'vitest'; @@ -738,6 +739,41 @@ describe('scaleUp with GHES', () => { } }); + it('discards an invalid label group and continues processing valid groups', async () => { + const invalidMessage = { + ...TEST_DATA_SINGLE, + labels: ['self-hosted', 'ghr-provider-size:invalid'], + messageId: 'invalid-message', + }; + const validMessage = { + ...TEST_DATA_SINGLE, + labels: ['self-hosted', 'ghr-provider-size:large'], + messageId: 'valid-message', + }; + mockResolveLabelsForRunners.mockImplementation(async (labels) => { + if (labels.includes('ghr-provider-size:invalid')) { + throw new InvalidRunnerLabelsError('Invalid runner labels'); + } + return { + runnerLabels: labels.filter((label) => label.startsWith('ghr-')), + state: testProviderState, + }; + }); + + await expect(scaleUpModule.scaleUp([invalidMessage, validMessage])).resolves.toEqual([]); + + expect(mockCreateRunners).toHaveBeenCalledTimes(1); + expect(mockCreateRunners).toHaveBeenCalledWith( + expect.objectContaining({ + githubRunnerConfig: expect.objectContaining({ + runnerLabels: 'base-label,ghr-provider-size:large', + }), + }), + ); + expect(mockPublishRetryMessage).toHaveBeenCalledTimes(1); + expect(mockPublishRetryMessage).toHaveBeenCalledWith(validMessage); + }); + it('preserves base RUNNER_LABELS for each group without mutation', async () => { process.env.RUNNER_LABELS = 'ubuntu-2404,x64'; diff --git a/lambdas/functions/control-plane/src/scale-runners/scale-up.ts b/lambdas/functions/control-plane/src/scale-runners/scale-up.ts index 44d522a1f0..c43650dd13 100644 --- a/lambdas/functions/control-plane/src/scale-runners/scale-up.ts +++ b/lambdas/functions/control-plane/src/scale-runners/scale-up.ts @@ -1,4 +1,5 @@ import { addPersistentContextToChildLogger, createChildLogger } from '@aws-github-runner/aws-powertools-util'; +import { InvalidRunnerLabelsError } from '@aws-github-runner/compute-providers/core'; import { resolveComputeProviderType } from '@aws-github-runner/compute-providers/provider-types'; import { Octokit } from '@octokit/rest'; import yn from 'yn'; @@ -20,6 +21,7 @@ import type { ActionRequestMessageSQS, CreateGitHubRunnerConfig, CreateRunnerResult, + RunnerLabelResolution, } from './types'; const logger = createChildLogger('scale-up'); @@ -202,7 +204,21 @@ export async function scaleUp(payloads: ActionRequestMessageSQS[]): Promise 0 ? (messages[0].labels ?? []) : []; - const runnerLabelResolution = await computeProvider.resolveLabelsForRunners(messageLabels); + let runnerLabelResolution: RunnerLabelResolution; + try { + runnerLabelResolution = await computeProvider.resolveLabelsForRunners(messageLabels); + } catch (error) { + if (!(error instanceof InvalidRunnerLabelsError)) { + throw error; + } + + logger.warn('Invalid runner labels; messages will not be retried.', { + error, + labels: messageLabels, + messageIds: messages.map(({ messageId }) => messageId), + }); + continue; + } const resolvedRunnerLabels = runnerLabelResolution.runnerLabels; if (resolvedRunnerLabels.length > 0) { diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/dynamic-labels.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/dynamic-labels.ts index ec6ac2489d..5bbc28a929 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/dynamic-labels.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/dynamic-labels.ts @@ -29,6 +29,7 @@ import { VolumeType, } from '@aws-sdk/client-ec2'; +import { InvalidRunnerLabelsError } from '../../../../core'; import { Ec2OverrideConfig } from '../runners.d'; const EC2_OVERRIDE_LIST_VALUE_SEPARATOR = ';'; @@ -333,6 +334,12 @@ export function parseEc2OverrideConfig( return Object.keys(config).length > 0 ? config : undefined; } +export function validateEc2OverrideConfig(config: Ec2OverrideConfig): void { + if (config.InstanceType && config.InstanceRequirements) { + throw new InvalidRunnerLabelsError('InstanceType and InstanceRequirements cannot be used together'); + } +} + function splitEc2OverrideListValue(value: string): string[] { return value.split(EC2_OVERRIDE_LIST_VALUE_SEPARATOR); } 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 22785e0268..f99cd035c5 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 @@ -1,8 +1,9 @@ +import { InvalidRunnerLabelsError } from '../../../../core'; import type { CreateGitHubRunnerConfig, CreateStartRunnerConfig, RunnerType } from '../../../../core'; import type { Octokit } from '@octokit/rest'; import { beforeEach, describe, expect, it, vi } from 'vitest'; -import { parseEc2OverrideConfig } from './dynamic-labels'; +import { parseEc2OverrideConfig, validateEc2OverrideConfig } from './dynamic-labels'; import { EC2_TAG_VALUE_MAX_LENGTH, RUNNER_LABELS_TAG_MAX_COUNT } from './runner-creation'; import type { Ec2RunnerProvisioningOperations } from '../runners'; import type { RunnerInputParameters } from '../runners.d'; @@ -454,20 +455,40 @@ describe('scaleUp with GHES', () => { ); }); - it('includes both instance type and ec2OverrideConfig when both specified', async () => { - await createProviderRunners({ - baseRunnerLabels: 'base-label', - labels: ['self-hosted', 'ghr-ec2-instance-type:c5.xlarge', 'ghr-ec2-vcpu-count-min:4'], - }); - expect(mockCreateRunner).toHaveBeenCalledWith( - expect.objectContaining({ - ec2instanceCriteria: expect.objectContaining({ instanceTypes: ['t3.medium', 't3.large'] }), - ec2OverrideConfig: expect.objectContaining({ - InstanceType: 'c5.xlarge', - InstanceRequirements: expect.objectContaining({ VCpuCount: { Min: 4 } }), - }), + it('rejects instance type and instance requirements before runner creation', async () => { + await expect( + createProviderRunners({ + baseRunnerLabels: 'base-label', + labels: ['self-hosted', 'ghr-ec2-instance-type:c5.xlarge', 'ghr-ec2-vcpu-count-min:4'], }), - ); + ).rejects.toThrow(InvalidRunnerLabelsError); + + expect(mockCreateRunner).not.toHaveBeenCalled(); + }); + }); + + describe('validateEc2OverrideConfig', () => { + it('accepts instance requirements without an instance type', () => { + expect(() => + validateEc2OverrideConfig({ + InstanceRequirements: { + VCpuCount: { Min: 4 }, + MemoryMiB: { Min: 8192 }, + }, + }), + ).not.toThrow(); + }); + + it('rejects instance type with instance requirements', () => { + expect(() => + validateEc2OverrideConfig({ + InstanceType: 'c5.xlarge', + InstanceRequirements: { + VCpuCount: { Min: 4 }, + MemoryMiB: { Min: 8192 }, + }, + }), + ).toThrow(InvalidRunnerLabelsError); }); }); diff --git a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.ts b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.ts index 0cbda11a8f..01a2b7eab1 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/control-plane/scale-up.ts @@ -4,7 +4,11 @@ import yn from 'yn'; import type { Ec2RunnerProvisioningOperations } from '../runners'; import type { Ec2OverrideConfig } from '../runners.d'; -import { parseEc2OverrideConfig, shouldLoadLaunchTemplateBlockDeviceName } from './dynamic-labels'; +import { + parseEc2OverrideConfig, + shouldLoadLaunchTemplateBlockDeviceName, + validateEc2OverrideConfig, +} from './dynamic-labels'; import { createRunners, loadEc2ProviderConfig } from './runner-creation'; import type { CreateEC2RunnerConfig } from './runner-creation'; @@ -40,6 +44,7 @@ async function resolveEc2ScaleUpRunnerLabels( ec2OverrideConfig = parseEc2OverrideConfig(dynamicEC2Labels, defaultBlockDeviceName); if (ec2OverrideConfig) { + validateEc2OverrideConfig(ec2OverrideConfig); logger.debug('EC2 override config parsed from labels', { ec2OverrideConfig }); } } 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 dd80e7b4dd..9e5c116ef4 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/runners.test.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/runners.test.ts @@ -676,6 +676,49 @@ describe('create runner', () => { }); }); + it('uses InstanceRequirements without static InstanceType overrides', async () => { + const instanceRequirements = { + VCpuCount: { Min: 4, Max: 8 }, + MemoryMiB: { Min: 8192, Max: 16384 }, + AllowedInstanceTypes: ['c7i.*', 'm7i.*'], + }; + + await ec2Operations.create({ + ...createRunnerConfig(defaultRunnerConfig), + ec2OverrideConfig: { InstanceRequirements: instanceRequirements }, + }); + + expect(mockEC2Client).toHaveReceivedCommandWith(CreateFleetCommand, { + LaunchTemplateConfigs: [ + { + LaunchTemplateSpecification: { + LaunchTemplateName: 'lt-1', + Version: '$Default', + }, + Overrides: [ + { + InstanceRequirements: instanceRequirements, + SubnetId: 'subnet-123', + }, + { + InstanceRequirements: instanceRequirements, + SubnetId: 'subnet-456', + }, + ], + }, + ], + SpotOptions: { + AllocationStrategy: SpotAllocationStrategy.CAPACITY_OPTIMIZED, + }, + TagSpecifications: expect.any(Array), + TargetCapacitySpecification: { + DefaultTargetCapacityType: 'spot', + TotalTargetCapacity: 1, + }, + Type: 'instant', + }); + }); + it('overrides ImageId when specified in ec2OverrideConfig', async () => { await ec2Operations.create({ ...createRunnerConfig(defaultRunnerConfig), diff --git a/lambdas/libs/compute-providers/aws/ec2/src/runners.ts b/lambdas/libs/compute-providers/aws/ec2/src/runners.ts index 5889ab3857..ce7f3de80f 100644 --- a/lambdas/libs/compute-providers/aws/ec2/src/runners.ts +++ b/lambdas/libs/compute-providers/aws/ec2/src/runners.ts @@ -285,9 +285,20 @@ function generateFleetOverrides( // Use override values if available, otherwise use parameter arrays const subnetsToUse = ec2OverrideConfig?.SubnetId ? [ec2OverrideConfig.SubnetId] : subnetIds; - const instanceTypesToUse = ec2OverrideConfig?.InstanceType ? [ec2OverrideConfig.InstanceType] : instancesTypes; const amiIdToUse = ec2OverrideConfig?.ImageId ?? amiId; + if (ec2OverrideConfig?.InstanceRequirements) { + return subnetsToUse.map( + (subnetId): FleetLaunchTemplateOverridesRequest => ({ + SubnetId: subnetId, + ImageId: amiIdToUse, + ...ec2OverrideConfig, + }), + ); + } + + const instanceTypesToUse = ec2OverrideConfig?.InstanceType ? [ec2OverrideConfig.InstanceType] : instancesTypes; + // Both the on-demand 'prioritized' and the spot 'capacity-optimized-prioritized' strategies // honor the Priority field of the launch template overrides. const usesPriority = allocationStrategy === 'prioritized' || allocationStrategy === 'capacity-optimized-prioritized'; diff --git a/lambdas/libs/compute-providers/core/index.ts b/lambdas/libs/compute-providers/core/index.ts index c5560942fa..df3d24e817 100644 --- a/lambdas/libs/compute-providers/core/index.ts +++ b/lambdas/libs/compute-providers/core/index.ts @@ -60,6 +60,14 @@ export interface RunnerLabelResolution { state: TState; } +/** Signals that runner labels are permanently invalid and must not be retried. */ +export class InvalidRunnerLabelsError extends Error { + constructor(message: string) { + super(message); + this.name = 'InvalidRunnerLabelsError'; + } +} + export interface CreateRunnerResult { instances: string[]; retryableErrorCount: number;