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, the Save Cache job ran for approximately 5
minutes and now runs in less than 10 seconds. The Restore Cache job ran
in approximately 40 seconds and now runs in less than 10 seconds.
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 committed Sep 5, 2022
1 parent 8e0159b commit dc8862c
Show file tree
Hide file tree
Showing 7 changed files with 220 additions and 76 deletions.
2 changes: 1 addition & 1 deletion dist/main/index.js

Large diffs are not rendered by default.

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

Large diffs are not rendered by default.

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

describe("Docker images", (): void => {
const KEY = "a-cache-key";
const IMAGES_LIST = Array.from(
{ length: 4 },
(_elem: undefined, index: number): string => `test-docker-image:v${index}`
);
const IMAGES = IMAGES_LIST.join("\n");
const PREEXISTING_IMAGES = IMAGES_LIST.slice(1, 3).reverse().join("\n");
const NEW_IMAGES = [IMAGES_LIST[0], IMAGES_LIST[3]].join(" ");

let cache: Mocked<typeof import("@actions/cache")>;
let core: Mocked<typeof import("@actions/core")>;
Expand All @@ -48,34 +55,69 @@ describe("Docker images", (): void => {
const assertLoadDockerImages = (cacheHit: boolean): void => {
expect(core.getInput).lastCalledWith("key", { required: true });
expect(cache.restoreCache).lastCalledWith([docker.DOCKER_IMAGES_PATH], KEY);
expect(core.saveState).lastCalledWith(docker.CACHE_HIT, cacheHit);
expect(core.saveState).nthCalledWith<[string, boolean]>(
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 }}"'
);
}
expect(util.execBashCommand).toHaveBeenCalledTimes(1);
};

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

assertLoadDockerImages(cacheHit);
};

const mockedSaveDockerImages = async (
const assertSaveDockerImages = (
cacheHit: boolean,
readOnly: boolean = false
): void => {
expect(core.getInput).nthCalledWith<[string, InputOptions]>(1, "key", {
required: true,
});
expect(core.getState).nthCalledWith<[string]>(1, docker.CACHE_HIT);
if (!cacheHit) {
expect(core.getInput).lastCalledWith("read-only");
if (!readOnly) {
expect(core.getState).lastCalledWith(docker.DOCKER_IMAGES_LIST);
expect(core.info).nthCalledWith<[string]>(1, "Listing Docker images.");
expect(util.execBashCommand).nthCalledWith<[string]>(
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(PREEXISTING_IMAGES);
util.execBashCommand.mockResolvedValueOnce(stdout);
}
}
await docker.saveDockerImages();

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

const assertCacheHitSave = (): void => {
Expand All @@ -90,16 +132,18 @@ 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(util.execBashCommand).lastCalledWith(
`docker load --input ${docker.DOCKER_IMAGES_PATH}`
);
expect(core.saveState).toHaveBeenCalledTimes(1);

/* 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
Expand All @@ -113,19 +157,25 @@ describe("Docker images", (): void => {
);
});

test("aren't loaded on cache miss", async (): Promise<void> => {
test("that are present during restore step are recorded on cache miss", async (): Promise<void> => {
await mockedLoadDockerImages(false);

expect(util.execBashCommand).not.toHaveBeenCalled();
expect(core.info).lastCalledWith(
"Recording preexisting Docker images. These include standard images " +
"pre-cached by GitHub Actions when Docker is run as root."
);
expect(core.saveState).lastCalledWith(docker.DOCKER_IMAGES_LIST, IMAGES);
});

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

expect(core.info).lastCalledWith(
"Images present before restore step will be skipped; only new images " +
"will be saved."
);
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 @@ -137,6 +187,8 @@ describe("Docker images", (): void => {
core.getInput,
core.getState,
core.getInput,
core.getState,
util.execBashCommand,
util.execBashCommand,
cache.saveCache
);
Expand Down Expand Up @@ -164,4 +216,12 @@ 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, PREEXISTING_IMAGES);

expect(util.execBashCommand).toHaveBeenCalledTimes(1);
expect(core.info).lastCalledWith("No Docker images to save");
expect(cache.saveCache).not.toHaveBeenCalled();
});
});
41 changes: 35 additions & 6 deletions src/docker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,10 @@ 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 LIST_COMMAND =
'docker image list --format "{{ .Repository }}:{{ .Tag }}"';

const loadDockerImages = async (): Promise<void> => {
const requestedKey = getInput("key", { required: true });
Expand All @@ -14,6 +17,13 @@ const loadDockerImages = async (): Promise<void> => {
setOutput(CACHE_HIT, cacheHit);
if (cacheHit) {
await execBashCommand(`docker load --input ${DOCKER_IMAGES_PATH}`);
} else {
info(
"Recording preexisting Docker images. These include standard images " +
"pre-cached by GitHub Actions when Docker is run as root."
);
const dockerImages = await execBashCommand(LIST_COMMAND);
saveState(DOCKER_IMAGES_LIST, dockerImages);
}
};

Expand All @@ -27,13 +37,32 @@ const saveDockerImages = async (): Promise<void> => {
"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");
info("Listing Docker images.");
const images = await execBashCommand(LIST_COMMAND);
const imagesList = images.split("\n");
const newImages = imagesList.filter(
(image: string): boolean => !preexistingImages.includes(image)
);
await saveCache([DOCKER_IMAGES_PATH], key);
if (newImages.length === 0) {
info("No Docker images to save");
} else {
info(
"Images present before restore step will be skipped; only new images " +
"will be saved."
);
const newImagesArgs = newImages.join(" ");
const cmd = `docker save --output ${DOCKER_IMAGES_PATH} ${newImagesArgs}`;
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,
};
Loading

0 comments on commit dc8862c

Please sign in to comment.