From 7a68373a175b1e757b67f391030c643d0fde55d4 Mon Sep 17 00:00:00 2001 From: Thorarinn Sigurdsson Date: Mon, 27 May 2024 13:07:32 +0200 Subject: [PATCH] improvement(helm): use --wait when deploying (#6078) The `--wait` option for `helm deploy` and `helm upgrade` is now applied by default for `helm` Deploys. We had previously used `waitForResources` for this (since Helm's rollout statuses weren't reliable enough when using `--wait`). In the `helm` deploy handler, we now only use `waitForResources` to wait for resources updated for local or sync mode. --- .../src/plugins/kubernetes/helm/deployment.ts | 78 +++++++++++-------- core/src/plugins/kubernetes/status/status.ts | 26 +++++-- 2 files changed, 63 insertions(+), 41 deletions(-) diff --git a/core/src/plugins/kubernetes/helm/deployment.ts b/core/src/plugins/kubernetes/helm/deployment.ts index 52b7f79e58..16ec94fbf7 100644 --- a/core/src/plugins/kubernetes/helm/deployment.ts +++ b/core/src/plugins/kubernetes/helm/deployment.ts @@ -6,7 +6,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import { waitForResources } from "../status/status.js" +import { checkResourceStatuses, waitForResources } from "../status/status.js" import { helm } from "./helm-cli.js" import type { HelmGardenMetadataConfigMapData } from "./common.js" import { filterManifests, getReleaseName, getValueArgs, prepareManifests, prepareTemplates } from "./common.js" @@ -17,7 +17,6 @@ import { getForwardablePorts, killPortForwards } from "../port-forward.js" import { getActionNamespace, getActionNamespaceStatus } from "../namespace.js" import { configureSyncMode } from "../sync.js" import { KubeApi } from "../api.js" -import type { ConfiguredLocalMode } from "../local-mode.js" import { configureLocalMode, startServiceInLocalMode } from "../local-mode.js" import type { DeployActionHandler } from "../../../plugin/action-types.js" import type { HelmDeployAction } from "./config.js" @@ -25,6 +24,7 @@ import { isEmpty } from "lodash-es" import { getK8sIngresses } from "../status/ingress.js" import { toGardenError } from "../../../exceptions.js" import { upsertConfigMap } from "../util.js" +import type { SyncableResource } from "../types.js" export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async (params) => { const { ctx, action, log, force } = params @@ -54,6 +54,7 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async const timeout = action.getConfig("timeout") const commonArgs = [ + "--wait", "--namespace", namespace, "--timeout", @@ -62,7 +63,7 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async ] if (spec.atomic) { - // Make sure chart gets purged if it fails to install + // Make sure chart gets purged if it fails to install. Note: --atomic implies --wait. commonArgs.push("--atomic") } @@ -131,41 +132,50 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async // Because we need to modify the Deployment, and because there is currently no reliable way to do that before // installing/upgrading via Helm, we need to separately update the target here for sync-mode/local-mode. // Local mode always takes precedence over sync mode. - let configuredLocalMode: ConfiguredLocalMode | undefined = undefined + let deployedWithLocalMode = false + let updatedManifests: SyncableResource[] = [] if (mode === "local" && spec.localMode && !isEmpty(spec.localMode)) { - configuredLocalMode = await configureLocalMode({ - ctx, - spec: spec.localMode, - defaultTarget: spec.defaultTarget, - manifests, - action, - log, - }) - await apply({ log, ctx, api, provider, manifests: configuredLocalMode.updated, namespace }) + updatedManifests = ( + await configureLocalMode({ + ctx, + spec: spec.localMode, + defaultTarget: spec.defaultTarget, + manifests, + action, + log, + }) + ).updated + await apply({ log, ctx, api, provider, manifests: updatedManifests, namespace }) + deployedWithLocalMode = true } else if (mode === "sync" && spec.sync && !isEmpty(spec.sync)) { - const configured = await configureSyncMode({ + updatedManifests = ( + await configureSyncMode({ + ctx, + log, + provider, + action, + defaultTarget: spec.defaultTarget, + manifests, + spec: spec.sync, + }) + ).updated + await apply({ log, ctx, api, provider, manifests: updatedManifests, namespace }) + } + + // FIXME: we should get these resources from the cluster, and not use the manifests from the local `helm template` + // command, because they may be legitimately inconsistent. + if (updatedManifests.length) { + await waitForResources({ + namespace, ctx, - log, provider, - action, - defaultTarget: spec.defaultTarget, - manifests, - spec: spec.sync, + actionName: action.key(), + resources: updatedManifests, // We only wait for manifests updated for local / sync mode. + log, + timeoutSec: timeout, }) - await apply({ log, ctx, api, provider, manifests: configured.updated, namespace }) } - - // FIXME: we should get these objects from the cluster, and not from the local `helm template` command, because - // they may be legitimately inconsistent. - const statuses = await waitForResources({ - namespace, - ctx, - provider, - actionName: action.key(), - resources: manifests, - log, - timeoutSec: timeout, - }) + const statuses = await checkResourceStatuses({ api, namespace, manifests, log }) const forwardablePorts = getForwardablePorts({ resources: manifests, parentAction: action, mode }) @@ -173,11 +183,11 @@ export const helmDeploy: DeployActionHandler<"deploy", HelmDeployAction> = async killPortForwards(action, forwardablePorts || [], log) // Local mode always takes precedence over sync mode. - if (mode === "local" && spec.localMode && configuredLocalMode && configuredLocalMode.updated?.length) { + if (mode === "local" && spec.localMode && deployedWithLocalMode && updatedManifests.length) { await startServiceInLocalMode({ ctx, spec: spec.localMode, - targetResource: configuredLocalMode.updated[0], + targetResource: updatedManifests[0], manifests, action, namespace, diff --git a/core/src/plugins/kubernetes/status/status.ts b/core/src/plugins/kubernetes/status/status.ts index e19099fa70..272b0ccbfb 100644 --- a/core/src/plugins/kubernetes/status/status.ts +++ b/core/src/plugins/kubernetes/status/status.ts @@ -166,13 +166,19 @@ const objHandlers: { [kind: string]: StatusHandler } = { /** * Check if the specified Kubernetes objects are deployed and fully rolled out */ -export async function checkResourceStatuses( - api: KubeApi, - namespace: string, - manifests: KubernetesResource[], - log: Log, +export async function checkResourceStatuses({ + api, + namespace, + manifests, + log, + waitForJobs, +}: { + api: KubeApi + namespace: string + manifests: KubernetesResource[] + log: Log waitForJobs?: boolean -): Promise { +}): Promise { return Promise.all( manifests.map(async (manifest) => { return checkResourceStatus({ api, namespace, manifest, log, waitForJobs }) @@ -310,7 +316,13 @@ export async function waitForResources({ await sleep(2000 + 500 * loops) loops += 1 - const statuses = await checkResourceStatuses(api, namespace, Object.values(pendingResources), log, waitForJobs) + const statuses = await checkResourceStatuses({ + api, + namespace, + manifests: Object.values(pendingResources), + log, + waitForJobs, + }) for (const status of statuses) { const key = getResourceKey(status.resource)