Skip to content

Commit 1a26b61

Browse files
jeanschmidtjeanschmidt
authored
Fix orphan EBS volumes on runner termination (#7930)
**Impact:** AWS cost — eliminates ~1,000 orphaned 150 GiB gp3 volumes/day in us-east-1 **Risk:** low ## What Before terminating a runner instance, verify that all attached EBS volumes have `DeleteOnTermination=true` and fix any that don't. This prevents replacement root volumes (created by `createReplaceRootVolumeTask` during runner reuse) from persisting after instance termination. ## Why The runner reuse flow calls `createReplaceRootVolumeTask` to reset the root volume from the AMI snapshot. While the old volume is deleted (`DeleteReplacedRootVolume: true`), the new replacement volume does not reliably inherit `DeleteOnTermination=true`. When the instance is later terminated, the replacement volume survives — untagged and unattached. At the time of discovery: **10,436 orphaned volumes totaling 1.8 TiB** in us-east-1 alone, growing at ~1,000 volumes/day. ## How - The fix is **best-effort and non-blocking**: `ensureDeleteOnTermination` is wrapped in a try/catch that logs a warning on failure, so termination always proceeds. A subsequent scale-down run or cleanup script can catch any misses. - Runs **before every termination**, not just after reuse, because the cost of the extra `DescribeInstanceAttribute` call is negligible and it defends against any future code path that might also produce volumes without the flag. - IAM permissions are scoped with the existing `ec2:ResourceTag/Application: github-action-runner` condition, so the lambda can only modify runner instances. ## Changes - **`runners.ts`**: Added `ensureDeleteOnTermination(ec2, instanceId, awsRegion)` — queries `blockDeviceMapping` attribute, filters volumes where `DeleteOnTermination === false`, and calls `modifyInstanceAttribute` to fix them. Called at the top of `terminateRunner` before the terminate API call. - **`runners.test.ts`**: Added 5 unit tests for `ensureDeleteOnTermination` (all-good, needs-fix, mixed, empty, API-error) and 2 integration tests verifying `terminateRunner` calls it and still terminates on failure. - **`lambda-scale-down.json`**: Added `ec2:DescribeInstanceAttribute` and `ec2:ModifyInstanceAttribute` to the IAM policy, under the same tag-based condition as `ec2:TerminateInstances`. ## Testing - Unit tests cover all branches: all volumes OK, some need fixing, mixed, empty response, and API failure. - Integration tests confirm `terminateRunner` invokes the fix before termination and still terminates even if the fix fails. - Run `cd terraform-aws-github-runner/modules/runners/lambdas/runners && yarn test` to execute the test suite. --------- Signed-off-by: Jean Schmidt <contato@jschmidt.me>
1 parent 980d560 commit 1a26b61

4 files changed

Lines changed: 217 additions & 2 deletions

File tree

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/metrics.ts‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -896,6 +896,70 @@ export class Metrics {
896896
this.countEntry(`aws.ec2.perException.runInstances.exception`, count, new Map([['Exception', exceptionName]]));
897897
}
898898

899+
/* istanbul ignore next */
900+
ec2DescribeInstanceAttributeAWSCallSuccess(awsRegion: string, ms: number) {
901+
this.countEntry(`aws.calls.total`, 1);
902+
this.countEntry(`aws.ec2.calls.total`, 1);
903+
this.countEntry(`aws.ec2.describeInstanceAttribute.count`, 1);
904+
this.countEntry(`aws.ec2.describeInstanceAttribute.success`, 1);
905+
this.addEntry(`aws.ec2.describeInstanceAttribute.wallclock`, ms);
906+
907+
const dimensions = new Map([['Region', awsRegion]]);
908+
this.countEntry(`aws.calls.perRegion.total`, 1, dimensions);
909+
this.countEntry(`aws.ec2.calls.perRegion.total`, 1, dimensions);
910+
this.countEntry(`aws.ec2.perRegion.describeInstanceAttribute.count`, 1, dimensions);
911+
this.countEntry(`aws.ec2.perRegion.describeInstanceAttribute.success`, 1, dimensions);
912+
this.addEntry(`aws.ec2.perRegion.describeInstanceAttribute.wallclock`, ms, dimensions);
913+
}
914+
915+
/* istanbul ignore next */
916+
ec2DescribeInstanceAttributeAWSCallFailure(awsRegion: string, ms: number) {
917+
this.countEntry(`aws.calls.total`, 1);
918+
this.countEntry(`aws.ec2.calls.total`, 1);
919+
this.countEntry(`aws.ec2.describeInstanceAttribute.count`, 1);
920+
this.countEntry(`aws.ec2.describeInstanceAttribute.failure`, 1);
921+
this.addEntry(`aws.ec2.describeInstanceAttribute.wallclock`, ms);
922+
923+
const dimensions = new Map([['Region', awsRegion]]);
924+
this.countEntry(`aws.calls.perRegion.total`, 1, dimensions);
925+
this.countEntry(`aws.ec2.calls.perRegion.total`, 1, dimensions);
926+
this.countEntry(`aws.ec2.perRegion.describeInstanceAttribute.count`, 1, dimensions);
927+
this.countEntry(`aws.ec2.perRegion.describeInstanceAttribute.failure`, 1, dimensions);
928+
this.addEntry(`aws.ec2.perRegion.describeInstanceAttribute.wallclock`, ms, dimensions);
929+
}
930+
931+
/* istanbul ignore next */
932+
ec2ModifyInstanceAttributeAWSCallSuccess(awsRegion: string, ms: number) {
933+
this.countEntry(`aws.calls.total`, 1);
934+
this.countEntry(`aws.ec2.calls.total`, 1);
935+
this.countEntry(`aws.ec2.modifyInstanceAttribute.count`, 1);
936+
this.countEntry(`aws.ec2.modifyInstanceAttribute.success`, 1);
937+
this.addEntry(`aws.ec2.modifyInstanceAttribute.wallclock`, ms);
938+
939+
const dimensions = new Map([['Region', awsRegion]]);
940+
this.countEntry(`aws.calls.perRegion.total`, 1, dimensions);
941+
this.countEntry(`aws.ec2.calls.perRegion.total`, 1, dimensions);
942+
this.countEntry(`aws.ec2.perRegion.modifyInstanceAttribute.count`, 1, dimensions);
943+
this.countEntry(`aws.ec2.perRegion.modifyInstanceAttribute.success`, 1, dimensions);
944+
this.addEntry(`aws.ec2.perRegion.modifyInstanceAttribute.wallclock`, ms, dimensions);
945+
}
946+
947+
/* istanbul ignore next */
948+
ec2ModifyInstanceAttributeAWSCallFailure(awsRegion: string, ms: number) {
949+
this.countEntry(`aws.calls.total`, 1);
950+
this.countEntry(`aws.ec2.calls.total`, 1);
951+
this.countEntry(`aws.ec2.modifyInstanceAttribute.count`, 1);
952+
this.countEntry(`aws.ec2.modifyInstanceAttribute.failure`, 1);
953+
this.addEntry(`aws.ec2.modifyInstanceAttribute.wallclock`, ms);
954+
955+
const dimensions = new Map([['Region', awsRegion]]);
956+
this.countEntry(`aws.calls.perRegion.total`, 1, dimensions);
957+
this.countEntry(`aws.ec2.calls.perRegion.total`, 1, dimensions);
958+
this.countEntry(`aws.ec2.perRegion.modifyInstanceAttribute.count`, 1, dimensions);
959+
this.countEntry(`aws.ec2.perRegion.modifyInstanceAttribute.failure`, 1, dimensions);
960+
this.addEntry(`aws.ec2.perRegion.modifyInstanceAttribute.wallclock`, ms, dimensions);
961+
}
962+
899963
// RUN
900964
/* istanbul ignore next */
901965
getRunnerTypesSuccess() {

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.test.ts‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import {
22
RunnerInputParameters,
33
createRunner,
4+
ensureDeleteOnTermination,
45
findAmiID,
56
listRunners,
67
listSSMParameters,
@@ -26,6 +27,8 @@ const mockEC2 = {
2627
deleteTags: jest.fn().mockResolvedValue({}),
2728
createReplaceRootVolumeTask: jest.fn().mockResolvedValue({}),
2829
describeInstances: jest.fn().mockResolvedValue({}),
30+
describeInstanceAttribute: jest.fn().mockResolvedValue({ BlockDeviceMappings: [] }),
31+
modifyInstanceAttribute: jest.fn().mockResolvedValue({}),
2932
runInstances: jest.fn().mockResolvedValue({}),
3033
terminateInstances: jest.fn().mockResolvedValue({}),
3134
describeImages: jest.fn().mockImplementation(() =>
@@ -328,10 +331,81 @@ describe('listSSMParameters', () => {
328331
});
329332
});
330333

334+
describe('ensureDeleteOnTermination', () => {
335+
beforeEach(() => {
336+
mockEC2.describeInstanceAttribute.mockClear();
337+
mockEC2.modifyInstanceAttribute.mockClear();
338+
});
339+
340+
it('does nothing when all volumes have DeleteOnTermination=true', async () => {
341+
mockEC2.describeInstanceAttribute.mockResolvedValueOnce({
342+
BlockDeviceMappings: [{ DeviceName: '/dev/xvda', Ebs: { DeleteOnTermination: true, VolumeId: 'vol-111' } }],
343+
});
344+
345+
await ensureDeleteOnTermination(mockEC2 as any, 'i-1234', 'us-east-1', metrics);
346+
347+
expect(mockEC2.describeInstanceAttribute).toBeCalledWith({
348+
InstanceId: 'i-1234',
349+
Attribute: 'blockDeviceMapping',
350+
});
351+
expect(mockEC2.modifyInstanceAttribute).not.toBeCalled();
352+
});
353+
354+
it('fixes volumes with DeleteOnTermination=false', async () => {
355+
mockEC2.describeInstanceAttribute.mockResolvedValueOnce({
356+
BlockDeviceMappings: [{ DeviceName: '/dev/xvda', Ebs: { DeleteOnTermination: false, VolumeId: 'vol-111' } }],
357+
});
358+
359+
await ensureDeleteOnTermination(mockEC2 as any, 'i-5678', 'us-east-1', metrics);
360+
361+
expect(mockEC2.modifyInstanceAttribute).toBeCalledWith({
362+
InstanceId: 'i-5678',
363+
BlockDeviceMappings: [{ DeviceName: '/dev/xvda', Ebs: { DeleteOnTermination: true } }],
364+
});
365+
});
366+
367+
it('only fixes volumes that need fixing', async () => {
368+
mockEC2.describeInstanceAttribute.mockResolvedValueOnce({
369+
BlockDeviceMappings: [
370+
{ DeviceName: '/dev/xvda', Ebs: { DeleteOnTermination: true, VolumeId: 'vol-111' } },
371+
{ DeviceName: '/dev/sdb', Ebs: { DeleteOnTermination: false, VolumeId: 'vol-222' } },
372+
],
373+
});
374+
375+
await ensureDeleteOnTermination(mockEC2 as any, 'i-9999', 'us-east-1', metrics);
376+
377+
expect(mockEC2.modifyInstanceAttribute).toBeCalledWith({
378+
InstanceId: 'i-9999',
379+
BlockDeviceMappings: [{ DeviceName: '/dev/sdb', Ebs: { DeleteOnTermination: true } }],
380+
});
381+
});
382+
383+
it('does nothing when no block devices returned', async () => {
384+
mockEC2.describeInstanceAttribute.mockResolvedValueOnce({ BlockDeviceMappings: [] });
385+
386+
await ensureDeleteOnTermination(mockEC2 as any, 'i-1234', 'us-east-1', metrics);
387+
388+
expect(mockEC2.modifyInstanceAttribute).not.toBeCalled();
389+
});
390+
391+
it('does not throw on failure — logs warning instead', async () => {
392+
mockEC2.describeInstanceAttribute.mockRejectedValueOnce(new Error('API error'));
393+
const warnSpy = jest.spyOn(console, 'warn').mockImplementation();
394+
395+
await ensureDeleteOnTermination(mockEC2 as any, 'i-1234', 'us-east-1', metrics);
396+
397+
expect(warnSpy).toBeCalledWith(expect.stringContaining('Failed to fix DeleteOnTermination'));
398+
expect(mockEC2.modifyInstanceAttribute).not.toBeCalled();
399+
warnSpy.mockRestore();
400+
});
401+
});
402+
331403
describe('terminateRunner', () => {
332404
beforeEach(() => {
333405
mockSSMdescribeParametersRet.mockClear();
334406
mockEC2.terminateInstances.mockClear();
407+
mockEC2.describeInstanceAttribute.mockClear().mockResolvedValue({ BlockDeviceMappings: [] });
408+
mockEC2.modifyInstanceAttribute.mockClear();
335409
const config = {
336410
environment: 'gi-ci',
337411
minimumRunningTimeInMinutes: 45,
@@ -341,12 +415,32 @@ describe('terminateRunner', () => {
341415
resetRunnersCaches();
342416
});
343417

344-
it('calls terminateInstances', async () => {
418+
it('calls ensureDeleteOnTermination before terminateInstances', async () => {
419+
const runner: RunnerInfo = {
420+
awsRegion: Config.Instance.awsRegion,
421+
instanceId: 'i-1234',
422+
environment: 'gi-ci',
423+
};
424+
await terminateRunner(runner, metrics);
425+
426+
expect(mockEC2.describeInstanceAttribute).toBeCalledWith({
427+
InstanceId: 'i-1234',
428+
Attribute: 'blockDeviceMapping',
429+
});
430+
expect(mockEC2.terminateInstances).toBeCalledWith({
431+
InstanceIds: [runner.instanceId],
432+
});
433+
});
434+
435+
it('still terminates even if ensureDeleteOnTermination fails', async () => {
436+
mockEC2.describeInstanceAttribute.mockRejectedValueOnce(new Error('API error'));
345437
const runner: RunnerInfo = {
346438
awsRegion: Config.Instance.awsRegion,
347439
instanceId: 'i-1234',
348440
environment: 'gi-ci',
349441
};
442+
jest.spyOn(console, 'warn').mockImplementation();
443+
350444
await terminateRunner(runner, metrics);
351445

352446
expect(mockEC2.terminateInstances).toBeCalledWith({

‎terraform-aws-github-runner/modules/runners/lambdas/runners/src/scale-runners/runners.ts‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -332,10 +332,65 @@ export async function doDeleteSSMParameter(paramName: string, metrics: Metrics,
332332
}
333333
}
334334

335+
export async function ensureDeleteOnTermination(
336+
ec2: EC2,
337+
instanceId: string,
338+
awsRegion: string,
339+
metrics: Metrics,
340+
): Promise<void> {
341+
try {
342+
const attr = await expBackOff(() => {
343+
return metrics.trackRequestRegion(
344+
awsRegion,
345+
metrics.ec2DescribeInstanceAttributeAWSCallSuccess,
346+
metrics.ec2DescribeInstanceAttributeAWSCallFailure,
347+
() => {
348+
return ec2.describeInstanceAttribute({
349+
InstanceId: instanceId,
350+
Attribute: 'blockDeviceMapping',
351+
});
352+
},
353+
);
354+
});
355+
356+
const devices = attr.BlockDeviceMappings ?? [];
357+
const needsFix = devices.filter((d) => d.Ebs && d.Ebs.DeleteOnTermination === false);
358+
359+
if (needsFix.length === 0) return;
360+
361+
const mappings = needsFix.map((d) => ({
362+
DeviceName: d.DeviceName,
363+
Ebs: { DeleteOnTermination: true },
364+
}));
365+
366+
await expBackOff(() => {
367+
return metrics.trackRequestRegion(
368+
awsRegion,
369+
metrics.ec2ModifyInstanceAttributeAWSCallSuccess,
370+
metrics.ec2ModifyInstanceAttributeAWSCallFailure,
371+
() => {
372+
return ec2.modifyInstanceAttribute({
373+
InstanceId: instanceId,
374+
BlockDeviceMappings: mappings,
375+
});
376+
},
377+
);
378+
});
379+
380+
console.info(`[${awsRegion}] Fixed DeleteOnTermination on ${needsFix.length} volume(s) for ${instanceId}`);
381+
} catch (e) {
382+
// Log but don't block termination — this is a best-effort fix.
383+
// If it fails, the next scale-down run or the cleanup script will catch it.
384+
console.warn(`[${awsRegion}] Failed to fix DeleteOnTermination for ${instanceId}: ${e}`);
385+
}
386+
}
387+
335388
export async function terminateRunner(runner: RunnerInfo, metrics: Metrics): Promise<void> {
336389
try {
337390
const ec2 = new EC2({ region: runner.awsRegion });
338391

392+
await ensureDeleteOnTermination(ec2, runner.instanceId, runner.awsRegion, metrics);
393+
339394
await expBackOff(() => {
340395
return metrics.trackRequestRegion(
341396
runner.awsRegion,

‎terraform-aws-github-runner/modules/runners/policies/lambda-scale-down.json‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
{
2727
"Effect": "Allow",
2828
"Action": [
29-
"ec2:TerminateInstances"
29+
"ec2:TerminateInstances",
30+
"ec2:DescribeInstanceAttribute",
31+
"ec2:ModifyInstanceAttribute"
3032
],
3133
"Resource": ["*"],
3234
"Condition": {

0 commit comments

Comments
 (0)