From 04a5a36ae82caf3cfba0d8aa1b294d3c65b6fd3d Mon Sep 17 00:00:00 2001 From: Jon Edvald Date: Tue, 25 Jun 2019 18:16:24 +0200 Subject: [PATCH] fix(k8s): error when test+task result log exceeded 1MB This comes down to a limitation of K8s ConfigMap resources. I've refactored that code a bit and ensured we limit the log size we store. I also made sure the ConfigMap keys didn't grow too long by hashing the key parameters. Lastly I made the results more easily searchable by adding labels. --- garden-service/src/plugins/kubernetes/api.ts | 12 ++-- .../src/plugins/kubernetes/constants.ts | 2 + .../plugins/kubernetes/container/handlers.ts | 2 +- .../src/plugins/kubernetes/container/run.ts | 1 + .../src/plugins/kubernetes/container/test.ts | 2 +- .../src/plugins/kubernetes/helm/handlers.ts | 2 +- .../src/plugins/kubernetes/helm/run.ts | 10 ++- .../src/plugins/kubernetes/helm/test.ts | 2 +- .../src/plugins/kubernetes/task-results.ts | 61 +++++++++---------- .../kubernetes/{test.ts => test-results.ts} | 41 ++++++------- garden-service/src/plugins/kubernetes/util.ts | 47 +++++++++++++- garden-service/src/util/string.ts | 28 ++++++++- garden-service/src/util/util.ts | 21 ++----- garden-service/test/unit/src/util/string.ts | 19 ++++++ 14 files changed, 166 insertions(+), 84 deletions(-) rename garden-service/src/plugins/kubernetes/{test.ts => test-results.ts} (74%) create mode 100644 garden-service/test/unit/src/util/string.ts diff --git a/garden-service/src/plugins/kubernetes/api.ts b/garden-service/src/plugins/kubernetes/api.ts index 07d83ca0a9..8eecf7a417 100644 --- a/garden-service/src/plugins/kubernetes/api.ts +++ b/garden-service/src/plugins/kubernetes/api.ts @@ -413,13 +413,15 @@ async function getContextConfig(log: LogEntry, context: string): Promise isObject(v) || k[0] === "_"), + const response = err.response || {} + const body = response.body || err.body + const wrapped = new KubernetesError(`Got error from Kubernetes API - ${body.message}`, { + body, + request: omitBy(response.request, (v, k) => isObject(v) || k[0] === "_"), }) - wrapped.code = err.response.statusCode + wrapped.code = response.statusCode return wrapped } else { return err diff --git a/garden-service/src/plugins/kubernetes/constants.ts b/garden-service/src/plugins/kubernetes/constants.ts index df5e4ff2bb..e97609d071 100644 --- a/garden-service/src/plugins/kubernetes/constants.ts +++ b/garden-service/src/plugins/kubernetes/constants.ts @@ -9,3 +9,5 @@ export const RSYNC_PORT = 873 export const CLUSTER_REGISTRY_PORT = 5000 export const CLUSTER_REGISTRY_DEPLOYMENT_NAME = "garden-docker-registry" +export const MAX_CONFIGMAP_DATA_SIZE = 1024 * 1024 // max ConfigMap data size is 1MB +export const MAX_RUN_RESULT_OUTPUT_LENGTH = 900 * 1024 // max ConfigMap data size is 1MB, so 900kB gives enough margin diff --git a/garden-service/src/plugins/kubernetes/container/handlers.ts b/garden-service/src/plugins/kubernetes/container/handlers.ts index fb4b716ad0..0d1ab6087a 100644 --- a/garden-service/src/plugins/kubernetes/container/handlers.ts +++ b/garden-service/src/plugins/kubernetes/container/handlers.ts @@ -16,7 +16,7 @@ import { configureContainerModule } from "../../container/container" import { KubernetesProvider } from "../config" import { ConfigureModuleParams } from "../../../types/plugin/module/configure" import { getContainerServiceStatus } from "./status" -import { getTestResult } from "../test" +import { getTestResult } from "../test-results" import { ContainerModule } from "../../container/config" import { configureMavenContainerModule, MavenContainerModule } from "../../maven-container/maven-container" import { getTaskResult } from "../task-results" diff --git a/garden-service/src/plugins/kubernetes/container/run.ts b/garden-service/src/plugins/kubernetes/container/run.ts index 07cc5bace8..41ea55e9a0 100644 --- a/garden-service/src/plugins/kubernetes/container/run.ts +++ b/garden-service/src/plugins/kubernetes/container/run.ts @@ -152,6 +152,7 @@ export async function runContainerTask( await storeTaskResult({ ctx, log, + module, result, taskVersion, taskName: task.name, diff --git a/garden-service/src/plugins/kubernetes/container/test.ts b/garden-service/src/plugins/kubernetes/container/test.ts index d674ecf799..03983f9f10 100644 --- a/garden-service/src/plugins/kubernetes/container/test.ts +++ b/garden-service/src/plugins/kubernetes/container/test.ts @@ -9,7 +9,7 @@ import { ContainerModule } from "../../container/config" import { DEFAULT_TEST_TIMEOUT } from "../../../constants" import { runContainerModule } from "./run" -import { storeTestResult } from "../test" +import { storeTestResult } from "../test-results" import { TestModuleParams } from "../../../types/plugin/module/testModule" import { TestResult } from "../../../types/plugin/module/getTestResult" diff --git a/garden-service/src/plugins/kubernetes/helm/handlers.ts b/garden-service/src/plugins/kubernetes/helm/handlers.ts index b01fcf0917..2d3c3724d4 100644 --- a/garden-service/src/plugins/kubernetes/helm/handlers.ts +++ b/garden-service/src/plugins/kubernetes/helm/handlers.ts @@ -11,7 +11,7 @@ import { HelmModule, validateHelmModule as configureHelmModule, helmModuleSpecSc import { buildHelmModule } from "./build" import { getServiceStatus } from "./status" import { deployService, deleteService } from "./deployment" -import { getTestResult } from "../test" +import { getTestResult } from "../test-results" import { runHelmTask, runHelmModule } from "./run" import { hotReloadHelmChart } from "./hot-reload" import { getServiceLogs } from "./logs" diff --git a/garden-service/src/plugins/kubernetes/helm/run.ts b/garden-service/src/plugins/kubernetes/helm/run.ts index 814b8d1ecf..88f68a97dd 100644 --- a/garden-service/src/plugins/kubernetes/helm/run.ts +++ b/garden-service/src/plugins/kubernetes/helm/run.ts @@ -18,6 +18,8 @@ import { storeTaskResult } from "../task-results" import { RunModuleParams } from "../../../types/plugin/module/runModule" import { RunResult } from "../../../types/plugin/base" import { RunTaskParams, RunTaskResult } from "../../../types/plugin/task/runTask" +import { MAX_RUN_RESULT_OUTPUT_LENGTH } from "../constants" +import { tailString } from "../../../util/string" export async function runHelmModule( { @@ -78,11 +80,17 @@ export async function runHelmTask( log, }) - const result = { ...res, taskName: task.name } + const result = { + ...res, + // Make sure we don't exceed max length of ConfigMap + output: tailString(res.output, MAX_RUN_RESULT_OUTPUT_LENGTH, true), + taskName: task.name, + } await storeTaskResult({ ctx, log, + module, result, taskVersion, taskName: task.name, diff --git a/garden-service/src/plugins/kubernetes/helm/test.ts b/garden-service/src/plugins/kubernetes/helm/test.ts index e841ca0abd..ca8248b71b 100644 --- a/garden-service/src/plugins/kubernetes/helm/test.ts +++ b/garden-service/src/plugins/kubernetes/helm/test.ts @@ -7,7 +7,7 @@ */ import { DEFAULT_TEST_TIMEOUT } from "../../../constants" -import { storeTestResult } from "../test" +import { storeTestResult } from "../test-results" import { HelmModule } from "./config" import { getAppNamespace } from "../namespace" import { runPod } from "../run" diff --git a/garden-service/src/plugins/kubernetes/task-results.ts b/garden-service/src/plugins/kubernetes/task-results.ts index ffff25afb4..2811633402 100644 --- a/garden-service/src/plugins/kubernetes/task-results.ts +++ b/garden-service/src/plugins/kubernetes/task-results.ts @@ -14,17 +14,22 @@ import { KubernetesPluginContext, KubernetesProvider } from "./config" import { KubeApi } from "./api" import { getMetadataNamespace } from "./namespace" import { RunTaskResult } from "../../types/plugin/task/runTask" -import { deserializeValues, serializeValues } from "../../util/util" +import { deserializeValues } from "../../util/util" import { PluginContext } from "../../plugin-context" import { LogEntry } from "../../logger/log-entry" +import { gardenAnnotationKey, tailString } from "../../util/string" +import { Module } from "../../types/module" +import * as hasha from "hasha" +import { upsertConfigMap } from "./util" +import { MAX_RUN_RESULT_OUTPUT_LENGTH } from "./constants" export async function getTaskResult( - { ctx, log, task, taskVersion }: GetTaskResultParams, + { ctx, log, module, task, taskVersion }: GetTaskResultParams, ): Promise { const k8sCtx = ctx const api = await KubeApi.factory(log, k8sCtx.provider.config.context) const ns = await getMetadataNamespace(k8sCtx, log, k8sCtx.provider) - const resultKey = getTaskResultKey(task.name, taskVersion) + const resultKey = getTaskResultKey(ctx, module, task.name, taskVersion) try { const res = await api.core.readNamespacedConfigMap(resultKey, ns) @@ -38,13 +43,16 @@ export async function getTaskResult( } } -export function getTaskResultKey(taskName: string, version: ModuleVersion) { - return `task-result--${taskName}--${version.versionString}` +export function getTaskResultKey(ctx: PluginContext, module: Module, taskName: string, version: ModuleVersion) { + const key = `${ctx.projectName}--${module.name}--${taskName}--${version.versionString}` + const hash = hasha(key, { algorithm: "sha1" }) + return `task-result--${hash.slice(0, 32)}` } interface StoreTaskResultParams { ctx: PluginContext, log: LogEntry, + module: Module, taskName: string, taskVersion: ModuleVersion, result: RunTaskResult, @@ -56,34 +64,25 @@ interface StoreTaskResultParams { * TODO: Implement a CRD for this. */ export async function storeTaskResult( - { ctx, log, taskName, taskVersion, result }: StoreTaskResultParams, -): Promise { + { ctx, log, module, taskName, taskVersion, result }: StoreTaskResultParams, +) { const provider = ctx.provider const api = await KubeApi.factory(log, provider.config.context) - const ns = await getMetadataNamespace(ctx, log, provider) - const resultKey = getTaskResultKey(taskName, taskVersion) + const namespace = await getMetadataNamespace(ctx, log, provider) - const body = { - apiVersion: "v1", - kind: "ConfigMap", - metadata: { - name: resultKey, - annotations: { - "garden.io/generated": "true", - }, - }, - data: serializeValues(result), - } - - try { - await api.core.createNamespacedConfigMap(ns, body) - } catch (err) { - if (err.code === 409) { - await api.core.patchNamespacedConfigMap(resultKey, ns, body) - } else { - throw err - } - } + // Make sure the output isn't too large for a ConfigMap + result.output = tailString(result.output, MAX_RUN_RESULT_OUTPUT_LENGTH, true) - return result + await upsertConfigMap({ + api, + namespace, + key: getTaskResultKey(ctx, module, taskName, taskVersion), + labels: { + [gardenAnnotationKey("module")]: module.name, + [gardenAnnotationKey("task")]: taskName, + [gardenAnnotationKey("moduleVersion")]: module.version.versionString, + [gardenAnnotationKey("version")]: taskVersion.versionString, + }, + data: result, + }) } diff --git a/garden-service/src/plugins/kubernetes/test.ts b/garden-service/src/plugins/kubernetes/test-results.ts similarity index 74% rename from garden-service/src/plugins/kubernetes/test.ts rename to garden-service/src/plugins/kubernetes/test-results.ts index 02eb4404cc..52247421be 100644 --- a/garden-service/src/plugins/kubernetes/test.ts +++ b/garden-service/src/plugins/kubernetes/test-results.ts @@ -7,7 +7,7 @@ */ import { ContainerModule } from "../container/config" -import { deserializeValues, serializeValues } from "../../util/util" +import { deserializeValues } from "../../util/util" import { KubeApi } from "./api" import { Module } from "../../types/module" import { ModuleVersion } from "../../vcs/vcs" @@ -18,6 +18,9 @@ import { systemMetadataNamespace } from "./system" import { LogEntry } from "../../logger/log-entry" import { GetTestResultParams, TestResult } from "../../types/plugin/module/getTestResult" import { RunResult } from "../../types/plugin/base" +import * as hasha from "hasha" +import { gardenAnnotationKey } from "../../util/string" +import { upsertConfigMap } from "./util" const testResultNamespace = systemMetadataNamespace @@ -41,7 +44,9 @@ export async function getTestResult( } export function getTestResultKey(ctx: PluginContext, module: Module, testName: string, version: ModuleVersion) { - return `test-result--${ctx.projectName}--${module.name}--${testName}--${version.versionString}` + const key = `${ctx.projectName}--${module.name}--${testName}--${version.versionString}` + const hash = hasha(key, { algorithm: "sha1" }) + return `test-result--${hash.slice(0, 32)}` } interface StoreTestResultParams { @@ -69,28 +74,18 @@ export async function storeTestResult( testName, } - const resultKey = getTestResultKey(k8sCtx, module, testName, testVersion) - const body = { - apiVersion: "v1", - kind: "ConfigMap", - metadata: { - name: resultKey, - annotations: { - "garden.io/generated": "true", - }, + await upsertConfigMap({ + api, + namespace: testResultNamespace, + key: getTestResultKey(k8sCtx, module, testName, testVersion), + labels: { + [gardenAnnotationKey("module")]: module.name, + [gardenAnnotationKey("test")]: testName, + [gardenAnnotationKey("moduleVersion")]: module.version.versionString, + [gardenAnnotationKey("version")]: testVersion.versionString, }, - data: serializeValues(testResult), - } - - try { - await api.core.createNamespacedConfigMap(testResultNamespace, body) - } catch (err) { - if (err.code === 409) { - await api.core.patchNamespacedConfigMap(resultKey, testResultNamespace, body) - } else { - throw err - } - } + data: testResult, + }) return testResult } diff --git a/garden-service/src/plugins/kubernetes/util.ts b/garden-service/src/plugins/kubernetes/util.ts index 44a809a49f..2bf9b235c8 100644 --- a/garden-service/src/plugins/kubernetes/util.ts +++ b/garden-service/src/plugins/kubernetes/util.ts @@ -14,13 +14,15 @@ const AsyncLock = require("async-lock") import { V1Pod } from "@kubernetes/client-node" import { KubernetesResource, KubernetesWorkload, KubernetesPod, KubernetesServerResource } from "./types" -import { splitLast } from "../../util/util" -import { KubeApi } from "./api" +import { splitLast, serializeValues } from "../../util/util" +import { KubeApi, KubernetesError } from "./api" import { PluginContext } from "../../plugin-context" import { LogEntry } from "../../logger/log-entry" import { KubernetesPluginContext } from "./config" import { kubectl } from "./kubectl" import { registerCleanupFunction } from "../../util/util" +import { gardenAnnotationKey, base64 } from "../../util/string" +import { MAX_CONFIGMAP_DATA_SIZE } from "./constants" export const workloadTypes = ["Deployment", "DaemonSet", "ReplicaSet", "StatefulSet"] @@ -216,3 +218,44 @@ const suffixTable = { Gi: 2, Mi: 1, } + +export async function upsertConfigMap( + { api, namespace, key, labels, data }: + { api: KubeApi, namespace: string, key: string, labels: { [key: string]: string }, data: { [key: string]: any } }, +) { + const serializedData = serializeValues(data) + + if (base64(JSON.stringify(serializedData)).length > MAX_CONFIGMAP_DATA_SIZE) { + throw new KubernetesError(`Attempting to store too much data in ConfigMap ${key}`, { + key, + namespace, + labels, + data, + }) + } + + const body = { + apiVersion: "v1", + kind: "ConfigMap", + metadata: { + name: key, + annotations: { + [gardenAnnotationKey("generated")]: "true", + // Set all the labels as annotations as well + ...labels, + }, + labels, + }, + data: serializedData, + } + + try { + await api.core.createNamespacedConfigMap(namespace, body) + } catch (err) { + if (err.code === 409) { + await api.core.patchNamespacedConfigMap(key, namespace, body) + } else { + throw err + } + } +} diff --git a/garden-service/src/util/string.ts b/garden-service/src/util/string.ts index e5dfb78ae0..279f7a11a5 100644 --- a/garden-service/src/util/string.ts +++ b/garden-service/src/util/string.ts @@ -16,8 +16,34 @@ export const deline = _deline const gardenAnnotationPrefix = "garden.io/" -export type GardenAnnotationKey = "generated" | "service" | "version" +export type GardenAnnotationKey = "generated" | "module" | "moduleVersion" | "service" | "task" | "test" | "version" export function gardenAnnotationKey(key: GardenAnnotationKey) { return gardenAnnotationPrefix + key } + +/** + * Truncates the first n characters from a string where n equals the number by + * which the string byte length exceeds the `maxLength`. + * + * Optionally scan towards the next line break after trimming the bytes, and trim to there. + * + * Note that a UTF-8 character can be 1-4 bytes so this is a naive but inexpensive approach. + */ +export function tailString(str: string, maxLength: number, nextLine = false) { + const overflow = Buffer.byteLength(str, "utf8") - maxLength + if (overflow > 0) { + if (nextLine) { + const lineBreakIdx = str.indexOf("\n", overflow) + if (lineBreakIdx) { + return str.substr(lineBreakIdx + 1) + } + } + return str.substr(overflow) + } + return str +} + +export function base64(str: string) { + return Buffer.from(str).toString("base64") +} diff --git a/garden-service/src/util/util.ts b/garden-service/src/util/util.ts index ecf7ff3e09..8314cf9999 100644 --- a/garden-service/src/util/util.ts +++ b/garden-service/src/util/util.ts @@ -20,6 +20,7 @@ import highlight from "cli-highlight" import chalk from "chalk" import { safeDump } from "js-yaml" import { createHash } from "crypto" +import { tailString } from "./string" // shim to allow async generator functions if (typeof (Symbol as any).asyncIterator === "undefined") { @@ -87,20 +88,6 @@ export interface SpawnOutput { proc: any } -/** - * Truncates the first n characters from a string where n equals the number by - * which the string byte length exceeds the MAX_BUFFER_SIZE. - * - * Note that a utf8 character can be 1-4 bytes so this is a naive but inexpensive approach. - */ -function naivelyTruncateBytes(str: string) { - const overflow = Buffer.byteLength(str, "utf8") - MAX_BUFFER_SIZE - if (overflow > 0) { - str = str.substr(overflow) - } - return str -} - // TODO Dump output to a log file if it exceeds the MAX_BUFFER_SIZE export function spawn(cmd: string, args: string[], opts: SpawnOpts = {}) { const { timeout = 0, cwd, data, ignoreError = false, env, tty, wait = true } = opts @@ -131,12 +118,12 @@ export function spawn(cmd: string, args: string[], opts: SpawnOpts = {}) { } else { // We ensure the output strings never exceed the MAX_BUFFER_SIZE proc.stdout!.on("data", (s) => { - result.output = naivelyTruncateBytes(result.output + s) - result.stdout! = naivelyTruncateBytes(result.stdout! + s) + result.output = tailString(result.output + s, MAX_BUFFER_SIZE, true) + result.stdout! = tailString(result.stdout! + s, MAX_BUFFER_SIZE, true) }) proc.stderr!.on("data", (s) => { - result.stderr! = naivelyTruncateBytes(result.stderr! + s) + result.stderr! = tailString(result.stderr! + s, MAX_BUFFER_SIZE, true) }) if (data) { diff --git a/garden-service/test/unit/src/util/string.ts b/garden-service/test/unit/src/util/string.ts new file mode 100644 index 0000000000..8166d100ae --- /dev/null +++ b/garden-service/test/unit/src/util/string.ts @@ -0,0 +1,19 @@ +import { expect } from "chai" +import { tailString } from "../../../../src/util/string" + +describe("tailString", () => { + it("should return string unchanged if it's shorter than maxLength", () => { + const str = "123456789" + expect(tailString(str, 10)).to.equal(str) + }) + + it("should trim off first bytes if string is longer than maxLength", () => { + const str = "1234567890" + expect(tailString(str, 5)).to.equal("67890") + }) + + it("should trim until next newline if string is longer than maxLength and nextLine=true", () => { + const str = "1234567\n890" + expect(tailString(str, 5, true)).to.equal("890") + }) +})