Skip to content

Commit

Permalink
feat(cli): allow disabling parallel asset publishing (#22579)
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
```


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
  • Loading branch information
rix0rrr authored Oct 26, 2022
1 parent 42160fc commit 69981ac
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading

0 comments on commit 69981ac

Please sign in to comment.