Skip to content

Commit

Permalink
feat(cli): allow disabling parallel asset publishing
Browse files Browse the repository at this point in the history
Once again, any change to anything anywhere broke someone. In this
particular case, parallel asset publishing (#19367, and in particular
building) broke a use case where someone uses a tool to build assets
that is not concurrency-safe. So -- now we need to make that
configurable.

Command line:

```
cdk deploy --no-asset-parallelism
```

cdk.json:

```
{
  "assetParallelism": false
}
```

Environment variables:

```
export CDK_ASSET_PARALLELISM=false
```
  • Loading branch information
rix0rrr committed Oct 20, 2022
1 parent be6074a commit 4b9ce0d
Show file tree
Hide file tree
Showing 9 changed files with 158 additions and 14 deletions.
26 changes: 24 additions & 2 deletions packages/aws-cdk/lib/api/cloudformation-deployments.ts
Expand Up @@ -2,7 +2,7 @@ import * as cxapi from '@aws-cdk/cx-api';
import { AssetManifest } from 'cdk-assets';
import { Tag } from '../cdk-toolkit';
import { debug, warning } from '../logging';
import { buildAssets, publishAssets } from '../util/asset-publishing';
import { buildAssets, publishAssets, BuildAssetsOptions, PublishAssetsOptions } from '../util/asset-publishing';
import { Mode } from './aws-auth/credentials';
import { ISDK } from './aws-auth/sdk';
import { SdkProvider } from './aws-auth/sdk-provider';
Expand Down Expand Up @@ -253,6 +253,13 @@ export interface DeployStackOptions {
* @default true To remain backward compatible.
*/
readonly buildAssets?: boolean;

/**
* Whether to build/publish assets in parallel
*
* @default true To remain backward compatible.
*/
readonly assetParallelism?: boolean;
}

export interface BuildStackAssetsOptions {
Expand All @@ -274,6 +281,11 @@ export interface BuildStackAssetsOptions {
* @default - Current role
*/
readonly roleArn?: string;

/**
* Options to pass on to `buildAsests()` function
*/
readonly buildOptions?: BuildAssetsOptions;
}

interface PublishStackAssetsOptions {
Expand All @@ -283,6 +295,11 @@ interface PublishStackAssetsOptions {
* @default true To remain backward compatible.
*/
readonly buildAssets?: boolean;

/**
* Options to pass on to `publishAsests()` function
*/
readonly publishOptions?: Omit<PublishAssetsOptions, 'buildAssets'>;
}

export interface DestroyStackOptions {
Expand Down Expand Up @@ -401,6 +418,9 @@ export class CloudFormationDeployments {
if (options.resourcesToImport === undefined) {
await this.publishStackAssets(options.stack, toolkitInfo, {
buildAssets: options.buildAssets ?? true,
publishOptions: {
parallel: options.assetParallelism,
},
});
}

Expand Down Expand Up @@ -434,6 +454,7 @@ export class CloudFormationDeployments {
extraUserAgent: options.extraUserAgent,
resourcesToImport: options.resourcesToImport,
overrideTemplate: options.overrideTemplate,
assetParallelism: options.assetParallelism,
});
}

Expand Down Expand Up @@ -529,7 +550,7 @@ export class CloudFormationDeployments {
toolkitInfo);

const manifest = AssetManifest.fromFile(assetArtifact.file);
await buildAssets(manifest, this.sdkProvider, stackEnv);
await buildAssets(manifest, this.sdkProvider, stackEnv, options.buildOptions);
}
}

Expand All @@ -549,6 +570,7 @@ export class CloudFormationDeployments {

const manifest = AssetManifest.fromFile(assetArtifact.file);
await publishAssets(manifest, this.sdkProvider, stackEnv, {
...options.publishOptions,
buildAssets: options.buildAssets ?? true,
});
}
Expand Down
11 changes: 10 additions & 1 deletion packages/aws-cdk/lib/api/deploy-stack.ts
Expand Up @@ -198,6 +198,13 @@ export interface DeployStackOptions {
* @default - Use the stored template
*/
readonly overrideTemplate?: any;

/**
* Whether to build/publish assets in parallel
*
* @default true To remain backward compatible.
*/
readonly assetParallelism?: boolean;
}

export type DeploymentMethod =
Expand Down Expand Up @@ -287,7 +294,9 @@ export async function deployStack(options: DeployStackOptions): Promise<DeploySt
options.toolkitInfo,
options.sdk,
options.overrideTemplate);
await publishAssets(legacyAssets.toManifest(stackArtifact.assembly.directory), options.sdkProvider, stackEnv);
await publishAssets(legacyAssets.toManifest(stackArtifact.assembly.directory), options.sdkProvider, stackEnv, {
parallel: options.assetParallelism,
});

if (options.hotswap) {
// attempt to short-circuit the deployment if possible
Expand Down
15 changes: 14 additions & 1 deletion packages/aws-cdk/lib/cdk-toolkit.ts
Expand Up @@ -258,6 +258,7 @@ export class CdkToolkit {
hotswap: options.hotswap,
extraUserAgent: options.extraUserAgent,
buildAssets: false,
assetParallelism: options.assetParallelism,
});

const message = result.noOp
Expand Down Expand Up @@ -764,7 +765,7 @@ export class CdkToolkit {
}
}

private async buildAllAssetsForSingleStack(stack: cxapi.CloudFormationStackArtifact, options: Pick<DeployOptions, 'roleArn' | 'toolkitStackName'>): Promise<void> {
private async buildAllAssetsForSingleStack(stack: cxapi.CloudFormationStackArtifact, options: Pick<DeployOptions, 'roleArn' | 'toolkitStackName' | 'assetParallelism'>): Promise<void> {
// Check whether the stack has an asset manifest before trying to build and publish.
if (!stack.dependencies.some(cxapi.AssetManifestArtifact.isAssetManifestArtifact)) {
return;
Expand All @@ -775,6 +776,9 @@ export class CdkToolkit {
stack,
roleArn: options.roleArn,
toolkitStackName: options.toolkitStackName,
buildOptions: {
parallel: options.assetParallelism,
},
});
print('\n%s: assets built\n', chalk.bold(stack.displayName));
}
Expand Down Expand Up @@ -1029,6 +1033,15 @@ export interface DeployOptions extends CfnDeployOptions, WatchOptions {
* @default 1
*/
readonly concurrency?: number;

/**
* Build/publish assets for a single stack in parallel
*
* Independent of whether stacks are being done in parallel or no.
*
* @default true
*/
readonly assetParallelism?: boolean;
}

export interface ImportOptions extends CfnDeployOptions {
Expand Down
4 changes: 3 additions & 1 deletion packages/aws-cdk/lib/cli.ts
Expand Up @@ -155,7 +155,8 @@ async function parseCommandLineArguments() {
"'true' by default, use --no-logs to turn off. " +
"Only in effect if specified alongside the '--watch' option",
})
.option('concurrency', { type: 'number', desc: 'Maximum number of simultaneous deployments (dependency permitting) to execute.', default: 1, requiresArg: true }),
.option('concurrency', { type: 'number', desc: 'Maximum number of simultaneous deployments (dependency permitting) to execute.', default: 1, requiresArg: true })
.option('asset-parallelism', { type: 'boolean', desc: 'Whether to build/publish assets in parallel' }),
)
.command('import [STACK]', 'Import existing resource(s) into the given STACK', (yargs: Argv) => yargs
.option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true })
Expand Down Expand Up @@ -514,6 +515,7 @@ async function initCommandLine() {
watch: args.watch,
traceLogs: args.logs,
concurrency: args.concurrency,
assetParallelism: args.assetParallelism,
});

case 'import':
Expand Down
1 change: 1 addition & 0 deletions packages/aws-cdk/lib/settings.ts
Expand Up @@ -289,6 +289,7 @@ export class Settings {
lookups: argv.lookups,
rollback: argv.rollback,
notices: argv.notices,
assetParallelism: argv['asset-parallelism'],
});
}

Expand Down
18 changes: 16 additions & 2 deletions packages/aws-cdk/lib/util/asset-publishing.ts
Expand Up @@ -18,6 +18,13 @@ export interface PublishAssetsOptions {
* @default true To remain backward compatible.
*/
readonly buildAssets?: boolean;

/**
* Whether to build/publish assets in parallel
*
* @default true To remain backward compatible.
*/
readonly parallel?: boolean;
}

/**
Expand All @@ -44,7 +51,7 @@ export async function publishAssets(
aws: new PublishingAws(sdk, targetEnv),
progressListener: new PublishingProgressListener(options.quiet ?? false),
throwOnError: false,
publishInParallel: true,
publishInParallel: options.parallel ?? true,
buildAssets: options.buildAssets ?? true,
publishAssets: true,
});
Expand All @@ -59,6 +66,13 @@ export interface BuildAssetsOptions {
* Print progress at 'debug' level
*/
readonly quiet?: boolean;

/**
* Build assets in parallel
*
* @default true
*/
readonly parallel?: boolean;
}

/**
Expand All @@ -85,7 +99,7 @@ export async function buildAssets(
aws: new PublishingAws(sdk, targetEnv),
progressListener: new PublishingProgressListener(options.quiet ?? false),
throwOnError: false,
publishInParallel: true,
publishInParallel: options.parallel ?? true,
buildAssets: true,
publishAssets: false,
});
Expand Down
Expand Up @@ -904,7 +904,7 @@ test('building assets', async () => {
name: 'aws://account/region',
region: 'region',
});
expect(buildAssets).toBeCalledWith(expectedAssetManifest, sdkProvider, expectedEnvironment);
expect(buildAssets).toBeCalledWith(expectedAssetManifest, sdkProvider, expectedEnvironment, undefined);
});

function pushStackResourceSummaries(stackName: string, ...items: CloudFormation.StackResourceSummary[]) {
Expand Down
71 changes: 66 additions & 5 deletions packages/aws-cdk/test/cdk-toolkit.test.ts
Expand Up @@ -52,8 +52,11 @@ jest.mock('../lib/logging', () => ({
...jest.requireActual('../lib/logging'),
data: mockData,
}));
jest.setTimeout(30_000);

import * as path from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import { Manifest } from '@aws-cdk/cloud-assembly-schema';
import * as cxapi from '@aws-cdk/cx-api';
import { Bootstrapper } from '../lib/api/bootstrap';
import { CloudFormationDeployments, DeployStackOptions, DestroyStackOptions } from '../lib/api/cloudformation-deployments';
Expand All @@ -62,7 +65,8 @@ import { Template } from '../lib/api/util/cloudformation';
import { CdkToolkit, Tag } from '../lib/cdk-toolkit';
import { RequireApproval } from '../lib/diff';
import { flatten } from '../lib/util';
import { instanceMockFrom, MockCloudExecutable, TestStackArtifact } from './util';
import { instanceMockFrom, MockCloudExecutable, TestStackArtifact, withMocked } from './util';
import { MockSdkProvider } from './util/mock-sdk';

let cloudExecutable: MockCloudExecutable;
let bootstrapper: jest.Mocked<Bootstrapper>;
Expand Down Expand Up @@ -555,6 +559,36 @@ describe('deploy', () => {
expect(cloudExecutable.hasApp).toEqual(false);
expect(mockSynthesize).not.toHaveBeenCalled();
});

test('can disable asset parallelism', async () => {
// GIVEN
cloudExecutable = new MockCloudExecutable({
stacks: [MockStack.MOCK_STACK_WITH_ASSET],
});
const fakeCloudFormation = new FakeCloudFormation({});

const toolkit = new CdkToolkit({
cloudExecutable,
configuration: cloudExecutable.configuration,
sdkProvider: cloudExecutable.sdkProvider,
cloudFormation: fakeCloudFormation,
});

// WHEN
// Not the best test but following this through to the asset publishing library fails
await withMocked(fakeCloudFormation, 'buildStackAssets', async (mockBuildStackAssets) => {
await toolkit.deploy({
selector: { patterns: ['Test-Stack-Asset'] },
assetParallelism: false,
});

expect(mockBuildStackAssets).toHaveBeenCalledWith(expect.objectContaining({
buildOptions: expect.objectContaining({
parallel: false,
}),
}));
});
});
});
});

Expand Down Expand Up @@ -911,6 +945,23 @@ class MockStack {
},
displayName: 'Test-Stack-A/witherrors',
}
public static readonly MOCK_STACK_WITH_ASSET: TestStackArtifact = {
stackName: 'Test-Stack-Asset',
template: { Resources: { TemplateName: 'Test-Stack-Asset' } },
env: 'aws://123456789012/bermuda-triangle-1',
assetManifest: {
version: Manifest.version(),
files: {
xyz: {
source: {
path: path.resolve(__dirname, '..', 'LICENSE'),
},
destinations: {
},
},
},
},
}
}

class FakeCloudFormation extends CloudFormationDeployments {
Expand All @@ -921,7 +972,7 @@ class FakeCloudFormation extends CloudFormationDeployments {
expectedTags: { [stackName: string]: { [key: string]: string } } = {},
expectedNotificationArns?: string[],
) {
super({ sdkProvider: undefined as any });
super({ sdkProvider: new MockSdkProvider() });

for (const [stackName, tags] of Object.entries(expectedTags)) {
this.expectedTags[stackName] =
Expand All @@ -934,9 +985,17 @@ class FakeCloudFormation extends CloudFormationDeployments {
}

public deployStack(options: DeployStackOptions): Promise<DeployStackResult> {
expect([MockStack.MOCK_STACK_A.stackName, MockStack.MOCK_STACK_B.stackName, MockStack.MOCK_STACK_C.stackName])
.toContain(options.stack.stackName);
expect(options.tags).toEqual(this.expectedTags[options.stack.stackName]);
expect([
MockStack.MOCK_STACK_A.stackName,
MockStack.MOCK_STACK_B.stackName,
MockStack.MOCK_STACK_C.stackName,
MockStack.MOCK_STACK_WITH_ASSET.stackName,
]).toContain(options.stack.stackName);

if (this.expectedTags[options.stack.stackName]) {
expect(options.tags).toEqual(this.expectedTags[options.stack.stackName]);
}

expect(options.notificationArns).toEqual(this.expectedNotificationArns);
return Promise.resolve({
stackArn: `arn:aws:cloudformation:::stack/${options.stack.stackName}/MockedOut`,
Expand All @@ -959,6 +1018,8 @@ class FakeCloudFormation extends CloudFormationDeployments {
return Promise.resolve({});
case MockStack.MOCK_STACK_C.stackName:
return Promise.resolve({});
case MockStack.MOCK_STACK_WITH_ASSET.stackName:
return Promise.resolve({});
default:
return Promise.reject(`Not an expected mock stack: ${stack.stackName}`);
}
Expand Down

0 comments on commit 4b9ce0d

Please sign in to comment.