fix(core): regression: source directory is fingerprinted even if bundling is skipped #11440

jogold · 2020-11-12T10:51:26Z

If the asset uses OUTPUT or BUNDLE it's generally because its source is
a very large directory. This is the case for the NodejsFunction which
mounts the project root (along with the node_modules folder!).

Use a custom hash in this case when skipping bundling. Otherwise running
cdk ls can result in heavy fingerprinting operations (again this is
the case for the NodejsFunction) and can be much slower than running
cdk synth or cdk diff, making it pointless to skip bundling.

Regression introduced in #11008
(https://github.com/aws/aws-cdk/pull/11008/files#diff-62eef996be8abeb157518522c3cbf84a33dd4751c103304df87b04eb6d7bbab6L160)

Before #11008:

aws-cdk/packages/@aws-cdk/core/lib/asset-staging.ts

Lines 159 to 160 in c145314

    
           return props.assetHashType === AssetHashType.BUNDLE || props.assetHashType === AssetHashType.OUTPUT 
        
             ? this.calculateHash(AssetHashType.CUSTOM, this.node.path) // Use node path as dummy hash because we're not bundling

Closes #11459
Closes #11460

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

If the asset uses OUTPUT or BUNDLE it's generally because its source is a very large directory. This is the case for the `NodejsFunction` which mounts the project root (along with the `node_modules` folder!). Use a custom hash in this case when skipping bundling. Otherwise running `cdk ls` can result in heavy fingerprinting operations (again this is the case for the `NodejsFunction`). Regression introduced in aws#11008 (https://github.com/aws/aws-cdk/pull/11008/files#diff-62eef996be8abeb157518522c3cbf84a33dd4751c103304df87b04eb6d7bbab6L160)

gitpod-io · 2020-11-12T10:51:30Z

jogold · 2020-11-12T10:51:46Z

@rix0rrr can you take a look?

jogold · 2020-11-12T14:09:58Z

packages/@aws-cdk/core/lib/asset-staging.ts

      return {
-        assetHash: this.calculateHash(AssetHashType.SOURCE),
+        assetHash: this.calculateHash(hashType),


Maybe we can always use a custom hash in this case?

mrgrain · 2020-11-13T14:37:27Z

This looks good! Using a unique id seems appropriate. I was considering hashing the BundlingOptions object.

I don't think it would fix #11459 though, since the AssetHashType is set to source unless it's one of the output/bundle ones.

I was thinking maybe something like this? It mimics the hash calculation before bundling (just after the skip block).

    if (skip) {
      // We should have bundled, but didn't to save time. Still pretend to have a hash.
      // If the asset uses OUTPUT (or BUNDLE), we use a CUSTOM hash to avoid fingerprinting
      // a potentially very large source directory. Other hash types are kept the same.
      let hashType = this.hashType;
      if (hashType === AssetHashType.OUTPUT || hashType === AssetHashType.BUNDLE) {
        this.customSourceFingerprint = Names.uniqueId(this);
        hashType = AssetHashType.CUSTOM;
      }

      return {
        assetHash: this.calculateHash(hashType, bundling),
        stagedPath: this.sourcePath,
      };
    }

jogold · 2020-11-19T08:31:41Z

@rix0rrr ping... would be nice if this could be merged before the next release with the new NodejsFunction. Without this doing cdk ls on an app with NodejsFunction almost never completes.

rix0rrr · 2020-11-23T13:56:45Z

packages/@aws-cdk/core/lib/asset-staging.ts

+      // If the asset uses OUTPUT or BUNDLE, we use a CUSTOM hash to avoid fingerprinting
+      // a potentially very large source directory. Other hash types are kept the same.
+      let hashType = this.hashType;
+      if (hashType === AssetHashType.OUTPUT || AssetHashType.BUNDLE) {


That's not what that means :)

What do you mean here exactly? This is not a Docker performance issue here. It's the fact that we are fingerprinting the wrong directory.

If someone asks for a hash based on the bundle/output you currently change it to source if bundling is skipped. You introduced this in #11008.

Before #11008, when bundling was skipped hash calculation was changed to CUSTOM.

It's a regression because we are now fingerprinting the source arbitrally and it could be a very large directory.

I agree that there should be a discussion around deferring bundling but how can we first fix this regression?

A note on why the NodejsFunction uses OUTPUT as hash type and not SOURCE: it bundles not only the user's code but also all referenced node modules so it would be impractical to fingerprint the whole node modules folder. Moreover, we don't want to fingerprint .ts/.js files that represent the CDK infra code because this would incorrectly impact the hash of an asset representing runtime code.

No I mean,

if (hashType === AssetHashType.OUTPUT || AssetHashType.BUNDLE) {

was probably intended to read:

if (hashType === AssetHashType.OUTPUT || hashType === AssetHashType.BUNDLE) {

rix0rrr · 2020-11-23T14:02:59Z

Feels like running the Docker container during synth is just asking for all kinds of performance issues, which then need a big slew of workarounds.

Is there really now way to defer this work to asset publishing time? Is the whole reason we do this during synth simply to get an artifact hash?

And if that's true, is that only because a source hash is too expensive?

I'm starting to think we need an alternative approach here (though I don't know what it should be yet).

mrgrain · 2020-11-24T09:28:11Z

I think you're right @rix0rrr about the need for a more generic solution to this. I have also noticed that Lambda Functions (maybe CodeAssets?) cause the asset to be staged every time a CDK command is run, whereas most other assets are cached and won't be run again if they exist already (or maybe it's got to do with the hash being different?).

In any case, I believe we have two regressions at the moment, which I tried to put into #11459 and #11646

These come down to the current implementation for bundling:

It ignores a custom asset hash.
It prefers the source hash in an attempt to "speed" things up. However this is a very biased assumption and the best way to really speed things up would be using a static or options based hash. Alternatively I think no assumption should be made and the code should be bundled and use the output hash as one would expect.

In my opinion these are regressions that should be addresses asap before looking into a future solution.

mrgrain · 2020-11-24T09:32:06Z

I'm starting to think we need an alternative approach here (though I don't know what it should be yet).
For anything using Node.js we can get around this mostly by using a source hash and ignore the node_modules directory. However the current version has deprecated the exclude option on some higher level asset related classes.

Generally speaking, I'm not sure we should make an assumption here for the user. If they decide to hash based on the output of a bundle, that's their decision. We could however provide a better interface to allow different hashing at various stages. I.e. leave it up to them to pick between OUTPUT (default), SOURCE, CUSTOM or even a function?

jogold · 2020-11-25T11:54:51Z

The problem is also present when doing cdk synth STACK1 -e.

Pull request has been modified.

jogold · 2020-11-30T16:43:55Z

Merging master because this branch is too much behind it and gives incorrect API breaking changes

Updated assembly:  @aws-cdk/[email protected]
API elements with incompatible changes:
err  - METHOD @aws-cdk/core.Lazy.list: has been removed [removed:@aws-cdk/core.Lazy.list]
err  - IFACE @aws-cdk/core.IStableAnyProducer: has been removed [removed:@aws-cdk/core.IStableAnyProducer]
err  - IFACE @aws-cdk/core.IStableNumberProducer: has been removed [removed:@aws-cdk/core.IStableNumberProducer]
err  - IFACE @aws-cdk/core.IStableStringProducer: has been removed [removed:@aws-cdk/core.IStableStringProducer]
err  - PROP @aws-cdk/core.BootstraplessSynthesizer.DEFAULT_FILE_ASSET_PREFIX: has been removed [removed:@aws-cdk/core.BootstraplessSynthesizer.DEFAULT_FILE_ASSET_PREFIX]
err  - PROP @aws-cdk/core.DefaultStackSynthesizer.DEFAULT_FILE_ASSET_PREFIX: has been removed [removed:@aws-cdk/core.DefaultStackSynthesizer.DEFAULT_FILE_ASSET_PREFIX]
err  - METHOD @aws-cdk/core.Lazy.any: has been removed [removed:@aws-cdk/core.Lazy.any]
err  - IFACE @aws-cdk/core.IStableListProducer: has been removed [removed:@aws-cdk/core.IStableListProducer]
err  - METHOD @aws-cdk/core.Lazy.number: has been removed [removed:@aws-cdk/core.Lazy.number]
err  - METHOD @aws-cdk/core.Lazy.string: has been removed [removed:@aws-cdk/core.Lazy.string]
err  - METHOD @aws-cdk/core.Lazy.uncachedAny: has been removed [removed:@aws-cdk/core.Lazy.uncachedAny]
err  - METHOD @aws-cdk/core.Lazy.uncachedList: has been removed [removed:@aws-cdk/core.Lazy.uncachedList]
err  - METHOD @aws-cdk/core.Lazy.uncachedNumber: has been removed [removed:@aws-cdk/core.Lazy.uncachedNumber]
err  - METHOD @aws-cdk/core.Lazy.uncachedString: has been removed [removed:@aws-cdk/core.Lazy.uncachedString]
err  - PROP @aws-cdk/core.DefaultStackSynthesizerProps.bucketPrefix: has been removed [removed:@aws-cdk/core.DefaultStackSynthesizerProps.bucketPrefix]

Pull request has been modified.

mergify · 2020-12-01T09:43:10Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2020-12-01T10:12:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: b3cae9a
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-12-01T10:12:48Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

jogold commented Nov 12, 2020

View reviewed changes

SomayaB assigned rix0rrr Nov 13, 2020

jogold mentioned this pull request Nov 13, 2020

[core] AssetStaging by bundling ignoring AssetHashType.OUTPUT when doing quick 'skip' hash #11460

Closed

jogold added 2 commits November 13, 2020 15:43

Merge branch 'master' into custom-hash-skip-bundling

13682e6

change type only when OUTPUT or BUNDLE

3e3fe90

rix0rrr previously requested changes Nov 23, 2020

View reviewed changes

Update asset-staging.ts

f9346de

rix0rrr previously approved these changes Nov 30, 2020

View reviewed changes

rix0rrr changed the title ~~fix(core): heavy source fingerprinting when bundling is skipped~~ fix(core): regression: source directory is fingerprinted even if bundling is skipped Nov 30, 2020

Merge branch 'master' into custom-hash-skip-bundling

c77517a

jogold requested a review from rix0rrr November 30, 2020 16:44

Merge branch 'master' into custom-hash-skip-bundling

7a9b1eb

rix0rrr approved these changes Dec 1, 2020

View reviewed changes

Merge branch 'master' into custom-hash-skip-bundling

b3cae9a

mergify bot merged commit 3cbc7fa into aws:master Dec 1, 2020

jogold deleted the custom-hash-skip-bundling branch December 1, 2020 10:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(core): regression: source directory is fingerprinted even if bundling is skipped #11440

fix(core): regression: source directory is fingerprinted even if bundling is skipped #11440

jogold commented Nov 12, 2020 •

edited

Loading

gitpod-io bot commented Nov 12, 2020 •

edited

Loading

jogold commented Nov 12, 2020

jogold Nov 12, 2020

mrgrain commented Nov 13, 2020 •

edited

Loading

jogold commented Nov 19, 2020 •

edited

Loading

rix0rrr Nov 23, 2020

jogold Nov 23, 2020

jogold Nov 23, 2020

rix0rrr Nov 30, 2020

rix0rrr commented Nov 23, 2020

mrgrain commented Nov 24, 2020

mrgrain commented Nov 24, 2020

jogold commented Nov 25, 2020

jogold commented Nov 30, 2020

mergify bot commented Dec 1, 2020

aws-cdk-automation commented Dec 1, 2020

mergify bot commented Dec 1, 2020

	return props.assetHashType === AssetHashType.BUNDLE \|\| props.assetHashType === AssetHashType.OUTPUT
	? this.calculateHash(AssetHashType.CUSTOM, this.node.path) // Use node path as dummy hash because we're not bundling

fix(core): regression: source directory is fingerprinted even if bundling is skipped #11440

fix(core): regression: source directory is fingerprinted even if bundling is skipped #11440

Conversation

jogold commented Nov 12, 2020 • edited Loading

gitpod-io bot commented Nov 12, 2020 • edited Loading

jogold commented Nov 12, 2020

jogold Nov 12, 2020

Choose a reason for hiding this comment

mrgrain commented Nov 13, 2020 • edited Loading

jogold commented Nov 19, 2020 • edited Loading

rix0rrr Nov 23, 2020

Choose a reason for hiding this comment

jogold Nov 23, 2020

Choose a reason for hiding this comment

jogold Nov 23, 2020

Choose a reason for hiding this comment

rix0rrr Nov 30, 2020

Choose a reason for hiding this comment

rix0rrr commented Nov 23, 2020

mrgrain commented Nov 24, 2020

mrgrain commented Nov 24, 2020

jogold commented Nov 25, 2020

jogold commented Nov 30, 2020

mergify bot commented Dec 1, 2020

aws-cdk-automation commented Dec 1, 2020

AWS CodeBuild CI Report

mergify bot commented Dec 1, 2020

jogold commented Nov 12, 2020 •

edited

Loading

gitpod-io bot commented Nov 12, 2020 •

edited

Loading

mrgrain commented Nov 13, 2020 •

edited

Loading

jogold commented Nov 19, 2020 •

edited

Loading