From 1533445cbdd1c3da0b6fa7bbb048c202329a7c08 Mon Sep 17 00:00:00 2001 From: Henri Normak Date: Tue, 28 Feb 2023 13:19:15 +0700 Subject: [PATCH] Performance improvements (#453) --- .../functions/container/exec-container.ts | 44 ++++--------------- src/docker/functions/image/image-exists.ts | 20 ++++++++- src/docker/functions/image/pull-image.ts | 23 ++++++---- src/generic-container/generic-container.ts | 2 - src/log-system-diagnostics.ts | 5 +++ src/logger.ts | 9 ++++ src/port-check.test.ts | 3 ++ src/port-check.ts | 2 +- 8 files changed, 59 insertions(+), 49 deletions(-) diff --git a/src/docker/functions/container/exec-container.ts b/src/docker/functions/container/exec-container.ts index ba68d329f..f7a8e7100 100644 --- a/src/docker/functions/container/exec-container.ts +++ b/src/docker/functions/container/exec-container.ts @@ -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 }; @@ -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 => { - try { - const inspectResult = await exec.inspect(); +const waitForExec = async (exec: Dockerode.Exec, stream: Readable): Promise => { + 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 => - new Promise((resolve) => { - const interval = setInterval(async () => { - const { running, exitCode } = await inspectExec(exec); - - if (!running) { - clearInterval(interval); - resolve(exitCode); - } - }, 100); - }); diff --git a/src/docker/functions/image/image-exists.ts b/src/docker/functions/image/image-exists.ts index 78c24061c..edf06d791 100644 --- a/src/docker/functions/image/image-exists.ts +++ b/src/docker/functions/image/image-exists.ts @@ -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(); +const imageCheckLock = new AsyncLock(); export const imageExists = async (dockerode: Dockerode, imageName: DockerImageName): Promise => { log.debug(`Checking if image exists: ${imageName}`); - return (await listImages(dockerode)).some((image) => image.equals(imageName)); + + return imageCheckLock.acquire(imageName.toString(), async () => { + if (existingImages.has(imageName.toString())) { + return true; + } + + const images = await listImages(dockerode); + images.forEach((name) => { + existingImages.add(name.toString()); + }); + + return existingImages.has(imageName.toString()); + }); }; diff --git a/src/docker/functions/image/pull-image.ts b/src/docker/functions/image/pull-image.ts index 3d1e331f5..534ec3189 100644 --- a/src/docker/functions/image/pull-image.ts +++ b/src/docker/functions/image/pull-image.ts @@ -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 => { 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; diff --git a/src/generic-container/generic-container.ts b/src/generic-container/generic-container.ts index 6fe73d41f..9f151f0dc 100644 --- a/src/generic-container/generic-container.ts +++ b/src/generic-container/generic-container.ts @@ -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, @@ -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.reuse && !this.imageName.isReaper()) { diff --git a/src/log-system-diagnostics.ts b/src/log-system-diagnostics.ts index d08bf16ca..e85762e79 100644 --- a/src/log-system-diagnostics.ts +++ b/src/log-system-diagnostics.ts @@ -4,6 +4,11 @@ import { getDockerInfo } from "./docker/functions/get-info"; import Dockerode from "dockerode"; export const logSystemDiagnostics = async (dockerode: Dockerode): Promise => { + 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 = { diff --git a/src/logger.ts b/src/logger.ts index d3a1f0d3b..37f04cbc8 100644 --- a/src/logger.ts +++ b/src/logger.ts @@ -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; @@ -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}`); } @@ -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); } diff --git a/src/port-check.test.ts b/src/port-check.test.ts index 7b0e3499a..9ba6be0cc 100644 --- a/src/port-check.test.ts +++ b/src/port-check.test.ts @@ -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 () => { diff --git a/src/port-check.ts b/src/port-check.ts index 3a13ce34b..e9137953d 100644 --- a/src/port-check.ts +++ b/src/port-check.ts @@ -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) {