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.

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

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

Merged
merged 7 commits into from
Feb 21, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 50 additions & 38 deletions packages/aws-cdk/lib/api/toolkit-info.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,11 +99,11 @@ export class ToolkitInfo {
/**
* Prepare an ECR repository for uploading to using Docker
*/
public async prepareEcrRepository(id: string, imageTag: string): Promise<EcrRepositoryInfo> {
public async prepareEcrRepository(assetId: string): Promise<EcrRepositoryInfo> {
const ecr = await this.props.sdk.ecr(this.props.environment, Mode.ForWriting);

// Create the repository if it doesn't exist yet
const repositoryName = 'cdk/' + id.replace(/[:/]/g, '-').toLowerCase();
// Repository name based on asset id
const repositoryName = 'cdk/' + assetId.replace(/[:/]/g, '-').toLowerCase();

let repository;
try {
Expand All @@ -115,32 +115,34 @@ export class ToolkitInfo {
}

if (repository) {
try {
debug(`${repositoryName}: checking for image ${imageTag}`);
await ecr.describeImages({ repositoryName, imageIds: [{ imageTag }] }).promise();

// If we got here, the image already exists. Nothing else needs to be done.
return {
alreadyExists: true,
repositoryUri: repository.repositoryUri!,
repositoryName
};
} catch (e) {
if (e.code !== 'ImageNotFoundException') { throw e; }
}
} else {
debug(`${repositoryName}: creating`);
const response = await ecr.createRepository({ repositoryName }).promise();
repository = response.repository!;

// Better put a lifecycle policy on this so as to not cost too much money
await ecr.putLifecyclePolicy({
repositoryName,
lifecyclePolicyText: JSON.stringify(DEFAULT_REPO_LIFECYCLE)
}).promise();
return {
repositoryUri: repository.repositoryUri!,
repositoryName
};
}

// The repo exists, image just needs to be uploaded. Get auth to do so.
debug(`${repositoryName}: creating`);
const response = await ecr.createRepository({ repositoryName }).promise();
repository = response.repository!;

// Better put a lifecycle policy on this so as to not cost too much money
await ecr.putLifecyclePolicy({
repositoryName,
lifecyclePolicyText: JSON.stringify(DEFAULT_REPO_LIFECYCLE)
}).promise();

return {
repositoryUri: repository.repositoryUri!,
repositoryName
};
}

/**
* Get ECR credentials
*/
public async getEcrCredentials(): Promise<EcrCredentials> {
const ecr = await this.props.sdk.ecr(this.props.environment, Mode.ForReading);

debug(`Fetching ECR authorization token`);
const authData = (await ecr.getAuthorizationToken({ }).promise()).authorizationData || [];
if (authData.length === 0) {
Expand All @@ -150,28 +152,38 @@ export class ToolkitInfo {
const [username, password] = token.split(':');

return {
alreadyExists: false,
repositoryUri: repository.repositoryUri!,
repositoryName,
username,
password,
endpoint: authData[0].proxyEndpoint!,
};
}
}

export type EcrRepositoryInfo = CompleteEcrRepositoryInfo | UploadableEcrRepositoryInfo;
/**
* Check if image already exists in ECR repository
*/
public async checkEcrImage(repositoryName: string, imageTag: string): Promise<boolean> {
const ecr = await this.props.sdk.ecr(this.props.environment, Mode.ForReading);

export interface CompleteEcrRepositoryInfo {
repositoryUri: string;
repositoryName: string;
alreadyExists: true;
try {
debug(`${repositoryName}: checking for image ${imageTag}`);
await ecr.describeImages({ repositoryName, imageIds: [{ imageTag }] }).promise();

// If we got here, the image already exists. Nothing else needs to be done.
return true;
} catch (e) {
if (e.code !== 'ImageNotFoundException') { throw e; }
}

return false;
}
}

export interface UploadableEcrRepositoryInfo {
export interface EcrRepositoryInfo {
repositoryUri: string;
repositoryName: string;
alreadyExists: false;
}

export interface EcrCredentials {
username: string;
password: string;
endpoint: string;
Expand Down
56 changes: 48 additions & 8 deletions packages/aws-cdk/lib/docker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,43 +18,69 @@ import { PleaseHold } from './util/please-hold';
*
* As a workaround, we calculate our own digest over parts of the manifest that
* are unlikely to change, and tag based on that.
*
* When running in CI, we pull the latest image first and use it as cache for
* the build. CI is detected by the presence of the `CI` environment variable.
*/
export async function prepareContainerAsset(asset: ContainerImageAssetMetadataEntry, toolkitInfo: ToolkitInfo): Promise<CloudFormation.Parameter[]> {
debug(' 👑 Preparing Docker image asset:', asset.path);

const buildHold = new PleaseHold(` ⌛ Building Docker image for ${asset.path}; this may take a while.`);
try {
const ecr = await toolkitInfo.prepareEcrRepository(asset.id);
const latest = `${ecr.repositoryUri}:latest`;

let loggedIn = false;

// In CI we try to pull latest first
if (process.env.CI) {
await dockerLogin(toolkitInfo);
loggedIn = true;

try {
await shell(['docker', 'pull', latest]);
} catch (e) {
debug('Failed to pull latest image from ECR repository');
}
}

buildHold.start();

const command = ['docker',
const baseCommand = ['docker',
'build',
'--quiet',
asset.path];
const command = process.env.CI
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to use the variable here.

? [...baseCommand, '--cache-from', latest] // This does not fail if latest is not available
: baseCommand;
const imageId = (await shell(command, { quiet: true })).trim();

buildHold.stop();

const tag = await calculateImageFingerprint(imageId);

debug(` ⌛ Image has tag ${tag}, preparing ECR repository`);
const ecr = await toolkitInfo.prepareEcrRepository(asset.id, tag);
debug(` ⌛ Image has tag ${tag}, checking ECR repository`);
const imageExists = await toolkitInfo.checkEcrImage(ecr.repositoryName, tag);

if (ecr.alreadyExists) {
if (imageExists) {
debug(' 👑 Image already uploaded.');
} else {
// Login and push
debug(` ⌛ Image needs to be uploaded first.`);

await shell(['docker', 'login',
'--username', ecr.username,
'--password', ecr.password,
ecr.endpoint]);
if (!loggedIn) { // We could be already logged in if in CI
await dockerLogin(toolkitInfo);
}

const qualifiedImageName = `${ecr.repositoryUri}:${tag}`;

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

Choose a reason for hiding this comment

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

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


// There's no way to make this quiet, so we can't use a PleaseHold. Print a header message.
print(` ⌛ Pusing Docker image for ${asset.path}; this may take a while.`);
await shell(['docker', 'push', qualifiedImageName]);
await shell(['docker', 'push', latest]);
debug(` 👑 Docker image for ${asset.path} pushed.`);
}

Expand All @@ -72,6 +98,17 @@ export async function prepareContainerAsset(asset: ContainerImageAssetMetadataEn
}
}

/**
* Get credentials from ECR and run docker login
*/
async function dockerLogin(toolkitInfo: ToolkitInfo) {
const credentials = await toolkitInfo.getEcrCredentials();
await shell(['docker', 'login',
'--username', credentials.username,
'--password', credentials.password,
credentials.endpoint]);
}

/**
* Calculate image fingerprint.
*
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


// We're interested in the image itself, not any running instaces of it
delete manifest.Container;
delete manifest.ContainerConfig;
Expand Down