Skip to content

Commit

Permalink
fix(cli): assets shared between stages lead to an error (#25907)
Browse files Browse the repository at this point in the history 
The problem would manifest as this error message:

```
❌ Deployment failed: Error: Duplicate use of node id: 07a6878c7a2ec9b49ef3c0ece94cef1c2dd20fba34ca9650dfa6e7e00f2b9961:current_account-current_region-build
```

The problem was that we were using the full asset "destination identifier" for both the build and publish steps, but then were trying to use the `source` object to deduplicate build steps.

A more robust solution is to only use the asset identifier (excluding the destination identifier) for the build step, which includes all data necessary to deduplicate the asset. No need to look at the source at all anymore.

----

*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
rix0rrr authored Jun 8, 2023
1 parent 7a74513 commit 3196cbc
Show file tree
Hide file tree
Showing 3 changed files with 83 additions and 23 deletions.
19 changes: 9 additions & 10 deletions packages/aws-cdk/lib/util/work-graph-builder.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ export class WorkGraphBuilder {
'stack': 5,
};
private readonly graph = new WorkGraph();
private readonly assetBuildNodes = new Map<string, AssetBuildNode>;

constructor(private readonly prebuildAssets: boolean, private readonly idPrefix = '') { }

Expand All @@ -39,12 +38,16 @@ export class WorkGraphBuilder {
*/
// eslint-disable-next-line max-len
private addAsset(parentStack: cxapi.CloudFormationStackArtifact, assetArtifact: cxapi.AssetManifestArtifact, assetManifest: AssetManifest, asset: IManifestEntry) {
const buildId = `${this.idPrefix}${asset.id}-build`;
// Just the artifact identifier
const assetId = asset.id.assetId;
// Unique per destination where the artifact needs to go
const assetDestinationId = `${asset.id}`;

// Add the build node, but only one per "source"
// The genericSource includes a relative path we could make absolute to do more effective deduplication of build steps. Not doing that right now.
const assetBuildNodeKey = JSON.stringify(asset.genericSource);
if (!this.assetBuildNodes.has(assetBuildNodeKey)) {
const buildId = `${this.idPrefix}${assetId}-build`;
const publishNodeId = `${this.idPrefix}${assetDestinationId}-publish`;

// Build node only gets added once because they are all the same
if (!this.graph.tryGetNode(buildId)) {
const node: AssetBuildNode = {
type: 'asset-build',
id: buildId,
Expand All @@ -60,13 +63,9 @@ export class WorkGraphBuilder {
deploymentState: DeploymentState.PENDING,
priority: WorkGraphBuilder.PRIORITIES['asset-build'],
};
this.assetBuildNodes.set(assetBuildNodeKey, node);
this.graph.addNodes(node);
}

// Always add the publish
const publishNodeId = `${this.idPrefix}${asset.id}-publish`;

const publishNode = this.graph.tryGetNode(publishNodeId);
if (!publishNode) {
this.graph.addNodes({
Expand Down
74 changes: 63 additions & 11 deletions packages/aws-cdk/test/work-graph-builder.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as path from 'path';
import * as cxschema from '@aws-cdk/cloud-assembly-schema';
import * as cxapi from '@aws-cdk/cx-api';
import { CloudAssemblyBuilder } from '@aws-cdk/cx-api';
import { WorkGraph } from '../lib/util/work-graph';
import { WorkGraphBuilder } from '../lib/util/work-graph-builder';
import { AssetBuildNode, AssetPublishNode, StackNode, WorkNode } from '../lib/util/work-graph-types';

Expand Down Expand Up @@ -36,14 +37,14 @@ describe('with some stacks and assets', () => {

expect(graph.node('F1:D1-publish')).toEqual(expect.objectContaining({
type: 'asset-publish',
dependencies: new Set(['F1:D1-build']),
dependencies: new Set(['F1-build']),
} as Partial<AssetPublishNode>));
});

test('with prebuild off, asset building inherits dependencies from their parent stack', () => {
const graph = new WorkGraphBuilder(false).build(assembly.artifacts);

expect(graph.node('F1:D1-build')).toEqual(expect.objectContaining({
expect(graph.node('F1-build')).toEqual(expect.objectContaining({
type: 'asset-build',
dependencies: new Set(['stack0', 'stack1']),
} as Partial<AssetBuildNode>));
Expand All @@ -52,7 +53,7 @@ describe('with some stacks and assets', () => {
test('with prebuild on, assets only have their own dependencies', () => {
const graph = new WorkGraphBuilder(true).build(assembly.artifacts);

expect(graph.node('F1:D1-build')).toEqual(expect.objectContaining({
expect(graph.node('F1-build')).toEqual(expect.objectContaining({
type: 'asset-build',
dependencies: new Set(['stack0']),
} as Partial<AssetBuildNode>));
Expand Down Expand Up @@ -138,17 +139,11 @@ describe('tests that use assets', () => {

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); },
});
const traversal = await traverseAndRecord(graph);

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-build',
'work-graph-builder.test.js:D1-publish',
'StackA',
'StackB',
Expand All @@ -171,6 +166,53 @@ describe('tests that use assets', () => {
// THEN
expect(graph.findCycle()).toBeUndefined();
});

test('the same asset to different destinations is only built once', async () => {
addStack(rootBuilder, 'StackA', {
environment: 'aws://11111/us-east-1',
dependencies: ['StackA.assets'],
});
addAssets(rootBuilder, 'StackA.assets', {
files: {
abcdef: {
source: { path: __dirname },
destinations: {
D1: { bucketName: 'bucket1', objectKey: 'key' },
D2: { bucketName: 'bucket2', objectKey: 'key' },
},
},
},
});

addStack(rootBuilder, 'StackB', {
environment: 'aws://11111/us-east-1',
dependencies: ['StackB.assets', 'StackA'],
});
addAssets(rootBuilder, 'StackB.assets', {
files: {
abcdef: {
source: { path: __dirname },
destinations: {
D3: { bucketName: 'bucket3', objectKey: 'key' },
},
},
},
});

const assembly = rootBuilder.buildAssembly();

const graph = new WorkGraphBuilder(true).build(assembly.artifacts);
const traversal = await traverseAndRecord(graph);

expect(traversal).toEqual([
'abcdef-build',
'abcdef:D1-publish',
'abcdef:D2-publish',
'StackA',
'abcdef:D3-publish',
'StackB',
]);
});
});

/**
Expand Down Expand Up @@ -251,3 +293,13 @@ function assertableNode<A extends WorkNode>(x: A) {
dependencies: Array.from(x.dependencies),
};
}

async function traverseAndRecord(graph: WorkGraph) {
const ret: string[] = [];
await graph.doParallel(1, {
deployStack: async (node) => { ret.push(node.id); },
buildAsset: async (node) => { ret.push(node.id); },
publishAsset: async (node) => { ret.push(node.id); },
});
return ret;
}
13 changes: 11 additions & 2 deletions packages/cdk-assets/lib/asset-manifest.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,10 @@ export class AssetManifest {
}

/**
* List of assets, splat out to destinations
* List of assets per destination
*
* Returns one asset for every publishable destination. Multiple asset
* destinations may share the same asset source.
*/
public get entries(): IManifestEntry[] {
return [
Expand Down Expand Up @@ -145,7 +148,7 @@ const ASSET_TYPES: AssetType[] = ['files', 'dockerImages'];
*/
export interface IManifestEntry {
/**
* The identifier of the asset
* The identifier of the asset and its destination
*/
readonly id: DestinationIdentifier;

Expand Down Expand Up @@ -209,10 +212,16 @@ export class DockerImageManifestEntry implements IManifestEntry {

/**
* Identify an asset destination in an asset manifest
*
* When stringified, this will be a combination of the source
* and destination IDs.
*/
export class DestinationIdentifier {
/**
* Identifies the asset, by source.
*
* The assetId will be the same between assets that represent
* the same physical file or image.
*/
public readonly assetId: string;

Expand Down

0 comments on commit 3196cbc

Please sign in to comment.