Skip to content

Commit 7480fd5

Browse files
authored
fix(termination-watcher): only emit SpotInterruptionWarning metric for actual spot interruption events (github-aws-runners#5245)
## Problem Since v7.10.0 (github-aws-runners#5055), the `SpotInterruptionWarning` CloudWatch metric is inflated by 100-350x because it's emitted for **all** instance shutdowns — not just actual spot interruption warnings. The `instance-termination` EventBridge rule added in github-aws-runners#5055 sends `EC2 Instance State-change Notification` (state: `shutting-down`) events to the same `interruptionWarning` Lambda handler. The handler emits `SpotInterruptionWarning` unconditionally for all events passing the tag filter, without checking the event's `detail-type`. ### Impact in production | Period | Lambda Invocations/day | SpotInterruptionWarning Sum/day | |--------|----------------------|-------------------------------| | Before (normal) | 116-272 | 9-44 | | After v7.10.0 | 54,787-93,945 | 7,672-16,446 | ## Fix Gate metric emission on the event's `detail-type`. Only emit `SpotInterruptionWarning` when the event is an actual `EC2 Spot Instance Interruption Warning`. Runner deregistration still triggers for all event types as intended by github-aws-runners#5055. ```typescript const isSpotInterruption = event['detail-type'] === 'EC2 Spot Instance Interruption Warning'; const metricName = isSpotInterruption && config.createSpotWarningMetric ? 'SpotInterruptionWarning' : undefined; ``` ## Testing Added a test case for `EC2 Instance State-change Notification` events verifying: - Metric is **not** emitted (metricName is `undefined`) - Runner deregistration **is** still called Fixes github-aws-runners#5244 --------- Signed-off-by: Brend Smits <brend.smits@philips.com>
1 parent 17cd322 commit 7480fd5

4 files changed

Lines changed: 54 additions & 21 deletions

File tree

lambdas/functions/termination-watcher/src/lambda.ts

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,15 +6,12 @@ import { Context, SQSEvent } from 'aws-lambda';
66
import { handle as handleTerminationWarning } from './termination-warning';
77
import { handle as handleTermination } from './termination';
88
import { handleDeregisterRetry, DeregisterRetryMessage } from './deregister';
9-
import { BidEvictedDetail, BidEvictedEvent, SpotInterruptionWarning, SpotTerminationDetail } from './types';
9+
import { BidEvictedDetail, BidEvictedEvent, TerminationWatcherEvent } from './types';
1010
import { Config } from './ConfigResolver';
1111

1212
const config = new Config();
1313

14-
export async function interruptionWarning(
15-
event: SpotInterruptionWarning<SpotTerminationDetail>,
16-
context: Context,
17-
): Promise<void> {
14+
export async function interruptionWarning(event: TerminationWatcherEvent, context: Context): Promise<void> {
1815
setContext(context, 'lambda.ts');
1916
logger.logEventIfEnabled(event);
2017
logger.debug('Configuration of the lambda', { config });

lambdas/functions/termination-watcher/src/termination-warning.test.ts

Lines changed: 35 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { EC2Client, Instance } from '@aws-sdk/client-ec2';
22
import { mockClient } from 'aws-sdk-client-mock';
33
import 'aws-sdk-client-mock-jest';
44
import { handle } from './termination-warning';
5-
import { SpotInterruptionWarning, SpotTerminationDetail } from './types';
5+
import { SpotInterruptionWarning, SpotTerminationDetail, InstanceStateChangeEvent } from './types';
66
import { metricEvent } from './metric-event';
77
import { deregisterRunner } from './deregister';
88

@@ -36,7 +36,7 @@ const config = {
3636
ghesApiUrl: '',
3737
};
3838

39-
const event: SpotInterruptionWarning<SpotTerminationDetail> = {
39+
const spotEvent: SpotInterruptionWarning<SpotTerminationDetail> = {
4040
version: '0',
4141
id: '1',
4242
'detail-type': 'EC2 Spot Instance Interruption Warning',
@@ -51,8 +51,23 @@ const event: SpotInterruptionWarning<SpotTerminationDetail> = {
5151
},
5252
};
5353

54+
const stateChangeEvent: InstanceStateChangeEvent = {
55+
version: '0',
56+
id: '2',
57+
'detail-type': 'EC2 Instance State-change Notification',
58+
source: 'aws.ec2',
59+
account: '123456789012',
60+
time: '2015-11-11T21:30:00Z',
61+
region: 'us-east-1',
62+
resources: ['arn:aws:ec2:us-east-1b:instance/i-abcd1111'],
63+
detail: {
64+
'instance-id': 'i-abcd1111',
65+
state: 'shutting-down',
66+
},
67+
};
68+
5469
const instance: Instance = {
55-
InstanceId: event.detail['instance-id'],
70+
InstanceId: 'i-abcd1111',
5671
InstanceType: 't2.micro',
5772
Tags: [
5873
{ Key: 'Name', Value: 'test-instance' },
@@ -68,28 +83,27 @@ describe('handle termination warning', () => {
6883
vi.clearAllMocks();
6984
});
7085

71-
it('should log and create an metric', async () => {
86+
it('should emit metric for spot interruption events', async () => {
7287
vi.mocked(getInstances).mockResolvedValue([instance]);
73-
await handle(event, config);
88+
await handle(spotEvent, config);
7489

75-
expect(metricEvent).toHaveBeenCalled();
76-
expect(metricEvent).toHaveBeenCalledWith(instance, event, 'SpotInterruptionWarning', expect.anything());
90+
expect(metricEvent).toHaveBeenCalledWith(instance, spotEvent, 'SpotInterruptionWarning', expect.anything());
7791
expect(deregisterRunner).toHaveBeenCalledWith(instance, config);
7892
});
7993

80-
it('should log details and not create a metric', async () => {
94+
it('should not emit metric when createSpotWarningMetric is false', async () => {
8195
vi.mocked(getInstances).mockResolvedValue([instance]);
8296

8397
const noMetricConfig = { ...config, createSpotWarningMetric: false };
84-
await handle(event, noMetricConfig);
85-
expect(metricEvent).toHaveBeenCalledWith(instance, event, undefined, expect.anything());
98+
await handle(spotEvent, noMetricConfig);
99+
expect(metricEvent).toHaveBeenCalledWith(instance, spotEvent, undefined, expect.anything());
86100
expect(deregisterRunner).toHaveBeenCalledWith(instance, noMetricConfig);
87101
});
88102

89-
it('should not create a metric if filter not matched.', async () => {
103+
it('should not emit metric or deregister if filter not matched', async () => {
90104
vi.mocked(getInstances).mockResolvedValue([instance]);
91105

92-
await handle(event, {
106+
await handle(spotEvent, {
93107
createSpotWarningMetric: true,
94108
createSpotTerminationMetric: false,
95109
tagFilters: { 'ghr:environment': '_NO_MATCH_' },
@@ -101,4 +115,13 @@ describe('handle termination warning', () => {
101115
expect(metricEvent).not.toHaveBeenCalled();
102116
expect(deregisterRunner).not.toHaveBeenCalled();
103117
});
118+
119+
it('should not emit metric for instance state-change events but still deregister', async () => {
120+
vi.mocked(getInstances).mockResolvedValue([instance]);
121+
122+
await handle(stateChangeEvent, config);
123+
124+
expect(metricEvent).toHaveBeenCalledWith(instance, stateChangeEvent, undefined, expect.anything());
125+
expect(deregisterRunner).toHaveBeenCalledWith(instance, config);
126+
});
104127
});

lambdas/functions/termination-watcher/src/termination-warning.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { createChildLogger, getTracedAWSV3Client } from '@aws-github-runner/aws-powertools-util';
2-
import { SpotInterruptionWarning, SpotTerminationDetail } from './types';
2+
import { TerminationWatcherEvent } from './types';
33
import { EC2Client, Instance } from '@aws-sdk/client-ec2';
44
import { Config } from './ConfigResolver';
55
import { tagFilter, getInstances } from './ec2';
@@ -8,7 +8,7 @@ import { deregisterRunner } from './deregister';
88

99
const logger = createChildLogger('termination-warning');
1010

11-
async function handle(event: SpotInterruptionWarning<SpotTerminationDetail>, config: Config): Promise<void> {
11+
async function handle(event: TerminationWatcherEvent, config: Config): Promise<void> {
1212
logger.debug('Received spot notification warning:', { event });
1313
const ec2 = getTracedAWSV3Client(new EC2Client({ region: process.env.AWS_REGION }));
1414
const instances = await getInstances(ec2, [event.detail['instance-id']]);
@@ -19,14 +19,16 @@ async function handle(event: SpotInterruptionWarning<SpotTerminationDetail>, con
1919

2020
async function createMetricForInstances(
2121
instances: Instance[],
22-
event: SpotInterruptionWarning<SpotTerminationDetail>,
22+
event: TerminationWatcherEvent,
2323
config: Config,
2424
): Promise<void> {
2525
for (const instance of instances) {
2626
const matchFilter = tagFilter(instance, config.tagFilters);
2727

2828
if (matchFilter) {
29-
metricEvent(instance, event, config.createSpotWarningMetric ? 'SpotInterruptionWarning' : undefined, logger);
29+
const isSpotInterruption = event['detail-type'] === 'EC2 Spot Instance Interruption Warning';
30+
const metricName = isSpotInterruption && config.createSpotWarningMetric ? 'SpotInterruptionWarning' : undefined;
31+
metricEvent(instance, event, metricName, logger);
3032
await deregisterRunner(instance, config);
3133
} else {
3234
logger.debug(

lambdas/functions/termination-watcher/src/types.d.ts

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,3 +42,14 @@ interface UserIdentity {
4242
interface ServiceEventDetails {
4343
instanceIdSet: string[];
4444
}
45+
46+
export interface InstanceStateChangeDetail {
47+
'instance-id': string;
48+
state: string;
49+
}
50+
51+
// eslint-disable-next-line @typescript-eslint/no-empty-object-type
52+
export interface InstanceStateChangeEvent
53+
extends EventBridgeEvent<'EC2 Instance State-change Notification', InstanceStateChangeDetail> {}
54+
55+
export type TerminationWatcherEvent = SpotInterruptionWarning<SpotTerminationDetail> | InstanceStateChangeEvent;

0 commit comments

Comments
 (0)