Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(cli): garbage collection ignores review_in_progress stacks #31906

Merged
merged 2 commits into from
Oct 25, 2024
Merged

fix(cli): garbage collection ignores review_in_progress stacks #31906

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
71a34fb
feat(cli): garbage collection ignores review_in_progress stacks
kaizencc Oct 25, 2024
b34738b
Merge branch 'main' into conroy/gcreview
kaizencc Oct 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 1 addition & 11 deletions packages/aws-cdk/lib/api/garbage-collection/garbage-collector.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,13 +109,6 @@ interface GarbageCollectorProps {
*/
readonly bootstrapStackName?: string;

/**
* Max wait time for retries in milliseconds (for testing purposes).
*
* @default 60000
*/
readonly maxWaitTime?: number;

/**
* Confirm with the user before actual deletion happens
*
Expand All @@ -133,7 +126,6 @@ export class GarbageCollector {
private permissionToDelete: boolean;
private permissionToTag: boolean;
private bootstrapStackName: string;
private maxWaitTime: number;
private confirm: boolean;

public constructor(readonly props: GarbageCollectorProps) {
Expand All @@ -144,7 +136,6 @@ export class GarbageCollector {

this.permissionToDelete = ['delete-tagged', 'full'].includes(props.action);
this.permissionToTag = ['tag', 'full'].includes(props.action);
this.maxWaitTime = props.maxWaitTime ?? 60000;
this.confirm = props.confirm ?? true;

this.bootstrapStackName = props.bootstrapStackName ?? DEFAULT_TOOLKIT_STACK_NAME;
Expand All @@ -168,13 +159,12 @@ export class GarbageCollector {
const activeAssets = new ActiveAssetCache();

// Grab stack templates first
await refreshStacks(cfn, activeAssets, this.maxWaitTime, qualifier);
await refreshStacks(cfn, activeAssets, qualifier);
// Start the background refresh
const backgroundStackRefresh = new BackgroundStackRefresh({
cfn,
activeAssets,
qualifier,
maxWaitTime: this.maxWaitTime,
});
backgroundStackRefresh.start();

Expand Down
43 changes: 6 additions & 37 deletions packages/aws-cdk/lib/api/garbage-collection/stack-refresh.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { CloudFormation } from 'aws-sdk';
import { sleep } from '../../../test/util';
import { debug } from '../../logging';

export class ActiveAssetCache {
Expand Down Expand Up @@ -30,41 +29,18 @@ async function paginateSdkCall(cb: (nextToken?: string) => Promise<string | unde
}
}

/** We cannot operate on REVIEW_IN_PROGRESS stacks because we do not know what the template looks like in this case
* If we encounter this status, we will wait up to the maxWaitTime before erroring out
*/
async function listStacksNotBeingReviewed(cfn: CloudFormation, maxWaitTime: number, nextToken: string | undefined) {
let sleepMs = 500;
const deadline = Date.now() + maxWaitTime;

while (Date.now() <= deadline) {
let stacks = await cfn.listStacks({ NextToken: nextToken }).promise();
if (!stacks.StackSummaries?.some(s => s.StackStatus == 'REVIEW_IN_PROGRESS')) {
return stacks;
}
await sleep(Math.floor(Math.random() * sleepMs));
sleepMs = sleepMs * 2;
}

throw new Error(`Stacks still in REVIEW_IN_PROGRESS state after waiting for ${maxWaitTime} ms.`);
}

/**
* Fetches all relevant stack templates from CloudFormation. It ignores the following stacks:
* - stacks in DELETE_COMPLETE or DELETE_IN_PROGRESS stage
* - stacks that are using a different bootstrap qualifier
*
* It fails on the following stacks because we cannot get the template and therefore have an imcomplete
* understanding of what assets are being used.
* - stacks in REVIEW_IN_PROGRESS stage
*/
async function fetchAllStackTemplates(cfn: CloudFormation, maxWaitTime: number, qualifier?: string) {
async function fetchAllStackTemplates(cfn: CloudFormation, qualifier?: string) {
const stackNames: string[] = [];
await paginateSdkCall(async (nextToken) => {
const stacks = await listStacksNotBeingReviewed(cfn, maxWaitTime, nextToken);
const stacks = await cfn.listStacks({ NextToken: nextToken }).promise();

// We ignore stacks with these statuses because their assets are no longer live
const ignoredStatues = ['CREATE_FAILED', 'DELETE_COMPLETE', 'DELETE_IN_PROGRESS', 'DELETE_FAILED'];
const ignoredStatues = ['CREATE_FAILED', 'DELETE_COMPLETE', 'DELETE_IN_PROGRESS', 'DELETE_FAILED', 'REVIEW_IN_PROGRESS'];
stackNames.push(
...(stacks.StackSummaries ?? [])
.filter(s => !ignoredStatues.includes(s.StackStatus))
Expand Down Expand Up @@ -119,9 +95,9 @@ function bootstrapFilter(parameters?: CloudFormation.ParameterDeclarations, qual
splitBootstrapVersion[2] != qualifier);
}

export async function refreshStacks(cfn: CloudFormation, activeAssets: ActiveAssetCache, maxWaitTime: number, qualifier?: string) {
export async function refreshStacks(cfn: CloudFormation, activeAssets: ActiveAssetCache, qualifier?: string) {
try {
const stacks = await fetchAllStackTemplates(cfn, maxWaitTime, qualifier);
const stacks = await fetchAllStackTemplates(cfn, qualifier);
for (const stack of stacks) {
activeAssets.rememberStack(stack);
}
Expand All @@ -148,13 +124,6 @@ export interface BackgroundStackRefreshProps {
* Stack bootstrap qualifier
*/
readonly qualifier?: string;

/**
* Maximum wait time when waiting for stacks to leave REVIEW_IN_PROGRESS stage.
*
* @default 60000
*/
readonly maxWaitTime?: number;
}

/**
Expand All @@ -178,7 +147,7 @@ export class BackgroundStackRefresh {
private async refresh() {
const startTime = Date.now();

await refreshStacks(this.props.cfn, this.props.activeAssets, this.props.maxWaitTime ?? 60000, this.props.qualifier);
await refreshStacks(this.props.cfn, this.props.activeAssets, this.props.qualifier);
this.justRefreshedStacks();

// If the last invocation of refreshStacks takes <5 minutes, the next invocation starts 5 minutes after the last one started.
Expand Down
88 changes: 0 additions & 88 deletions packages/aws-cdk/test/api/garbage-collection.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,6 @@ function gc(props: {
rollbackBufferDays?: number;
createdAtBufferDays?: number;
action: 'full' | 'print' | 'tag' | 'delete-tagged';
maxWaitTime?: number;
}): GarbageCollector {
return new GarbageCollector({
sdkProvider: sdk,
Expand All @@ -47,7 +46,6 @@ function gc(props: {
rollbackBufferDays: props.rollbackBufferDays ?? 0,
createdBufferDays: props.createdAtBufferDays ?? 0,
type: props.type,
maxWaitTime: props.maxWaitTime,
confirm: false,
});
}
Expand Down Expand Up @@ -435,91 +433,6 @@ describe('Garbage Collection', () => {
},
});
});

test('stackStatus in REVIEW_IN_PROGRESS means we wait until it changes', async () => {
mockTheToolkitInfo({
Outputs: [
{
OutputKey: 'BootstrapVersion',
OutputValue: '999',
},
],
});

// Mock the listStacks call
const mockListStacksStatus = jest.fn()
.mockResolvedValueOnce({
StackSummaries: [
{ StackName: 'Stack1', StackStatus: 'REVIEW_IN_PROGRESS' },
{ StackName: 'Stack2', StackStatus: 'UPDATE_COMPLETE' },
],
})
.mockResolvedValueOnce({
StackSummaries: [
{ StackName: 'Stack1', StackStatus: 'UPDATE_COMPLETE' },
{ StackName: 'Stack2', StackStatus: 'UPDATE_COMPLETE' },
],
});

sdk.stubCloudFormation({
listStacks: mockListStacksStatus,
getTemplateSummary: mockGetTemplateSummary,
getTemplate: mockGetTemplate,
});

garbageCollector = garbageCollector = gc({
type: 's3',
rollbackBufferDays: 3,
action: 'full',
});
await garbageCollector.garbageCollect();

// list are called as expected
expect(mockListStacksStatus).toHaveBeenCalledTimes(2);

// everything else runs as expected:
// assets tagged
expect(mockGetObjectTagging).toHaveBeenCalledTimes(3);
expect(mockPutObjectTagging).toHaveBeenCalledTimes(2); // one object already has the tag

// no deleting
expect(mockDeleteObjects).toHaveBeenCalledTimes(0);
}, 60000);

test('fails when stackStatus stuck in REVIEW_IN_PROGRESS', async () => {
mockTheToolkitInfo({
Outputs: [
{
OutputKey: 'BootstrapVersion',
OutputValue: '999',
},
],
});

// Mock the listStacks call
const mockListStacksStatus = jest.fn()
.mockResolvedValue({
StackSummaries: [
{ StackName: 'Stack1', StackStatus: 'REVIEW_IN_PROGRESS' },
{ StackName: 'Stack2', StackStatus: 'UPDATE_COMPLETE' },
],
});

sdk.stubCloudFormation({
listStacks: mockListStacksStatus,
getTemplateSummary: mockGetTemplateSummary,
getTemplate: mockGetTemplate,
});

garbageCollector = garbageCollector = gc({
type: 's3',
rollbackBufferDays: 3,
action: 'full',
maxWaitTime: 600, // Wait only 600 ms in tests
});

await expect(garbageCollector.garbageCollect()).rejects.toThrow(/Stacks still in REVIEW_IN_PROGRESS state after waiting/);
}, 60000);
});

let mockListObjectsV2Large: (params: AWS.S3.Types.ListObjectsV2Request) => AWS.S3.Types.ListObjectsV2Output;
Expand Down Expand Up @@ -680,7 +593,6 @@ describe('BackgroundStackRefresh', () => {
refreshProps = {
cfn: sdk.mockSdk.cloudFormation(),
activeAssets: new ActiveAssetCache(),
maxWaitTime: 60000, // 1 minute
};

backgroundRefresh = new BackgroundStackRefresh(refreshProps);
Expand Down
Loading