From f7057580671b33bd38c3d5efd7928a80a87e3809 Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Fri, 29 Nov 2019 17:41:40 +0100 Subject: [PATCH] fix(build): fix intermittent concurrency issues when staging build Because both the BuildTask and GetServiceTask need to stage builds, they could in some cases end up stepping on each other because one doesn't depend on the other. I resolved that by adding a StageBuildTask that both depend on. --- .../src/plugins/kubernetes/system.ts | 6 +- garden-service/src/tasks/base.ts | 1 + garden-service/src/tasks/build.ts | 44 ++++++---- garden-service/src/tasks/delete-service.ts | 14 +++- garden-service/src/tasks/deploy.ts | 57 +++++++------ .../src/tasks/get-service-status.ts | 15 ++-- garden-service/src/tasks/stage-build.ts | 81 +++++++++++++++++++ .../test/unit/src/commands/build.ts | 5 ++ .../test/unit/src/commands/deploy.ts | 6 ++ .../test/unit/src/commands/publish.ts | 6 ++ garden-service/test/unit/src/server/server.ts | 2 + garden-service/test/unit/src/tasks/helpers.ts | 1 + 12 files changed, 188 insertions(+), 50 deletions(-) create mode 100644 garden-service/src/tasks/stage-build.ts diff --git a/garden-service/src/plugins/kubernetes/system.ts b/garden-service/src/plugins/kubernetes/system.ts index 69b623281e..d809cfd03f 100644 --- a/garden-service/src/plugins/kubernetes/system.ts +++ b/garden-service/src/plugins/kubernetes/system.ts @@ -27,6 +27,7 @@ import { combineStates } from "../../types/service" import { KubernetesResource } from "./types" import { defaultDotIgnoreFiles } from "../../util/fs" import { HelmServiceStatus } from "./helm/status" +import { LogLevel } from "../../logger/log-node" const GARDEN_VERSION = getPackageVersion() const SYSTEM_NAMESPACE_MIN_VERSION = "0.9.0" @@ -156,7 +157,10 @@ export async function getSystemServiceStatus({ const actions = await sysGarden.getActionRouter() - const serviceStatuses = await actions.getServiceStatuses({ log, serviceNames }) + const serviceStatuses = await actions.getServiceStatuses({ + log: log.placeholder(LogLevel.verbose, true), + serviceNames, + }) const state = combineStates(values(serviceStatuses).map((s) => (s && s.state) || "unknown")) // Add the Kubernetes dashboard to the Garden dashboard diff --git a/garden-service/src/tasks/base.ts b/garden-service/src/tasks/base.ts index f950125d00..ac96703836 100644 --- a/garden-service/src/tasks/base.ts +++ b/garden-service/src/tasks/base.ts @@ -25,6 +25,7 @@ export type TaskType = | "hot-reload" | "publish" | "resolve-provider" + | "stage-build" | "task" | "test" diff --git a/garden-service/src/tasks/build.ts b/garden-service/src/tasks/build.ts index 7398227191..378587088c 100644 --- a/garden-service/src/tasks/build.ts +++ b/garden-service/src/tasks/build.ts @@ -8,12 +8,12 @@ import Bluebird from "bluebird" import chalk from "chalk" -import pluralize = require("pluralize") import { Module, getModuleKey } from "../types/module" import { BuildResult } from "../types/plugin/module/build" import { BaseTask, TaskType } from "../tasks/base" import { Garden } from "../garden" import { LogEntry } from "../logger/log-entry" +import { StageBuildTask } from "./stage-build" export interface BuildTaskParams { garden: Garden @@ -42,7 +42,14 @@ export class BuildTask extends BaseTask { const dg = await this.garden.getConfigGraph() const deps = (await dg.getDependencies("build", this.getName(), false)).build - return Bluebird.map(deps, async (m: Module) => { + const stageBuildTask = new StageBuildTask({ + garden: this.garden, + log: this.log, + module: this.module, + force: this.force, + }) + + const buildTasks = await Bluebird.map(deps, async (m: Module) => { return new BuildTask({ garden: this.garden, log: this.log, @@ -52,6 +59,8 @@ export class BuildTask extends BaseTask { hotReloadServiceNames: this.hotReloadServiceNames, }) }) + + return [stageBuildTask, ...buildTasks] } getName() { @@ -66,27 +75,24 @@ export class BuildTask extends BaseTask { const module = this.module const actions = await this.garden.getActionRouter() - const log = this.log.info({ - section: this.getName(), - msg: `Preparing build (${pluralize("file", module.version.files.length, true)})...`, - status: "active", - }) + let log: LogEntry const logSuccess = () => { - log.setSuccess({ - msg: chalk.green(`Done (took ${log.getDuration(1)} sec)`), - append: true, - }) + if (log) { + log.setSuccess({ + msg: chalk.green(`Done (took ${log.getDuration(1)} sec)`), + append: true, + }) + } } - const graph = await this.garden.getConfigGraph() - await this.garden.buildDir.syncFromSrc(this.module, log) - await this.garden.buildDir.syncDependencyProducts(this.module, graph, log) - if (!this.force) { - log.setState({ + log = this.log.info({ + section: this.getName(), msg: `Getting build status for ${module.version.versionString}...`, + status: "active", }) + const status = await actions.getBuildStatus({ log: this.log, module }) if (status.ready) { @@ -95,7 +101,11 @@ export class BuildTask extends BaseTask { } } - log.setState({ msg: `Building version ${module.version.versionString}...` }) + log = this.log.info({ + section: this.getName(), + msg: `Building version ${module.version.versionString}...`, + status: "active", + }) let result: BuildResult try { diff --git a/garden-service/src/tasks/delete-service.ts b/garden-service/src/tasks/delete-service.ts index a3286c121b..f38c2fb85b 100644 --- a/garden-service/src/tasks/delete-service.ts +++ b/garden-service/src/tasks/delete-service.ts @@ -12,6 +12,7 @@ import { Service, ServiceStatus } from "../types/service" import { Garden } from "../garden" import { ConfigGraph } from "../config-graph" import { TaskResults, TaskResult } from "../task-graph" +import { StageBuildTask } from "./stage-build" export interface DeleteServiceTaskParams { garden: Garden @@ -36,14 +37,21 @@ export class DeleteServiceTask extends BaseTask { } async getDependencies() { + const stageBuildTask = new StageBuildTask({ + garden: this.garden, + log: this.log, + module: this.service.module, + force: this.force, + }) + if (!this.includeDependants) { - return [] + return [stageBuildTask] } // Note: We delete in _reverse_ dependency order, so we query for dependants const deps = await this.graph.getDependants("service", this.getName(), false) - return deps.service.map((service) => { + const dependants = deps.service.map((service) => { return new DeleteServiceTask({ garden: this.garden, graph: this.graph, @@ -52,6 +60,8 @@ export class DeleteServiceTask extends BaseTask { includeDependants: this.includeDependants, }) }) + + return [stageBuildTask, ...dependants] } getName() { diff --git a/garden-service/src/tasks/deploy.ts b/garden-service/src/tasks/deploy.ts index d3a4bd53f8..a0f729946b 100644 --- a/garden-service/src/tasks/deploy.ts +++ b/garden-service/src/tasks/deploy.ts @@ -71,32 +71,28 @@ export class DeployTask extends BaseTask { (depNode) => !(depNode.type === "service" && includes(this.hotReloadServiceNames, depNode.name)) ) - const tasks: BaseTask[] = deps.service.map((service) => { - return new DeployTask({ - garden: this.garden, - graph: this.graph, - log: this.log, - service, - force: false, - forceBuild: this.forceBuild, - fromWatch: this.fromWatch, - hotReloadServiceNames: this.hotReloadServiceNames, - }) + const statusTask = new GetServiceStatusTask({ + garden: this.garden, + graph: this.graph, + log: this.log, + service: this.service, + force: false, + hotReloadServiceNames: this.hotReloadServiceNames, }) - tasks.push( - new GetServiceStatusTask({ - garden: this.garden, - graph: this.graph, - log: this.log, - service: this.service, - force: false, - hotReloadServiceNames: this.hotReloadServiceNames, - }) - ) - if (this.fromWatch && includes(this.hotReloadServiceNames, this.service.name)) { // Only need to get existing statuses and results when hot-reloading + const dependencyStatusTasks = deps.service.map((service) => { + return new GetServiceStatusTask({ + garden: this.garden, + graph: this.graph, + log: this.log, + service, + force: false, + hotReloadServiceNames: this.hotReloadServiceNames, + }) + }) + const taskResultTasks = await Bluebird.map(deps.task, async (task) => { return new GetTaskResultTask({ garden: this.garden, @@ -107,8 +103,21 @@ export class DeployTask extends BaseTask { }) }) - return [...tasks, ...taskResultTasks] + return [statusTask, ...dependencyStatusTasks, ...taskResultTasks] } else { + const deployTasks = deps.service.map((service) => { + return new DeployTask({ + garden: this.garden, + graph: this.graph, + log: this.log, + service, + force: false, + forceBuild: this.forceBuild, + fromWatch: this.fromWatch, + hotReloadServiceNames: this.hotReloadServiceNames, + }) + }) + const taskTasks = await Bluebird.map(deps.task, (task) => { return TaskTask.factory({ task, @@ -129,7 +138,7 @@ export class DeployTask extends BaseTask { hotReloadServiceNames: this.hotReloadServiceNames, }) - return [...tasks, ...taskTasks, buildTask] + return [statusTask, ...deployTasks, ...taskTasks, buildTask] } } diff --git a/garden-service/src/tasks/get-service-status.ts b/garden-service/src/tasks/get-service-status.ts index 967c4837f5..5e1d9fb61f 100644 --- a/garden-service/src/tasks/get-service-status.ts +++ b/garden-service/src/tasks/get-service-status.ts @@ -16,6 +16,7 @@ import { TaskResults } from "../task-graph" import { prepareRuntimeContext } from "../runtime-context" import { getTaskVersion, TaskTask } from "./task" import Bluebird from "bluebird" +import { StageBuildTask } from "./stage-build" export interface GetServiceStatusTaskParams { garden: Garden @@ -43,6 +44,13 @@ export class GetServiceStatusTask extends BaseTask { async getDependencies() { const deps = await this.graph.getDependencies("service", this.getName(), false) + const stageBuildTask = new StageBuildTask({ + garden: this.garden, + log: this.log, + module: this.service.module, + force: this.force, + }) + const statusTasks = deps.service.map((service) => { return new GetServiceStatusTask({ garden: this.garden, @@ -66,7 +74,7 @@ export class GetServiceStatusTask extends BaseTask { }) }) - return [...statusTasks, ...taskTasks] + return [stageBuildTask, ...statusTasks, ...taskTasks] } getName() { @@ -98,11 +106,6 @@ export class GetServiceStatusTask extends BaseTask { const actions = await this.garden.getActionRouter() - // Some handlers expect builds to have been staged when resolving services statuses. - const graph = await this.garden.getConfigGraph() - await this.garden.buildDir.syncFromSrc(this.service.module, log) - await this.garden.buildDir.syncDependencyProducts(this.service.module, graph, log) - let status = await actions.getServiceStatus({ service: this.service, log, diff --git a/garden-service/src/tasks/stage-build.ts b/garden-service/src/tasks/stage-build.ts new file mode 100644 index 0000000000..877d6339cf --- /dev/null +++ b/garden-service/src/tasks/stage-build.ts @@ -0,0 +1,81 @@ +/* + * Copyright (C) 2018 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 Bluebird from "bluebird" +import chalk from "chalk" +import pluralize from "pluralize" +import { Module, getModuleKey } from "../types/module" +import { BuildResult } from "../types/plugin/module/build" +import { BaseTask, TaskType } from "../tasks/base" +import { Garden } from "../garden" +import { LogEntry } from "../logger/log-entry" + +export interface StageBuildTaskParams { + garden: Garden + log: LogEntry + module: Module + force: boolean +} + +export class StageBuildTask extends BaseTask { + type: TaskType = "stage-build" + + private module: Module + + constructor({ garden, log, module, force }: StageBuildTaskParams) { + super({ garden, log, force, version: module.version }) + this.module = module + } + + async getDependencies() { + const dg = await this.garden.getConfigGraph() + const deps = (await dg.getDependencies("build", this.getName(), false)).build + + return Bluebird.map(deps, async (m: Module) => { + return new StageBuildTask({ + garden: this.garden, + log: this.log, + module: m, + force: this.force, + }) + }) + } + + getName() { + return getModuleKey(this.module.name, this.module.plugin) + } + + getDescription() { + return `staging build for ${this.getName()}` + } + + async process(): Promise { + let log: LogEntry | undefined = undefined + + if (this.module.version.files.length > 0) { + log = this.log.info({ + section: this.getName(), + msg: `Syncing module sources (${pluralize("file", this.module.version.files.length, true)})...`, + status: "active", + }) + } + + const graph = await this.garden.getConfigGraph() + await this.garden.buildDir.syncFromSrc(this.module, log || this.log) + await this.garden.buildDir.syncDependencyProducts(this.module, graph, log || this.log) + + if (log) { + log.setSuccess({ + msg: chalk.green(`Done (took ${log.getDuration(1)} sec)`), + append: true, + }) + } + + return {} + } +} diff --git a/garden-service/test/unit/src/commands/build.ts b/garden-service/test/unit/src/commands/build.ts index f4bcc74117..839e774202 100644 --- a/garden-service/test/unit/src/commands/build.ts +++ b/garden-service/test/unit/src/commands/build.ts @@ -23,6 +23,9 @@ describe("commands.build", () => { "build.module-a": { fresh: true, buildLog: "A" }, "build.module-b": { fresh: true, buildLog: "B" }, "build.module-c": {}, + "stage-build.module-a": {}, + "stage-build.module-b": {}, + "stage-build.module-c": {}, }) }) @@ -44,6 +47,8 @@ describe("commands.build", () => { expect(taskResultOutputs(result!)).to.eql({ "build.module-a": { fresh: true, buildLog: "A" }, "build.module-b": { fresh: true, buildLog: "B" }, + "stage-build.module-a": {}, + "stage-build.module-b": {}, }) }) }) diff --git a/garden-service/test/unit/src/commands/deploy.ts b/garden-service/test/unit/src/commands/deploy.ts index e9ca4555d4..369ac6fb01 100644 --- a/garden-service/test/unit/src/commands/deploy.ts +++ b/garden-service/test/unit/src/commands/deploy.ts @@ -128,6 +128,9 @@ describe("DeployCommand", () => { } expect(taskResultOutputs(result!)).to.eql({ + "stage-build.module-a": {}, + "stage-build.module-b": {}, + "stage-build.module-c": {}, "build.module-a": { fresh: true, buildLog: "A" }, "build.module-b": { fresh: true, buildLog: "B" }, "build.module-c": {}, @@ -197,6 +200,9 @@ describe("DeployCommand", () => { } expect(taskResultOutputs(result!)).to.eql({ + "stage-build.module-a": {}, + "stage-build.module-b": {}, + "stage-build.module-c": {}, "build.module-a": { fresh: true, buildLog: "A" }, "build.module-b": { fresh: true, buildLog: "B" }, "build.module-c": {}, diff --git a/garden-service/test/unit/src/commands/publish.ts b/garden-service/test/unit/src/commands/publish.ts index f11c7323fe..e1e71db7a8 100644 --- a/garden-service/test/unit/src/commands/publish.ts +++ b/garden-service/test/unit/src/commands/publish.ts @@ -79,6 +79,8 @@ describe("PublishCommand", () => { "publish.module-a": { published: true }, "publish.module-b": { published: true }, "publish.module-c": { published: false }, + "stage-build.module-a": {}, + "stage-build.module-b": {}, }) }) @@ -107,6 +109,8 @@ describe("PublishCommand", () => { "publish.module-a": { published: true }, "publish.module-b": { published: true }, "publish.module-c": { published: false }, + "stage-build.module-a": {}, + "stage-build.module-b": {}, }) }) @@ -132,6 +136,7 @@ describe("PublishCommand", () => { expect(taskResultOutputs(result!)).to.eql({ "build.module-a": { fresh: false }, "publish.module-a": { published: true }, + "stage-build.module-a": {}, }) }) @@ -189,6 +194,7 @@ describe("PublishCommand", () => { published: false, message: chalk.yellow("No publish handler available for module type test"), }, + "stage-build.module-a": {}, }) }) }) diff --git a/garden-service/test/unit/src/server/server.ts b/garden-service/test/unit/src/server/server.ts index 1c6a244f79..daa9178f85 100644 --- a/garden-service/test/unit/src/server/server.ts +++ b/garden-service/test/unit/src/server/server.ts @@ -98,6 +98,7 @@ describe("startServer", () => { buildLog: "A", fresh: true, }, + "stage-build.module-a": {}, }) }) }) @@ -243,6 +244,7 @@ describe("startServer", () => { buildLog: "A", fresh: true, }, + "stage-build.module-a": {}, }, }) done() diff --git a/garden-service/test/unit/src/tasks/helpers.ts b/garden-service/test/unit/src/tasks/helpers.ts index bd5c558a2d..ceaf8b3921 100644 --- a/garden-service/test/unit/src/tasks/helpers.ts +++ b/garden-service/test/unit/src/tasks/helpers.ts @@ -58,6 +58,7 @@ describe("TaskHelpers", () => { "build.build-dependency", "build.good-morning", "get-service-status.good-morning", + "stage-build.good-morning", "task.good-morning-task", ].sort() )