From 31d6e6596cde03daa7de48aab8aaae1277fd405e Mon Sep 17 00:00:00 2001 From: Josh K Date: Mon, 13 Jul 2020 05:52:41 -0600 Subject: [PATCH] chore(core): allow the bundler to re-use pre-existing bundler output (#8916) This PR changes `AssetStaging` so that the bundler will re-use pre-existing output. Before, the bundler would re-run Docker without considering pre-existing assets, which was slow. Now, when handling a `SOURCE` hash type, the bundler detects and returns pre-existing asset output without re-running Docker. For all other hash types, the bundler outputs to a temp directory before calculating asset hashes. Closes #8882 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../aws-lambda/test/integ.bundling.ts | 1 + .../test/integ.assets.bundling.lit.ts | 1 + packages/@aws-cdk/core/lib/asset-staging.ts | 102 +++++++--- packages/@aws-cdk/core/test/docker-stub.sh | 1 + packages/@aws-cdk/core/test/test.staging.ts | 183 ++++++++++++++++-- 5 files changed, 251 insertions(+), 37 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/test/integ.bundling.ts b/packages/@aws-cdk/aws-lambda/test/integ.bundling.ts index 200ca72f204b6..56c92cfbefbf6 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.bundling.ts +++ b/packages/@aws-cdk/aws-lambda/test/integ.bundling.ts @@ -1,3 +1,4 @@ +/// !cdk-integ pragma:ignore-assets import * as path from 'path'; import { App, CfnOutput, Construct, Stack, StackProps } from '@aws-cdk/core'; import * as lambda from '../lib'; diff --git a/packages/@aws-cdk/aws-s3-assets/test/integ.assets.bundling.lit.ts b/packages/@aws-cdk/aws-s3-assets/test/integ.assets.bundling.lit.ts index 1ba1ad26deee2..abcf8c9598e2a 100644 --- a/packages/@aws-cdk/aws-s3-assets/test/integ.assets.bundling.lit.ts +++ b/packages/@aws-cdk/aws-s3-assets/test/integ.assets.bundling.lit.ts @@ -1,3 +1,4 @@ +/// !cdk-integ pragma:ignore-assets import * as path from 'path'; import * as iam from '@aws-cdk/aws-iam'; import { App, BundlingDockerImage, Construct, Stack, StackProps } from '@aws-cdk/core'; diff --git a/packages/@aws-cdk/core/lib/asset-staging.ts b/packages/@aws-cdk/core/lib/asset-staging.ts index 92a2b6b9bdf97..17a26e1b058ca 100644 --- a/packages/@aws-cdk/core/lib/asset-staging.ts +++ b/packages/@aws-cdk/core/lib/asset-staging.ts @@ -88,12 +88,23 @@ export class AssetStaging extends Construct { this.sourcePath = props.sourcePath; this.fingerprintOptions = props; + // Determine the hash type based on the props as props.assetHashType is + // optional from a caller perspective. + const hashType = this.determineHashType(props.assetHashType, props.assetHash); + if (props.bundling) { - this.bundleDir = this.bundle(props.bundling); + // Determine the source hash in advance of bundling if the asset hash type + // is source so that the bundler can opt to re-use its bundle dir. + const sourceHash = hashType === AssetHashType.SOURCE + ? this.calculateHash(hashType, props.assetHash, props.bundling) + : undefined; + + this.bundleDir = this.bundle(props.bundling, sourceHash); + this.assetHash = sourceHash ?? this.calculateHash(hashType, props.assetHash, props.bundling); + } else { + this.assetHash = this.calculateHash(hashType, props.assetHash); } - this.assetHash = this.calculateHash(props); - const stagingDisabled = this.node.tryGetContext(cxapi.DISABLE_ASSET_STAGING_CONTEXT); if (stagingDisabled) { this.stagedPath = this.bundleDir ?? this.sourcePath; @@ -137,15 +148,37 @@ export class AssetStaging extends Construct { } } - private bundle(options: BundlingOptions): string { + /** + * @Property sourceHash The source hash of the asset. If specified, the bundler + * will attempt to re-use the bundling directory, which is based on a hash of + * both the sourceHash and options. If bundling finds a pre-existing directory, + * the bundler will return it as-is and won't regenerate the bundle. + */ + private bundle(options: BundlingOptions, sourceHash?: string): string { // Temp staging directory in the working directory const stagingTmp = path.join('.', STAGING_TMP); fs.ensureDirSync(stagingTmp); - // Create temp directory for bundling inside the temp staging directory - const bundleDir = path.resolve(fs.mkdtempSync(path.join(stagingTmp, 'asset-bundle-'))); - // Chmod the bundleDir to full access. - fs.chmodSync(bundleDir, 0o777); + let bundleDir: string; + if (sourceHash) { + // When an asset hash is known in advance of bundling, bundling is done into a dedicated staging directory. + bundleDir = path.resolve(path.join(stagingTmp, 'asset-bundle-hash-' + sourceHash)); + + if (fs.existsSync(bundleDir)) { + // Pre-existing bundle directory. The bundle has already been generated once before, so lets provide it + // as-is to the caller. + return bundleDir; + } + + fs.ensureDirSync(bundleDir); + } else { + // When the asset hash isn't known in advance, bundling is done into a temporary staging directory. + + // Create temp directory for bundling inside the temp staging directory + bundleDir = path.resolve(fs.mkdtempSync(path.join(stagingTmp, 'asset-bundle-temp-'))); + // Chmod the bundleDir to full access. + fs.chmodSync(bundleDir, 0o777); + } let user: string; if (options.user) { @@ -181,7 +214,18 @@ export class AssetStaging extends Construct { workingDirectory: options.workingDirectory ?? AssetStaging.BUNDLING_INPUT_DIR, }); } catch (err) { - throw new Error(`Failed to run bundling Docker image for asset ${this.node.path}: ${err}`); + // When bundling fails, keep the bundle output for diagnosability, but + // rename it out of the way so that the next run doesn't assume it has a + // valid bundleDir. + + const bundleErrorDir = bundleDir + '-error'; + if (fs.existsSync(bundleErrorDir)) { + // Remove the last bundleErrorDir. + fs.removeSync(bundleErrorDir); + } + + fs.renameSync(bundleDir, bundleErrorDir); + throw new Error(`Failed to run bundling Docker image for asset ${this.node.path}, bundle output is located at ${bundleErrorDir}: ${err}`); } if (FileSystem.isEmpty(bundleDir)) { @@ -191,35 +235,49 @@ export class AssetStaging extends Construct { return bundleDir; } - private calculateHash(props: AssetStagingProps): string { - let hashType: AssetHashType; - - if (props.assetHash) { - if (props.assetHashType && props.assetHashType !== AssetHashType.CUSTOM) { - throw new Error(`Cannot specify \`${props.assetHashType}\` for \`assetHashType\` when \`assetHash\` is specified. Use \`CUSTOM\` or leave \`undefined\`.`); + /** + * Determines the hash type from user-given prop values. + * + * @param assetHashType Asset hash type construct prop + * @param assetHash Asset hash given in the construct props + */ + private determineHashType(assetHashType?: AssetHashType, assetHash?: string) { + if (assetHash) { + if (assetHashType && assetHashType !== AssetHashType.CUSTOM) { + throw new Error(`Cannot specify \`${assetHashType}\` for \`assetHashType\` when \`assetHash\` is specified. Use \`CUSTOM\` or leave \`undefined\`.`); } - hashType = AssetHashType.CUSTOM; - } else if (props.assetHashType) { - hashType = props.assetHashType; + return AssetHashType.CUSTOM; + } else if (assetHashType) { + return assetHashType; } else { - hashType = AssetHashType.SOURCE; + return AssetHashType.SOURCE; } + } + private calculateHash(hashType: AssetHashType, assetHash?: string, bundling?: BundlingOptions): string { switch (hashType) { case AssetHashType.SOURCE: - return FileSystem.fingerprint(this.sourcePath, this.fingerprintOptions); + const sourceHash = FileSystem.fingerprint(this.sourcePath, this.fingerprintOptions); + if (bundling) { + return crypto.createHash('sha256') + .update(JSON.stringify(bundling)) + .update(sourceHash) + .digest('hex'); + } else { + return sourceHash; + } case AssetHashType.BUNDLE: if (!this.bundleDir) { throw new Error('Cannot use `AssetHashType.BUNDLE` when `bundling` is not specified.'); } return FileSystem.fingerprint(this.bundleDir, this.fingerprintOptions); case AssetHashType.CUSTOM: - if (!props.assetHash) { + if (!assetHash) { throw new Error('`assetHash` must be specified when `assetHashType` is set to `AssetHashType.CUSTOM`.'); } // Hash the hash to make sure we can use it in a file/directory name. // The resulting hash will also have the same length as for the other hash types. - return crypto.createHash('sha256').update(props.assetHash).digest('hex'); + return crypto.createHash('sha256').update(assetHash).digest('hex'); default: throw new Error('Unknown asset hash type.'); } diff --git a/packages/@aws-cdk/core/test/docker-stub.sh b/packages/@aws-cdk/core/test/docker-stub.sh index 45a78ef881ebd..fe48e93d4a207 100755 --- a/packages/@aws-cdk/core/test/docker-stub.sh +++ b/packages/@aws-cdk/core/test/docker-stub.sh @@ -6,6 +6,7 @@ set -euo pipefail # `/tmp/docker-stub.input` and accepts one of 3 commands that impact it's # behavior. +echo "$@" >> /tmp/docker-stub.input.concat echo "$@" > /tmp/docker-stub.input if echo "$@" | grep "DOCKER_STUB_SUCCESS_NO_OUTPUT"; then diff --git a/packages/@aws-cdk/core/test/test.staging.ts b/packages/@aws-cdk/core/test/test.staging.ts index bff677342bf7f..f0553ba7b5944 100644 --- a/packages/@aws-cdk/core/test/test.staging.ts +++ b/packages/@aws-cdk/core/test/test.staging.ts @@ -7,6 +7,8 @@ import * as sinon from 'sinon'; import { App, AssetHashType, AssetStaging, BundlingDockerImage, Stack } from '../lib'; const STUB_INPUT_FILE = '/tmp/docker-stub.input'; +const STUB_INPUT_CONCAT_FILE = '/tmp/docker-stub.input.concat'; +const STAGING_TMP_DIRECTORY = path.join('.', '.cdk.staging'); enum DockerStubCommand { SUCCESS = 'DOCKER_STUB_SUCCESS', @@ -26,6 +28,12 @@ export = { if (fs.existsSync(STUB_INPUT_FILE)) { fs.unlinkSync(STUB_INPUT_FILE); } + if (fs.existsSync(STUB_INPUT_CONCAT_FILE)) { + fs.unlinkSync(STUB_INPUT_CONCAT_FILE); + } + if (fs.existsSync(STAGING_TMP_DIRECTORY)) { + fs.removeSync(STAGING_TMP_DIRECTORY); + } cb(); sinon.restore(); }, @@ -106,8 +114,6 @@ export = { const stack = new Stack(app, 'stack'); const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); const ensureDirSyncSpy = sinon.spy(fs, 'ensureDirSync'); - const mkdtempSyncSpy = sinon.spy(fs, 'mkdtempSync'); - const chmodSyncSpy = sinon.spy(fs, 'chmodSync'); const consoleErrorSpy = sinon.spy(console, 'error'); // WHEN @@ -126,7 +132,7 @@ export = { `run --rm ${USER_ARG} -v /input:/asset-input:delegated -v /output:/asset-output:delegated -w /asset-input alpine DOCKER_STUB_SUCCESS`, ); test.deepEqual(fs.readdirSync(assembly.directory), [ - 'asset.2f37f937c51e2c191af66acf9b09f548926008ec68c575bd2ee54b6e997c0e00', + 'asset.dec215520dfd57a87aa1362e9e15131938583cd6e5a2bd9a45d38fe5dc5ab3d7', 'cdk.out', 'manifest.json', 'stack.template.json', @@ -134,10 +140,8 @@ export = { ]); // asset is bundled in a directory inside .cdk.staging - const stagingTmp = path.join('.', '.cdk.staging'); - test.ok(ensureDirSyncSpy.calledWith(stagingTmp)); - test.ok(mkdtempSyncSpy.calledWith(sinon.match(path.join(stagingTmp, 'asset-bundle-')))); - test.ok(chmodSyncSpy.calledWith(sinon.match(path.join(stagingTmp, 'asset-bundle-')), 0o777)); + test.ok(ensureDirSyncSpy.calledWith(STAGING_TMP_DIRECTORY)); + test.ok(ensureDirSyncSpy.calledWith(path.resolve(path.join(STAGING_TMP_DIRECTORY, 'asset-bundle-hash-dec215520dfd57a87aa1362e9e15131938583cd6e5a2bd9a45d38fe5dc5ab3d7')))); // shows a message before bundling test.ok(consoleErrorSpy.calledWith('Bundling asset stack/Asset...')); @@ -145,6 +149,148 @@ export = { test.done(); }, + 'bundler reuses its output when it can'(test: Test) { + // GIVEN + const app = new App(); + const stack = new Stack(app, 'stack'); + const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); + const ensureDirSyncSpy = sinon.spy(fs, 'ensureDirSync'); + + // WHEN + new AssetStaging(stack, 'Asset', { + sourcePath: directory, + bundling: { + image: BundlingDockerImage.fromRegistry('alpine'), + command: [ DockerStubCommand.SUCCESS ], + }, + }); + + new AssetStaging(stack, 'AssetDuplicate', { + sourcePath: directory, + bundling: { + image: BundlingDockerImage.fromRegistry('alpine'), + command: [ DockerStubCommand.SUCCESS ], + }, + }); + + // THEN + app.synth(); + + // We're testing that docker was run exactly once even though there are two bundling assets. + test.deepEqual( + readDockerStubInputConcat(), + `run --rm ${USER_ARG} -v /input:/asset-input:delegated -v /output:/asset-output:delegated -w /asset-input alpine DOCKER_STUB_SUCCESS`, + ); + + // asset is bundled in a directory inside .cdk.staging + test.ok(ensureDirSyncSpy.calledWith(STAGING_TMP_DIRECTORY)); + test.ok(ensureDirSyncSpy.calledWith(path.resolve(path.join(STAGING_TMP_DIRECTORY, 'asset-bundle-hash-dec215520dfd57a87aa1362e9e15131938583cd6e5a2bd9a45d38fe5dc5ab3d7')))); + + test.done(); + }, + + 'bundler considers its options when reusing bundle output'(test: Test) { + // GIVEN + const app = new App(); + const stack = new Stack(app, 'stack'); + const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); + const ensureDirSyncSpy = sinon.spy(fs, 'ensureDirSync'); + + // WHEN + new AssetStaging(stack, 'Asset', { + sourcePath: directory, + bundling: { + image: BundlingDockerImage.fromRegistry('alpine'), + command: [ DockerStubCommand.SUCCESS ], + }, + }); + + new AssetStaging(stack, 'AssetWithDifferentBundlingOptions', { + sourcePath: directory, + bundling: { + image: BundlingDockerImage.fromRegistry('alpine'), + command: [ DockerStubCommand.SUCCESS ], + environment: { + UNIQUE_ENV_VAR: 'SOMEVALUE', + }, + }, + }); + + // THEN + const assembly = app.synth(); + + // We're testing that docker was run twice - once for each set of bundler options + // operating on the same source asset. + test.deepEqual( + readDockerStubInputConcat(), + `run --rm ${USER_ARG} -v /input:/asset-input:delegated -v /output:/asset-output:delegated -w /asset-input alpine DOCKER_STUB_SUCCESS\n` + + `run --rm ${USER_ARG} -v /input:/asset-input:delegated -v /output:/asset-output:delegated --env UNIQUE_ENV_VAR=SOMEVALUE -w /asset-input alpine DOCKER_STUB_SUCCESS`, + ); + + // asset is bundled in a directory inside .cdk.staging + test.ok(ensureDirSyncSpy.calledWith(STAGING_TMP_DIRECTORY)); + + test.deepEqual(fs.readdirSync(assembly.directory), [ + 'asset.a33245f0209379d58d125d89906c2b47d38382ae745375f25697760a8c475c6b', // 'Asset' + 'asset.dec215520dfd57a87aa1362e9e15131938583cd6e5a2bd9a45d38fe5dc5ab3d7', // 'AssetWithDifferentBundlingOptions' + 'cdk.out', + 'manifest.json', + 'stack.template.json', + 'tree.json', + ]); + + test.done(); + }, + + 'bundler outputs to a temp dir when using bundle asset type'(test: Test) { + // GIVEN + const app = new App(); + const stack = new Stack(app, 'stack'); + const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); + const mkdtempSyncSpy = sinon.spy(fs, 'mkdtempSync'); + const chmodSyncSpy = sinon.spy(fs, 'chmodSync'); + + // WHEN + new AssetStaging(stack, 'Asset', { + sourcePath: directory, + assetHashType: AssetHashType.BUNDLE, + bundling: { + image: BundlingDockerImage.fromRegistry('alpine'), + command: [ DockerStubCommand.SUCCESS ], + }, + }); + + // THEN + test.ok(mkdtempSyncSpy.calledWith(sinon.match(path.join(STAGING_TMP_DIRECTORY, 'asset-bundle-temp-')))); + test.ok(chmodSyncSpy.calledWith(sinon.match(path.join(STAGING_TMP_DIRECTORY, 'asset-bundle-temp-')), 0o777)); + + test.done(); + }, + + 'bundling failure preserves the bundleDir for diagnosability'(test: Test) { + // GIVEN + const app = new App(); + const stack = new Stack(app, 'stack'); + const directory = path.join(__dirname, 'fs', 'fixtures', 'test1'); + + // WHEN + test.throws(() => new AssetStaging(stack, 'Asset', { + sourcePath: directory, + bundling: { + image: BundlingDockerImage.fromRegistry('alpine'), + command: [ DockerStubCommand.FAIL ], + }, + }), /Failed to run bundling.*asset-bundle-hash.*-error/); + + // THEN + test.ok(!fs.existsSync(path.resolve(path.join(STAGING_TMP_DIRECTORY, + 'asset-bundle-hash-e40b2b1537234d458e9e524494dc0a7a364079d457a2886a44b1f3c28a956469')))); + test.ok(fs.existsSync(path.join(STAGING_TMP_DIRECTORY, + 'asset-bundle-hash-e40b2b1537234d458e9e524494dc0a7a364079d457a2886a44b1f3c28a956469-error'))); + + test.done(); + }, + 'bundling throws when /asset-ouput is empty'(test: Test) { // GIVEN const app = new App(); @@ -228,10 +374,6 @@ export = { assetHash: 'my-custom-hash', assetHashType: AssetHashType.BUNDLE, }), /Cannot specify `bundle` for `assetHashType`/); - test.equal( - readDockerStubInput(), - `run --rm ${USER_ARG} -v /input:/asset-input:delegated -v /output:/asset-output:delegated -w /asset-input alpine DOCKER_STUB_SUCCESS`, - ); test.done(); }, @@ -291,9 +433,20 @@ export = { }, }; +// Reads a docker stub and cleans the volume paths out of the stub. +function readAndCleanDockerStubInput(file: string) { + return fs + .readFileSync(file, 'utf-8') + .trim() + .replace(/-v ([^:]+):\/asset-input/g, '-v /input:/asset-input') + .replace(/-v ([^:]+):\/asset-output/g, '-v /output:/asset-output'); +} + +// Last docker input since last teardown function readDockerStubInput() { - const out = fs.readFileSync(STUB_INPUT_FILE, 'utf-8').trim(); - return out - .replace(/-v ([^:]+):\/asset-input/, '-v /input:/asset-input') - .replace(/-v ([^:]+):\/asset-output/, '-v /output:/asset-output'); + return readAndCleanDockerStubInput(STUB_INPUT_FILE); +} +// Concatenated docker inputs since last teardown +function readDockerStubInputConcat() { + return readAndCleanDockerStubInput(STUB_INPUT_CONCAT_FILE); }