Skip to content
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
15 changes: 15 additions & 0 deletions packages/k8s/src/hooks/run-container-step.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,12 @@ import { dirname } from 'path'
import {
createContainerStepPod,
deletePod,
describePodFailure,
execCpFromPod,
execCpToPod,
execPodStep,
getContainerTerminatedErrors,
getPodByName,
getPrepareJobTimeoutSeconds,
waitForPodPhases
} from '../k8s'
Expand Down Expand Up @@ -116,6 +119,18 @@ export async function runContainerStep(
fs.rmSync(runnerPath, { force: true })
}
} catch (error) {
try {
const pod = await getPodByName(podName)
const terminatedErrors = getContainerTerminatedErrors(pod)
if (terminatedErrors.length > 0) {
const details = await describePodFailure(podName)
Comment on lines +123 to +126

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Calling getPodByName(podName) and then describePodFailure(podName) results in two consecutive, redundant Kubernetes API calls to fetch the exact same Pod resource (since describePodFailure internally calls readPod to fetch the pod again). To optimize performance and avoid unnecessary API rate-limiting, consider refactoring describePodFailure to accept an optional pre-fetched V1Pod object, allowing you to reuse the pod instance fetched on line 123.

core.error(
`Pod ${podName} has unrecoverable container errors:\n${terminatedErrors.join('\n')}\n${details}`
)
}
} catch {
// Best-effort: pod may already be deleted or unreachable
}
core.error(`Failed to run container step: ${error}`)
throw error
} finally {
Expand Down
42 changes: 41 additions & 1 deletion packages/k8s/src/k8s/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -679,7 +679,8 @@ export const UNRECOVERABLE_WAITING_REASONS = new Set([
'ErrImagePull',
'InvalidImageName',
'CreateContainerConfigError',
'CreateContainerError'
'CreateContainerError',
'FailedMount'
])

// Pod *event* reasons (from the event stream, not container status) that
Expand All @@ -704,6 +705,12 @@ export const UNRECOVERABLE_EVENT_REASONS = new Set([
'FailedBinding'
])

export const UNRECOVERABLE_TERMINATED_REASONS = new Set([
'OOMKilled',
'Error',
'FailedPostStartHookError'
])

// Maximum number of Warning events to include in a pod's failure description so
// that the GitHub Actions log is not flooded.
const MAX_DIAGNOSTIC_EVENTS = 10
Expand Down Expand Up @@ -745,6 +752,21 @@ export function getUnrecoverableEventReasons(): Set<string> {
return reasons
}

export function getUnrecoverableTerminatedReasons(): Set<string> {
const extra = process.env['ACTIONS_RUNNER_K8S_UNRECOVERABLE_TERMINATED_REASONS']
if (!extra) {
return UNRECOVERABLE_TERMINATED_REASONS
}
const reasons = new Set(UNRECOVERABLE_TERMINATED_REASONS)
for (const reason of extra.split(',')) {
const trimmed = reason.trim()
if (trimmed) {
reasons.add(trimmed)
}
}
return reasons
}

export function getContainerErrors(pod: k8s.V1Pod): string[] {
const errors: string[] = []
const unrecoverableReasons = getUnrecoverableWaitingReasons()
Expand All @@ -764,6 +786,24 @@ export function getContainerErrors(pod: k8s.V1Pod): string[] {
return errors
}

export function getContainerTerminatedErrors(pod: k8s.V1Pod): string[] {
const errors: string[] = []
const unrecoverableReasons = getUnrecoverableTerminatedReasons()
const allStatuses = [
...(pod.status?.initContainerStatuses ?? []),
...(pod.status?.containerStatuses ?? [])
]
for (const cs of allStatuses) {
const terminated = cs.state?.terminated
if (terminated?.reason && unrecoverableReasons.has(terminated.reason)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

To prevent potential false positives where a container successfully exits with code 0 but is flagged due to a whitelisted termination reason, we should explicitly check that terminated.exitCode !== 0 before reporting it as an error. This also aligns with the logic used in describePodFailure (line 939).

Suggested change
if (terminated?.reason && unrecoverableReasons.has(terminated.reason)) {
if (terminated?.reason && terminated.exitCode !== 0 && unrecoverableReasons.has(terminated.reason)) {

const reason = ` ✗ container "${cs.name}": ${terminated.reason} (exit code ${terminated.exitCode})`
const detail = terminated.message ? `\n ${terminated.message}` : ''
errors.push(detail ? `${reason}${detail}` : reason)
}
}
return errors
}

// Inspects the pod's status.conditions for deterministic scheduling failures.
// This is the RBAC-free companion to getPodEventErrors: pod conditions are
// always readable without the optional 'events' list permission.
Expand Down
177 changes: 177 additions & 0 deletions packages/k8s/tests/run-container-step-terminated-test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
import * as k8s from '@kubernetes/client-node'
import * as core from '@actions/core'
import { runContainerStep } from '../src/hooks'
import * as k8sModule from '../src/k8s'
import { RunContainerStepArgs } from 'hooklib'

jest.mock('@actions/core', () => ({
debug: jest.fn(),
error: jest.fn(),
warning: jest.fn(),
info: jest.fn()
}))

function terminatedContainer(
name: string,
reason: string,
exitCode: number,
message?: string
): k8s.V1ContainerStatus {
return {
name,
state: { terminated: { reason, exitCode, message } }
} as k8s.V1ContainerStatus
}

function buildPodWithTerminated(
containerStatuses: k8s.V1ContainerStatus[]
): k8s.V1Pod {
return {
metadata: { name: 'test-step-pod' },
status: {
phase: 'Failed',
containerStatuses
}
} as k8s.V1Pod
}

function makeMinimalArgs(): RunContainerStepArgs {
return {
image: 'ubuntu:latest',
entryPoint: 'sh',
entryPointArgs: ['-c', 'echo test']
} as RunContainerStepArgs
}

describe('runContainerStep terminated error detection', () => {
let createStepPodSpy: jest.SpyInstance
let waitForPodPhasesSpy: jest.SpyInstance
let getPodByNameSpy: jest.SpyInstance
let getContainerTerminatedErrorsSpy: jest.SpyInstance
let describePodFailureSpy: jest.SpyInstance
let deletePodSpy: jest.SpyInstance
let coreErrorSpy: jest.SpyInstance

beforeEach(() => {
process.env['ACTIONS_RUNNER_KUBERNETES_NAMESPACE'] = 'default'
process.env['RUNNER_WORKSPACE'] = '/tmp/runner/_work/repo'
process.env['GITHUB_WORKSPACE'] = '/tmp/runner/_work/repo/repo'
process.env['ACTIONS_RUNNER_POD_NAME'] = 'test-runner-pod'

createStepPodSpy = jest
.spyOn(k8sModule, 'createContainerStepPod')
.mockResolvedValue({
metadata: { name: 'test-step-pod' }
} as k8s.V1Pod)

waitForPodPhasesSpy = jest
.spyOn(k8sModule, 'waitForPodPhases')
.mockRejectedValue(new Error('Pod test-step-pod has unrecoverable errors'))

getPodByNameSpy = jest
.spyOn(k8sModule, 'getPodByName')

getContainerTerminatedErrorsSpy = jest
.spyOn(k8sModule, 'getContainerTerminatedErrors')

describePodFailureSpy = jest
.spyOn(k8sModule, 'describePodFailure')
.mockResolvedValue('Pod status: Failed\nContainer details:\n ✗ container "job" terminated: OOMKilled (exit code 137)')

deletePodSpy = jest
.spyOn(k8sModule, 'deletePod')
.mockResolvedValue(undefined)

coreErrorSpy = jest.spyOn(core, 'error')
})

afterEach(() => {
jest.restoreAllMocks()
delete process.env['ACTIONS_RUNNER_KUBERNETES_NAMESPACE']
delete process.env['RUNNER_WORKSPACE']
delete process.env['GITHUB_WORKSPACE']
delete process.env['ACTIONS_RUNNER_POD_NAME']
})

it('detects OOMKilled and logs describePodFailure output', async () => {
getPodByNameSpy.mockResolvedValue(
buildPodWithTerminated([
terminatedContainer('job', 'OOMKilled', 137, 'The node was low on resource: memory')
])
)
getContainerTerminatedErrorsSpy.mockReturnValue([
' ✗ container "job": OOMKilled (exit code 137)\n The node was low on resource: memory'
])

await expect(runContainerStep(makeMinimalArgs())).rejects.toThrow()

expect(getPodByNameSpy).toHaveBeenCalledWith('test-step-pod')
expect(getContainerTerminatedErrorsSpy).toHaveBeenCalled()
expect(describePodFailureSpy).toHaveBeenCalledWith('test-step-pod')
expect(coreErrorSpy).toHaveBeenCalledWith(
expect.stringContaining('OOMKilled')
)
})

it('detects Error (exit non-zero) and logs describePodFailure output', async () => {
getPodByNameSpy.mockResolvedValue(
buildPodWithTerminated([
terminatedContainer('job', 'Error', 1)
])
)
getContainerTerminatedErrorsSpy.mockReturnValue([
' ✗ container "job": Error (exit code 1)'
])

await expect(runContainerStep(makeMinimalArgs())).rejects.toThrow()

expect(getPodByNameSpy).toHaveBeenCalledWith('test-step-pod')
expect(getContainerTerminatedErrorsSpy).toHaveBeenCalled()
expect(describePodFailureSpy).toHaveBeenCalledWith('test-step-pod')
expect(coreErrorSpy).toHaveBeenCalledWith(
expect.stringContaining('Error (exit code 1)')
)
})

it('detects FailedPostStartHookError and logs describePodFailure output', async () => {
getPodByNameSpy.mockResolvedValue(
buildPodWithTerminated([
terminatedContainer('job', 'FailedPostStartHookError', 137, 'postStart hook failed')
])
)
getContainerTerminatedErrorsSpy.mockReturnValue([
' ✗ container "job": FailedPostStartHookError (exit code 137)\n postStart hook failed'
])

await expect(runContainerStep(makeMinimalArgs())).rejects.toThrow()

expect(getPodByNameSpy).toHaveBeenCalledWith('test-step-pod')
expect(getContainerTerminatedErrorsSpy).toHaveBeenCalled()
expect(describePodFailureSpy).toHaveBeenCalledWith('test-step-pod')
expect(coreErrorSpy).toHaveBeenCalledWith(
expect.stringContaining('FailedPostStartHookError')
)
})

it('does not log terminated errors when pod has none', async () => {
getPodByNameSpy.mockResolvedValue(buildPodWithTerminated([]))
getContainerTerminatedErrorsSpy.mockReturnValue([])

await expect(runContainerStep(makeMinimalArgs())).rejects.toThrow()

expect(getPodByNameSpy).toHaveBeenCalledWith('test-step-pod')
expect(getContainerTerminatedErrorsSpy).toHaveBeenCalled()
expect(describePodFailureSpy).not.toHaveBeenCalled()
})

it('gracefully handles getPodByName failure', async () => {
getPodByNameSpy.mockRejectedValue(new Error('pod not found'))

await expect(runContainerStep(makeMinimalArgs())).rejects.toThrow()

expect(getPodByNameSpy).toHaveBeenCalledWith('test-step-pod')
expect(coreErrorSpy).toHaveBeenCalledWith(
expect.stringContaining('Failed to run container step')
)
})
})
Loading