Skip to content

Commit

Permalink
perf(action): Filter out pre-cached images
Browse files Browse the repository at this point in the history 
Only save relevant Docker images. Improve performance when run as root,
e.g., in test workflow. Previously, unnecessary Docker images were saved
because GitHub Actions pre-caches a standard set of Docker images when
Docker runs as root; those images aren't accessible when using
rootless-docker, so this change doesn't impact users of rootless-docker.
  • Loading branch information
@mwarres
mwarres committed Aug 8, 2022
1 parent 90cc1a2 commit 872f1ae
Show file tree
Hide file tree
Showing 6 changed files with 184 additions and 69 deletions.
2 changes: 1 addition & 1 deletion dist/main/index.js
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/post/index.js
View file

Large diffs are not rendered by default.

79 changes: 60 additions & 19 deletions src/docker.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,13 @@ const assertCalledInOrder = (...mocks: jest.Mock[]): void => {

describe("Docker images", (): void => {
const KEY = "a-cache-key";
const imagesList = Array.from(
{ length: 4 },
(_v, i) => `test-docker-image:v${i}`
);
const IMAGES = imagesList.join("\n");
const PRE_EXISTING_IMAGES = imagesList.slice(1, 3).reverse().join("\n");
const NEW_IMAGES = [imagesList[0], imagesList[3]].join(" ");

let cache: Mocked<typeof import("@actions/cache")>;
let core: Mocked<typeof import("@actions/core")>;
Expand All @@ -46,27 +53,57 @@ describe("Docker images", (): void => {

const mockedLoadDockerImages = async (cacheHit: boolean): Promise<void> => {
cache.restoreCache.mockResolvedValueOnce(cacheHit ? KEY : undefined);
util.execBashCommand.mockResolvedValueOnce(cacheHit ? "" : IMAGES);
await docker.loadDockerImages();

expect(core.getInput).lastCalledWith("key", { required: true });
expect(cache.restoreCache).lastCalledWith([docker.DOCKER_IMAGES_PATH], KEY);
expect(core.saveState).nthCalledWith(1, docker.CACHE_HIT, cacheHit);
expect(core.setOutput).lastCalledWith(docker.CACHE_HIT, cacheHit);
if (cacheHit) {
expect(util.execBashCommand).lastCalledWith(
`docker load --input ${docker.DOCKER_IMAGES_PATH}`
);
} else {
expect(util.execBashCommand).lastCalledWith(
'docker image list --format "{{ .Repository }}:{{ .Tag }}"'
);
}
};

const mockedSaveDockerImages = async (
const assertSaveDockerImages = (
cacheHit: boolean,
readOnly: boolean = false
): void => {
expect(core.getInput).nthCalledWith(1, "key", { required: true });
expect(core.getState).nthCalledWith(1, docker.CACHE_HIT);
if (!cacheHit) {
expect(core.getInput).lastCalledWith("read-only");
if (!readOnly) {
expect(core.getState).lastCalledWith(docker.DOCKER_IMAGES_LIST);
expect(util.execBashCommand).nthCalledWith(
1,
'docker image list --format "{{ .Repository }}:{{ .Tag }}"'
);
}
}
};

const mockedSaveDockerImages = async (
cacheHit: boolean,
readOnly: boolean = false,
stdout: string = IMAGES
): Promise<void> => {
core.getState.mockReturnValueOnce(cacheHit.toString());
if (!cacheHit) {
core.getInput.mockReturnValueOnce(readOnly.toString());
if (!readOnly) {
core.getState.mockReturnValueOnce(PRE_EXISTING_IMAGES);
util.execBashCommand.mockResolvedValueOnce(stdout);
}
}
await docker.saveDockerImages();

expect(core.getInput).nthCalledWith(1, "key", { required: true });
if (!cacheHit) {
expect(core.getInput).lastCalledWith("read-only");
}
expect(core.getState).lastCalledWith(docker.CACHE_HIT);
assertSaveDockerImages(cacheHit, readOnly);
};

const assertCacheHitSave = (): void => {
Expand All @@ -81,19 +118,17 @@ describe("Docker images", (): void => {
expect(docker.CACHE_HIT).toBe("cache-hit");
});

test("exports DOCKER_IMAGES_LIST", (): void => {
expect(docker.DOCKER_IMAGES_LIST).toBe("docker-images-list");
});

test("exports DOCKER_IMAGES_PATH", (): void => {
expect(docker.DOCKER_IMAGES_PATH).toBe("~/.docker-images.tar");
});

test("are loaded on cache hit", async (): Promise<void> => {
await mockedLoadDockerImages(true);

expect(core.saveState).lastCalledWith(docker.CACHE_HIT, true);
expect(core.setOutput).lastCalledWith(docker.CACHE_HIT, true);
expect(util.execBashCommand).lastCalledWith(
`docker load --input ${docker.DOCKER_IMAGES_PATH}`
);

/* The cache must be restored before the Docker images can be loaded. This
* at least checks that the calls are made in the right order, but doesn't
* ensure that the cache finished restoring before the Docker images started
Expand All @@ -109,18 +144,14 @@ describe("Docker images", (): void => {
test("aren't loaded on cache miss", async (): Promise<void> => {
await mockedLoadDockerImages(false);

expect(core.saveState).lastCalledWith(docker.CACHE_HIT, false);
expect(core.setOutput).lastCalledWith(docker.CACHE_HIT, false);
expect(util.execBashCommand).not.toHaveBeenCalled();
expect(core.saveState).lastCalledWith(docker.DOCKER_IMAGES_LIST, IMAGES);
});

test("are saved on cache miss", async (): Promise<void> => {
await mockedSaveDockerImages(false);

expect(util.execBashCommand).lastCalledWith(
'docker image list --format "{{ .Repository }}:{{ .Tag }}" | ' +
'2>&1 xargs --delimiter="\n" --no-run-if-empty --verbose --exit ' +
`docker save --output ${docker.DOCKER_IMAGES_PATH}`
`docker save --output ${docker.DOCKER_IMAGES_PATH} ${NEW_IMAGES}`
);
expect(cache.saveCache).lastCalledWith([docker.DOCKER_IMAGES_PATH], KEY);

Expand All @@ -131,6 +162,9 @@ describe("Docker images", (): void => {
assertCalledInOrder(
core.getInput,
core.getState,
core.getInput,
core.getState,
util.execBashCommand,
util.execBashCommand,
cache.saveCache
);
Expand Down Expand Up @@ -158,4 +192,11 @@ describe("Docker images", (): void => {

assertCacheHitSave();
});

test("aren't saved on cache miss when new Docker image list is empty", async (): Promise<void> => {
await mockedSaveDockerImages(false, false, PRE_EXISTING_IMAGES);
expect(util.execBashCommand).toHaveBeenCalledTimes(1);
expect(core.info).toHaveBeenCalledWith("No Docker images to save");
expect(cache.saveCache).not.toHaveBeenCalled();
});
});
35 changes: 29 additions & 6 deletions src/docker.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { getInput, getState, info, saveState, setOutput } from "@actions/core";
import { execBashCommand } from "./util.js";

const CACHE_HIT = "cache-hit";
const DOCKER_IMAGES_LIST = "docker-images-list";
const DOCKER_IMAGES_PATH = "~/.docker-images.tar";

const loadDockerImages = async (): Promise<void> => {
Expand All @@ -14,6 +15,11 @@ const loadDockerImages = async (): Promise<void> => {
setOutput(CACHE_HIT, cacheHit);
if (cacheHit) {
await execBashCommand(`docker load --input ${DOCKER_IMAGES_PATH}`);
} else {
const dockerImages = await execBashCommand(
'docker image list --format "{{ .Repository }}:{{ .Tag }}"'
);
saveState(DOCKER_IMAGES_LIST, dockerImages);
}
};

Expand All @@ -26,13 +32,30 @@ const saveDockerImages = async (): Promise<void> => {
`Cache miss occurred on the primary key ${key}. Not saving cache as read-only option was selected.`
);
} else {
await execBashCommand(
'docker image list --format "{{ .Repository }}:{{ .Tag }}" | ' +
'2>&1 xargs --delimiter="\n" --no-run-if-empty --verbose --exit ' +
`docker save --output ${DOCKER_IMAGES_PATH}`
const preexistingImages = getState(DOCKER_IMAGES_LIST).split("\n");
const images = await execBashCommand(
'docker image list --format "{{ .Repository }}:{{ .Tag }}"'
);
await saveCache([DOCKER_IMAGES_PATH], key);
const imagesList = images.split("\n");
const newImages = imagesList.filter(
(image) => !preexistingImages.includes(image)
);
if (newImages.length === 0) {
info("No Docker images to save");
} else {
const cmd = `docker save --output ${DOCKER_IMAGES_PATH} ${newImages.join(
" "
)}`;
await execBashCommand(cmd);
await saveCache([DOCKER_IMAGES_PATH], key);
}
}
};

export { saveDockerImages, loadDockerImages, CACHE_HIT, DOCKER_IMAGES_PATH };
export {
saveDockerImages,
loadDockerImages,
CACHE_HIT,
DOCKER_IMAGES_LIST,
DOCKER_IMAGES_PATH,
};
128 changes: 88 additions & 40 deletions src/integration.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,12 @@ const getKey = (paths: string[], key: string): string => {
describe("Integration Test", (): void => {
const KEY = "a-cache-key";
const EXEC_OPTIONS = { shell: "/usr/bin/bash" };
const STDOUT = "standard output from Bash command";
const STDERR = "error output from Bash command";
const LIST_COMMAND =
'docker image list --format "{{ .Repository }}:{{ .Tag }}"';

const dockerImages: string[] = [];

let child_process: Mocked<typeof import("node:child_process")>;
let cache: Mocked<typeof import("@actions/cache")>;
Expand All @@ -23,6 +29,7 @@ describe("Integration Test", (): void => {

let inMemoryCache: Record<string, string>;
let state: Record<string, string>;
let loadCommand: string;

beforeEach(async (): Promise<void> => {
child_process = <any>await import("node:child_process");
Expand All @@ -32,6 +39,30 @@ describe("Integration Test", (): void => {

inMemoryCache = {};
state = {};
loadCommand = `docker load --input ${docker.DOCKER_IMAGES_PATH}`;

let callCount = 0;

child_process.exec.mockImplementation(
(_command: any, _options: any, callback: any): any => {
let stdout;
if (_command === LIST_COMMAND) {
/* When Docker is running as root, docker image list generates a list that includes a
* standard set of Docker images that are pre-cached by GitHub Actions. The production
* code filters out the Docker images present during the restore step from the list of
* images to save since caching pre-cached images would harm performance and waste
* cache space. This mock implementation of docker image list ensures a non-empty
* difference between the restore and save steps so there is something to save.
*/
callCount++;
dockerImages.push(`test-docker-image:v${callCount}`);
stdout = dockerImages.join("\n");
} else {
stdout = STDOUT;
}
callback(null, { stdout, stderr: STDERR });
}
);

core.getInput.mockReturnValue(KEY);

Expand All @@ -58,65 +89,82 @@ describe("Integration Test", (): void => {
});
});

const mockedExec = async (load: boolean, command: string): Promise<void> => {
const stdout = "standard output from Bash command";
const stderr = "error output from Bash command";
child_process.exec.mockImplementationOnce(
(_command: any, _options: any, callback: any): any => {
callback(null, { stdout, stderr });
}
);

await (load ? docker.loadDockerImages() : docker.saveDockerImages());

expect(core.getInput).nthCalledWith(1, "key", { required: true });
expect(core.info).nthCalledWith<[string]>(1, command);
expect(child_process.exec).lastCalledWith(
const assertExecBashCommand = (n: number, command: string): void => {
const infoN = 2 * n - 1; // core.info is called twice for each time docker image list is called.
expect(core.info).nthCalledWith<[string]>(infoN, command);
expect(child_process.exec).nthCalledWith(
n,
command,
EXEC_OPTIONS,
expect.anything()
);
expect(core.info).lastCalledWith(stdout);
expect(core.error).lastCalledWith(stderr);
expect(core.error).lastCalledWith(STDERR);
expect(core.setFailed).not.toHaveBeenCalled();
if (command === LIST_COMMAND) {
expect(core.info).nthCalledWith(2, dockerImages.join("\n"));
}
};

test("cache misses, then hits", async (): Promise<void> => {
// Attempt first cache restore.
await docker.loadDockerImages();
const assertLoadDockerImages = (cacheHit: boolean): void => {
expect(cache.restoreCache).lastCalledWith([docker.DOCKER_IMAGES_PATH], KEY);
expect(core.saveState).nthCalledWith(1, docker.CACHE_HIT, cacheHit);
expect(core.setOutput).lastCalledWith(docker.CACHE_HIT, cacheHit);
if (cacheHit) {
expect(core.getInput).nthCalledWith(1, "key", { required: true });
assertExecBashCommand(1, loadCommand);
} else {
expect(core.setOutput).lastCalledWith(docker.CACHE_HIT, false);
expect(core.getInput).nthCalledWith(1, "key", { required: true });
assertExecBashCommand(1, LIST_COMMAND);
expect(core.saveState).lastCalledWith(
docker.DOCKER_IMAGES_LIST,
"test-docker-image:v1"
);
}
};

// Expect cache miss since cache has never been saved.
expect(core.getInput).nthCalledWith(1, "key", { required: true });
expect(core.setOutput).lastCalledWith(docker.CACHE_HIT, false);
const assertSaveCacheHit = (): void => {
expect(core.getInput).lastCalledWith("key", { required: true });
expect(core.info).lastCalledWith(
`Cache hit occurred on the primary key ${KEY}, not saving cache.`
);
expect(child_process.exec).not.toHaveBeenCalled();
expect(core.setFailed).not.toHaveBeenCalled();
};

const assertSaveCacheMiss = (): void => {
expect(core.getInput).nthCalledWith(1, "key", { required: true });
assertExecBashCommand(1, LIST_COMMAND);
expect(core.getInput).lastCalledWith("read-only");
expect(core.getState).lastCalledWith(docker.DOCKER_IMAGES_LIST);
const saveCommand = `docker save --output ${docker.DOCKER_IMAGES_PATH} test-docker-image:v2`;
assertExecBashCommand(2, saveCommand);
expect(cache.saveCache).lastCalledWith([docker.DOCKER_IMAGES_PATH], KEY);
};

const assertSaveDockerImages = (cacheHit: boolean): void => {
expect(core.getState).nthCalledWith(1, docker.CACHE_HIT);
cacheHit ? assertSaveCacheHit() : assertSaveCacheMiss();
};

test("cache misses, then hits", async (): Promise<void> => {
// Attempt first cache restore.
await docker.loadDockerImages();
assertLoadDockerImages(false); // Expect cache miss since cache has never been saved.
jest.clearAllMocks();

// Run post step first time, expecting cache saved on cache miss.
const saveCommand =
'docker image list --format "{{ .Repository }}:{{ .Tag }}" | ' +
'2>&1 xargs --delimiter="\n" --no-run-if-empty --verbose --exit ' +
`docker save --output ${docker.DOCKER_IMAGES_PATH}`;
await mockedExec(false, saveCommand);
await docker.saveDockerImages();
assertSaveDockerImages(false);
jest.clearAllMocks();

// Attempt second cache restore.
const loadCommand = `docker load --input ${docker.DOCKER_IMAGES_PATH}`;
await mockedExec(true, loadCommand);

// Expect cache hit since cache has been saved.
expect(core.setOutput).lastCalledWith(docker.CACHE_HIT, true);
await docker.loadDockerImages();
assertLoadDockerImages(true); // Expect cache hit since cache has been saved.
jest.clearAllMocks();

// Run post step second time.
await docker.saveDockerImages();

// Expect cache not to have been saved on cache hit.
expect(core.getInput).lastCalledWith("key", { required: true });
expect(core.info).lastCalledWith(
`Cache hit occurred on the primary key ${KEY}, not saving cache.`
);
expect(child_process.exec).not.toHaveBeenCalled();
expect(core.setFailed).not.toHaveBeenCalled();
assertSaveDockerImages(true);
});
});
7 changes: 5 additions & 2 deletions src/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,18 @@ const execAsPromised = promisify(exec);

import { error, info, setFailed } from "@actions/core";

const execBashCommand = async (command: string): Promise<void> => {
const execBashCommand = async (command: string): Promise<string> => {
info(command);
let output = "";
try {
const result = await execAsPromised(command, { shell: "/usr/bin/bash" });
info(result.stdout);
output = result.stdout;
info(output);
error(result.stderr);
} catch (error: any) {
setFailed(error.toString());
}
return output;
};

export { execBashCommand };

0 comments on commit 872f1ae

Please sign in to comment.