From 4a352767d52fda9e0c0f6af269391eca9bd11274 Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Mon, 14 May 2018 22:33:00 +0200 Subject: [PATCH] feat: Detect circular dependencies. Added a check for circular build or service dependencies to the scanModules method of the Garden class. --- src/garden.ts | 7 +- src/plugins/kubernetes/kubectl.ts | 2 +- src/util/detectCycles.ts | 110 ++++++++++++++++++ src/{util.ts => util/index.ts} | 6 +- .../test-project-circular-deps/garden.yml | 6 + .../module-a/garden.yml | 14 +++ .../module-b/garden.yml | 15 +++ .../module-c/garden.yml | 14 +++ test/src/garden.ts | 41 ++++++- test/src/util.ts | 5 +- 10 files changed, 211 insertions(+), 9 deletions(-) create mode 100644 src/util/detectCycles.ts rename src/{util.ts => util/index.ts} (98%) create mode 100644 test/data/test-project-circular-deps/garden.yml create mode 100644 test/data/test-project-circular-deps/module-a/garden.yml create mode 100644 test/data/test-project-circular-deps/module-b/garden.yml create mode 100644 test/data/test-project-circular-deps/module-c/garden.yml diff --git a/src/garden.ts b/src/garden.ts index 095a8115a7..db33534532 100644 --- a/src/garden.ts +++ b/src/garden.ts @@ -95,6 +95,7 @@ import { Task } from "./types/task" import { LocalConfigStore, } from "./config-store" +import { detectCircularDependencies } from "./util/detectCycles" export interface ModuleMap { [key: string]: T @@ -571,7 +572,7 @@ export class Garden { } /* - Scans the project root for modules and adds them to the context + Scans the project root for modules and adds them to the context. */ async scanModules() { const ignorer = getIgnorer(this.projectRoot) @@ -603,6 +604,10 @@ export class Garden { } this.modulesScanned = true + + await detectCircularDependencies( + await this.getModules(), + (await this.getServices()).map(s => s.name)) } /* diff --git a/src/plugins/kubernetes/kubectl.ts b/src/plugins/kubernetes/kubectl.ts index daafcd7a72..ca7215e5ba 100644 --- a/src/plugins/kubernetes/kubectl.ts +++ b/src/plugins/kubernetes/kubectl.ts @@ -47,7 +47,7 @@ export class Kubectl { args: string[], { data, ignoreError = false, silent = true, timeout = KUBECTL_DEFAULT_TIMEOUT }: KubectlParams = {}, ): Promise { - // TODO: use the spawn helper from util.ts + // TODO: use the spawn helper from index.ts const logger = getLogger() const out: KubectlOutput = { code: 0, diff --git a/src/util/detectCycles.ts b/src/util/detectCycles.ts new file mode 100644 index 0000000000..7aa9623804 --- /dev/null +++ b/src/util/detectCycles.ts @@ -0,0 +1,110 @@ +/* + * 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 { forEach, get, isEqual, join, set, uniqWith } from "lodash" +import { Module, ModuleConfig } from "../types/module" +import { ConfigurationError } from "../exceptions" + +export type Cycle = string[] + +/* + Implements a variation on the Floyd-Warshall algorithm to compute minimal cycles. + + This is approximately O(m^3) + O(s^3), where m is the number of modules and s is the number of services. + + Throws an error if cycles were found. +*/ +export async function detectCircularDependencies(modules: Module[], serviceNames: string[]) { + + // Sparse matrices + const buildGraph = {} + const serviceGraph = {} + + /* + There's no need to account for test dependencies here, since any circularities there + are accounted for via service dependencies. + */ + for (const module of modules) { + const conf: ModuleConfig = await module.getConfig() + + // Build dependencies + for (const buildDep of get(conf, ["build", "dependencies"], [])) { + const depName = buildDep.name + set(buildGraph, [module.name, depName], { distance: 1, next: depName }) + } + + // Service dependencies + forEach(get(conf, ["services"], {}), (serviceConfig, serviceName) => { + for (const depName of get(serviceConfig, ["dependencies"], [])) { + set(serviceGraph, [serviceName, depName], { distance: 1, next: depName }) + } + }) + + } + + const buildCycles = detectCycles(buildGraph, modules.map(m => m.name)) + const serviceCycles = detectCycles(serviceGraph, serviceNames) + + if (buildCycles.length > 0 || serviceCycles.length > 0) { + const detail = {} + + if (buildCycles.length > 0) { + detail["circular-build-dependencies"] = cyclesToString(buildCycles) + } + + if (serviceCycles.length > 0) { + detail["circular-service-dependencies"] = cyclesToString(serviceCycles) + } + + throw new ConfigurationError("Circular dependencies detected", detail) + } +} + +export function detectCycles(graph, vertices: string[]): Cycle[] { + // Compute shortest paths + for (const k of vertices) { + for (const i of vertices) { + for (const j of vertices) { + const distanceViaK: number = distance(graph, i, k) + distance(graph, k, j) + if (distanceViaK < distance(graph, i, j)) { + const nextViaK = next(graph, i, k) + set(graph, [i, j], { distance: distanceViaK, next: nextViaK }) + } + } + } + } + + // Reconstruct cycles, if any + const cycleVertices = vertices.filter(v => next(graph, v, v)) + const cycles: Cycle[] = cycleVertices.map(v => { + const cycle = [v] + let nextInCycle = next(graph, v, v) + while (nextInCycle !== v) { + cycle.push(nextInCycle) + nextInCycle = next(graph, nextInCycle, v) + } + return cycle + }) + + return uniqWith( + cycles, // The concat calls below are to prevent in-place sorting. + (c1, c2) => isEqual(c1.concat().sort(), c2.concat().sort())) +} + +function distance(graph, source, destination): number { + return get(graph, [source, destination, "distance"], Infinity) +} + +function next(graph, source, destination): string { + return get(graph, [source, destination, "next"]) +} + +function cyclesToString(cycles: Cycle[]) { + const cycleDescriptions = cycles.map(c => join(c.concat([c[0]]), " <- ")) + return cycleDescriptions.length === 1 ? cycleDescriptions[0] : cycleDescriptions +} diff --git a/src/util.ts b/src/util/index.ts similarity index 98% rename from src/util.ts rename to src/util/index.ts index e291dd950c..6298002fcc 100644 --- a/src/util.ts +++ b/src/util/index.ts @@ -19,16 +19,16 @@ import { spawn as _spawn } from "child_process" import { existsSync, readFileSync, writeFileSync } from "fs" import { join } from "path" import { find } from "lodash" -import { getLogger, RootLogNode } from "./logger" +import { getLogger, RootLogNode } from "../logger/index" import { TimeoutError, GardenError, -} from "./exceptions" +} from "../exceptions" import { PassThrough } from "stream" import { isArray, isPlainObject, extend, mapValues, pickBy } from "lodash" import highlight from "cli-highlight" import chalk from "chalk" -import { FancyConsoleWriter } from "./logger/writers" +import { FancyConsoleWriter } from "../logger/writers" import hasAnsi = require("has-ansi") // shim to allow async generator functions diff --git a/test/data/test-project-circular-deps/garden.yml b/test/data/test-project-circular-deps/garden.yml new file mode 100644 index 0000000000..fc289ee552 --- /dev/null +++ b/test/data/test-project-circular-deps/garden.yml @@ -0,0 +1,6 @@ +project: + name: test-project-b + environments: + - name: local + providers: + - name: test-plugin diff --git a/test/data/test-project-circular-deps/module-a/garden.yml b/test/data/test-project-circular-deps/module-a/garden.yml new file mode 100644 index 0000000000..e6bab029d5 --- /dev/null +++ b/test/data/test-project-circular-deps/module-a/garden.yml @@ -0,0 +1,14 @@ +module: + name: module-a + type: generic + services: + - name: service-a + endpoints: + - paths: [/path-a] + containerPort: 8080 + dependencies: + - service-c + build: + command: echo A + dependencies: + - module-c diff --git a/test/data/test-project-circular-deps/module-b/garden.yml b/test/data/test-project-circular-deps/module-b/garden.yml new file mode 100644 index 0000000000..d46aff4466 --- /dev/null +++ b/test/data/test-project-circular-deps/module-b/garden.yml @@ -0,0 +1,15 @@ +module: + name: module-b + type: generic + services: + - name: service-b: + endpoints: + - paths: [/path-b] + containerPort: 8080 + dependencies: + - service-a + - service-c + build: + command: echo B + dependencies: + - module-a diff --git a/test/data/test-project-circular-deps/module-c/garden.yml b/test/data/test-project-circular-deps/module-c/garden.yml new file mode 100644 index 0000000000..d2d55057f5 --- /dev/null +++ b/test/data/test-project-circular-deps/module-c/garden.yml @@ -0,0 +1,14 @@ +module: + name: module-c + type: generic + allowPush: false + services: + - name: service-c: + endpoints: + - paths: [/path-c] + containerPort: 8080 + dependencies: + - service-b + build: + dependencies: + - module-b diff --git a/test/src/garden.ts b/test/src/garden.ts index dabb4f2a15..4059f21461 100644 --- a/test/src/garden.ts +++ b/test/src/garden.ts @@ -1,6 +1,7 @@ +import { expect } from "chai" import { join } from "path" import { Garden } from "../../src/garden" -import { expect } from "chai" +import { detectCycles } from "../../src/util/detectCycles" import { dataDir, expectError, @@ -226,9 +227,45 @@ describe("Garden", () => { const garden = await makeTestGardenA() await garden.scanModules() - const modules = await garden.getModules(undefined, true) + const modules = await garden.getModules(undefined, true) expect(getNames(modules)).to.eql(["module-a", "module-b", "module-c"]) }) + + describe("detectCircularDependencies", () => { + it("should throw an exception when circular dependencies are present", async () => { + const circularProjectRoot = join(__dirname, "..", "data", "test-project-circular-deps") + const garden = await makeTestGarden(circularProjectRoot) + await expectError( + async () => await garden.scanModules(), + "configuration") + }) + + it("should not throw an exception when no circular dependencies are present", async () => { + const nonCircularProjectRoot = join(__dirname, "..", "data", "test-project-b") + const garden = await makeTestGarden(nonCircularProjectRoot) + expect(async () => { await garden.scanModules() }).to.not.throw() + }) + }) + + describe("detectCycles", () => { + it("should detect self-to-self cycles", () => { + const cycles = detectCycles({ + a: {a: {distance: 1, next: "a"}}, + }, ["a"]) + + expect(cycles).to.deep.eq([["a"]]) + }) + + it("should preserve dependency order when returning cycles", () => { + const cycles = detectCycles({ + foo: {bar: {distance: 1, next: "bar"}}, + bar: {baz: {distance: 1, next: "baz"}}, + baz: {foo: {distance: 1, next: "foo"}}, + }, ["foo", "bar", "baz"]) + + expect(cycles).to.deep.eq([["foo", "bar", "baz"]]) + }) + }) }) describe("addModule", () => { diff --git a/test/src/util.ts b/test/src/util.ts index 56d234bdf3..7f356b2564 100644 --- a/test/src/util.ts +++ b/test/src/util.ts @@ -1,6 +1,6 @@ -import { scanDirectory } from "../../src/util" -import { join } from "path" import { expect } from "chai" +import { join } from "path" +import { scanDirectory } from "../../src/util" describe("util", () => { describe("scanDirectory", () => { @@ -33,4 +33,5 @@ describe("util", () => { expect(count).to.eq(3) }) }) + })