From c124886fbcabea166f34250cad84f7526e05b1bf Mon Sep 17 00:00:00 2001 From: Rico Huijbers Date: Tue, 3 Nov 2020 16:14:29 +0100 Subject: [PATCH] fix(core): many nested stacks make NodeJS run out of memory (#11250) Because of a confluence of factors (but principally the poor design behind how cross-stack references are detected and rendered), having a lot of nested stacks leads to a wasteful amount of work being done and the NodeJS program running out of memory. An internal project has 26 nested stacks, and was taking `2m20` to synth and ran out of the default NodeJS heap (having to increase it to `8G`). The reason is because the entire construct tree is being resynthesized for every nested stack, and because this particular application was using CloudWatch Dashboards, a lot of temporary tokens were being created (and retained in the global encoded token map) on each of the 26 synthesis operations. With the current patch applied, that same project synthesizes in `51s` and takes `~1G` of memory (this can be brought down further to about 60% of this number by caching of `Lazy`s, but that change will be introduced in a different PR). The problem ----------- (Legacy) assets work by exploiting cross-stack references: they add `CfnParameter`s at the top-level, and then reference them inside a nested template. Cross-stack reference detection picks this up and adds `CfnParameters` and `AWS::CloudFormation::Stack` parameters to all stacks on the root path to "transport" the values to the right nested stack. To define a nested stack asset, we need to know the template hash, so we must have already resolved stack references to their final form. However, the process of defining the nested stack assets may add new references (as mentioned before) which need to be resolved, leading to our existing implementation of calling `resolveReferences()` once for every nested stack in the application. Calling `resolveReferences()` leads to all Tokens being resolved. It happens that the Tokens are `Lazy`s that resolve to other `Lazy`s, perhaps indirectly in the following form: ```ts Lazy.stringValue({ produce: () => someFunction() }) function someFunction() { return Lazy.stringValue({ produce: () => /* something else */ }); } ``` For every resolution for every nested stack, the *inner* `Lazy` would be reconstructed anew, and subsequently registered in the global token map (along with its stack trace). A wasteful amount of temporary Token objects are created and infinitely retained in this process. The fix ------- We resolve references twice, instead of once per nested stack. Once at the start, to resolve all cross-stack references put there by the user to their final form. Then we add nested stack assets in the right order, and then we finally do one more "resolve references" to make sure the stack asset parameters are forwarded to the right nested stacks. The nested stack asset hash will now be calculated over a template that is not yet the final template, but the hash still includes the user-mutable parts and will change if the user introduces changes (compare it to a `sourceHash` we use for other assets). ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../test/integ.nested-stacks-multi-refs.ts | 3 +- .../test/integ.nested-stacks-multi.ts | 1 + .../test/test.nested-stack.ts | 41 +++++++++++++++++-- .../test/nested-stacks.test.ts | 34 ++++++++------- .../@aws-cdk/core/lib/private/prepare-app.ts | 27 ++++++++---- 5 files changed, 80 insertions(+), 26 deletions(-) diff --git a/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi-refs.ts b/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi-refs.ts index f133d07bea8c9..b64a4d488e956 100644 --- a/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi-refs.ts +++ b/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi-refs.ts @@ -1,3 +1,4 @@ +/// !cdk-integ pragma:ignore-assets import * as sns from '@aws-cdk/aws-sns'; import { App, Fn, Stack } from '@aws-cdk/core'; import { NestedStack } from '../lib'; @@ -28,4 +29,4 @@ app.synth(); // Stack1-NestedUnderStack1NestedStackNestedUnderStack1NestedStackResourceF616305B-EM64TEGA04J9-TopicInNestedUnderStack115E329C4-HEO7NLYC1AFL function shortName(topicName: string) { return Fn.select(1, Fn.split('-', topicName)); -} \ No newline at end of file +} diff --git a/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi.ts b/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi.ts index b3e51a8c049f1..109b151138b19 100644 --- a/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi.ts +++ b/packages/@aws-cdk/aws-cloudformation/test/integ.nested-stacks-multi.ts @@ -1,3 +1,4 @@ +/// !cdk-integ pragma:ignore-assets import * as sns from '@aws-cdk/aws-sns'; import { App, Construct, Stack } from '@aws-cdk/core'; import * as cfn from '../lib'; diff --git a/packages/@aws-cdk/aws-cloudformation/test/test.nested-stack.ts b/packages/@aws-cdk/aws-cloudformation/test/test.nested-stack.ts index de102b4b1cf51..5f809e7e544bf 100644 --- a/packages/@aws-cdk/aws-cloudformation/test/test.nested-stack.ts +++ b/packages/@aws-cdk/aws-cloudformation/test/test.nested-stack.ts @@ -3,7 +3,7 @@ import * as path from 'path'; import { expect, haveResource, matchTemplate, SynthUtils } from '@aws-cdk/assert'; import * as s3_assets from '@aws-cdk/aws-s3-assets'; import * as sns from '@aws-cdk/aws-sns'; -import { App, CfnParameter, CfnResource, Construct, ContextProvider, Stack } from '@aws-cdk/core'; +import { App, CfnParameter, CfnResource, Construct, ContextProvider, LegacyStackSynthesizer, Stack } from '@aws-cdk/core'; import { Test } from 'nodeunit'; import { NestedStack } from '../lib/nested-stack'; @@ -714,6 +714,8 @@ export = { }, }); + const middleStackHash = 'b2670b4c0c3fdf1d8fd9b9272bb8bf8173d18c0f67a888ba165cc569a248a84f'; + // nested1 wires the nested2 template through parameters, so we expect those expect(nested1).to(haveResource('Resource::1')); const nested2Template = SynthUtils.toCloudFormation(nested1); @@ -729,9 +731,9 @@ export = { AssetParameters8169c6f8aaeaf5e2e8620f5f895ffe2099202ccb4b6889df48fe0967a894235cS3BucketDE3B88D6: { Type: 'String', Description: 'S3 bucket for asset "8169c6f8aaeaf5e2e8620f5f895ffe2099202ccb4b6889df48fe0967a894235c"' }, AssetParameters8169c6f8aaeaf5e2e8620f5f895ffe2099202ccb4b6889df48fe0967a894235cS3VersionKey3A62EFEA: { Type: 'String', Description: 'S3 key for asset version "8169c6f8aaeaf5e2e8620f5f895ffe2099202ccb4b6889df48fe0967a894235c"' }, AssetParameters8169c6f8aaeaf5e2e8620f5f895ffe2099202ccb4b6889df48fe0967a894235cArtifactHash7DC546E0: { Type: 'String', Description: 'Artifact hash for asset "8169c6f8aaeaf5e2e8620f5f895ffe2099202ccb4b6889df48fe0967a894235c"' }, - AssetParameters8b50795a950cca6b01352f162c45d9d274dee6bc409f2f2b2ed029ad6828b3bfS3Bucket76ACFB38: { Type: 'String', Description: 'S3 bucket for asset "8b50795a950cca6b01352f162c45d9d274dee6bc409f2f2b2ed029ad6828b3bf"' }, - AssetParameters8b50795a950cca6b01352f162c45d9d274dee6bc409f2f2b2ed029ad6828b3bfS3VersionKey04162EF1: { Type: 'String', Description: 'S3 key for asset version "8b50795a950cca6b01352f162c45d9d274dee6bc409f2f2b2ed029ad6828b3bf"' }, - AssetParameters8b50795a950cca6b01352f162c45d9d274dee6bc409f2f2b2ed029ad6828b3bfArtifactHashF227ADD3: { Type: 'String', Description: 'Artifact hash for asset "8b50795a950cca6b01352f162c45d9d274dee6bc409f2f2b2ed029ad6828b3bf"' }, + [`AssetParameters${middleStackHash}S3Bucket3DB431CB`]: { Type: 'String', Description: `S3 bucket for asset "${middleStackHash}"` }, + [`AssetParameters${middleStackHash}S3VersionKeyBFFDABE9`]: { Type: 'String', Description: `S3 key for asset version "${middleStackHash}"` }, + [`AssetParameters${middleStackHash}ArtifactHash8EA52875`]: { Type: 'String', Description: `Artifact hash for asset "${middleStackHash}"` }, }); // proxy asset params to nested stack @@ -935,6 +937,37 @@ export = { test.done(); }, + '3-level stacks: legacy synthesizer parameters are added to the middle-level stack'(test: Test) { + // GIVEN + const app = new App(); + const top = new Stack(app, 'stack', { + synthesizer: new LegacyStackSynthesizer(), + }); + const middle = new NestedStack(top, 'nested1'); + const bottom = new NestedStack(middle, 'nested2'); + + // WHEN + new CfnResource(bottom, 'Something', { + type: 'BottomLevel', + }); + + // THEN + const asm = app.synth(); + const middleTemplate = JSON.parse(fs.readFileSync(path.join(asm.directory, middle.templateFile), { encoding: 'utf-8' })); + + const hash = 'bc3c51e4d3545ee0a0069401e5a32c37b66d044b983f12de416ba1576ecaf0a4'; + test.deepEqual(middleTemplate.Parameters ?? {}, { + [`referencetostackAssetParameters${hash}S3BucketD7C30435Ref`]: { + Type: 'String', + }, + [`referencetostackAssetParameters${hash}S3VersionKeyB667DBE1Ref`]: { + Type: 'String', + }, + }); + + test.done(); + }, + 'references to a resource from a deeply nested stack'(test: Test) { // GIVEN const app = new App(); diff --git a/packages/@aws-cdk/cloudformation-include/test/nested-stacks.test.ts b/packages/@aws-cdk/cloudformation-include/test/nested-stacks.test.ts index bf729dcedd38b..0e9dcadc5ce60 100644 --- a/packages/@aws-cdk/cloudformation-include/test/nested-stacks.test.ts +++ b/packages/@aws-cdk/cloudformation-include/test/nested-stacks.test.ts @@ -444,6 +444,9 @@ describe('CDK Include for nested stacks', () => { let child: inc.IncludedNestedStack; let grandChild: inc.IncludedNestedStack; + let hash1: string; + let hash2: string; + let parentBucketParam: string; let parentKeyParam: string; let grandChildBucketParam: string; @@ -471,13 +474,16 @@ describe('CDK Include for nested stacks', () => { child = parentTemplate.getNestedStack('ChildStack'); grandChild = child.includedTemplate.getNestedStack('GrandChildStack'); - parentBucketParam = 'AssetParameters5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50S3BucketEAA24F0C'; - parentKeyParam = 'AssetParameters5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50S3VersionKey1194CAB2'; - grandChildBucketParam = 'referencetoAssetParameters5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50S3BucketEAA24F0CRef'; - grandChildKeyParam = 'referencetoAssetParameters5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50S3VersionKey1194CAB2Ref'; + hash1 = '5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50'; + hash2 = '7775730164edb5faae717ac1d2e90d9c0d0fdbeafe48763e5c1b7fb5e39e00a5'; + + parentBucketParam = `AssetParameters${hash1}S3BucketEAA24F0C`; + parentKeyParam = `AssetParameters${hash1}S3VersionKey1194CAB2`; + grandChildBucketParam = `referencetoAssetParameters${hash1}S3BucketEAA24F0CRef`; + grandChildKeyParam = `referencetoAssetParameters${hash1}S3VersionKey1194CAB2Ref`; - childBucketParam = 'AssetParameters891fd3ec75dc881b0fe40dc9fd1b433672637585c015265a5f0dab6bf79818d5S3Bucket23278F13'; - childKeyParam = 'AssetParameters891fd3ec75dc881b0fe40dc9fd1b433672637585c015265a5f0dab6bf79818d5S3VersionKey7316205A'; + childBucketParam = `AssetParameters${hash2}S3BucketDEB194C6`; + childKeyParam = `AssetParameters${hash2}S3VersionKey8B342ED1`; }); test('correctly creates parameters in the parent stack, and passes them to the child stack', () => { @@ -485,27 +491,27 @@ describe('CDK Include for nested stacks', () => { "Parameters": { [parentBucketParam]: { "Type": "String", - "Description": "S3 bucket for asset \"5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50\"", + "Description": `S3 bucket for asset \"${hash1}\"`, }, [parentKeyParam]: { "Type": "String", - "Description": "S3 key for asset version \"5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50\"", + "Description": `S3 key for asset version \"${hash1}\"`, }, - "AssetParameters5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50ArtifactHash9C417847": { + [`AssetParameters${hash1}ArtifactHash9C417847`]: { "Type": "String", - "Description": "Artifact hash for asset \"5dc7d4a99cfe2979687dc74f2db9fd75f253b5505a1912b5ceecf70c9aefba50\"", + "Description": `Artifact hash for asset \"${hash1}\"`, }, [childBucketParam]: { "Type": "String", - "Description": "S3 bucket for asset \"891fd3ec75dc881b0fe40dc9fd1b433672637585c015265a5f0dab6bf79818d5\"", + "Description": `S3 bucket for asset \"${hash2}\"`, }, [childKeyParam]: { "Type": "String", - "Description": "S3 key for asset version \"891fd3ec75dc881b0fe40dc9fd1b433672637585c015265a5f0dab6bf79818d5\"", + "Description": `S3 key for asset version \"${hash2}\"`, }, - "AssetParameters891fd3ec75dc881b0fe40dc9fd1b433672637585c015265a5f0dab6bf79818d5ArtifactHashA1DE5198": { + [`AssetParameters${hash2}ArtifactHashAA82D4CC`]: { "Type": "String", - "Description": "Artifact hash for asset \"891fd3ec75dc881b0fe40dc9fd1b433672637585c015265a5f0dab6bf79818d5\"", + "Description": `Artifact hash for asset \"${hash2}\"`, }, }, "Resources": { diff --git a/packages/@aws-cdk/core/lib/private/prepare-app.ts b/packages/@aws-cdk/core/lib/private/prepare-app.ts index ad900acf803c0..e90a40ff211f8 100644 --- a/packages/@aws-cdk/core/lib/private/prepare-app.ts +++ b/packages/@aws-cdk/core/lib/private/prepare-app.ts @@ -28,19 +28,32 @@ export function prepareApp(root: IConstruct) { } } + resolveReferences(root); + // depth-first (children first) queue of nested stacks. We will pop a stack // from the head of this queue to prepare its template asset. + // + // Depth-first since the a nested stack's template hash will be reflected in + // its parent's template, which then changes the parent's hash, etc. const queue = findAllNestedStacks(root); - while (true) { - resolveReferences(root); - - const nested = queue.shift(); - if (!nested) { - break; + if (queue.length > 0) { + while (queue.length > 0) { + const nested = queue.shift()!; + defineNestedStackAsset(nested); } - defineNestedStackAsset(nested); + // ▷[ Given the legacy synthesizer and a 3-or-deeper nesting of nested stacks ] + // + // Adding nested stack assets may haved added CfnParameters to the top-level + // stack which are referenced in a deeper-level stack. The values of these + // parameters need to be carried through to the right location via Nested + // Stack parameters, which `resolveReferences()` will do. + // + // Yes, this may add `Parameter` elements to a template whose hash has + // already been calculated, but the invariant that if the functional part + // of the template changes its hash will change is still upheld. + resolveReferences(root); } }