From 385b1dd19f40a25711651d45a8c13b9da2ab25d7 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Wed, 13 Nov 2019 16:40:20 +0100 Subject: [PATCH] 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 | 15 +++----- .../commands/cleanup-cluster-registry.ts | 11 +++--- .../src/plugins/kubernetes/container/build.ts | 33 +++++++++++++----- .../src/plugins/kubernetes/hot-reload.ts | 23 ++++++++----- garden-service/src/plugins/kubernetes/util.ts | 14 +++++++- garden-service/src/util/string.ts | 7 ++++ .../build-sync/templates/deployment.yaml | 7 ++++ garden-service/test/e2e/garden.yml | 2 +- garden-service/test/e2e/src/pre-release.ts | 6 ---- .../unit/src/plugins/kubernetes/hot-reload.ts | 34 +++++++++++++++---- 10 files changed, 106 insertions(+), 46 deletions(-) diff --git a/garden-service/src/build-dir.ts b/garden-service/src/build-dir.ts index 123e18c389..7b4a30eff1 100644 --- a/garden-service/src/build-dir.ts +++ b/garden-service/src/build-dir.ts @@ -148,7 +148,10 @@ export class BuildDir { files?: string[] }): Promise { 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") } diff --git a/garden-service/src/plugins/kubernetes/commands/cleanup-cluster-registry.ts b/garden-service/src/plugins/kubernetes/commands/cleanup-cluster-registry.ts index b9aaea9b8f..f79523ff1b 100644 --- a/garden-service/src/plugins/kubernetes/commands/cleanup-cluster-registry.ts +++ b/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, diff --git a/garden-service/src/plugins/kubernetes/container/build.ts b/garden-service/src/plugins/kubernetes/container/build.ts index f811bfb574..5279e71962 100644 --- a/garden-service/src/plugins/kubernetes/container/build.ts +++ b/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 { diff --git a/garden-service/src/plugins/kubernetes/hot-reload.ts b/garden-service/src/plugins/kubernetes/hot-reload.ts index db9c86d721..4a469554ab 100644 --- a/garden-service/src/plugins/kubernetes/hot-reload.ts +++ b/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 diff --git a/garden-service/src/plugins/kubernetes/util.ts b/garden-service/src/plugins/kubernetes/util.ts index 96d5f4cfe2..fb602a9b62 100644 --- a/garden-service/src/plugins/kubernetes/util.ts +++ b/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) +} diff --git a/garden-service/src/util/string.ts b/garden-service/src/util/string.ts index be3f2b1cd9..b17f374b0d 100644 --- a/garden-service/src/util/string.ts +++ b/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("") +} diff --git a/garden-service/static/kubernetes/system/build-sync/templates/deployment.yaml b/garden-service/static/kubernetes/system/build-sync/templates/deployment.yaml index a6eed9fb04..c13fe85804 100644 --- a/garden-service/static/kubernetes/system/build-sync/templates/deployment.yaml +++ b/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" diff --git a/garden-service/test/e2e/garden.yml b/garden-service/test/e2e/garden.yml index 90805b2835..4e7f6ddaf2 100644 --- a/garden-service/test/e2e/garden.yml +++ b/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 diff --git a/garden-service/test/e2e/src/pre-release.ts b/garden-service/test/e2e/src/pre-release.ts index e43f72ad3e..6e694816b2 100644 --- a/garden-service/test/e2e/src/pre-release.ts +++ b/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") diff --git a/garden-service/test/unit/src/plugins/kubernetes/hot-reload.ts b/garden-service/test/unit/src/plugins/kubernetes/hot-reload.ts index c3ad1b255f..4d7cc5d5d2 100644 --- a/garden-service/test/unit/src/plugins/kubernetes/hot-reload.ts +++ b/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)