From 118d4ce0ab5fab570ccdf6beb4164d5ab88b3a5b Mon Sep 17 00:00:00 2001 From: Rob Jack Stewart Date: Tue, 2 Apr 2024 11:36:24 +0000 Subject: [PATCH 1/7] Disable cache on feature build when specified --- src/spec-node/containerFeatures.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/spec-node/containerFeatures.ts b/src/spec-node/containerFeatures.ts index 21649f10a..4d991d80d 100644 --- a/src/spec-node/containerFeatures.ts +++ b/src/spec-node/containerFeatures.ts @@ -82,7 +82,7 @@ export async function extendImage(params: DockerResolverParameters, config: Subs args.push('--load'); // (short for --output=docker, i.e. load into normal 'docker images' collection) } } - if (params.buildxCacheTo) { + if (params.buildxCacheTo && !params.buildNoCache) { args.push('--cache-to', params.buildxCacheTo); } @@ -95,6 +95,9 @@ export async function extendImage(params: DockerResolverParameters, config: Subs 'build', ); } + if (params.buildNoCache) { + args.push('--no-cache'); + } for (const buildArg in featureBuildInfo.buildArgs) { args.push('--build-arg', `${buildArg}=${featureBuildInfo.buildArgs[buildArg]}`); } From 6adb74c6407cbb61b1911253fc57fdc71b3d3dad Mon Sep 17 00:00:00 2001 From: Rob Jack Stewart Date: Wed, 10 Apr 2024 08:57:20 +0000 Subject: [PATCH 2/7] dont check --no-cache arg in case of --cache-to --- src/spec-node/containerFeatures.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/spec-node/containerFeatures.ts b/src/spec-node/containerFeatures.ts index 4d991d80d..67b985b3f 100644 --- a/src/spec-node/containerFeatures.ts +++ b/src/spec-node/containerFeatures.ts @@ -82,7 +82,7 @@ export async function extendImage(params: DockerResolverParameters, config: Subs args.push('--load'); // (short for --output=docker, i.e. load into normal 'docker images' collection) } } - if (params.buildxCacheTo && !params.buildNoCache) { + if (params.buildxCacheTo) { args.push('--cache-to', params.buildxCacheTo); } From e766ad94bde858e21a8c3f70ae3c37ae3c65f4a3 Mon Sep 17 00:00:00 2001 From: Rob Jack Stewart Date: Wed, 10 Apr 2024 09:58:02 +0000 Subject: [PATCH 3/7] Add test for --no-cache --- src/test/cli.build.test.ts | 57 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 57 insertions(+) diff --git a/src/test/cli.build.test.ts b/src/test/cli.build.test.ts index 633eba897..414b3226c 100644 --- a/src/test/cli.build.test.ts +++ b/src/test/cli.build.test.ts @@ -10,6 +10,7 @@ import * as os from 'os'; import { buildKitOptions, shellExec } from './testUtils'; import { ImageDetails } from '../spec-shutdown/dockerUtils'; import { envListToObj } from '../spec-node/utils'; +import * as crypto from 'crypto'; const pkg = require('../../package.json'); @@ -64,6 +65,62 @@ describe('Dev Containers CLI', function () { }); }); + describe('caching', () => { + let buildCommandRerunLog = ''; + let buildWithoutCacheLog = ''; + this.beforeEach(() => { + buildCommandRerunLog = `${os.tmpdir()}/${crypto.randomUUID()}`; + buildWithoutCacheLog = `${os.tmpdir()}/${crypto.randomUUID()}`; + console.log(`rerun log: ${buildCommandRerunLog}`); + console.log(`no cache log: ${buildWithoutCacheLog}`); + }); + + this.afterEach(() => { + for (const file in [buildCommandRerunLog, buildWithoutCacheLog]) { + if (fs.existsSync(file)) { + // fs.unlinkSync(file); + } + } + }); + + it('should not use docker cache for features when `--no-cache` flag is passed', async () => { + // Arrange + const testFolder = `${__dirname}/configs/image-with-features`; + const buildCommand = `${cli} build --workspace-folder ${testFolder}`; + const buildWithoutCacheCommand = `${buildCommand} --no-cache`; + const expectedFeatures = [ + 'ghcr.io/devcontainers/feature-starter/hello', + 'ghcr.io/devcontainers/features/docker-in-docker' + ]; + + // Act + await shellExec(`${buildCommand}`); // initial run of command + await shellExec(`${buildCommand} > ${buildCommandRerunLog} 2>&1`); // rerun command using cache + await shellExec(`${buildWithoutCacheCommand} > ${buildWithoutCacheLog} 2>&1`); // rerun command without cache + + // Assert + const buildCommandRerunLogContents = fs.readFileSync(buildCommandRerunLog, 'utf8'); + const buildWithoutCacheCommandLogContents = fs.readFileSync(buildWithoutCacheLog, 'utf8'); + + for (const logContent in [buildCommandRerunLogContents, buildWithoutCacheCommandLogContents]) { + assert.notEqual(logContent, null); + assert.notEqual(logContent, undefined); + assert.notEqual(logContent, ''); + } + + function countInstances(subject: string, search: string) { + return subject.split(search).length - 1; + } + + for (const expectedFeature of expectedFeatures) { + const featureNameInstanceCountInRerunOfBuildCommandLogs = countInstances(buildCommandRerunLogContents, expectedFeature); + const featureNameInstanceCountInBuildWithNoCache = countInstances(buildWithoutCacheCommandLogContents, expectedFeature); + assert.equal(featureNameInstanceCountInRerunOfBuildCommandLogs, 1, 'because a cached build prints features being installed in the image once at the start of the build when they are being resolved'); + assert.equal(featureNameInstanceCountInBuildWithNoCache, 2, 'because a non-cached build prints the features being installed in the image twice. Once at the start of the build when resolving the image, then again when installing the features in the image.'); + } + }); + }); + it('should fail with "not found" error when config is not found', async () => { let success = false; try { From f3b2df75183de066b8af4eb613ac91f281921236 Mon Sep 17 00:00:00 2001 From: Rob Jack Stewart Date: Fri, 12 Apr 2024 11:46:38 +0000 Subject: [PATCH 4/7] Assert based on layer shas --- src/test/cli.build.test.ts | 86 ++++++++++++++------------------------ 1 file changed, 32 insertions(+), 54 deletions(-) diff --git a/src/test/cli.build.test.ts b/src/test/cli.build.test.ts index 414b3226c..a33a2e9c2 100644 --- a/src/test/cli.build.test.ts +++ b/src/test/cli.build.test.ts @@ -65,60 +65,38 @@ describe('Dev Containers CLI', function () { }); }); - describe('caching', () => { - let buildCommandRerunLog = ''; - let buildWithoutCacheLog = ''; - this.beforeEach(() => { - buildCommandRerunLog = `${os.tmpdir()}/${crypto.randomUUID()}`; - buildWithoutCacheLog = `${os.tmpdir()}/${crypto.randomUUID()}`; - console.log(`rerun log: ${buildCommandRerunLog}`); - console.log(`no cache log: ${buildWithoutCacheLog}`); - }); - - this.afterEach(() => { - for (const file in [buildCommandRerunLog, buildWithoutCacheLog]) { - if (fs.existsSync(file)) { - // fs.unlinkSync(file); - } - } - }); - - it('should not use docker cache for features when `--no-cache` flag is passed', async () => { - // Arrange - const testFolder = `${__dirname}/configs/image-with-features`; - const buildCommand = `${cli} build --workspace-folder ${testFolder}`; - const buildWithoutCacheCommand = `${buildCommand} --no-cache`; - const expectedFeatures = [ - 'ghcr.io/devcontainers/feature-starter/hello', - 'ghcr.io/devcontainers/features/docker-in-docker' - ]; - - // Act - await shellExec(`${buildCommand}`); // initial run of command - await shellExec(`${buildCommand} > ${buildCommandRerunLog} 2>&1`); // rerun command using cache - await shellExec(`${buildWithoutCacheCommand} > ${buildWithoutCacheLog} 2>&1`); // rerun command without cache - - // Assert - const buildCommandRerunLogContents = fs.readFileSync(buildCommandRerunLog, 'utf8'); - const buildWithoutCacheCommandLogContents = fs.readFileSync(buildWithoutCacheLog, 'utf8'); - - for (const logContent in [buildCommandRerunLogContents, buildWithoutCacheCommandLogContents]) { - assert.notEqual(logContent, null); - assert.notEqual(logContent, undefined); - assert.notEqual(logContent, ''); - } - - function countInstances(subject: string, search: string) { - return subject.split(search).length - 1; - } - - for (const expectedFeature of expectedFeatures) { - const featureNameInstanceCountInRerunOfBuildCommandLogs = countInstances(buildCommandRerunLogContents, expectedFeature); - const featureNameInstanceCountInBuildWithNoCache = countInstances(buildWithoutCacheCommandLogContents, expectedFeature); - assert.equal(featureNameInstanceCountInRerunOfBuildCommandLogs, 1, 'because a cached build prints features being installed in the image once at the start of the build when they are being resolved'); - assert.equal(featureNameInstanceCountInBuildWithNoCache, 2, 'because a non-cached build prints the features being installed in the image twice. Once at the start of the build when resolving the image, then again when installing the features in the image.'); - } - }); + it.only('should not use docker cache for features when `--no-cache` flag is passed', async () => { + // Arrange + const testFolder = `${__dirname}/configs/image-with-features`; + const originalImageName = 'feature-cache-test-original-image'; + const cachedImageName = 'feature-cache-test-rerun-image'; + const nonCachedImageName = 'feature-cache-test-no-cache-image'; + + const commandBase = `${cli} build --workspace-folder ${testFolder}`; + const buildCommand = `${commandBase} --image-name ${originalImageName}`; + const cachedBuildCommand = `${commandBase} --image-name ${cachedImageName}`; + const buildWithoutCacheCommand = `${commandBase} --image-name ${nonCachedImageName} --no-cache`; + + // Act + await shellExec(buildCommand); // initial run of command + await shellExec(cachedBuildCommand); // rerun command using cache + await shellExec(buildWithoutCacheCommand); // rerun command without cache + + // Assert + const originalImageInspectCommandResult = await shellExec(`docker inspect ${originalImageName}`); + const cachedImageInspectCommandResult = await shellExec(`docker inspect ${cachedImageName}`); + const noCacheImageInspectCommandResult = await shellExec(`docker inspect ${nonCachedImageName}`); + + const originalImageDetails = JSON.parse(originalImageInspectCommandResult.stdout); + const cachedImageDetails = JSON.parse(cachedImageInspectCommandResult.stdout); + const noCacheImageDetails = JSON.parse(noCacheImageInspectCommandResult.stdout); + + const originalImageLayers: string[] = originalImageDetails[0].RootFS.Layers; + const cachedImageLayers: string[] = cachedImageDetails[0].RootFS.Layers; + const nonCachedImageLayers: string[] = noCacheImageDetails[0].RootFS.Layers; + + assert.deepEqual(originalImageLayers, cachedImageLayers); + assert.notDeepEqual(cachedImageLayers, nonCachedImageLayers); }); it('should fail with "not found" error when config is not found', async () => { From a540f83b4ec48418c7880fc4a6de3fb2ddee331c Mon Sep 17 00:00:00 2001 From: Rob Jack Stewart Date: Fri, 12 Apr 2024 11:49:48 +0000 Subject: [PATCH 5/7] add 'because' statements --- src/test/cli.build.test.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/cli.build.test.ts b/src/test/cli.build.test.ts index a33a2e9c2..943346af8 100644 --- a/src/test/cli.build.test.ts +++ b/src/test/cli.build.test.ts @@ -78,9 +78,9 @@ describe('Dev Containers CLI', function () { const buildWithoutCacheCommand = `${commandBase} --image-name ${nonCachedImageName} --no-cache`; // Act - await shellExec(buildCommand); // initial run of command - await shellExec(cachedBuildCommand); // rerun command using cache - await shellExec(buildWithoutCacheCommand); // rerun command without cache + await shellExec(buildCommand); + await shellExec(cachedBuildCommand); + await shellExec(buildWithoutCacheCommand); // Assert const originalImageInspectCommandResult = await shellExec(`docker inspect ${originalImageName}`); @@ -95,8 +95,8 @@ describe('Dev Containers CLI', function () { const cachedImageLayers: string[] = cachedImageDetails[0].RootFS.Layers; const nonCachedImageLayers: string[] = noCacheImageDetails[0].RootFS.Layers; - assert.deepEqual(originalImageLayers, cachedImageLayers); - assert.notDeepEqual(cachedImageLayers, nonCachedImageLayers); + assert.deepEqual(originalImageLayers, cachedImageLayers, 'because they were built csequentially and should have used caching'); + assert.notDeepEqual(cachedImageLayers, nonCachedImageLayers, 'because we passed the --no-cache argument disabling caching'); }); it('should fail with "not found" error when config is not found', async () => { From a0943d93bebdd5ddb9a64b9554417eb8a9b15510 Mon Sep 17 00:00:00 2001 From: Rob Jack Stewart Date: Fri, 12 Apr 2024 12:43:19 +0000 Subject: [PATCH 6/7] extract base image layers from built image layers --- src/test/cli.build.test.ts | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/src/test/cli.build.test.ts b/src/test/cli.build.test.ts index 943346af8..8fa844cea 100644 --- a/src/test/cli.build.test.ts +++ b/src/test/cli.build.test.ts @@ -10,7 +10,6 @@ import * as os from 'os'; import { buildKitOptions, shellExec } from './testUtils'; import { ImageDetails } from '../spec-shutdown/dockerUtils'; import { envListToObj } from '../spec-node/utils'; -import * as crypto from 'crypto'; const pkg = require('../../package.json'); @@ -68,6 +67,11 @@ describe('Dev Containers CLI', function () { it.only('should not use docker cache for features when `--no-cache` flag is passed', async () => { // Arrange const testFolder = `${__dirname}/configs/image-with-features`; + const devContainerJson = `${testFolder}/.devcontainer.json`; + + const devContainerFileContents = JSON.parse(fs.readFileSync(devContainerJson, 'utf8')); + const baseImage = devContainerFileContents.image; + const originalImageName = 'feature-cache-test-original-image'; const cachedImageName = 'feature-cache-test-rerun-image'; const nonCachedImageName = 'feature-cache-test-no-cache-image'; @@ -77,26 +81,54 @@ describe('Dev Containers CLI', function () { const cachedBuildCommand = `${commandBase} --image-name ${cachedImageName}`; const buildWithoutCacheCommand = `${commandBase} --image-name ${nonCachedImageName} --no-cache`; + function arrayStartsWithArray(subject: string[], startsWith: string[]) { + if (subject.length < startsWith.length) { + return false; + } + for (let i = 0; i < startsWith.length; i++) { + if (subject[i] !== startsWith[i]) { + return false; + } + } + return true; + } + + function haveCommonEntries(arr1: string[], arr2: string[]) { + return arr1.every(item => arr2.includes(item)); + } + // Act + await shellExec(`docker pull ${baseImage}`); // pull base image so we can inspect it later await shellExec(buildCommand); await shellExec(cachedBuildCommand); await shellExec(buildWithoutCacheCommand); // Assert + const baseImageInspectCommandResult = await shellExec(`docker inspect ${baseImage}`); const originalImageInspectCommandResult = await shellExec(`docker inspect ${originalImageName}`); const cachedImageInspectCommandResult = await shellExec(`docker inspect ${cachedImageName}`); const noCacheImageInspectCommandResult = await shellExec(`docker inspect ${nonCachedImageName}`); + const baseImageDetails = JSON.parse(baseImageInspectCommandResult.stdout); const originalImageDetails = JSON.parse(originalImageInspectCommandResult.stdout); const cachedImageDetails = JSON.parse(cachedImageInspectCommandResult.stdout); const noCacheImageDetails = JSON.parse(noCacheImageInspectCommandResult.stdout); + const baseImageLayers: string[] = baseImageDetails[0].RootFS.Layers; const originalImageLayers: string[] = originalImageDetails[0].RootFS.Layers; const cachedImageLayers: string[] = cachedImageDetails[0].RootFS.Layers; const nonCachedImageLayers: string[] = noCacheImageDetails[0].RootFS.Layers; - assert.deepEqual(originalImageLayers, cachedImageLayers, 'because they were built csequentially and should have used caching'); - assert.notDeepEqual(cachedImageLayers, nonCachedImageLayers, 'because we passed the --no-cache argument disabling caching'); + assert.equal(arrayStartsWithArray(originalImageLayers, baseImageLayers), true, 'because the image is made up of feature layers on top of the base image'); + assert.equal(arrayStartsWithArray(cachedImageLayers, baseImageLayers), true, 'because the image is made up of feature layers on top of the base image'); + assert.equal(arrayStartsWithArray(nonCachedImageLayers, baseImageLayers), true, 'because the image is made up of feature layers on top of the base image'); + + const originalImageWithoutBaseImageLayers = originalImageLayers.slice(baseImageLayers.length); + const cachedImageWithoutBaseImageLayers = cachedImageLayers.slice(baseImageLayers.length); + const nonCachedImageWithoutBaseImageLayers = nonCachedImageLayers.slice(baseImageLayers.length); + + assert.deepEqual(originalImageWithoutBaseImageLayers, cachedImageWithoutBaseImageLayers, 'because they are the same image built sequentially therefore the second should have used caching'); + assert.equal(haveCommonEntries(cachedImageWithoutBaseImageLayers, nonCachedImageWithoutBaseImageLayers), false, 'because we passed the --no-cache argument which disables the use of the cache, therefore the non-base image layers should have nothin in common'); }); it('should fail with "not found" error when config is not found', async () => { From b7e6236ac934c9f923b5e8b6c132a94a7def7225 Mon Sep 17 00:00:00 2001 From: Rob Jack Stewart Date: Fri, 12 Apr 2024 12:43:58 +0000 Subject: [PATCH 7/7] remove only --- src/test/cli.build.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/cli.build.test.ts b/src/test/cli.build.test.ts index 8fa844cea..c22e3913f 100644 --- a/src/test/cli.build.test.ts +++ b/src/test/cli.build.test.ts @@ -64,7 +64,7 @@ describe('Dev Containers CLI', function () { }); }); - it.only('should not use docker cache for features when `--no-cache` flag is passed', async () => { + it('should not use docker cache for features when `--no-cache` flag is passed', async () => { // Arrange const testFolder = `${__dirname}/configs/image-with-features`; const devContainerJson = `${testFolder}/.devcontainer.json`;