Merge pull request #371 from garden-io/hot-reload-path-fixes

fix: fixes to hot reload source/target handling

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:
 ```

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(<any>{ 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 },
       )
     }
   }

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])
     })

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(/\/*$/, "/")
 }
 

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])
+      })
+    }
+  })
+})