Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: don't upload the same asset multiple times #1011

Merged
merged 2 commits into from
Nov 1, 2018
Merged

feat: don't upload the same asset multiple times #1011

merged 2 commits into from
Nov 1, 2018

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Oct 25, 2018

This change implements two asset upload frugality measures:

  • If a lambda.AssetCode object is reused for multiple Lambdas,
    the same underyling Asset object will be reused (which leads to
    the asset data only being uploaded once).
  • If nonetheless multiple Asset objects are created for the same
    source data, the data will only be uploaded once and subsequently
    copied on the server-side to avoid the additional data transfer.

Fixes #989.

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

feat: don't upload the same asset multiple times
813ab47 
This change implements two asset upload frugality measures:

- If a lambda.AssetCode object is reused for multiple Lambdas,
  the same underyling Asset object will be reused (which leads to
  the asset data only being uploaded once).
- If nonetheless multiple Asset objects are created for the same
  source data, the data will only be uploaded once and subsequently
  copied on the server-side to avoid the additional data transfer.

Fixes #989.
@rix0rrr rix0rrr self-assigned this Oct 25, 2018
eladb
eladb reviewed Oct 25, 2018
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might be missing something... Why not just reuse the same file (i.e. pass in the same bucket/key if the file was already uploaded)

packages/@aws-cdk/aws-lambda/lib/code.ts
path: this.path,
packaging: this.packaging
});
// If the same AssetCode is used multiple times, retain only the first instantiation.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for this?

packages/aws-cdk/lib/api/toolkit-info.ts

// Upload if it's new or server-side copy if it was already uploaded previously
const previous = this.previousUploads[hash];
if (previous) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just reuse the same file that was already uploaded?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're organizing the data in folders, by Asset instance.

This is currently so that we can give permissions on the folder to constructs that require access to the asset, so they will be able to download all versions of it, so that even if we upload a new version and the Execution Role is updated to only allow downloading the latest version of the asset, old runs/definitions will not stop working. This is not required for Lambdas which have a point-in-time handover but it is required for CodeBuild script assets, for example, which download the file at an arbitrary later point in time.

If we reused the same file between multiple assets, we'd either have to fall back to "only permissions to the latest version of the asset" or "permission to all assets in the bucket".

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Nov 1, 2018

Ping

eladb
eladb approved these changes Nov 1, 2018
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dare not ask - toolkit integration test?

packages/aws-cdk/lib/api/toolkit-info.ts
/**
* A cache of previous uploads done in this session
*/
private readonly previousUploads: {[key: string]: Uploaded} = {};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename key to something meaningful

@rix0rrr
Copy link
Contributor Author

rix0rrr commented Nov 1, 2018

I dare not ask - toolkit integration test?

It's integ-tested by integ.multi-assets.ts

@rix0rrr rix0rrr merged commit 35937b6 into master Nov 1, 2018
@rix0rrr rix0rrr deleted the huijbers/asset-frugality branch November 1, 2018 14:04
rix0rrr pushed a commit that referenced this pull request Nov 6, 2018
v0.15.0
1a864d1 
Bug Fixes
=========

* **aws-autoscaling:** allow minSize to be set to 0 ([#1015](#1015)) ([67f7fa1](67f7fa1))
* **aws-codebuild:** correctly pass the timeout property to CFN when creating a Project. ([#1071](#1071)) ([b1322bb](b1322bb))
* **aws-codebuild:** correctly set S3 path when using it as artifact. ([#1072](#1072)) ([f32cba9](f32cba9))
* **aws-kms:** add output value when exporting an encryption key ([#1036](#1036)) ([cb490be](cb490be))
* Switch from `js-yaml` to `yaml` ([#1092](#1092)) ([0b132b5](0b132b5))

Features
=========

* **applets:** integrate into toolkit ([#1039](#1039)) ([fdabe95](fdabe95)), closes [#849](#849) [#342](#342) [#291](#291)
* **aws-codecommit:** use CloudWatch Events instead of polling by default in the CodePipeline Action. ([#1026](#1026)) ([d09d30c](d09d30c))
* **aws-dynamodb:** allow specifying partition/sort keys in props ([#1054](#1054)) ([ec87331](ec87331)), closes [#1051](#1051)
* **aws-ec2:** AmazonLinuxImage supports AL2 ([#1081](#1081)) ([97b57a5](97b57a5)), closes [#1062](#1062)
* **aws-lambda:** high level API for event sources ([#1063](#1063)) ([1be3442](1be3442))
* **aws-sqs:** improvements to IAM grants API ([#1052](#1052)) ([6f2475e](6f2475e))
* don't upload the same asset multiple times ([#1011](#1011)) ([35937b6](35937b6)), closes [#989](#989)
* **codepipeline/cfn:** Use fewer statements for pipeline permissions ([#1009](#1009)) ([8f4c2ab](8f4c2ab))
* add a new construct library for ECS ([#1058](#1058)) ([ae03ddb](ae03ddb))
* **pkglint:** Make sure .snk files are ignored ([#1049](#1049)) ([53c8d76](53c8d76)), closes [#643](#643)
* **toolkit:** deployment ui improvements ([#1067](#1067)) ([c832eaf](c832eaf))

BREAKING CHANGES
=========

* The ec2.Connections object has been changed to be able to manage multiple
  security groups. The relevant property has been changed from `securityGroup`
  to `securityGroups` (an array of security group objects).
* **aws-codecommit:** This modifies the default behavior of the CodeCommit
  Action.  It also changes the internal API contract between the
  aws-codepipeline-api module and the CodePipeline Actions in the service
  packages.
* **applets:** The applet schema has changed to allow Multiple applets can be
  define in one file by structuring the files like this:
* **applets:** The applet schema has changed to allow definition of multiple
  applets in the same file.

The schema now looks like this:

    applets:
      MyApplet:
        type: ./my-applet-file
        properties:
          property1: value
          ...
By starting an applet specifier with npm://, applet modules can
directly be referenced in NPM. You can include a version specifier
(@1.2.3) to reference specific versions.
* **aws-sqs:** `queue.grantReceiveMessages` has been removed. It is unlikely
  that this would be sufficient to interact with a queue. Alternatively you can
  use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if
  there's a need to only grant this action.
@rix0rrr rix0rrr mentioned this pull request Nov 6, 2018
Merged
rix0rrr pushed a commit that referenced this pull request Nov 6, 2018
v0.15.0
68e18a4 
Bug Fixes
========

* **aws-autoscaling:** allow minSize to be set to 0 ([#1015](#1015)) ([67f7fa1](67f7fa1))
* **aws-codebuild:** correctly pass the timeout property to CFN when creating a Project. ([#1071](#1071)) ([b1322bb](b1322bb))
* **aws-codebuild:** correctly set S3 path when using it as artifact. ([#1072](#1072)) ([f32cba9](f32cba9))
* **aws-kms:** add output value when exporting an encryption key ([#1036](#1036)) ([cb490be](cb490be))
* Switch from `js-yaml` to `yaml` ([#1092](#1092)) ([0b132b5](0b132b5))

Features
========

* don't upload the same asset multiple times ([#1011](#1011)) ([35937b6](35937b6)), closes [#989](#989)
* **app-delivery:** CI/CD for CDK Stacks ([#1022](#1022)) ([f2fe4e9](f2fe4e9))
* add a new construct library for ECS ([#1058](#1058)) ([ae03ddb](ae03ddb))
* **applets:** integrate into toolkit ([#1039](#1039)) ([fdabe95](fdabe95)), closes [#849](#849) [#342](#342) [#291](#291)
* **aws-codecommit:** use CloudWatch Events instead of polling by default in the CodePipeline Action. ([#1026](#1026)) ([d09d30c](d09d30c))
* **aws-dynamodb:** allow specifying partition/sort keys in props ([#1054](#1054)) ([ec87331](ec87331)), closes [#1051](#1051)
* **aws-ec2:** AmazonLinuxImage supports AL2 ([#1081](#1081)) ([97b57a5](97b57a5)), closes [#1062](#1062)
* **aws-lambda:** high level API for event sources ([#1063](#1063)) ([1be3442](1be3442))
* **aws-sqs:** improvements to IAM grants API ([#1052](#1052)) ([6f2475e](6f2475e))
* **codepipeline/cfn:** Use fewer statements for pipeline permissions ([#1009](#1009)) ([8f4c2ab](8f4c2ab))
* **pkglint:** Make sure .snk files are ignored ([#1049](#1049)) ([53c8d76](53c8d76)), closes [#643](#643)
* **toolkit:** deployment ui improvements ([#1067](#1067)) ([c832eaf](c832eaf))
* Update to CloudFormation resource specification v2.11.0

BREAKING CHANGES
========

* The ec2.Connections object has been changed to be able to manage multiple
  security groups. The relevant property has been changed from `securityGroup`
  to `securityGroups` (an array of security group objects).
* **aws-codecommit:** this modifies the default behavior of the CodeCommit
  Action.  It also changes the internal API contract between the
  aws-codepipeline-api module and the CodePipeline Actions in the service
  packages.
* **applets:** The applet schema has changed to allow Multiple applets can be
  define in one file by structuring the files like this:
* **applets:** The applet schema has changed to allow definition of multiple
  applets in the same file.

The schema now looks like this:

    applets:
      MyApplet:
        type: ./my-applet-file
        properties:
          property1: value
          ...
By starting an applet specifier with npm://, applet modules can directly be
referenced in NPM. You can include a version specifier (@1.2.3) to reference
specific versions.
* **aws-sqs:** `queue.grantReceiveMessages` has been removed. It is unlikely
  that this would be sufficient to interact with a queue. Alternatively you can
  use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if
  there's a need to only grant this action.
rix0rrr added a commit that referenced this pull request Nov 6, 2018
@rix0rrr
v0.15.0 (#1094)
fbb2067 
Bug Fixes
========

* **aws-autoscaling:** allow minSize to be set to 0 ([#1015](#1015)) ([67f7fa1](67f7fa1))
* **aws-codebuild:** correctly pass the timeout property to CFN when creating a Project. ([#1071](#1071)) ([b1322bb](b1322bb))
* **aws-codebuild:** correctly set S3 path when using it as artifact. ([#1072](#1072)) ([f32cba9](f32cba9))
* **aws-kms:** add output value when exporting an encryption key ([#1036](#1036)) ([cb490be](cb490be))
* Switch from `js-yaml` to `yaml` ([#1092](#1092)) ([0b132b5](0b132b5))

Features
========

* don't upload the same asset multiple times ([#1011](#1011)) ([35937b6](35937b6)), closes [#989](#989)
* **app-delivery:** CI/CD for CDK Stacks ([#1022](#1022)) ([f2fe4e9](f2fe4e9))
* add a new construct library for ECS ([#1058](#1058)) ([ae03ddb](ae03ddb))
* **applets:** integrate into toolkit ([#1039](#1039)) ([fdabe95](fdabe95)), closes [#849](#849) [#342](#342) [#291](#291)
* **aws-codecommit:** use CloudWatch Events instead of polling by default in the CodePipeline Action. ([#1026](#1026)) ([d09d30c](d09d30c))
* **aws-dynamodb:** allow specifying partition/sort keys in props ([#1054](#1054)) ([ec87331](ec87331)), closes [#1051](#1051)
* **aws-ec2:** AmazonLinuxImage supports AL2 ([#1081](#1081)) ([97b57a5](97b57a5)), closes [#1062](#1062)
* **aws-lambda:** high level API for event sources ([#1063](#1063)) ([1be3442](1be3442))
* **aws-sqs:** improvements to IAM grants API ([#1052](#1052)) ([6f2475e](6f2475e))
* **codepipeline/cfn:** Use fewer statements for pipeline permissions ([#1009](#1009)) ([8f4c2ab](8f4c2ab))
* **pkglint:** Make sure .snk files are ignored ([#1049](#1049)) ([53c8d76](53c8d76)), closes [#643](#643)
* **toolkit:** deployment ui improvements ([#1067](#1067)) ([c832eaf](c832eaf))
* Update to CloudFormation resource specification v2.11.0

BREAKING CHANGES
========

* The ec2.Connections object has been changed to be able to manage multiple
  security groups. The relevant property has been changed from `securityGroup`
  to `securityGroups` (an array of security group objects).
* **aws-codecommit:** this modifies the default behavior of the CodeCommit
  Action.  It also changes the internal API contract between the
  aws-codepipeline-api module and the CodePipeline Actions in the service
  packages.
* **applets:** The applet schema has changed to allow Multiple applets can be
  define in one file by structuring the files like this:
* **applets:** The applet schema has changed to allow definition of multiple
  applets in the same file.

The schema now looks like this:

    applets:
      MyApplet:
        type: ./my-applet-file
        properties:
          property1: value
          ...
By starting an applet specifier with npm://, applet modules can directly be
referenced in NPM. You can include a version specifier (@1.2.3) to reference
specific versions.
* **aws-sqs:** `queue.grantReceiveMessages` has been removed. It is unlikely
  that this would be sufficient to interact with a queue. Alternatively you can
  use `queue.grantConsumeMessages` or `queue.grant('sqs:ReceiveMessage')` if
  there's a need to only grant this action.
@NGL321 NGL321 added the contribution/core This is a PR that came from AWS. label Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eladb eladb eladb approved these changes

@RomainMuller RomainMuller Awaiting requested review from RomainMuller

Assignees

@rix0rrr rix0rrr

Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

When multiple Lambda functions share code, build a single asset shared between them
3 participants
@rix0rrr @eladb @NGL321