fix(k8s): make hot reloading respect excludes

Before this fix, hot reloading would sync the entire source directory,
including files that are not version controlled.

This would cause issues when e.g. `node_modules` were present in the
target service.

This change  should improve performance and stability for hot reloading.

BREAKING CHANGE:

Hot reloading now uses the tracked file list for its sync, similarly to
how the build sync does.

This means that untracked files will no longer be synced by hot
reloading.

garden-service/src/build-dir.ts

@@ -18,9 +18,9 @@ import { LogEntry } from "./logger/log-entry"
 import { ModuleConfig } from "./config/module"
 import { ConfigGraph } from "./config-graph"
 import { exec } from "./util/util"
-import { LogLevel } from "./logger/log-node"
 import { deline } from "./util/string"
 import { Profile } from "./util/profiling"
+import { syncWithOptions } from "./util/sync"
 
 const minRsyncVersion = "3.1.0"
 const versionRegex = /rsync  version ([\d\.]+)  /
@@ -99,9 +99,8 @@ export class BuildDir {
       return
     }
 
-    const files = module.version.files
-      // Normalize to relative POSIX-style paths
-      .map((f) => normalize(isAbsolute(f) ? relative(module.path, f) : f))
+    // Normalize to relative POSIX-style paths
+    const files = module.version.files.map((f) => normalize(isAbsolute(f) ? relative(module.path, f) : f))
 
     await this.sync({
       module,
@@ -245,36 +244,7 @@ export class BuildDir {
 
     log.debug(logMsg)
 
-    // rsync benefits from file lists being sorted
-    files && files.sort()
-    let input: string | undefined
-
-    if (withDelete) {
-      syncOpts.push("--prune-empty-dirs")
-
-      if (files === undefined) {
-        syncOpts.push("--delete")
-      } else {
-        // Workaround for this issue: https://stackoverflow.com/questions/1813907
-        syncOpts.push("--include-from=-", "--exclude=*", "--delete-excluded")
-
-        // -> Make sure the file list is anchored (otherwise filenames are matched as patterns)
-        files = files.map((f) => "/" + f)
-
-        input = "/**/\n" + files.join("\n")
-      }
-    } else if (files !== undefined) {
-      syncOpts.push("--files-from=-")
-      input = files.join("\n")
-    }
-
-    // Avoid rendering the full file list except when at the silly log level
-    if (log.root.level === LogLevel.silly) {
-      log.silly(`File list: ${JSON.stringify(files)}`)
-      log.silly(`Rsync args: ${[...syncOpts, sourcePath, destinationPath].join(" ")}`)
-    }
-
-    await exec("rsync", [...syncOpts, sourcePath, destinationPath], { input })
+    await syncWithOptions({ log, syncOpts, sourcePath, destinationPath, withDelete, files })
   }
 }
 

garden-service/src/plugins/kubernetes/hot-reload.ts

@@ -11,7 +11,7 @@ import normalizePath = require("normalize-path")
 import { V1Deployment, V1DaemonSet, V1StatefulSet } from "@kubernetes/client-node"
 import { ContainerModule, ContainerHotReloadSpec } from "../container/config"
 import { RuntimeError, ConfigurationError } from "../../exceptions"
-import { resolve as resolvePath, dirname, posix } from "path"
+import { resolve as resolvePath, dirname, posix, relative, resolve } from "path"
 import { deline, gardenAnnotationKey } from "../../util/string"
 import { set, sortBy, flatten } from "lodash"
 import { Service } from "../../types/service"
@@ -28,8 +28,9 @@ import { normalizeLocalRsyncPath } from "../../util/fs"
 import { createWorkloadManifest } from "./container/deployment"
 import { kubectl } from "./kubectl"
 import { labelSelectorToString } from "./util"
-import { exec } from "../../util/util"
 import { KubeApi } from "./api"
+import { syncWithOptions } from "../../util/sync"
+import { Module } from "../../types/module"
 
 export const RSYNC_PORT_NAME = "garden-rsync"
 
@@ -297,15 +298,39 @@ export async function syncToService({ ctx, service, hotReloadSpec, namespace, wo
 
   const doSync = async () => {
     const portForward = await getPortForward({ ctx, log, namespace, targetResource, port: RSYNC_PORT })
+    const module = service.module
 
     const syncResult = await Bluebird.map(hotReloadSpec.sync, ({ source, target }) => {
-      const src = rsyncSourcePath(service.sourceModule.path, source)
-      const destination = `rsync://localhost:${portForward.localPort}/volume/root/${rsyncTargetPath(target)}`
-      const tmpDir = `/tmp/${rsyncTargetPath(target)}`.slice(0, -1) // Trim the trailing slash
+      const sourcePath = rsyncSourcePath(service.sourceModule.path, source)
+      const destinationPath = `rsync://localhost:${portForward.localPort}/volume/root/${rsyncTargetPath(target)}`
 
-      log.debug(`Hot-reloading from ${src} to ${destination}`)
+      log.debug(`Hot-reloading from ${sourcePath} to ${destinationPath}`)
 
-      return exec("rsync", ["-vrpztgo", "--temp-dir", tmpDir, src, destination])
+      const tmpDir = `/tmp/${rsyncTargetPath(target)}`.slice(0, -1) // Trim the trailing slash
+      const syncOpts = [
+        "--verbose",
+        "--recursive",
+        "--compress",
+        // Preserve modification times
+        "--times",
+        // Preserve owner + group
+        "--owner",
+        "--group",
+        // Copy permissions
+        "--perms",
+        // Set a temp directory outside of the target directory to avoid potential conflicts
+        "--temp-dir",
+        tmpDir,
+      ]
+
+      return syncWithOptions({
+        syncOpts,
+        sourcePath,
+        destinationPath,
+        withDelete: false,
+        log,
+        files: filesForSync(module, source),
+      })
     })
 
     const postSyncCommand = hotReloadSpec.postSyncCommand
@@ -345,3 +370,17 @@ export async function syncToService({ ctx, service, hotReloadSpec, namespace, wo
     })
   }
 }
+
+/**
+ * Returns the relative paths (from `source`) to each of `module.version.files` that is nested within `source`.
+ *
+ * So e.g. `source` = `mydir` would transform a tracked path `/path/to/module/mydir/subdir/myfile` to
+ * `subdir/myfile` in the output, and if `source` = `.` or `*`, it would be transformed to `mydir/subdir/myfile`.
+ */
+export function filesForSync(module: Module, source: string): string[] {
+  const normalizedSource = resolve(module.path, source.replace("**/", "").replace("*", ""))
+  const moduleFiles = module.version.files
+  const files = normalizedSource === "" ? moduleFiles : moduleFiles.filter((path) => path.startsWith(normalizedSource))
+  const normalizedFiles = files.map((f) => relative(normalizedSource, f))
+  return normalizedFiles
+}

garden-service/src/util/sync.ts

@@ -0,0 +1,72 @@
+/*
+ * Copyright (C) 2018-2020 Garden Technologies, Inc. <[email protected]>
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+
+import { exec } from "./util"
+import { LogEntry } from "../logger/log-entry"
+import { LogLevel } from "../logger/log-node"
+
+/**
+ * Syncs `sourcePath` with `destinationPath` using `syncOpts`. Adds options to `syncOpts` as appropriate for the
+ * `withDelete` and `files` parameters.
+ *
+ * @param destinationPath
+ *   May be a local path or a remote destination.
+ * @param withDelete
+ *   If `true`, files/folders in `destinationPath` that are not in `sourcePath` will also be deleted.
+ * @param files
+ *   If provided, only those paths will be synced. Should be relative paths from `sourcePath`.
+ */
+export async function syncWithOptions({
+  log,
+  syncOpts,
+  sourcePath,
+  destinationPath,
+  withDelete,
+  files,
+}: {
+  log: LogEntry
+  syncOpts: string[]
+  sourcePath: string
+  destinationPath: string
+  withDelete: boolean
+  files?: string[]
+}): Promise<void> {
+  const opts = [...syncOpts] // We create a new array in case the caller wants to reuse the syncOpts array passed in.
+
+  // rsync benefits from file lists being sorted.
+  files && files.sort()
+
+  let input: string | undefined
+
+  if (withDelete) {
+    opts.push("--prune-empty-dirs")
+
+    if (files === undefined) {
+      opts.push("--delete")
+    } else {
+      // Workaround for this issue: https://stackoverflow.com/questions/1813907
+      opts.push("--include-from=-", "--exclude=*", "--delete-excluded")
+
+      // -> Make sure the file list is anchored (otherwise filenames are matched as patterns)
+      files = files.map((f) => "/" + f)
+
+      input = "/**/\n" + files.join("\n")
+    }
+  } else if (files !== undefined) {
+    opts.push("--files-from=-")
+    input = files.join("\n")
+  }
+
+  // Avoid rendering the full file list except when at the silly log level
+  if (log.root.level === LogLevel.silly) {
+    log.silly(`File list: ${JSON.stringify(files)}`)
+    log.silly(`Rsync args: ${[...opts, sourcePath, destinationPath].join(" ")}`)
+  }
+
+  await exec("rsync", [...opts, sourcePath, destinationPath], { input })
+}

garden-service/test/data/test-projects/include-exclude/module-a/garden.yml

@@ -1,4 +1,5 @@
 kind: Module
 name: module-a
 type: test
-include: ["yes.txt"]
+include: ["yes.txt", "somedir/*"]
+exclude: ["somedir/nope.txt"]

garden-service/test/data/test-projects/include-exclude/module-a/somedir/nope.txt

@@ -0,0 +1 @@
+Go away!

garden-service/test/data/test-projects/include-exclude/module-a/somedir/yes.txt

@@ -0,0 +1 @@
+Oh hai!

garden-service/test/unit/src/plugins/kubernetes/hot-reload.ts

@@ -9,14 +9,15 @@
 import { platform } from "os"
 import { expect } from "chai"
 import td from "testdouble"
-import { HotReloadableResource, rsyncSourcePath } from "../../../../../src/plugins/kubernetes/hot-reload"
+import { HotReloadableResource, rsyncSourcePath, filesForSync } from "../../../../../src/plugins/kubernetes/hot-reload"
 
 import {
   removeTrailingSlashes,
   makeCopyCommand,
   configureHotReload,
 } from "../../../../../src/plugins/kubernetes/hot-reload"
-import { setPlatform } from "../../../../helpers"
+import { setPlatform, makeTestGarden, TestGarden, getDataDir } from "../../../../helpers"
+import { ConfigGraph } from "../../../../../src/config-graph"
 
 describe("configureHotReload", () => {
   it("should correctly augment a resource manifest with containers and volume for hot reloading", async () => {
@@ -288,3 +289,32 @@ describe("makeCopyCommand", () => {
     })
   })
 })
+
+describe("filesForSync", () => {
+  let garden: TestGarden
+  let graph: ConfigGraph
+  const projectRoot = getDataDir("test-projects", "include-exclude")
+
+  before(async () => {
+    garden = await makeTestGarden(projectRoot)
+    graph = await garden.getConfigGraph(garden.log)
+  })
+
+  it("should respect module include and exclude", async () => {
+    const moduleA = await graph.getModule("module-a")
+    const files = filesForSync(moduleA, "*")
+    expect(files).to.eql(["somedir/yes.txt", "yes.txt"])
+  })
+
+  it("should treat '.' sources the same as '*'", async () => {
+    const moduleA = await graph.getModule("module-a")
+    const files = filesForSync(moduleA, ".")
+    expect(files).to.eql(["somedir/yes.txt", "yes.txt"])
+  })
+
+  it("should filter files on source prefix, and return the relative path from the source path", async () => {
+    const moduleA = await graph.getModule("module-a")
+    const files = filesForSync(moduleA, "somedir")
+    expect(files).to.eql(["yes.txt"])
+  })
+})

garden-service/test/unit/src/vcs/vcs.ts

@@ -125,7 +125,10 @@ describe("VcsHandler", () => {
 
       const version = await handler.getTreeVersion(gardenA.log, moduleConfig)
 
-      expect(version.files).to.eql([resolve(moduleConfig.path, "yes.txt")])
+      expect(version.files).to.eql([
+        resolve(moduleConfig.path, "somedir/yes.txt"),
+        resolve(moduleConfig.path, "yes.txt"),
+      ])
     })
 
     it("should respect the exclude field, if specified", async () => {