Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

improvement(k8s): less verbose Run/Test errors #4894

Merged
merged 1 commit into from
Aug 1, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't seem to read this explicitly in the caller-side code. But this will be printed to the error.log if the RunResult is not successful, right? Then it's definitely good to have more details in the logs.

}
}
}
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