From 94619f61854d338b4e4470eded9d9f466541eb85 Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Tue, 12 Mar 2019 15:23:10 +0100 Subject: [PATCH] fix: use abs target paths in HR copy commands This fixes various issues with hot reload sync configuration and simplifies the internal logic. One issue resolved by this change is that Dockerfiles with WORKDIRs nested under one of the sync targets no longer cause problems with hot reloading. --- garden-service/package-lock.json | 52 +++++----------- .../src/plugins/kubernetes/hot-reload.ts | 43 ++++++------- .../test/src/plugins/kubernetes/hot-reload.ts | 61 +++++++------------ 3 files changed, 52 insertions(+), 104 deletions(-) diff --git a/garden-service/package-lock.json b/garden-service/package-lock.json index 3668cf9549..371ca8faa8 100644 --- a/garden-service/package-lock.json +++ b/garden-service/package-lock.json @@ -3801,8 +3801,7 @@ "ansi-regex": { "version": "2.1.1", "resolved": false, - "integrity": "sha1-w7M6te42DYbg5ijwRorn7yfWVN8=", - "optional": true + "integrity": "sha1-w7M6te42DYbg5ijwRorn7yfWVN8=" }, "aproba": { "version": "1.2.0", @@ -3823,14 +3822,12 @@ "balanced-match": { "version": "1.0.0", "resolved": false, - "integrity": "sha1-ibTRmasr7kneFk6gK4nORi1xt2c=", - "optional": true + "integrity": "sha1-ibTRmasr7kneFk6gK4nORi1xt2c=" }, "brace-expansion": { "version": "1.1.11", "resolved": false, "integrity": "sha512-iCuPHDFgrHX7H2vEI/5xpz07zSHB00TpugqhmYtVmMO6518mCuRMoOYFldEBl0g187ufozdaHgWKcYFb61qGiA==", - "optional": true, "requires": { "balanced-match": "^1.0.0", "concat-map": "0.0.1" @@ -3845,20 +3842,17 @@ "code-point-at": { "version": "1.1.0", "resolved": false, - "integrity": "sha1-DQcLTQQ6W+ozovGkDi7bPZpMz3c=", - "optional": true + "integrity": "sha1-DQcLTQQ6W+ozovGkDi7bPZpMz3c=" }, "concat-map": { "version": "0.0.1", "resolved": false, - "integrity": "sha1-2Klr13/Wjfd5OnMDajug1UBdR3s=", - "optional": true + "integrity": "sha1-2Klr13/Wjfd5OnMDajug1UBdR3s=" }, "console-control-strings": { "version": "1.1.0", "resolved": false, - "integrity": "sha1-PXz0Rk22RG6mRL9LOVB/mFEAjo4=", - "optional": true + "integrity": "sha1-PXz0Rk22RG6mRL9LOVB/mFEAjo4=" }, "core-util-is": { "version": "1.0.2", @@ -3975,8 +3969,7 @@ "inherits": { "version": "2.0.3", "resolved": false, - "integrity": "sha1-Yzwsg+PaQqUC9SRmAiSA9CCCYd4=", - "optional": true + "integrity": "sha1-Yzwsg+PaQqUC9SRmAiSA9CCCYd4=" }, "ini": { "version": "1.3.5", @@ -3988,7 +3981,6 @@ "version": "1.0.0", "resolved": false, "integrity": "sha1-754xOG8DGn8NZDr4L95QxFfvAMs=", - "optional": true, "requires": { "number-is-nan": "^1.0.0" } @@ -4003,7 +3995,6 @@ "version": "3.0.4", "resolved": false, "integrity": "sha512-yJHVQEhyqPLUTgt9B83PXu6W3rx4MvvHvSUvToogpwoGDOUQ+yDrR0HRot+yOCdCO7u4hX3pWft6kWBBcqh0UA==", - "optional": true, "requires": { "brace-expansion": "^1.1.7" } @@ -4011,14 +4002,12 @@ "minimist": { "version": "0.0.8", "resolved": false, - "integrity": "sha1-hX/Kv8M5fSYluCKCYuhqp6ARsF0=", - "optional": true + "integrity": "sha1-hX/Kv8M5fSYluCKCYuhqp6ARsF0=" }, "minipass": { "version": "2.2.4", "resolved": false, "integrity": "sha512-hzXIWWet/BzWhYs2b+u7dRHlruXhwdgvlTMDKC6Cb1U7ps6Ac6yQlR39xsbjWJE377YTCtKwIXIpJ5oP+j5y8g==", - "optional": true, "requires": { "safe-buffer": "^5.1.1", "yallist": "^3.0.0" @@ -4037,7 +4026,6 @@ "version": "0.5.1", "resolved": false, "integrity": "sha1-MAV0OOrGz3+MR2fzhkjWaX11yQM=", - "optional": true, "requires": { "minimist": "0.0.8" } @@ -4118,8 +4106,7 @@ "number-is-nan": { "version": "1.0.1", "resolved": false, - "integrity": "sha1-CXtgK1NCKlIsGvuHkDGDNpQaAR0=", - "optional": true + "integrity": "sha1-CXtgK1NCKlIsGvuHkDGDNpQaAR0=" }, "object-assign": { "version": "4.1.1", @@ -4131,7 +4118,6 @@ "version": "1.4.0", "resolved": false, "integrity": "sha1-WDsap3WWHUsROsF9nFC6753Xa9E=", - "optional": true, "requires": { "wrappy": "1" } @@ -4217,8 +4203,7 @@ "safe-buffer": { "version": "5.1.1", "resolved": false, - "integrity": "sha512-kKvNJn6Mm93gAczWVJg7wH+wGYWNrDHdWvpUmHyEsgCtIwwo3bqPtV4tR5tuPaUhTOo/kvhVwd8XwwOllGYkbg==", - "optional": true + "integrity": "sha512-kKvNJn6Mm93gAczWVJg7wH+wGYWNrDHdWvpUmHyEsgCtIwwo3bqPtV4tR5tuPaUhTOo/kvhVwd8XwwOllGYkbg==" }, "safer-buffer": { "version": "2.1.2", @@ -4254,7 +4239,6 @@ "version": "1.0.2", "resolved": false, "integrity": "sha1-EYvfW4zcUaKn5w0hHgfisLmxB9M=", - "optional": true, "requires": { "code-point-at": "^1.0.0", "is-fullwidth-code-point": "^1.0.0", @@ -4274,7 +4258,6 @@ "version": "3.0.1", "resolved": false, "integrity": "sha1-ajhfuIU9lS1f8F0Oiq+UJ43GPc8=", - "optional": true, "requires": { "ansi-regex": "^2.0.0" } @@ -4318,14 +4301,12 @@ "wrappy": { "version": "1.0.2", "resolved": false, - "integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8=", - "optional": true + "integrity": "sha1-tSQ9jz7BqjXxNkYFvA0QNuMKtp8=" }, "yallist": { "version": "3.0.2", "resolved": false, - "integrity": "sha1-hFK0u36Dx8GI2AQcGoN8dz1ti7k=", - "optional": true + "integrity": "sha1-hFK0u36Dx8GI2AQcGoN8dz1ti7k=" } } }, @@ -7506,7 +7487,6 @@ "resolved": "https://registry.npmjs.org/align-text/-/align-text-0.1.4.tgz", "integrity": "sha1-DNkKVhCT810KmSVsIrcGlDP60Rc=", "dev": true, - "optional": true, "requires": { "kind-of": "^3.0.2", "longest": "^1.0.1", @@ -7876,8 +7856,7 @@ "version": "1.1.6", "resolved": "https://registry.npmjs.org/is-buffer/-/is-buffer-1.1.6.tgz", "integrity": "sha512-NcdALwpXkTm5Zvvbk7owOUSvVvBKDgKP5/ewfXEznmQFfs4ZRmanOeKBTjRVjka3QFoN6XJ+9F3USqfHqTaU5w==", - "dev": true, - "optional": true + "dev": true }, "is-builtin-module": { "version": "1.0.0", @@ -7973,7 +7952,6 @@ "resolved": "https://registry.npmjs.org/kind-of/-/kind-of-3.2.2.tgz", "integrity": "sha1-MeohpzS6ubuw8yRm2JOupR5KPGQ=", "dev": true, - "optional": true, "requires": { "is-buffer": "^1.1.5" } @@ -8026,8 +8004,7 @@ "version": "1.0.1", "resolved": "https://registry.npmjs.org/longest/-/longest-1.0.1.tgz", "integrity": "sha1-MKCy2jj3N3DoKUoNIuZiXtd9AJc=", - "dev": true, - "optional": true + "dev": true }, "lru-cache": { "version": "4.1.3", @@ -8330,8 +8307,7 @@ "version": "1.6.1", "resolved": "https://registry.npmjs.org/repeat-string/-/repeat-string-1.6.1.tgz", "integrity": "sha1-jcrkcOHIirwtYA//Sndihtp15jc=", - "dev": true, - "optional": true + "dev": true }, "require-directory": { "version": "2.1.1", diff --git a/garden-service/src/plugins/kubernetes/hot-reload.ts b/garden-service/src/plugins/kubernetes/hot-reload.ts index b15839e77d..00172f5403 100644 --- a/garden-service/src/plugins/kubernetes/hot-reload.ts +++ b/garden-service/src/plugins/kubernetes/hot-reload.ts @@ -17,7 +17,7 @@ import { getAppNamespace } from "./namespace" import { kubectl } from "./kubectl" import getPort = require("get-port") import { RuntimeError, ConfigurationError } from "../../exceptions" -import { resolve as resolvePath, normalize } from "path" +import { resolve as resolvePath, normalize, dirname } from "path" import { Omit, registerCleanupFunction } from "../../util/util" import { deline } from "../../util/string" import { set } from "lodash" @@ -26,7 +26,6 @@ import { PluginContext } from "../../plugin-context" import { LogEntry } from "../../logger/log-entry" import { getResourceContainer } from "./helm/common" import { waitForContainerService } from "./container/status" -import { FileCopySpec } from "../../types/module" export const RSYNC_PORT = 873 export const RSYNC_PORT_NAME = "garden-rsync" @@ -65,14 +64,13 @@ export function configureHotReload({ // We're copying the target folder, not just its contents const syncConfig = hotReloadSpec.sync - const syncTarget = syncConfig.map(pair => removeTrailingSlashes(pair.target)) - - const copyCommands = makeCopyCommands(syncConfig) + const targets = syncConfig.map(pair => removeTrailingSlashes(pair.target)) + const copyCommand = makeCopyCommand(targets) const initContainer = { name: "garden-sync-init", image: mainContainer.image, - command: ["/bin/sh", "-c", copyCommands], + command: ["/bin/sh", "-c", copyCommand], env: mainContainer.env || [], imagePullPolicy: "IfNotPresent", volumeMounts: [{ @@ -81,7 +79,7 @@ export function configureHotReload({ }], } - const syncMounts = syncTarget.map(t => { + const syncMounts = targets.map(t => { return { name: syncVolumeName, mountPath: t, @@ -178,31 +176,24 @@ export async function hotReloadContainer( /** * Creates the initial copy command for the sync init container. * - * This handles copying the contents from source into a volume for - * the rsync container to update. + * This handles copying the target paths from the service's container into a volume that is then shared with the + * rsync sidecar container. * - * This needs to deal with nested pathing as well as root. - * This will first create the path, and then copy the contents from the - * docker image into the shared volume as a base for the rsync command - * to update. + * Changes to a source path in a given sync spec are then applied to the corresponding target path (from the same + * spec) inside the rsync sidecar container, which propagates the changes into the running service's container + * (which mounts mounts the volume at the appropriate subpaths). * - * @param syncConfig + * @param syncTargets */ -export function makeCopyCommands(syncConfig: FileCopySpec[]) { - const commands = syncConfig.map(({ source, target }) => { - const adjustedSource = `${removeTrailingSlashes(source)}/.` - const absTarget = normalize(`/.garden/hot_reload/${target}/`) - return `mkdir -p ${absTarget} && cp -r ${adjustedSource} ${absTarget}` +export function makeCopyCommand(syncTargets: string[]) { + const commands = syncTargets.map((target) => { + const syncCopySource = normalize(`${target}/`) + const syncVolumeTarget = normalize(`/.garden/hot_reload/${target}/`) + return `mkdir -p ${dirname(syncVolumeTarget)} && cp -r ${syncCopySource} ${syncVolumeTarget}` }) return commands.join(" && ") } -/** - * Ensure that there's no trailing slash - * - * converts /src/foo/ into /src/foo - * @param path - */ export function removeTrailingSlashes(path: string) { return path.replace(/\/*$/, "") } @@ -215,7 +206,7 @@ export function rsyncSourcePath(modulePath: string, sourcePath: string) { /** * Removes leading slash, and ensures there's exactly one trailing slash. * - * converts /src/foo into src/foo/ + * Converts /src/foo into src/foo/ * @param target */ export function rsyncTargetPath(path: string) { diff --git a/garden-service/test/src/plugins/kubernetes/hot-reload.ts b/garden-service/test/src/plugins/kubernetes/hot-reload.ts index 145c5e1441..839f832a3f 100644 --- a/garden-service/test/src/plugins/kubernetes/hot-reload.ts +++ b/garden-service/test/src/plugins/kubernetes/hot-reload.ts @@ -3,7 +3,7 @@ import { HotReloadableResource } from "../../../../src/plugins/kubernetes/hot-re import { removeTrailingSlashes, - makeCopyCommands, + makeCopyCommand, configureHotReload, } from "../../../../src/plugins/kubernetes/hot-reload" @@ -105,7 +105,7 @@ describe("configureHotReload", () => { command: [ "/bin/sh", "-c", - "mkdir -p /.garden/hot_reload/app/ && cp -r */. /.garden/hot_reload/app/", + "mkdir -p /.garden/hot_reload && cp -r /app/ /.garden/hot_reload/app/", ], env: [], imagePullPolicy: "IfNotPresent", @@ -140,42 +140,23 @@ describe("removeTrailingSlashes", () => { } }) -describe("makeCopyCommands", () => { - // Test cases for each type - const configs: any[] = [ - // Source is missing slash - [ - [{ source: "src", target: "/app/src" }], - "mkdir -p /.garden/hot_reload/app/src/ && cp -r src/. /.garden/hot_reload/app/src/", - ], - // Source ends on slash - [ - [{ source: "src/", target: "/app/src" }], - "mkdir -p /.garden/hot_reload/app/src/ && cp -r src/. /.garden/hot_reload/app/src/", - ], - // Base root of the module - [ - [{ source: ".", target: "/app" }], - "mkdir -p /.garden/hot_reload/app/ && cp -r ./. /.garden/hot_reload/app/", - ], - // Subdirectory not ending on a slash - [ - [{ source: "src/foo", target: "/app/foo" }], - "mkdir -p /.garden/hot_reload/app/foo/ && cp -r src/foo/. /.garden/hot_reload/app/foo/", - ], - // Multiple pairs - [ - [ - { source: "src1", target: "/app/src1" }, - { source: "src2", target: "/app/src2" }, - ], - "mkdir -p /.garden/hot_reload/app/src1/ && cp -r src1/. /.garden/hot_reload/app/src1/ && " + - "mkdir -p /.garden/hot_reload/app/src2/ && cp -r src2/. /.garden/hot_reload/app/src2/", - ], - ] - for (const config of configs) { - it("correctly generates copy commands for the hot reload init container", () => { - expect(makeCopyCommands(config[0])).to.eql(config[1]) - }) - } +describe("makeCopyCommand", () => { + + it("ensures a trailing slash in the copy source and target", () => { + const cmd = "mkdir -p /.garden/hot_reload && cp -r /app/ /.garden/hot_reload/app/" + expect(makeCopyCommand(["/app/"])).to.eql(cmd) + expect(makeCopyCommand(["/app"])).to.eql(cmd) + }) + + it("correctly handles target paths with more than one path component", () => { + const cmd = "mkdir -p /.garden/hot_reload/app/src && cp -r /app/src/foo/ /.garden/hot_reload/app/src/foo/" + expect(makeCopyCommand(["/app/src/foo"])).to.eql(cmd) + }) + + it("correctly handles multiple target paths", () => { + const cmd = "mkdir -p /.garden/hot_reload/app && cp -r /app/src1/ /.garden/hot_reload/app/src1/ && " + + "mkdir -p /.garden/hot_reload/app && cp -r /app/src2/ /.garden/hot_reload/app/src2/" + expect(makeCopyCommand(["/app/src1", "/app/src2/"])).to.eql(cmd) + }) + })