From 0b61ddd14a148e88468005aecf15d40e4ad6b2a4 Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Fri, 7 Feb 2020 09:21:33 +0100 Subject: [PATCH] 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 | 38 ++-------- .../src/plugins/kubernetes/hot-reload.ts | 53 ++++++++++++-- garden-service/src/util/sync.ts | 72 +++++++++++++++++++ .../include-exclude/module-a/garden.yml | 3 +- .../include-exclude/module-a/somedir/nope.txt | 1 + .../include-exclude/module-a/somedir/yes.txt | 1 + .../unit/src/plugins/kubernetes/hot-reload.ts | 34 ++++++++- garden-service/test/unit/src/vcs/vcs.ts | 5 +- 8 files changed, 162 insertions(+), 45 deletions(-) create mode 100644 garden-service/src/util/sync.ts create mode 100644 garden-service/test/data/test-projects/include-exclude/module-a/somedir/nope.txt create mode 100644 garden-service/test/data/test-projects/include-exclude/module-a/somedir/yes.txt diff --git a/garden-service/src/build-dir.ts b/garden-service/src/build-dir.ts index 9dcea3f724..69b928bf9e 100644 --- a/garden-service/src/build-dir.ts +++ b/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 }) } } diff --git a/garden-service/src/plugins/kubernetes/hot-reload.ts b/garden-service/src/plugins/kubernetes/hot-reload.ts index ff2a414f8a..e7312227c6 100644 --- a/garden-service/src/plugins/kubernetes/hot-reload.ts +++ b/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 +} diff --git a/garden-service/src/util/sync.ts b/garden-service/src/util/sync.ts new file mode 100644 index 0000000000..a654c8f448 --- /dev/null +++ b/garden-service/src/util/sync.ts @@ -0,0 +1,72 @@ +/* + * Copyright (C) 2018-2020 Garden Technologies, Inc. + * + * 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 { + 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 }) +} diff --git a/garden-service/test/data/test-projects/include-exclude/module-a/garden.yml b/garden-service/test/data/test-projects/include-exclude/module-a/garden.yml index 506273076e..c0bf435fcb 100644 --- a/garden-service/test/data/test-projects/include-exclude/module-a/garden.yml +++ b/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"] \ No newline at end of file diff --git a/garden-service/test/data/test-projects/include-exclude/module-a/somedir/nope.txt b/garden-service/test/data/test-projects/include-exclude/module-a/somedir/nope.txt new file mode 100644 index 0000000000..c5cfed716a --- /dev/null +++ b/garden-service/test/data/test-projects/include-exclude/module-a/somedir/nope.txt @@ -0,0 +1 @@ +Go away! \ No newline at end of file diff --git a/garden-service/test/data/test-projects/include-exclude/module-a/somedir/yes.txt b/garden-service/test/data/test-projects/include-exclude/module-a/somedir/yes.txt new file mode 100644 index 0000000000..80cccdde3b --- /dev/null +++ b/garden-service/test/data/test-projects/include-exclude/module-a/somedir/yes.txt @@ -0,0 +1 @@ +Oh hai! \ No newline at end of file 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 4f7ea0310a..21500c4a0f 100644 --- a/garden-service/test/unit/src/plugins/kubernetes/hot-reload.ts +++ b/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"]) + }) +}) diff --git a/garden-service/test/unit/src/vcs/vcs.ts b/garden-service/test/unit/src/vcs/vcs.ts index 1883a73706..77ca1cc625 100644 --- a/garden-service/test/unit/src/vcs/vcs.ts +++ b/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 () => {