diff --git a/packages/aws-cdk/lib/cdk-toolkit.ts b/packages/aws-cdk/lib/cdk-toolkit.ts index 7cf0ccc6b42f1..70d7650f58f7b 100644 --- a/packages/aws-cdk/lib/cdk-toolkit.ts +++ b/packages/aws-cdk/lib/cdk-toolkit.ts @@ -359,7 +359,7 @@ export class CdkToolkit { const graphConcurrency: Concurrency = { 'stack': concurrency, 'asset-build': 1, // This will be CPU-bound/memory bound, mostly matters for Docker builds - 'asset-publish': options.assetParallelism ? 8 : 1, // This will be I/O-bound, 8 in parallel seems reasonable + 'asset-publish': (options.assetParallelism ?? true) ? 8 : 1, // This will be I/O-bound, 8 in parallel seems reasonable }; await workGraph.doParallel(graphConcurrency, { diff --git a/packages/aws-cdk/lib/util/work-graph-builder.ts b/packages/aws-cdk/lib/util/work-graph-builder.ts index 7c5a0d0ff3762..979c68e4d915e 100644 --- a/packages/aws-cdk/lib/util/work-graph-builder.ts +++ b/packages/aws-cdk/lib/util/work-graph-builder.ts @@ -50,8 +50,8 @@ export class WorkGraphBuilder { id: buildId, dependencies: new Set([ ...this.getDepIds(assetArtifact.dependencies), - // If we disable prebuild, then assets inherit dependencies from their parent stack - ...!this.prebuildAssets ? this.getDepIds(parentStack.dependencies) : [], + // If we disable prebuild, then assets inherit (stack) dependencies from their parent stack + ...!this.prebuildAssets ? this.getDepIds(onlyStacks(parentStack.dependencies)) : [], ]), parentStack, assetManifestArtifact: assetArtifact, @@ -66,27 +66,35 @@ export class WorkGraphBuilder { // Always add the publish const publishNodeId = `${this.idPrefix}${asset.id}-publish`; - this.graph.addNodes({ - type: 'asset-publish', - id: publishNodeId, - dependencies: new Set([ - buildId, - // The asset publish step also depends on the stacks that the parent depends on. - // This is purely cosmetic: if we don't do this, the progress printing of asset publishing - // is going to interfere with the progress bar of the stack deployment. We could remove this - // for overall faster deployments if we ever have a better method of progress displaying. - // Note: this may introduce a cycle if one of the parent's dependencies is another stack that - // depends on this asset. To workaround this we remove these cycles once all nodes have - // been added to the graph. - ...this.getDepIds(parentStack.dependencies.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact)), - ]), - parentStack, - assetManifestArtifact: assetArtifact, - assetManifest, - asset, - deploymentState: DeploymentState.PENDING, - priority: WorkGraphBuilder.PRIORITIES['asset-publish'], - }); + + const publishNode = this.graph.tryGetNode(publishNodeId); + if (!publishNode) { + this.graph.addNodes({ + type: 'asset-publish', + id: publishNodeId, + dependencies: new Set([ + buildId, + ]), + parentStack, + assetManifestArtifact: assetArtifact, + assetManifest, + asset, + deploymentState: DeploymentState.PENDING, + priority: WorkGraphBuilder.PRIORITIES['asset-publish'], + }); + } + + for (const inheritedDep of this.getDepIds(onlyStacks(parentStack.dependencies))) { + // The asset publish step also depends on the stacks that the parent depends on. + // This is purely cosmetic: if we don't do this, the progress printing of asset publishing + // is going to interfere with the progress bar of the stack deployment. We could remove this + // for overall faster deployments if we ever have a better method of progress displaying. + // Note: this may introduce a cycle if one of the parent's dependencies is another stack that + // depends on this asset. To workaround this we remove these cycles once all nodes have + // been added to the graph. + this.graph.addDependency(publishNodeId, inheritedDep); + } + // This will work whether the stack node has been added yet or not this.graph.addDependency(`${this.idPrefix}${parentStack.id}`, publishNodeId); } @@ -137,20 +145,17 @@ export class WorkGraphBuilder { return ids; } + /** + * We may have accidentally introduced cycles in an attempt to make the messages printed to the + * console not interfere with each other too much. Remove them again. + */ private removeStackPublishCycles() { - const stacks = this.graph.nodesOfType('stack'); - for (const stack of stacks) { - for (const dep of stack.dependencies) { - const node = this.graph.nodes[dep]; - - if (!node || node.type !== 'asset-publish' || !node.dependencies.has(stack.id)) { - continue; + const publishSteps = this.graph.nodesOfType('asset-publish'); + for (const publishStep of publishSteps) { + for (const dep of publishStep.dependencies) { + if (this.graph.reachable(dep, publishStep.id)) { + publishStep.dependencies.delete(dep); } - - // Delete the dependency from the asset-publish onto the stack. - // The publish -> stack dependencies are purely cosmetic to prevent publish output - // from interfering with the progress bar of the stack deployment. - node.dependencies.delete(stack.id); } } } @@ -166,4 +171,8 @@ function stacksFromAssets(artifacts: cxapi.CloudArtifact[]) { } return ret; +} + +function onlyStacks(artifacts: cxapi.CloudArtifact[]) { + return artifacts.filter(cxapi.CloudFormationStackArtifact.isCloudFormationStackArtifact); } \ No newline at end of file diff --git a/packages/aws-cdk/lib/util/work-graph.ts b/packages/aws-cdk/lib/util/work-graph.ts index 4d565ade4be3b..81eab777c27d6 100644 --- a/packages/aws-cdk/lib/util/work-graph.ts +++ b/packages/aws-cdk/lib/util/work-graph.ts @@ -14,6 +14,10 @@ export class WorkGraph { public addNodes(...nodes: WorkNode[]) { for (const node of nodes) { + if (this.nodes[node.id]) { + throw new Error(`Duplicate use of node id: ${node.id}`); + } + const ld = this.lazyDependencies.get(node.id); if (ld) { for (const x of ld) { @@ -72,6 +76,10 @@ export class WorkGraph { lazyDeps.push(toId); } + public tryGetNode(id: string): WorkNode | undefined { + return this.nodes[id]; + } + public node(id: string) { const ret = this.nodes[id]; if (!ret) { @@ -198,9 +206,24 @@ export class WorkGraph { } public toString() { - return Object.entries(this.nodes).map(([id, node]) => - `${id} := ${node.deploymentState} ${node.type} ${node.dependencies.size > 0 ? `(${Array.from(node.dependencies)})` : ''}`.trim(), - ).join(', '); + return [ + 'digraph D {', + ...Object.entries(this.nodes).flatMap(([id, node]) => renderNode(id, node)), + '}', + ].join('\n'); + + function renderNode(id: string, node: WorkNode): string[] { + const ret = []; + if (node.deploymentState === DeploymentState.COMPLETED) { + ret.push(` "${id}" [style=filled,fillcolor=yellow];`); + } else { + ret.push(` "${id}";`); + } + for (const dep of node.dependencies) { + ret.push(` "${id}" -> "${dep}";`); + } + return ret; + } } /** @@ -244,35 +267,28 @@ export class WorkGraph { } private updateReadyPool() { - let activeCount = 0; - let pendingCount = 0; - for (const node of Object.values(this.nodes)) { - switch (node.deploymentState) { - case DeploymentState.DEPLOYING: - activeCount += 1; - break; - case DeploymentState.PENDING: - pendingCount += 1; - if (Array.from(node.dependencies).every((id) => this.node(id).deploymentState === DeploymentState.COMPLETED)) { - node.deploymentState = DeploymentState.QUEUED; - this.readyPool.push(node); - } - break; - } - } + const activeCount = Object.values(this.nodes).filter((x) => x.deploymentState === DeploymentState.DEPLOYING).length; + const pendingCount = Object.values(this.nodes).filter((x) => x.deploymentState === DeploymentState.PENDING).length; - for (let i = 0; i < this.readyPool.length; i++) { - const node = this.readyPool[i]; - if (node.deploymentState !== DeploymentState.QUEUED) { - this.readyPool.splice(i, 1); - } + const newlyReady = Object.values(this.nodes).filter((x) => + x.deploymentState === DeploymentState.PENDING && + Array.from(x.dependencies).every((id) => this.node(id).deploymentState === DeploymentState.COMPLETED)); + + // Add newly available nodes to the ready pool + for (const node of newlyReady) { + node.deploymentState = DeploymentState.QUEUED; + this.readyPool.push(node); } + // Remove nodes from the ready pool that have already started deploying + retainOnly(this.readyPool, (node) => node.deploymentState === DeploymentState.QUEUED); + // Sort by reverse priority this.readyPool.sort((a, b) => (b.priority ?? 0) - (a.priority ?? 0)); if (this.readyPool.length === 0 && activeCount === 0 && pendingCount > 0) { - throw new Error(`Unable to make progress anymore, dependency cycle between remaining artifacts: ${this.findCycle().join(' -> ')}`); + const cycle = this.findCycle() ?? ['No cycle found!']; + throw new Error(`Unable to make progress anymore, dependency cycle between remaining artifacts: ${cycle.join(' -> ')}`); } } @@ -289,14 +305,14 @@ export class WorkGraph { * * Not the fastest, but effective and should be rare */ - private findCycle(): string[] { + public findCycle(): string[] | undefined { const seen = new Set(); const self = this; for (const nodeId of Object.keys(this.nodes)) { const cycle = recurse(nodeId, [nodeId]); if (cycle) { return cycle; } } - return ['No cycle found!']; + return undefined; function recurse(nodeId: string, path: string[]): string[] | undefined { if (seen.has(nodeId)) { @@ -319,6 +335,32 @@ export class WorkGraph { } } } + + /** + * Whether the `end` node is reachable from the `start` node, following the dependency arrows + */ + public reachable(start: string, end: string): boolean { + const seen = new Set(); + const self = this; + return recurse(start); + + function recurse(current: string) { + if (seen.has(current)) { + return false; + } + seen.add(current); + + if (current === end) { + return true; + } + for (const dep of self.nodes[current].dependencies) { + if (recurse(dep)) { + return true; + } + } + return false; + } + } } export interface WorkGraphActions { @@ -334,3 +376,7 @@ function sum(xs: number[]) { } return ret; } + +function retainOnly(xs: A[], pred: (x: A) => boolean) { + xs.splice(0, xs.length, ...xs.filter(pred)); +} diff --git a/packages/aws-cdk/test/work-graph-builder.test.ts b/packages/aws-cdk/test/work-graph-builder.test.ts index 39e01cc77c911..e89d8e35e911d 100644 --- a/packages/aws-cdk/test/work-graph-builder.test.ts +++ b/packages/aws-cdk/test/work-graph-builder.test.ts @@ -109,7 +109,7 @@ test('dependencies on unselected artifacts are silently ignored', async () => { })); }); -test('assets with shared contents between dependant stacks', async () => { +describe('tests that use assets', () => { const files = { // Referencing an existing file on disk is important here. // It means these two assets will have the same AssetManifest @@ -121,36 +121,56 @@ test('assets with shared contents between dependant stacks', async () => { }, }, }; + const environment = 'aws://11111/us-east-1'; - addStack(rootBuilder, 'StackA', { - environment: 'aws://11111/us-east-1', - dependencies: ['StackA.assets'], - }); - addAssets(rootBuilder, 'StackA.assets', { files }); + test('assets with shared contents between dependant stacks', async () => { + addStack(rootBuilder, 'StackA', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackA.assets'], + }); + addAssets(rootBuilder, 'StackA.assets', { files }); - addStack(rootBuilder, 'StackB', { - environment: 'aws://11111/us-east-1', - dependencies: ['StackB.assets', 'StackA'], + addStack(rootBuilder, 'StackB', { + environment: 'aws://11111/us-east-1', + dependencies: ['StackB.assets', 'StackA'], + }); + addAssets(rootBuilder, 'StackB.assets', { files }); + + const assembly = rootBuilder.buildAssembly(); + + const traversal: string[] = []; + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + await graph.doParallel(1, { + deployStack: async (node) => { traversal.push(node.id); }, + buildAsset: async (node) => { traversal.push(node.id); }, + publishAsset: async (node) => { traversal.push(node.id); }, + }); + + expect(traversal).toHaveLength(4); // 1 asset build, 1 asset publish, 2 stacks + expect(traversal).toEqual([ + 'work-graph-builder.test.js:D1-build', + 'work-graph-builder.test.js:D1-publish', + 'StackA', + 'StackB', + ]); }); - addAssets(rootBuilder, 'StackB.assets', { files }); - const assembly = rootBuilder.buildAssembly(); + test('a more complex way to make a cycle', async () => { + // A -> B -> C | A and C share an asset. The asset will have a dependency on B, that is not a *direct* reverse dependency, and will cause a cycle. + addStack(rootBuilder, 'StackA', { environment, dependencies: ['StackA.assets', 'StackB'] }); + addAssets(rootBuilder, 'StackA.assets', { files }); - const traversal: string[] = []; - const graph = new WorkGraphBuilder(true).build(assembly.artifacts); - await graph.doParallel(1, { - deployStack: async (node) => { traversal.push(node.id); }, - buildAsset: async (node) => { traversal.push(node.id); }, - publishAsset: async (node) => { traversal.push(node.id); }, - }); - - expect(traversal).toHaveLength(4); // 1 asset build, 1 asset publish, 2 stacks - expect(traversal).toEqual([ - 'work-graph-builder.test.js:D1-build', - 'work-graph-builder.test.js:D1-publish', - 'StackA', - 'StackB', - ]); + addStack(rootBuilder, 'StackB', { environment, dependencies: ['StackC'] }); + + addStack(rootBuilder, 'StackC', { environment, dependencies: ['StackC.assets'] }); + addAssets(rootBuilder, 'StackC.assets', { files }); + + const assembly = rootBuilder.buildAssembly(); + const graph = new WorkGraphBuilder(true).build(assembly.artifacts); + + // THEN + expect(graph.findCycle()).toBeUndefined(); + }); }); /** @@ -230,4 +250,4 @@ function assertableNode(x: A) { ...x, dependencies: Array.from(x.dependencies), }; -} \ No newline at end of file +}