From 1d1ca694c83f8da2ed709f0de3c4d70cad710564 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 14:39:44 +0100 Subject: [PATCH 1/8] Lock starting containers. --- .gitignore | 2 + .../src/integrations/tests/utils/index.ts | 53 ++++++++++++++----- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/.gitignore b/.gitignore index b68ddd975f..32d1416f4a 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,8 @@ bb-airgapped.tar.gz packages/server/build/oldClientVersions/**/* packages/builder/src/components/deploy/clientVersions.json +packages/server/src/integrations/tests/utils/*.lock + # Logs logs *.log diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index a54d0ac1a7..64e466e6a0 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -6,6 +6,9 @@ import * as mssql from "./mssql" import * as mariadb from "./mariadb" import { GenericContainer } from "testcontainers" import { testContainerUtils } from "@budibase/backend-core/tests" +import lockfile from "proper-lockfile" +import path from "path" +import fs from "fs" export type DatasourceProvider = () => Promise @@ -67,20 +70,44 @@ export async function rawQuery(ds: Datasource, sql: string): Promise { export async function startContainer(container: GenericContainer) { container = container.withReuse().withLabels({ "com.budibase": "true" }) - const startedContainer = await container.start() + // If two tests try to spin up the same container at the same time, there's a + // possibility that two containers of the same type will be started. To avoid + // this, we use a filesystem lock to ensure that only one container of a given + // type is started at a time. + const imageName = (container as any).imageName.string as string + const lockPath = path.resolve( + __dirname, + `${imageName.replaceAll("/", "-")}.lock` + ) - const info = testContainerUtils.getContainerById(startedContainer.getId()) - if (!info) { - throw new Error("Container not found") + // The `proper-lockfile` library needs the file we're locking on to exist + // before it can lock it. + if (!fs.existsSync(lockPath)) { + fs.writeFileSync(lockPath, "") } - // Some Docker runtimes, when you expose a port, will bind it to both - // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 - // addresses are not shared, and testcontainers will sometimes give you back - // the ipv6 port. There's no way to know that this has happened, and if you - // try to then connect to `localhost:port` you may attempt to bind to the v4 - // address which could be unbound or even an entirely different container. For - // that reason, we don't use testcontainers' `getExposedPort` function, - // preferring instead our own method that guaranteed v4 ports. - return testContainerUtils.getExposedV4Ports(info) + await lockfile.lock(lockPath, { + retries: 10, + }) + + try { + const startedContainer = await container.start() + + const info = testContainerUtils.getContainerById(startedContainer.getId()) + if (!info) { + throw new Error("Container not found") + } + + // Some Docker runtimes, when you expose a port, will bind it to both + // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 + // addresses are not shared, and testcontainers will sometimes give you back + // the ipv6 port. There's no way to know that this has happened, and if you + // try to then connect to `localhost:port` you may attempt to bind to the v4 + // address which could be unbound or even an entirely different container. For + // that reason, we don't use testcontainers' `getExposedPort` function, + // preferring instead our own method that guaranteed v4 ports. + return testContainerUtils.getExposedV4Ports(info) + } finally { + await lockfile.unlock(lockPath) + } } From af0802df303b22a277bd3ed070399e0684ad1bf6 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 14:43:45 +0100 Subject: [PATCH 2/8] Only hold the lock during container start. --- .../src/integrations/tests/utils/index.ts | 35 ++++++++++--------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index 64e466e6a0..537e523fc6 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -4,7 +4,7 @@ import * as mongodb from "./mongodb" import * as mysql from "./mysql" import * as mssql from "./mssql" import * as mariadb from "./mariadb" -import { GenericContainer } from "testcontainers" +import { GenericContainer, StartedTestContainer } from "testcontainers" import { testContainerUtils } from "@budibase/backend-core/tests" import lockfile from "proper-lockfile" import path from "path" @@ -90,24 +90,25 @@ export async function startContainer(container: GenericContainer) { retries: 10, }) + let startedContainer: StartedTestContainer try { - const startedContainer = await container.start() - - const info = testContainerUtils.getContainerById(startedContainer.getId()) - if (!info) { - throw new Error("Container not found") - } - - // Some Docker runtimes, when you expose a port, will bind it to both - // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 - // addresses are not shared, and testcontainers will sometimes give you back - // the ipv6 port. There's no way to know that this has happened, and if you - // try to then connect to `localhost:port` you may attempt to bind to the v4 - // address which could be unbound or even an entirely different container. For - // that reason, we don't use testcontainers' `getExposedPort` function, - // preferring instead our own method that guaranteed v4 ports. - return testContainerUtils.getExposedV4Ports(info) + startedContainer = await container.start() } finally { await lockfile.unlock(lockPath) } + + const info = testContainerUtils.getContainerById(startedContainer.getId()) + if (!info) { + throw new Error("Container not found") + } + + // Some Docker runtimes, when you expose a port, will bind it to both + // 127.0.0.1 and ::1, so ipv4 and ipv6. The port spaces of ipv4 and ipv6 + // addresses are not shared, and testcontainers will sometimes give you back + // the ipv6 port. There's no way to know that this has happened, and if you + // try to then connect to `localhost:port` you may attempt to bind to the v4 + // address which could be unbound or even an entirely different container. For + // that reason, we don't use testcontainers' `getExposedPort` function, + // preferring instead our own method that guaranteed v4 ports. + return testContainerUtils.getExposedV4Ports(info) } From 2e67ae115e024f74d9872921d88787370e43274d Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 15:33:18 +0100 Subject: [PATCH 3/8] Attempt to use unluck we get back from lock. --- packages/server/src/integrations/tests/utils/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index 537e523fc6..eaebb8f2d5 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -86,7 +86,7 @@ export async function startContainer(container: GenericContainer) { fs.writeFileSync(lockPath, "") } - await lockfile.lock(lockPath, { + const unlock = await lockfile.lock(lockPath, { retries: 10, }) @@ -94,7 +94,7 @@ export async function startContainer(container: GenericContainer) { try { startedContainer = await container.start() } finally { - await lockfile.unlock(lockPath) + await unlock() } const info = testContainerUtils.getContainerById(startedContainer.getId()) From eac6106b06aced3114ebc42287dbe459ce452357 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 15:38:25 +0100 Subject: [PATCH 4/8] Try the sync versions? --- packages/server/src/integrations/tests/utils/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index eaebb8f2d5..7487ee079c 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -86,7 +86,7 @@ export async function startContainer(container: GenericContainer) { fs.writeFileSync(lockPath, "") } - const unlock = await lockfile.lock(lockPath, { + const unlock = lockfile.lockSync(lockPath, { retries: 10, }) @@ -94,7 +94,7 @@ export async function startContainer(container: GenericContainer) { try { startedContainer = await container.start() } finally { - await unlock() + unlock() } const info = testContainerUtils.getContainerById(startedContainer.getId()) From af60ff4da77d61ad732dbbc4983b0e8cad0dacf0 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 15:43:12 +0100 Subject: [PATCH 5/8] Can't use retries with the sync API. --- packages/server/src/integrations/tests/utils/index.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index 7487ee079c..a462cdce05 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -86,9 +86,7 @@ export async function startContainer(container: GenericContainer) { fs.writeFileSync(lockPath, "") } - const unlock = lockfile.lockSync(lockPath, { - retries: 10, - }) + const unlock = lockfile.lockSync(lockPath) let startedContainer: StartedTestContainer try { From 6a54b58303002bc42cc62df7511631172554b37b Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 16:00:04 +0100 Subject: [PATCH 6/8] ? --- packages/server/specs/openapi.json | 12 +++++++++--- packages/server/specs/openapi.yaml | 6 ++++++ .../server/src/integrations/tests/utils/index.ts | 13 +++++-------- 3 files changed, 20 insertions(+), 11 deletions(-) diff --git a/packages/server/specs/openapi.json b/packages/server/specs/openapi.json index 7d07b424f0..b21554505b 100644 --- a/packages/server/specs/openapi.json +++ b/packages/server/specs/openapi.json @@ -860,8 +860,10 @@ "json", "internal", "barcodeqr", + "signature_single", "bigint", - "bb_reference" + "bb_reference", + "bb_reference_single" ], "description": "Defines the type of the column, most explain themselves, a link column is a relationship." }, @@ -1067,8 +1069,10 @@ "json", "internal", "barcodeqr", + "signature_single", "bigint", - "bb_reference" + "bb_reference", + "bb_reference_single" ], "description": "Defines the type of the column, most explain themselves, a link column is a relationship." }, @@ -1285,8 +1289,10 @@ "json", "internal", "barcodeqr", + "signature_single", "bigint", - "bb_reference" + "bb_reference", + "bb_reference_single" ], "description": "Defines the type of the column, most explain themselves, a link column is a relationship." }, diff --git a/packages/server/specs/openapi.yaml b/packages/server/specs/openapi.yaml index 3a798c424b..6a2ae89c61 100644 --- a/packages/server/specs/openapi.yaml +++ b/packages/server/specs/openapi.yaml @@ -782,8 +782,10 @@ components: - json - internal - barcodeqr + - signature_single - bigint - bb_reference + - bb_reference_single description: Defines the type of the column, most explain themselves, a link column is a relationship. constraints: @@ -948,8 +950,10 @@ components: - json - internal - barcodeqr + - signature_single - bigint - bb_reference + - bb_reference_single description: Defines the type of the column, most explain themselves, a link column is a relationship. constraints: @@ -1121,8 +1125,10 @@ components: - json - internal - barcodeqr + - signature_single - bigint - bb_reference + - bb_reference_single description: Defines the type of the column, most explain themselves, a link column is a relationship. constraints: diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index a462cdce05..d9a2960848 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -80,19 +80,16 @@ export async function startContainer(container: GenericContainer) { `${imageName.replaceAll("/", "-")}.lock` ) - // The `proper-lockfile` library needs the file we're locking on to exist - // before it can lock it. - if (!fs.existsSync(lockPath)) { - fs.writeFileSync(lockPath, "") - } - - const unlock = lockfile.lockSync(lockPath) + const unlock = await lockfile.lock(lockPath, { + retries: 10, + realpath: false, + }) let startedContainer: StartedTestContainer try { startedContainer = await container.start() } finally { - unlock() + await unlock() } const info = testContainerUtils.getContainerById(startedContainer.getId()) From 85c59c0350f87526566e4a071411e8c3d955b29c Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 17:41:48 +0100 Subject: [PATCH 7/8] Changing tactic to relying on stable container names to prevent duplication. --- .../src/integrations/tests/utils/index.ts | 49 ++++++++++++------- .../src/integrations/tests/utils/mssql.ts | 3 ++ .../src/integrations/tests/utils/mysql.ts | 3 ++ .../src/integrations/tests/utils/postgres.ts | 3 ++ 4 files changed, 39 insertions(+), 19 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index d9a2960848..afbf4310a8 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -9,6 +9,7 @@ import { testContainerUtils } from "@budibase/backend-core/tests" import lockfile from "proper-lockfile" import path from "path" import fs from "fs" +import _ from "lodash" export type DatasourceProvider = () => Promise @@ -68,28 +69,38 @@ export async function rawQuery(ds: Datasource, sql: string): Promise { } export async function startContainer(container: GenericContainer) { - container = container.withReuse().withLabels({ "com.budibase": "true" }) - - // If two tests try to spin up the same container at the same time, there's a - // possibility that two containers of the same type will be started. To avoid - // this, we use a filesystem lock to ensure that only one container of a given - // type is started at a time. const imageName = (container as any).imageName.string as string - const lockPath = path.resolve( - __dirname, - `${imageName.replaceAll("/", "-")}.lock` - ) + const key = imageName.replaceAll("/", "-").replaceAll(":", "-") - const unlock = await lockfile.lock(lockPath, { - retries: 10, - realpath: false, - }) + container = container + .withReuse() + .withLabels({ "com.budibase": "true" }) + .withName(key) - let startedContainer: StartedTestContainer - try { - startedContainer = await container.start() - } finally { - await unlock() + let startedContainer: StartedTestContainer | undefined = undefined + let lastError = undefined + for (let i = 0; i < 10; i++) { + try { + // container.start() is not an idempotent operation, calling `start` + // modifies the internal state of a GenericContainer instance such that + // the hash it uses to determine reuse changes. We need to clone the + // container before calling start to ensure that we're using the same + // reuse hash every time. + const containerCopy = _.cloneDeep(container) + startedContainer = await containerCopy.start() + lastError = undefined + break + } catch (e: any) { + lastError = e + await new Promise(resolve => setTimeout(resolve, 1000)) + } + } + + if (!startedContainer) { + if (lastError) { + throw lastError + } + throw new Error(`failed to start container: ${imageName}`) } const info = testContainerUtils.getContainerById(startedContainer.getId()) diff --git a/packages/server/src/integrations/tests/utils/mssql.ts b/packages/server/src/integrations/tests/utils/mssql.ts index 647f461272..57c5fe8049 100644 --- a/packages/server/src/integrations/tests/utils/mssql.ts +++ b/packages/server/src/integrations/tests/utils/mssql.ts @@ -29,6 +29,9 @@ export async function getDatasource(): Promise { } const port = (await ports).find(x => x.container === 1433)?.host + if (!port) { + throw new Error("SQL Server port not found") + } const datasource: Datasource = { type: "datasource_plus", diff --git a/packages/server/src/integrations/tests/utils/mysql.ts b/packages/server/src/integrations/tests/utils/mysql.ts index a78833e1de..560d6bb2d4 100644 --- a/packages/server/src/integrations/tests/utils/mysql.ts +++ b/packages/server/src/integrations/tests/utils/mysql.ts @@ -38,6 +38,9 @@ export async function getDatasource(): Promise { } const port = (await ports).find(x => x.container === 3306)?.host + if (!port) { + throw new Error("MySQL port not found") + } const datasource: Datasource = { type: "datasource_plus", diff --git a/packages/server/src/integrations/tests/utils/postgres.ts b/packages/server/src/integrations/tests/utils/postgres.ts index 4191b107e9..8c0cd886e8 100644 --- a/packages/server/src/integrations/tests/utils/postgres.ts +++ b/packages/server/src/integrations/tests/utils/postgres.ts @@ -21,6 +21,9 @@ export async function getDatasource(): Promise { } const port = (await ports).find(x => x.container === 5432)?.host + if (!port) { + throw new Error("Postgres port not found") + } const datasource: Datasource = { type: "datasource_plus", From 96efb17678e9503f14dfdb98e7e17f060c838b29 Mon Sep 17 00:00:00 2001 From: Sam Rose Date: Tue, 11 Jun 2024 17:52:02 +0100 Subject: [PATCH 8/8] Fix lint. --- packages/server/src/integrations/tests/utils/index.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/packages/server/src/integrations/tests/utils/index.ts b/packages/server/src/integrations/tests/utils/index.ts index afbf4310a8..64617461bb 100644 --- a/packages/server/src/integrations/tests/utils/index.ts +++ b/packages/server/src/integrations/tests/utils/index.ts @@ -6,10 +6,7 @@ import * as mssql from "./mssql" import * as mariadb from "./mariadb" import { GenericContainer, StartedTestContainer } from "testcontainers" import { testContainerUtils } from "@budibase/backend-core/tests" -import lockfile from "proper-lockfile" -import path from "path" -import fs from "fs" -import _ from "lodash" +import cloneDeep from "lodash/cloneDeep" export type DatasourceProvider = () => Promise @@ -86,7 +83,7 @@ export async function startContainer(container: GenericContainer) { // the hash it uses to determine reuse changes. We need to clone the // container before calling start to ensure that we're using the same // reuse hash every time. - const containerCopy = _.cloneDeep(container) + const containerCopy = cloneDeep(container) startedContainer = await containerCopy.start() lastError = undefined break