From c2b387643814a8de75b898070fcfb1b960252fcf Mon Sep 17 00:00:00 2001 From: Mads Hartmann Date: Fri, 6 May 2022 18:37:22 +0000 Subject: [PATCH 1/3] Always log the error that caused the job to fail Previously the error would only be sent to Slack, and added to the span but wouldn't be visible in the actual Werft logs. This fixes that --- .werft/build.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.werft/build.ts b/.werft/build.ts index 99085caadb4277..56aef0c73d4b07 100644 --- a/.werft/build.ts +++ b/.werft/build.ts @@ -27,12 +27,12 @@ Tracing.initialize() message: err }) + console.log('Error', err) + if (context.Repository.ref === "refs/heads/main") { reportBuildFailureInSlack(context, err).catch((error: Error) => { console.error("Failed to send message to Slack", error) }); - } else { - console.log('Error', err) } // Explicitly not using process.exit as we need to flush tracing, see tracing.js From ea2d114568ab0e150d1b21ff64f69007adba09cf Mon Sep 17 00:00:00 2001 From: Mads Hartmann Date: Fri, 6 May 2022 19:20:17 +0000 Subject: [PATCH 2/3] Handle preview deployment failures on main more robustly Rather than relying on catching errors everywhere and using exit 0 in the implementation of preview environment deployments, this moves the try/catch all the way to the top. This ensure that preview deployment errors never breaks the main build. Using exit 0 is also not ideal as it means we might not flush traces, so getting rid of those is nice. --- .werft/build.ts | 12 +++- .../build/deploy-to-preview-environment.ts | 60 ++++--------------- 2 files changed, 23 insertions(+), 49 deletions(-) diff --git a/.werft/build.ts b/.werft/build.ts index 56aef0c73d4b07..01273df7995b57 100644 --- a/.werft/build.ts +++ b/.werft/build.ts @@ -60,6 +60,16 @@ async function run(context: any) { return } - await deployToPreviewEnvironment(werft, config) + try { + await deployToPreviewEnvironment(werft, config) + } catch (e) { + // We currently don't support concurrent deployments to the same preview environment. + // Until we do we don't want errors to mark the main build as failed. + if (config.mainBuild) { + return + } + throw e + } + await triggerIntegrationTests(werft, config, context.Owner) } diff --git a/.werft/jobs/build/deploy-to-preview-environment.ts b/.werft/jobs/build/deploy-to-preview-environment.ts index edb842610dddaf..a0ce78167c5fc8 100644 --- a/.werft/jobs/build/deploy-to-preview-environment.ts +++ b/.werft/jobs/build/deploy-to-preview-environment.ts @@ -142,10 +142,7 @@ export async function deployToPreviewEnvironment(werft: Werft, jobConfig: JobCon await installMetaCertificates(werft, jobConfig.repository.branch, withVM, 'default', PREVIEW_K3S_KUBECONFIG_PATH, vmSlices.COPY_CERT_MANAGER_RESOURCES) werft.done(vmSlices.COPY_CERT_MANAGER_RESOURCES) } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail(vmSlices.COPY_CERT_MANAGER_RESOURCES, err); - } - exec('exit 0') + werft.fail(vmSlices.COPY_CERT_MANAGER_RESOURCES, err); } // Deploying monitoring satellite to VM-based preview environments is currently best-effort. @@ -262,10 +259,7 @@ async function deployToDevWithInstaller(werft: Werft, jobConfig: JobConfig, depl } werft.done(installerSlices.CLEAN_ENV_STATE); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail(installerSlices.CLEAN_ENV_STATE, err); - } - exec('exit 0') + werft.fail(installerSlices.CLEAN_ENV_STATE, err); } if (!withVM) { @@ -278,10 +272,7 @@ async function deployToDevWithInstaller(werft: Werft, jobConfig: JobConfig, depl await installMetaCertificates(werft, jobConfig.repository.branch, jobConfig.withVM, namespace, CORE_DEV_KUBECONFIG_PATH, installerSlices.COPY_CERTIFICATES); werft.done(installerSlices.COPY_CERTIFICATES); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail(installerSlices.COPY_CERTIFICATES, err); - } - exec('exit 0') + werft.fail(installerSlices.COPY_CERTIFICATES, err); } } @@ -295,10 +286,7 @@ async function deployToDevWithInstaller(werft: Werft, jobConfig: JobConfig, depl exec(`kubectl --kubeconfig ${deploymentKubeconfig} create secret docker-registry ${IMAGE_PULL_SECRET_NAME} -n ${namespace} --from-file=.dockerconfigjson=./${IMAGE_PULL_SECRET_NAME}`, { slice: installerSlices.IMAGE_PULL_SECRET }); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail(installerSlices.IMAGE_PULL_SECRET, err); - } - exec('exit 0') + werft.fail(installerSlices.IMAGE_PULL_SECRET, err); } } werft.done(installerSlices.IMAGE_PULL_SECRET); @@ -338,10 +326,7 @@ async function deployToDevWithInstaller(werft: Werft, jobConfig: JobConfig, depl installer.postProcessing(installerSlices.INSTALLER_POST_PROCESSING) installer.install(installerSlices.APPLY_INSTALL_MANIFESTS) } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail(phases.DEPLOY, err) - } - exec('exit 0') + werft.fail(phases.DEPLOY, err) } werft.log(installerSlices.DEPLOYMENT_WAITING, "Waiting until all pods are ready."); @@ -410,10 +395,7 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen await addDNSRecord(werft, deploymentConfig.namespace, deploymentConfig.domain, false, CORE_DEV_KUBECONFIG_PATH) werft.done('prep'); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail('prep', err); - } - exec('exit 0') + werft.fail('prep', err); } // core-dev specific section start @@ -435,10 +417,7 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen }` ); werft.done('secret'); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail('secret', err); - } - exec('exit 0') + werft.fail('secret', err); } werft.log("authProviders", "copy authProviders") @@ -450,10 +429,7 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen exec(`yq merge --inplace .werft/jobs/build/helm/values.dev.yaml ./authProviders`, { slice: "authProviders" }) werft.done('authProviders'); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail('authProviders', err); - } - exec('exit 0') + werft.fail('authProviders', err); } // core-dev specific section end @@ -478,10 +454,7 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen }); await installMonitoringSatellite.install() } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail('observability', err); - } - exec('exit 0') + werft.fail('observability', err); } } else { exec(`echo '"with-observability" annotation not set, skipping...'`, { slice: `observability` }) @@ -500,10 +473,7 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen werft.log('helm', 'done'); werft.done('helm'); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail('deploy', err); - } - exec('exit 0') + werft.fail('deploy', err); } finally { // produce the result independently of Helm succeding, so that in case Helm fails we still have the URL. exec(`werft log result -d "dev installation" -c github-check-preview-env url ${url}/workspaces`); @@ -559,10 +529,7 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen try { exec(`docker run --rm eu.gcr.io/gitpod-core-dev/build/versions:${version} cat /versions.yaml | tee versions.yaml`); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail('helm', err); - } - exec('exit 0') + werft.fail('helm', err); } const pathToVersions = `${shell.pwd().toString()}/versions.yaml`; flags += ` -f ${pathToVersions}`; @@ -590,10 +557,7 @@ async function deployToDevWithHelm(werft: Werft, jobConfig: JobConfig, deploymen await deleteNonNamespaceObjects(namespace, destname, CORE_DEV_KUBECONFIG_PATH, { ...shellOpts, slice: 'predeploy cleanup' }); werft.done('predeploy cleanup'); } catch (err) { - if (!jobConfig.mainBuild) { - werft.fail('predeploy cleanup', err); - } - exec('exit 0') + werft.fail('predeploy cleanup', err); } } } From 9b83bff80338aab49b53d71dd99a4b1cf0313a16 Mon Sep 17 00:00:00 2001 From: Mads Hartmann Date: Fri, 6 May 2022 19:20:32 +0000 Subject: [PATCH 3/3] Don't end all spans when using .fail --- .werft/util/werft.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.werft/util/werft.ts b/.werft/util/werft.ts index d01017561ef569..523dc86e0e2007 100644 --- a/.werft/util/werft.ts +++ b/.werft/util/werft.ts @@ -70,9 +70,10 @@ export class Werft { * Use this when you intend to fail the werft job */ public fail(slice, err) { + const span = this.sliceSpans[slice]; // Set the status on the span for the slice and also propagate the status to the phase and root span // as well so we can query on all phases that had an error regardless of which slice produced the error. - [this.sliceSpans[slice], this.rootSpan, this.currentPhaseSpan].forEach((span: Span) => { + [span, this.rootSpan, this.currentPhaseSpan].forEach((span: Span) => { if (!span) { return } @@ -82,7 +83,7 @@ export class Werft { }) }) - this.endAllSpans() + span.end() console.log(`[${slice}|FAIL] ${err}`); throw err;