fix(sync): fix intermittent concurrency issues while syncing directories

If two processes would copy at the same time to a target, one could
delete the files from the other. We now set rsync's temp directory
outside of the target directory, which resolves the problem.

garden-service/src/build-dir.ts

@@ -148,7 +148,10 @@ export class BuildDir {
     files?: string[]
   }): Promise<void> {
     const destinationDir = parse(destinationPath).dir
+    const tmpDir = destinationPath + ".tmp"
+
     await ensureDir(destinationDir)
+    await ensureDir(tmpDir)
 
     // this is so that the cygwin-based rsync client can deal with the paths
     sourcePath = normalizeLocalRsyncPath(sourcePath)
@@ -159,14 +162,7 @@ export class BuildDir {
     destinationPath = stripWildcard(destinationPath)
 
     // --exclude is required for modules where the module and project are in the same directory
-    const syncOpts = ["-rptgo", `--exclude=${this.buildDirPath}`, "--ignore-missing-args"]
-
-    // We have noticed occasional issues with the default rsync behavior of creating temp files when copying
-    // when using Windows/cwRsync. This workaround appears to do the trick, but is less optimal so we don't apply
-    // it for other platforms.
-    if (process.platform === "win32") {
-      syncOpts.push("--inplace")
-    }
+    const syncOpts = ["-rptgo", `--exclude=${this.buildDirPath}`, "--ignore-missing-args", "--temp-dir", tmpDir]
 
     let logMsg =
       `Syncing ${module.version.files.length} files from ` +
@@ -188,8 +184,7 @@ export class BuildDir {
       if (files === undefined) {
         syncOpts.push("--delete")
       } else {
-        // Workaround for this issue:
-        // https://stackoverflow.com/questions/1813907/rsync-delete-files-from-list-dest-does-not-delete-unwanted-files
+        // Workaround for this issue: https://stackoverflow.com/questions/1813907
         syncOpts.push("--include-from=-", "--exclude=*", "--delete-excluded")
         input = "/**/\n" + files.join("\n")
       }

garden-service/src/plugins/kubernetes/commands/cleanup-cluster-registry.ts

@@ -371,21 +371,24 @@ async function cleanupBuildSyncVolume(provider: KubernetesProvider, log: LogEntr
     podName,
   })
 
-  // Filter to directories last accessed more than workspaceSyncDirTtl ago
+  // Remove directories last accessed more than workspaceSyncDirTtl ago
   const minTimestamp = new Date().getTime() / 1000 - workspaceSyncDirTtl
 
-  const dirsToDelete = stat.stdout
+  const outdatedDirs = stat.stdout
     .split("\n")
     .filter(Boolean)
     .map((line) => {
       const [dirname, lastAccessed] = line.trim().split(" ")
       return { dirname, lastAccessed: parseInt(lastAccessed, 10) }
     })
-    .filter(({ lastAccessed }) => lastAccessed < minTimestamp)
+    .filter(({ dirname, lastAccessed }) => lastAccessed < minTimestamp && dirname !== "/data/tmp")
+    .map((d) => d.dirname)
+
+  const dirsToDelete = ["/data/tmp/*", ...outdatedDirs]
 
   // Delete the director
   log.info(`Deleting ${dirsToDelete.length} workspace directories.`)
-  const deleteArgs = ["rm", "-rf", ...dirsToDelete.map((d) => d.dirname)]
+  const deleteArgs = ["rm", "-rf", ...dirsToDelete]
   await execInBuildSync({
     provider,
     log,

garden-service/src/plugins/kubernetes/container/build.ts

@@ -13,7 +13,7 @@ import { containerHelpers } from "../../container/helpers"
 import { buildContainerModule, getContainerBuildStatus, getDockerBuildFlags } from "../../container/build"
 import { GetBuildStatusParams, BuildStatus } from "../../../types/plugin/module/getBuildStatus"
 import { BuildModuleParams, BuildResult } from "../../../types/plugin/module/build"
-import { getPods, millicpuToString, megabytesToString } from "../util"
+import { millicpuToString, megabytesToString, getRunningPodInDeployment } from "../util"
 import { systemNamespace } from "../system"
 import { RSYNC_PORT } from "../constants"
 import { posix, resolve } from "path"
@@ -140,13 +140,32 @@ const remoteBuild: BuildHandler = async (params) => {
   // The '/./' trick is used to automatically create the correct target directory with rsync:
   // https://stackoverflow.com/questions/1636889/rsync-how-can-i-configure-it-to-create-target-directory-on-server
   let src = normalizeLocalRsyncPath(`${buildRoot}`) + `/./${module.name}/`
-
   const destination = `rsync://localhost:${syncFwd.localPort}/volume/${ctx.workingCopyId}/`
+  const syncArgs = ["-vrpztgo", "--relative", "--delete", "--temp-dir", "/tmp", src, destination]
 
   log.debug(`Syncing from ${src} to ${destination}`)
 
+  // TODO: remove this after a few releases (from 0.10.15), since this is only necessary for environments initialized
+  // with 0.10.14 or earlier.
+  const buildSyncPod = await getRunningPodInDeployment(buildSyncDeploymentName, provider, log)
+
+  if (!buildSyncPod) {
+    throw new PluginError(`Could not find running build sync Pod`, {
+      deploymentName: buildSyncDeploymentName,
+      systemNamespace,
+    })
+  }
+
+  await kubectl.exec({
+    args: ["exec", "-i", buildSyncPod.metadata.name, "--", "mkdir", "-p", "/data/tmp"],
+    provider,
+    log,
+    namespace: systemNamespace,
+    timeout: 10,
+  })
+
   // We retry a couple of times, because we may get intermittent connection issues or concurrency issues
-  await pRetry(() => exec("rsync", ["-vrpztgo", "--relative", "--delete", src, destination]), {
+  await pRetry(() => exec("rsync", syncArgs), {
     retries: 3,
     minTimeout: 500,
   })
@@ -271,11 +290,7 @@ export async function execInBuilder({ provider, log, args, timeout, podName, std
 }
 
 export async function getBuilderPodName(provider: KubernetesProvider, log: LogEntry) {
-  const api = await KubeApi.factory(log, provider)
-
-  const builderStatusRes = await api.apps.readNamespacedDeployment(dockerDaemonDeploymentName, systemNamespace)
-  const builderPods = await getPods(api, systemNamespace, builderStatusRes.spec.selector.matchLabels)
-  const pod = builderPods[0]
+  const pod = await getRunningPodInDeployment(dockerDaemonDeploymentName, provider, log)
 
   if (!pod) {
     throw new PluginError(`Could not find running image builder`, {
@@ -284,7 +299,7 @@ export async function getBuilderPodName(provider: KubernetesProvider, log: LogEn
     })
   }
 
-  return builderPods[0].metadata.name
+  return pod.metadata.name
 }
 
 interface RunKanikoParams {

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

@@ -11,9 +11,9 @@ 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 } from "path"
+import { resolve as resolvePath, dirname, posix } from "path"
 import { deline, gardenAnnotationKey } from "../../util/string"
-import { set, sortBy } from "lodash"
+import { set, sortBy, flatten } from "lodash"
 import { Service } from "../../types/service"
 import { LogEntry } from "../../logger/log-entry"
 import { getResourceContainer } from "./helm/common"
@@ -91,7 +91,8 @@ export function configureHotReload({
     return {
       name: syncVolumeName,
       mountPath: t,
-      subPath: rsyncTargetPath(t),
+      // Need to prefix the target with "root" because we need a "tmp" folder next to it while syncing
+      subPath: posix.join("root", rsyncTargetPath(t)),
     }
   })
 
@@ -250,10 +251,15 @@ export function makeCopyCommand(syncTargets: string[]) {
     // Win32 style paths on Windows, whereas the command produced runs inside a container that expects
     // POSIX style paths.
     const syncCopySource = normalizePath(`${target}/`, false)
-    const syncVolumeTarget = normalizePath(`/.garden/hot_reload/${target}/`, false)
-    return `mkdir -p ${dirname(syncVolumeTarget)} && cp -r ${syncCopySource} ${syncVolumeTarget}`
+    const syncVolumeTarget = normalizePath(`/.garden/hot_reload/root/${target}/`, false)
+    const syncVolumeTmp = normalizePath(`/.garden/hot_reload/tmp/${target}/`, false)
+    return [
+      `mkdir -p ${dirname(syncVolumeTarget)}`,
+      `mkdir -p ${syncVolumeTmp}`,
+      `cp -r ${syncCopySource} ${syncVolumeTarget}`,
+    ]
   })
-  return commands.join(" && ")
+  return flatten(commands).join(" && ")
 }
 
 export function removeTrailingSlashes(path: string) {
@@ -296,11 +302,12 @@ export async function syncToService({ ctx, service, hotReloadSpec, namespace, wo
 
     const syncResult = await Bluebird.map(hotReloadSpec.sync, ({ source, target }) => {
       const src = rsyncSourcePath(service.sourceModule.path, source)
-      const destination = `rsync://localhost:${portForward.localPort}/volume/${rsyncTargetPath(target)}`
+      const destination = `rsync://localhost:${portForward.localPort}/volume/root/${rsyncTargetPath(target)}`
+      const tmpDir = `/tmp/${rsyncTargetPath(target)}`.slice(0, -1) // Trim the trailing slash
 
       log.debug(`Hot-reloading from ${src} to ${destination}`)
 
-      return exec("rsync", ["-vrpztgo", src, destination])
+      return exec("rsync", ["-vrpztgo", "--temp-dir", tmpDir, src, destination])
     })
 
     const postSyncCommand = hotReloadSpec.postSyncCommand

garden-service/src/plugins/kubernetes/util.ts

@@ -7,7 +7,7 @@
  */
 
 import Bluebird from "bluebird"
-import { get, flatten, uniqBy, sortBy } from "lodash"
+import { get, flatten, uniqBy, sortBy, sample } from "lodash"
 import { V1Pod, V1EnvVar } from "@kubernetes/client-node"
 
 import { KubernetesResource, KubernetesWorkload, KubernetesPod, KubernetesServerResource } from "./types"
@@ -17,6 +17,9 @@ import { gardenAnnotationKey, base64 } from "../../util/string"
 import { MAX_CONFIGMAP_DATA_SIZE } from "./constants"
 import { ContainerEnvVars } from "../container/config"
 import { ConfigurationError } from "../../exceptions"
+import { KubernetesProvider } from "./config"
+import { systemNamespace } from "./system"
+import { LogEntry } from "../../logger/log-entry"
 
 export const workloadTypes = ["Deployment", "DaemonSet", "ReplicaSet", "StatefulSet"]
 
@@ -345,3 +348,12 @@ export function convertDeprecatedManifestVersion(manifest: KubernetesResource):
 
   return manifest
 }
+
+export async function getRunningPodInDeployment(deploymentName: string, provider: KubernetesProvider, log: LogEntry) {
+  const api = await KubeApi.factory(log, provider)
+
+  const status = await api.apps.readNamespacedDeployment(deploymentName, systemNamespace)
+  const pods = await getPods(api, systemNamespace, status.spec.selector.matchLabels)
+
+  return sample(pods)
+}

garden-service/src/util/string.ts

@@ -77,3 +77,10 @@ export function naturalList(list: string[]) {
     return list.slice(0, -1).join(", ") + " and " + list[list.length - 1]
   }
 }
+
+/**
+ * Generate a random string of a specified `length`.
+ */
+export function randomString(length = 8) {
+  return [...Array(length)].map(() => (~~(Math.random() * 36)).toString(36)).join("")
+}

garden-service/static/kubernetes/system/build-sync/templates/deployment.yaml

@@ -27,6 +27,13 @@ spec:
         - name: garden-build-sync
           persistentVolumeClaim:
             claimName: {{ .Values.pvc.name }}
+      initContainers:
+        - name: init
+          image: "busybox:1.31.1"
+          command: ["mkdir", "-p", "/data/tmp"]
+          volumeMounts:
+            - mountPath: /data
+              name: garden-build-sync
       containers:
         - name: sync
           image: "gardendev/rsync:0.1"

garden-service/test/e2e/garden.yml

@@ -31,7 +31,7 @@ tests:
   - name: tasks # Tests for tasks are currently being skipped
     command: [npm, run, e2e-project, --, --project=tasks, "--showlog=${var.show-log}", "--env=${environment.name}"]
     timeout: ${var.timeout}
-  - name: hot-reload # Tests for hot-reload are currently being skipped
+  - name: hot-reload
     command: [npm, run, e2e-project, --, --project=hot-reload, "--showlog=${var.show-log}", "--env=${environment.name}"]
     timeout: ${var.timeout}
   # - name: openfaas

garden-service/test/e2e/src/pre-release.ts

@@ -141,12 +141,6 @@ describe("PreReleaseTests", () => {
   }
 
   if (project === "hot-reload") {
-    /*
-     * TODO: Re-enable once this has been debugged:
-     *
-     * Got error from Kubernetes API - a container name must be specified for pod node-service-85f48587df-lvjlp,
-     * choose one of: [node-service garden-rsync] or one of the init containers: [garden-sync-init]
-     */
     describe("hot-reload", () => {
       it("runs the dev command with hot reloading enabled", async () => {
         const hotReloadProjectPath = resolve(examplesDir, "hot-reload")

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

@@ -65,7 +65,7 @@ describe("configureHotReload", () => {
                   {
                     name: "garden-sync",
                     mountPath: "/app",
-                    subPath: "app/",
+                    subPath: "root/app/",
                   },
                 ],
                 ports: [],
@@ -106,7 +106,12 @@ describe("configureHotReload", () => {
               {
                 name: "garden-sync-init",
                 image: "garden-io/foo",
-                command: ["/bin/sh", "-c", "mkdir -p /.garden/hot_reload && cp -r /app/ /.garden/hot_reload/app/"],
+                command: [
+                  "/bin/sh",
+                  "-c",
+                  "mkdir -p /.garden/hot_reload/root && mkdir -p /.garden/hot_reload/tmp/app/ && " +
+                    "cp -r /app/ /.garden/hot_reload/root/app/",
+                ],
                 env: [],
                 imagePullPolicy: "IfNotPresent",
                 volumeMounts: [
@@ -222,11 +227,26 @@ describe("rsyncSourcePath", () => {
 })
 
 describe("makeCopyCommand", () => {
-  const resA = "mkdir -p /.garden/hot_reload && cp -r /app/ /.garden/hot_reload/app/"
-  const resB = "mkdir -p /.garden/hot_reload/app/src && cp -r /app/src/foo/ /.garden/hot_reload/app/src/foo/"
-  const resC =
-    "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/"
+  const resA = [
+    "mkdir -p /.garden/hot_reload/root",
+    "mkdir -p /.garden/hot_reload/tmp/app/",
+    "cp -r /app/ /.garden/hot_reload/root/app/",
+  ].join(" && ")
+
+  const resB = [
+    "mkdir -p /.garden/hot_reload/root/app/src",
+    "mkdir -p /.garden/hot_reload/tmp/app/src/foo/",
+    "cp -r /app/src/foo/ /.garden/hot_reload/root/app/src/foo/",
+  ].join(" && ")
+
+  const resC = [
+    "mkdir -p /.garden/hot_reload/root/app",
+    "mkdir -p /.garden/hot_reload/tmp/app/src1/",
+    "cp -r /app/src1/ /.garden/hot_reload/root/app/src1/",
+    "mkdir -p /.garden/hot_reload/root/app",
+    "mkdir -p /.garden/hot_reload/tmp/app/src2/",
+    "cp -r /app/src2/ /.garden/hot_reload/root/app/src2/",
+  ].join(" && ")
 
   it("ensures a trailing slash in the copy source and target", () => {
     expect(makeCopyCommand(["/app/"])).to.eql(resA)