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

Performance improvements #453

Merged
44 changes: 9 additions & 35 deletions src/docker/functions/container/exec-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,11 @@ export const execContainer = async (

stream.on("data", (chunk) => chunks.push(chunk));

if (shouldLog) {
if (shouldLog && execLog.enabled()) {
byline(stream).on("data", (line) => execLog.trace(`${container.id}: ${line}`));
}

const exitCode = await waitForExec(exec);

const exitCode = await waitForExec(exec, stream);
stream.destroy();

return { output: chunks.join(""), exitCode };
Expand All @@ -59,37 +58,12 @@ const startExec = async (exec: Dockerode.Exec, options: ExecContainerOptions): P
}
};

type ExecInspectResult = {
exitCode: number;
running: boolean;
entrypoint: string;
arguments: string[];
};

const inspectExec = async (exec: Dockerode.Exec): Promise<ExecInspectResult> => {
try {
const inspectResult = await exec.inspect();
const waitForExec = async (exec: Dockerode.Exec, stream: Readable): Promise<number> => {
await new Promise((res, rej) => {
stream.on("end", res);
stream.on("error", rej);
});

return {
exitCode: inspectResult.ExitCode === null ? -1 : inspectResult.ExitCode,
running: inspectResult.Running,
entrypoint: inspectResult.ProcessConfig.entrypoint,
arguments: inspectResult.ProcessConfig.arguments,
};
} catch (err) {
log.error(`Failed to inspect exec: ${err}`);
throw err;
}
const inspectResult = await exec.inspect();
return inspectResult.ExitCode === null ? -1 : inspectResult.ExitCode;
};

const waitForExec = async (exec: Dockerode.Exec): Promise<number> =>
new Promise((resolve) => {
const interval = setInterval(async () => {
const { running, exitCode } = await inspectExec(exec);

if (!running) {
clearInterval(interval);
resolve(exitCode);
}
}, 100);
});
20 changes: 18 additions & 2 deletions src/docker/functions/image/image-exists.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,25 @@
import { DockerImageName } from "../../../docker-image-name";
import { listImages } from "./list-images";
import { log } from "../../../logger";
import Dockerode from "dockerode";
import AsyncLock from "async-lock";
import { listImages } from "./list-images";

const existingImages = new Set<string>();
const imageCheckLock = new AsyncLock();

export const imageExists = async (dockerode: Dockerode, imageName: DockerImageName): Promise<boolean> => {
log.debug(`Checking if image exists: ${imageName}`);
return (await listImages(dockerode)).some((image) => image.equals(imageName));

return imageCheckLock.acquire(imageName.toString(), async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it necessary to provide a timeout here, in case something goes wrong and the lock can't be acquired? Likewise does the lock need to be released in some finally block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lock implementation handles the release in case the user code throws, it also allows configuring a timeout, but for now I left it out, as presumably the underlying operations already have timeouts?

if (existingImages.has(imageName.toString())) {
return true;
}

const images = await listImages(dockerode);
images.forEach((name) => {
existingImages.add(name.toString());
});

return existingImages.has(imageName.toString());
});
};
23 changes: 14 additions & 9 deletions src/docker/functions/image/pull-image.ts
Original file line number Diff line number Diff line change
@@ -1,27 +1,32 @@
import { DockerImageName } from "../../../docker-image-name";
import { log } from "../../../logger";
import { PullStreamParser } from "../../pull-stream-parser";
import { AuthConfig } from "../../types";
import { imageExists } from "./image-exists";
import Dockerode from "dockerode";
import { getAuthConfig } from "../../../registry-auth-locator";
import AsyncLock from "async-lock";

export type PullImageOptions = {
imageName: DockerImageName;
force: boolean;
authConfig?: AuthConfig;
};

const imagePullLock = new AsyncLock();

export const pullImage = async (dockerode: Dockerode, options: PullImageOptions): Promise<void> => {
try {
if ((await imageExists(dockerode, options.imageName)) && !options.force) {
log.debug(`Not pulling image as it already exists: ${options.imageName}`);
return;
}
return imagePullLock.acquire(options.imageName.toString(), async () => {
if (!options.force && (await imageExists(dockerode, options.imageName))) {
log.debug(`Not pulling image as it already exists: ${options.imageName}`);
return;
}

log.info(`Pulling image: ${options.imageName}`);
const stream = await dockerode.pull(options.imageName.toString(), { authconfig: options.authConfig });
log.info(`Pulling image: ${options.imageName}`);
const authconfig = await getAuthConfig(options.imageName.registry);
const stream = await dockerode.pull(options.imageName.toString(), { authconfig });

await new PullStreamParser(options.imageName, log).consume(stream);
await new PullStreamParser(options.imageName, log).consume(stream);
});
} catch (err) {
log.error(`Failed to pull image "${options.imageName}": ${err}`);
throw err;
Expand Down
2 changes: 0 additions & 2 deletions src/generic-container/generic-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ import { DockerImageName } from "../docker-image-name";
import { StartedTestContainer, TestContainer } from "../test-container";
import { defaultWaitStrategy, WaitStrategy } from "../wait-strategy";
import { PortForwarderInstance } from "../port-forwarder";
import { getAuthConfig } from "../registry-auth-locator";
import {
BindMount,
ContentToCopy,
Expand Down Expand Up @@ -83,7 +82,6 @@ export class GenericContainer implements TestContainer {
await pullImage((await dockerClient()).dockerode, {
imageName: this.imageName,
force: this.pullPolicy.shouldPull(),
authConfig: await getAuthConfig(this.imageName.registry),
});

if (!this.imageName.isReaper()) {
Expand Down
5 changes: 5 additions & 0 deletions src/log-system-diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@ import { getDockerInfo } from "./docker/functions/get-info";
import Dockerode from "dockerode";

export const logSystemDiagnostics = async (dockerode: Dockerode): Promise<void> => {
if (!log.enabled()) {
// Logs are not enabled, no point in doing the work of fetching the information
return;
}

log.debug("Fetching system diagnostics");

const info = {
Expand Down
9 changes: 9 additions & 0 deletions src/logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import debug, { IDebugger } from "debug";
type Message = string;

export interface Logger {
enabled(): boolean;
trace(message: Message): void;
debug(message: Message): void;
info(message: Message): void;
Expand All @@ -17,6 +18,10 @@ class DebugLogger implements Logger {
this.logger = debug(namespace);
}

public enabled(): boolean {
return this.logger.enabled;
}

public trace(message: Message): void {
this.logger(`TRACE ${message}`);
}
Expand Down Expand Up @@ -45,6 +50,10 @@ export class FakeLogger implements Logger {
public readonly warnLogs: Message[] = [];
public readonly errorLogs: Message[] = [];

public enabled(): boolean {
return true;
}

public trace(message: Message): void {
this.traceLogs.push(message);
}
Expand Down
3 changes: 3 additions & 0 deletions src/port-check.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,9 @@ describe("PortCheck", () => {
jest.resetAllMocks();
mockContainer = { id: "containerId" } as Dockerode.Container;
portCheck = new InternalPortCheck(mockContainer);

// Make sure logging is enabled to capture all logs
mockLogger.enabled.mockImplementation(() => true);
});

it("should return true when at least one command returns exit code 0", async () => {
Expand Down
2 changes: 1 addition & 1 deletion src/port-check.ts
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export class InternalPortCheck implements PortCheck {
);
const isBound = commandResults.some((result) => result.exitCode === 0);

if (!isBound) {
if (!isBound && log.enabled()) {
const shellExists = commandResults.some((result) => result.exitCode !== 126);
if (!shellExists) {
if (!this.isDistroless) {
Expand Down