Skip to content

Commit

Permalink
improvement(k8s): less verbose Run/Test errors (#4894)
Browse files Browse the repository at this point in the history
We now only log the full pod status on the debug log level and above
when a Test or Run fails.

This makes e.g. logs for failing Tests much less verbose by default,
which should be a UX improvement.
  • Loading branch information
thsig authored Aug 1, 2023
1 parent 1b511dc commit ea40c01
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 12 deletions.
5 changes: 5 additions & 0 deletions core/src/plugin/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ export interface RunResult {
startedAt: Date
completedAt: Date
log: string
diagnosticErrorMsg?: string
namespaceStatus?: NamespaceStatus
}

Expand All @@ -131,6 +132,10 @@ export const runResultSchema = createSchema({
startedAt: joi.date().required().description("When the module run was started."),
completedAt: joi.date().required().description("When the module run was completed."),
log: joi.string().allow("").default("").description("The output log from the run."),
diagnosticErrorMsg: joi
.string()
.optional()
.description("An optional, more detailed diagnostic error message from the plugin."),
}),
allowUnknown: true,
})
Expand Down
29 changes: 17 additions & 12 deletions core/src/plugins/kubernetes/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1236,6 +1236,14 @@ export class PodRunner extends PodRunnerParams {
throw err
}

function renderDiagnosticErrorMessage(error: KnownError): string | undefined {
if (error.type === "pod-runner" && error.detail.podStatus) {
return `PodStatus:\n${stringify(error.detail.podStatus, null, 2)}`
} else {
return undefined
}
}

function renderError(error: KnownError): string {
const errorDetail = error.detail
const logs = errorDetail.logs
Expand All @@ -1253,7 +1261,6 @@ export class PodRunner extends PodRunnerParams {

const containerState = errorDetail.containerStatus?.state
const terminatedContainerState = containerState?.terminated
const podStatus = errorDetail.podStatus

if (!!terminatedContainerState) {
let terminationDesc = ""
Expand All @@ -1263,21 +1270,18 @@ export class PodRunner extends PodRunnerParams {
if (!!terminatedContainerState.signal) {
terminationDesc += `Stopped with signal: ${terminatedContainerState.signal}. `
}
terminationDesc += `Reason: ${terminatedContainerState.reason || "unknown"}. `
terminationDesc += `Message: ${terminatedContainerState.message || "n/a"}.`
if (terminatedContainerState.reason) {
terminationDesc += `Reason: ${terminatedContainerState.reason}. `
}
if (terminatedContainerState.message) {
terminationDesc += `Message: ${terminatedContainerState.message}.`
}
terminationDesc = terminationDesc.trim()

if (!!terminationDesc) {
errorDesc += terminationDesc + "\n\n"
}
}
// Print Pod status if case of too generic and non-informative error in the terminated state
if (!terminatedContainerState?.message && terminatedContainerState?.reason === "Error") {
if (!!podStatus) {
const podStatusDesc = "PodStatus:\n" + stringify(podStatus, null, 2)
errorDesc += podStatusDesc + "\n\n"
}
}

if (!!logs) {
errorDesc += `Here are the logs until the error occurred:\n\n${logs}`
Expand Down Expand Up @@ -1305,16 +1309,17 @@ export class PodRunner extends PodRunnerParams {
}
}

const log = renderError(err)
return {
log,
log: renderError(err),
diagnosticErrorMsg: renderDiagnosticErrorMessage(err),
moduleName,
version,
success: false,
startedAt,
completedAt: new Date(),
command,
exitCode: err.detail.exitCode,
errorDetail: err.detail,
}
}
}
3 changes: 3 additions & 0 deletions core/src/tasks/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,9 @@ export class RunTask extends ExecuteActionTask<RunAction, GetRunResult> {
taskLog.success(`Done`)
} else {
taskLog.error(`Failed!`)
if (status.detail?.diagnosticErrorMsg) {
this.log.debug(`Additional context for the error:\n\n${status.detail.diagnosticErrorMsg}`)
}
throw new RunTaskError(status.detail?.log)
}

Expand Down
3 changes: 3 additions & 0 deletions core/src/tasks/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,9 @@ export class TestTask extends ExecuteActionTask<TestAction, GetTestResult> {
const exitCode = status.detail?.exitCode
const failedMsg = !!exitCode ? `Failed with code ${exitCode}!` : `Failed!`
this.log.error(failedMsg)
if (status.detail?.diagnosticErrorMsg) {
this.log.debug(`Additional context for the error:\n\n${status.detail.diagnosticErrorMsg}`)
}
throw new TestError(status.detail?.log)
}

Expand Down
12 changes: 12 additions & 0 deletions docs/reference/commands.md
Original file line number Diff line number Diff line change
Expand Up @@ -3340,6 +3340,9 @@ actions:
# The output log from the run.
log:
# An optional, more detailed diagnostic error message from the plugin.
diagnosticErrorMsg:
# A map of statuses for each configured Test.
Test:
<name>:
Expand Down Expand Up @@ -3370,6 +3373,9 @@ actions:
# The output log from the run.
log:
# An optional, more detailed diagnostic error message from the plugin.
diagnosticErrorMsg:
```

### garden get actions
Expand Down Expand Up @@ -3737,6 +3743,9 @@ detail:
# The output log from the run.
log:
# An optional, more detailed diagnostic error message from the plugin.
diagnosticErrorMsg:
# Local file paths to any exported artifacts from the Run's execution.
artifacts:
```
Expand Down Expand Up @@ -3789,6 +3798,9 @@ detail:
# The output log from the run.
log:
# An optional, more detailed diagnostic error message from the plugin.
diagnosticErrorMsg:
# Local file paths to any exported artifacts from the test run.
artifacts:
```
Expand Down

0 comments on commit ea40c01

Please sign in to comment.