From 271917b6b889ada9b329bec2c77363ee0d6830a0 Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Wed, 14 Nov 2018 11:55:55 +0100 Subject: [PATCH] fix: fixes to hot reload source/target handling --- docs/reference/config-files-reference.md | 4 + garden-service/src/plugins/container.ts | 22 +++-- .../src/plugins/kubernetes/actions.ts | 2 +- .../src/plugins/kubernetes/deployment.ts | 61 +++++++++++--- .../test/src/plugins/kubernetes/deployment.ts | 82 +++++++++++++++++++ 5 files changed, 149 insertions(+), 22 deletions(-) create mode 100644 garden-service/test/src/plugins/kubernetes/deployment.ts diff --git a/docs/reference/config-files-reference.md b/docs/reference/config-files-reference.md index 3c9b33535a..219ad4bc78 100644 --- a/docs/reference/config-files-reference.md +++ b/docs/reference/config-files-reference.md @@ -663,12 +663,16 @@ module: # top-level directory. Must be a relative path if provided. Defaults to the module's # top-level directory if no value is provided. # + # Example: "src" + # # Optional. source: . # POSIX-style absolute path to sync the directory to inside the container. The root path # (i.e. "/") is not allowed. # + # Example: "/app/src" + # # Required. target: ``` diff --git a/garden-service/src/plugins/container.ts b/garden-service/src/plugins/container.ts index ac9d091dfb..1a82dc7022 100644 --- a/garden-service/src/plugins/container.ts +++ b/garden-service/src/plugins/container.ts @@ -97,13 +97,15 @@ const hotReloadSyncSchema = Joi.object() .default(".") .description(deline` POSIX-style path of the directory to sync to the target, relative to the module's top-level directory. - Must be a relative path if provided. Defaults to the module's top-level directory if no value is provided.`), + Must be a relative path if provided. Defaults to the module's top-level directory if no value is provided.`) + .example("src"), target: Joi.string().uri({ relativeOnly: true }) .regex(absolutePathRegex) .required() .description(deline` POSIX-style absolute path to sync the directory to inside the container. The root path (i.e. "/") is - not allowed.`), + not allowed.`) + .example("/app/src"), }) export interface HotReloadConfigSpec { @@ -521,25 +523,27 @@ export async function validateContainerModule({ moduleConfig }: ValidateModulePa // validate hot reload configuration const hotReloadConfig = moduleConfig.spec.hotReload if (hotReloadConfig) { - // Verify that sync targets are mutually disjoint - i.e. that no target is a subdirectory of - // another target. + const invalidPairDescriptions: string[] = [] const targets = hotReloadConfig.sync.map(syncSpec => syncSpec.target) - const invalidTargetDescriptions: string[] = [] + + // Verify that sync targets are mutually disjoint - i.e. that no target is a subdirectory of + // another target. Mounting directories into mounted directories will cause unexpected results for (const t of targets) { for (const t2 of targets) { if (t2.startsWith(t) && t !== t2) { - invalidTargetDescriptions.push(`${t} is a subdirectory of ${t2}.`) + invalidPairDescriptions.push(`${t} is a subdirectory of ${t2}.`) } } } - if (invalidTargetDescriptions.length > 0) { + if (invalidPairDescriptions.length > 0) { + // TODO: Adapt this message to also handle source errors throw new ConfigurationError( dedent`Invalid hot reload configuration - a target may not be a subdirectory of another target \ in the same module. - ${invalidTargetDescriptions.join("\n")}`, - { invalidTargetDescriptions, hotReloadConfig }, + ${invalidPairDescriptions.join("\n")}`, + { invalidPairDescriptions, hotReloadConfig }, ) } } diff --git a/garden-service/src/plugins/kubernetes/actions.ts b/garden-service/src/plugins/kubernetes/actions.ts index bd38f3df56..d82c5a0f27 100644 --- a/garden-service/src/plugins/kubernetes/actions.ts +++ b/garden-service/src/plugins/kubernetes/actions.ts @@ -159,7 +159,7 @@ export async function hotReload( .nodePort await Bluebird.map(hotReloadConfig.sync, async ({ source, target }) => { - const src = rsyncSourcePath(module, source) + const src = rsyncSourcePath(module.path, source) const destination = `rsync://${hostname}:${rsyncNodePort}/volume/${rsyncTargetPath(target)}` await execa("rsync", ["-vrptgo", src, destination]) }) diff --git a/garden-service/src/plugins/kubernetes/deployment.ts b/garden-service/src/plugins/kubernetes/deployment.ts index b8e589d0f4..9c97027fee 100644 --- a/garden-service/src/plugins/kubernetes/deployment.ts +++ b/garden-service/src/plugins/kubernetes/deployment.ts @@ -7,7 +7,7 @@ */ import deline = require("deline") -import { resolve } from "path" +import { resolve, normalize } from "path" import { extend, get, @@ -16,9 +16,8 @@ import { toPairs, } from "lodash" import { DeployServiceParams, GetServiceStatusParams, PushModuleParams } from "../../types/plugin/params" -import { Module } from "../../types/module" import { RuntimeContext, Service, ServiceStatus } from "../../types/service" -import { helpers, ContainerModule, ContainerService } from "../container" +import { helpers, ContainerModule, ContainerService, SyncSpec } from "../container" import { createIngresses, getIngresses } from "./ingress" import { createServices, RSYNC_PORT, RSYNC_PORT_NAME } from "./service" import { waitForObjects, compareDeployedObjects } from "./status" @@ -399,15 +398,18 @@ function configureVolumes(deployment, container, spec): void { */ function configureHotReload(deployment, container, serviceSpec, moduleSpec, env, imageId) { - const syncVolumeName = `garden-sync-volume-${serviceSpec.name}` - const targets = moduleSpec.hotReload.sync.map(pair => pair.target) + const syncVolumeName = `garden-sync` - const copyCommand = `cp -r ${targets.join(" ")} /.garden/hot_reload` + // We're copying the target folder, not just its contents + const syncConfig = moduleSpec.hotReload.sync + const targets = syncConfig.map(pair => removeTrailingSlashes(pair.target)) + + const copyCommands = makeCopyCommands(syncConfig) const initContainer = { name: "garden-sync-init", image: imageId, - command: ["sh", "-c", copyCommand], + command: ["sh", "-c", copyCommands], env, imagePullPolicy: "IfNotPresent", volumeMounts: [{ @@ -438,7 +440,7 @@ function configureHotReload(deployment, container, serviceSpec, moduleSpec, env, } const rsyncContainer = { - name: "garden-rsync-container", + name: "garden-rsync", image: "eugenmayer/rsync", imagePullPolicy: "IfNotPresent", volumeMounts: [{ @@ -467,16 +469,51 @@ function configureHotReload(deployment, container, serviceSpec, moduleSpec, env, } -export function rsyncSourcePath(module: Module, sourcePath: string) { - return resolve(module.path, sourcePath) +/** + * 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 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. + * + * @param syncConfig + */ +export function makeCopyCommands(syncConfig: SyncSpec[]) { + 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}` + }) + 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(/\/*$/, "") +} + +export function rsyncSourcePath(modulePath: string, sourcePath: string) { + return resolve(modulePath, sourcePath) .replace(/\/*$/, "/") // ensure (exactly one) trailing slash } /** * Removes leading slash, and ensures there's exactly one trailing slash. + * + * converts /src/foo into src/foo/ + * @param target */ -export function rsyncTargetPath(target: string) { - return target.replace(/^\/*/, "") +export function rsyncTargetPath(path: string) { + return path.replace(/^\/*/, "") .replace(/\/*$/, "/") } diff --git a/garden-service/test/src/plugins/kubernetes/deployment.ts b/garden-service/test/src/plugins/kubernetes/deployment.ts new file mode 100644 index 0000000000..8c818a7857 --- /dev/null +++ b/garden-service/test/src/plugins/kubernetes/deployment.ts @@ -0,0 +1,82 @@ +import { expect } from "chai" +import { makeCopyCommands, removeTrailingSlashes, rsyncTargetPath } from "../../../../src/plugins/kubernetes/deployment" + +describe("deployment", () => { + + describe("removeTrailingSlashes", () => { + const paths = [ + ["/foo/bar", "/foo/bar"], + ["/foo/bar/", "/foo/bar"], + ["/foo", "/foo"], + ["/foo/", "/foo"], + ["/foo/bar//", "/foo/bar"], + ] + + for (const path of paths) { + it(`handles paths correctly for ${path[0]}`, () => { + expect(removeTrailingSlashes(path[0])).to.eql(path[1]) + }) + } + }) + + describe("rsyncTargetPath", () => { + const paths = [ + // Adds missing slash + ["/foo/bar", "foo/bar/"], + // Makes sure it doesn't add more to sub paths + ["/foo/bar/", "foo/bar/"], + // Handles basic 1 directory path with absolute path + ["/foo", "foo/"], + // Makes sure only a single slash is added + ["/foo/", "foo/"], + // Removes duplicate slashes (should never happen) + ["/foo/bar//", "foo/bar/"], + ] + + for (const path of paths) { + it(`handles paths correctly for ${path[0]}`, () => { + expect(rsyncTargetPath(path[0])).to.eql(path[1]) + }) + } + }) + + 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]) + }) + } + }) +})