feat(toolkit): improve docker build time in CI #1776

jogold · 2019-02-15T21:41:34Z

When running in CI, try to pull the latest image first and use it as cache for the build. CI is detected by the presence of the CI environment variable or the --ci command line option.

When pushing docker images, always tag with latest.

Closes #1748

To be discussed: name of env var for detection, CI is set by lots of CI providers (not by CodeBuild though), use a opt-in mechanism instead?

Test (with empty ECR):

$ docker images -a | grep "/cdk/" | awk '{print $3}' | xargs docker rmi # or a more specific grep
$ cdk deploy # should build and push image
$ docker images -a | grep "/cdk/" | awk '{print $3}' | xargs docker rmi --force # or a more specific grep
$ CI=true cdk deploy # should pull, build almost instantly but not push

Pull Request Checklist

Testing
- Unit test added
- CLI change?: coordinate update of integration tests with team
- cdk-init template change?: coordinated update of integration tests with team
Docs
- jsdocs: All public APIs documented
- README: README and/or documentation topic updated
Title and Description
- Change type: title prefixed with fix, feat will appear in changelog
- Title: use lower-case and doesn't end with a period
- Breaking?: last paragraph: "BREAKING CHANGE: <describe what changed + link for details>"
- Issues: Indicate issues fixed via: "Fixes #xxx" or "Closes #xxx"
Sensitive Modules (requires 2 PR approvers)
- IAM Policy Document (in @aws-cdk/aws-iam)
- EC2 Security Groups and ACLs (in @aws-cdk/aws-ec2)
- Grant APIs (only if not based on official documentation with a reference)

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

When running in CI, try to pull the latest image first and use it as cache for the build. CI is detected by the presence of the `CI` environment variable. Closes aws#1748

rix0rrr

In general, I like the change and the refactorings you made, thanks for that.

Can you describe a little more about the rationale behind the change? Are we not trading docker build time for docker pull time? Are we guaranteed that pulling will be faster than building? Why limit this behavior change to CI builds only?
Though I like the use of the CI environment variable, I think there needs to be a command-line switch --ci which is threaded through to the build step. When not supplied, it could default to the presence of the environment variable CI. Be aware that --no-ci should also work to disable an autodetected CI build (it'll be the difference between undefined and false in the yargs object).

rix0rrr · 2019-02-18T09:49:42Z

packages/aws-cdk/lib/docker.ts

@@ -95,6 +132,9 @@ async function calculateImageFingerprint(imageId: string) {
  // Metadata that has no bearing on the image contents
  delete manifest.Created;

+  // Parent can change when using --cache-from in CI
+  delete manifest.Parent;


What does Parent represent here? Please tell me it has nothing to do with disk images

It's the sha256 of the docker's parent image. When using --cache-from, the sha256 of the parent gets modified, even for the same final image.

Wait, if it's a parent image, and the parent image changes on every build, then we cannot assume that the "current" image is the same either, can we?

In this case the sha256 of the layers will change normally. I added this when I saw that for the same digest (on ECR) when using --cache-from, calculateImageFingerprint returned a different finger print, resulting in 3 tags on the same image (latest, fingerprint1 and fingerprint2). This needs further investigation. The optimal solution may be a mix between the custom finger print calculation and the docker digest. Looking into this.

I'm removing this delete manifest.Parent, it's a mistake and not needed here. The sha256 of the Parent is also present in the Config.Image key so deleting it in one place only should not have had an effect.

rix0rrr · 2019-02-18T09:54:01Z

packages/aws-cdk/lib/docker.ts

      await shell(['docker', 'tag', imageId, qualifiedImageName]);
+      await shell(['docker', 'tag', imageId, latest]); // Tag with `latest` also


You need to describe this change in the PR description as well.

jogold · 2019-02-18T11:10:23Z

Can you describe a little more about the rationale behind the change? Are we not trading docker build time for docker pull time? Are we guaranteed that pulling will be faster than building?

We are indeed trading docker build time for docker pull time. Generally pulling will be faster than building for Dockerfiles with OS packages installation and code packages installation (pip, npm, etc.). If nothing has changed in the current commit (in CI), it will be faster to pull than to build. If there are changes, it's more likely to see changes in the code that is copied in the image (COPY/ADD, the bottom layers) and again pulling should be faster than building.

For very simple Dockerfiles (FROM + COPY/ADD, few layers) or for a complete change of the Dockerfile, I can imagine that it could be faster to build.

Why limit this behavior change to CI builds only?

When running locally chances are that we already have a lot of layer cache available. In this case, pulling will almost always be slower than building.

Though I like the use of the CI environment variable, I think there needs to be a command-line switch --ci which is threaded through to the build step. When not supplied, it could default to the presence of the environment variable CI. Be aware that --no-ci should also work to disable an autodetected CI build (it'll be the difference between undefined and false in the yargs object).

OK, I will look into this.

rix0rrr · 2019-02-18T11:50:01Z

Thanks for the explanation. I'd like to see that rationale captured in one or two sentenced in a code comment somewhere.

jogold · 2019-02-18T16:35:10Z

packages/aws-cdk/bin/cdk.ts

@@ -61,6 +61,7 @@ async function parseCommandLineArguments() {
    .command('deploy [STACKS..]', 'Deploys the stack(s) named STACKS into your AWS account', yargs => yargs
      .option('exclusively', { type: 'boolean', alias: 'e', desc: 'only deploy requested stacks, don\'t include dependencies' })
      .option('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'what security-sensitive changes need manual approval' }))
+      .option('ci', { type: 'boolean', desc: 'Force CI detection. Use --no-ci to disable CI autodetection.', default: undefined })


@rix0rrr The description makes no reference to docker/asset preparation. I thought that this option could be used elsewhere in the future. What about the documentation for this new option? I see that https://github.com/awsdocs/aws-cdk-user-guide/blob/master/doc_source/tools.md is not up to date...

That seems good. @Doug-AWS will update the tools section once we release.

Please follow this by something like:

if (argv.ci === undefined) { argv.ci = process.env.CI !== undefined; }

To make sure that afterwards argv.ci is well-defined. Saves having to redo the CI-autodetection logic in every place where it's used. Configuring:

default: process.env.CI !== undefined

To force the default value to either true or false might also work.

Tools is updated. I should have a new public build of the guide by EOW.

rix0rrr · 2019-02-20T14:09:59Z

packages/aws-cdk/bin/cdk.ts

@@ -61,6 +61,7 @@ async function parseCommandLineArguments() {
    .command('deploy [STACKS..]', 'Deploys the stack(s) named STACKS into your AWS account', yargs => yargs
      .option('exclusively', { type: 'boolean', alias: 'e', desc: 'only deploy requested stacks, don\'t include dependencies' })
      .option('require-approval', { type: 'string', choices: [RequireApproval.Never, RequireApproval.AnyChange, RequireApproval.Broadening], desc: 'what security-sensitive changes need manual approval' }))
+      .option('ci', { type: 'boolean', desc: 'Force CI detection. Use --no-ci to disable CI autodetection.', default: undefined })


That seems good. @Doug-AWS will update the tools section once we release.

Please follow this by something like:

if (argv.ci === undefined) { argv.ci = process.env.CI !== undefined; }

To make sure that afterwards argv.ci is well-defined. Saves having to redo the CI-autodetection logic in every place where it's used. Configuring:

default: process.env.CI !== undefined

To force the default value to either true or false might also work.

rix0rrr · 2019-02-20T14:10:40Z

packages/aws-cdk/lib/docker.ts

+    let loggedIn = false;
+
+    // In CI we try to pull latest first
+    if (ci === true || (process.env.CI && ci !== false)) {


Don't do the CI autodetection here, move it up to cdk.ts.

Yep. I have a script that recreates the 'cdk --help' part of the tools section whenever we have a new release.

rix0rrr · 2019-02-20T14:10:59Z

packages/aws-cdk/lib/docker.ts

      'build',
      '--quiet',
      asset.path];
+    const command = process.env.CI


Need to use the variable here.

When running in CI, try to pull the latest image first and use it as cache for the build. CI is detected by the presence of the `CI` environment variable. Closes #1748

feat(toolkit): improve docker build time in CI

334c7b7

When running in CI, try to pull the latest image first and use it as cache for the build. CI is detected by the presence of the `CI` environment variable. Closes aws#1748

jogold requested a review from a team as a code owner February 15, 2019 21:41

rix0rrr reviewed Feb 18, 2019

View reviewed changes

rix0rrr self-assigned this Feb 18, 2019

jogold added 4 commits February 18, 2019 15:51

Remove delete manifest.Parent

7866319

Add --ci option to deploy

d9f3cc5

Always tag and push latest

e2df6f1

Add code comment

67947f6

jogold commented Feb 18, 2019

View reviewed changes

rix0rrr reviewed Feb 20, 2019

View reviewed changes

Move CI autodetection to cdk.ts

5238eb4

jogold force-pushed the docker-cache branch from f85c776 to 5238eb4 Compare February 20, 2019 15:37

Make ci required in cliDeploy

d35682e

rix0rrr approved these changes Feb 21, 2019

View reviewed changes

rix0rrr merged commit 1060b95 into aws:master Feb 21, 2019

eladb mentioned this pull request Mar 11, 2019

cdk-bundle #1954

Closed

5 tasks

jogold deleted the docker-cache branch March 12, 2019 11:23

danilofuchs mentioned this pull request Nov 9, 2020

(pipelines) Support docker image caching on CodeBuild #11379

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(toolkit): improve docker build time in CI #1776

feat(toolkit): improve docker build time in CI #1776

jogold commented Feb 15, 2019 •

edited

Loading

rix0rrr left a comment

rix0rrr Feb 18, 2019

jogold Feb 18, 2019

rix0rrr Feb 18, 2019

jogold Feb 18, 2019

jogold Feb 18, 2019

rix0rrr Feb 18, 2019

jogold commented Feb 18, 2019

rix0rrr commented Feb 18, 2019

jogold Feb 18, 2019

rix0rrr Feb 20, 2019

Doug-AWS Mar 7, 2019

rix0rrr Feb 20, 2019

rix0rrr Feb 20, 2019

Doug-AWS Feb 20, 2019

rix0rrr Feb 20, 2019

		await shell(['docker', 'tag', imageId, qualifiedImageName]);
		await shell(['docker', 'tag', imageId, latest]); // Tag with `latest` also

feat(toolkit): improve docker build time in CI #1776

feat(toolkit): improve docker build time in CI #1776

Conversation

jogold commented Feb 15, 2019 • edited Loading

Pull Request Checklist

rix0rrr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold commented Feb 18, 2019

rix0rrr commented Feb 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold commented Feb 15, 2019 •

edited

Loading