Skip to content

Commit

Permalink
Merge remote-tracking branch 'upstream/main'
Browse files Browse the repository at this point in the history
  • Loading branch information
benbz committed Apr 12, 2024
2 parents d0b4d61 + 9705dee commit eb68b52
Show file tree
Hide file tree
Showing 14 changed files with 171 additions and 62 deletions.
4 changes: 3 additions & 1 deletion docs/adrs/0096-hook-extensions.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

**Date:** 3 August 2023

**Status**: Accepted <!--Accepted|Rejected|Superceded|Deprecated-->
**Status**: Superceded [^1]

## Context

Expand Down Expand Up @@ -30,3 +30,5 @@ In case the hook is able to read the extended spec, it will first create a defau
## Consequences

The addition of hook extensions will provide a better user experience for users who need to customize the pods created by the container hook. However, it will require additional effort to provide the template to the runner pod, and configure it properly.

[^1]: Superseded by [ADR 0134](0134-hook-extensions.md)
41 changes: 41 additions & 0 deletions docs/adrs/0134-hook-extensions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# ADR 0134: Hook extensions

**Date:** 20 February 2024

**Status**: Accepted [^1]

## Context

The current implementation of container hooks does not allow users to customize the pods created by the hook.
While the implementation is designed to be used as is or as a starting point, building and maintaining a custom hook implementation just to specify additional fields is not a good user experience.

## Decision

We have decided to add hook extensions to the container hook implementation.
This will allow users to customize the pods created by the hook by specifying additional fields.
The hook extensions will be implemented in a way that is backwards-compatible with the existing hook implementation.

To allow customization, the runner executing the hook should have `ACTIONS_RUNNER_CONTAINER_HOOK_TEMPLATE` environment variable pointing to a yaml file on the runner system.
The extension specified in that file will be applied both for job pods, and container steps.

If environment variable is set, but the file can't be read, the hook will fail, signaling incorrect configuration.

If the environment variable does not exist, the hook will apply the default spec.

In case the hook is able to read the extended spec, it will first create a default configuration, and then merged modified fields in the following way:

1. The `.metadata` fields that will be appended if they are not reserved are `labels` and `annotations`.
2. The pod spec fields except for `containers` and `volumes` are applied from the template, possibly overwriting the field.
3. The volumes are applied in form of appending additional volumes to the default volumes.
4. The containers are merged based on the name assigned to them:
1. If the name of the container *is* "$job", the `name` and the `image` fields are going to be ignored and the spec will be applied so that `env`, `volumeMounts`, `ports` are appended to the default container spec created by the hook, while the rest of the fields are going to be applied to the newly created container spec.
2. If the name of the container *starts with* "$", and matches the name of the [container service](https://docs.github.com/en/actions/using-containerized-services/about-service-containers), the `name` and the `image` fields are going to be ignored and the spec will be applied to that service container, so that `env`, `volumeMounts`, `ports` are appended to the default container spec for service created by the hook, while the rest of the fields are going to be applied to the created container spec.
If there is no container service with such name defined in the workflow, such spec extension will be ignored.
3. If the name of the container *does not start with* "$", the entire spec of the container will be added to the pod definition.

## Consequences

The addition of hook extensions will provide a better user experience for users who need to customize the pods created by the container hook.
However, it will require additional effort to provide the template to the runner pod, and configure it properly.

[^1]: Supersedes [ADR 0096](0096-hook-extensions.md)
21 changes: 16 additions & 5 deletions examples/extension.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ spec:
runAsGroup: 3000
restartPolicy: Never
containers:
- name: $job # overwirtes job container
- name: $job # overwrites job container
env:
- name: ENV1
value: "value1"
Expand All @@ -20,11 +20,22 @@ spec:
args:
- -c
- sleep 50
- name: $redis # overwrites redis service
env:
- name: ENV2
value: "value2"
image: "busybox:1.28" # Ignored
resources:
requests:
memory: "1Mi"
cpu: "1"
limits:
memory: "1Gi"
cpu: "2"
- name: side-car
image: "ubuntu:latest" # required
command:
- sh
- sh
args:
- -c
- sleep 60

- -c
- sleep 60
16 changes: 8 additions & 8 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "hooks",
"version": "0.5.0-element",
"version": "0.6.0-element",
"description": "Three projects are included - k8s: a kubernetes hook implementation that spins up pods dynamically to run a job - docker: A hook implementation of the runner's docker implementation - A hook lib, which contains shared typescript definitions and utilities that the other packages consume",
"main": "",
"directories": {
Expand Down
12 changes: 6 additions & 6 deletions packages/k8s/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/k8s/src/hooks/constants.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ export function getSecretName(): string {

export const MAX_POD_NAME_LENGTH = 63
export const STEP_POD_NAME_SUFFIX_LENGTH = 8
export const CONTAINER_EXTENSION_PREFIX = '$'
export const JOB_CONTAINER_NAME = 'job'
export const JOB_CONTAINER_EXTENSION_NAME = '$job'

Expand Down
6 changes: 3 additions & 3 deletions packages/k8s/src/hooks/prepare-job.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import {
PodPhase,
fixArgs
} from '../k8s/utils'
import { JOB_CONTAINER_EXTENSION_NAME, JOB_CONTAINER_NAME } from './constants'
import { CONTAINER_EXTENSION_PREFIX, JOB_CONTAINER_NAME } from './constants'

export async function prepareJob(
args: PrepareJobArgs,
Expand Down Expand Up @@ -60,7 +60,7 @@ export async function prepareJob(
service,
generateContainerName(service.image),
false,
undefined
extension
)
})
}
Expand Down Expand Up @@ -255,7 +255,7 @@ export function createContainerSpec(
}

const from = extension.spec?.containers?.find(
c => c.name === JOB_CONTAINER_EXTENSION_NAME
c => c.name === CONTAINER_EXTENSION_PREFIX + name
)

if (from) {
Expand Down
13 changes: 9 additions & 4 deletions packages/k8s/src/hooks/run-container-step.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,10 @@ import {
} from '../k8s'
import {
containerVolumes,
PodPhase,
fixArgs,
mergeContainerWithOptions,
readExtensionFromFile,
fixArgs
PodPhase,
readExtensionFromFile
} from '../k8s/utils'
import { JOB_CONTAINER_EXTENSION_NAME, JOB_CONTAINER_NAME } from './constants'

Expand Down Expand Up @@ -65,7 +65,12 @@ export async function runContainerStep(

await waitForPodPhases(
podName,
new Set([PodPhase.COMPLETED, PodPhase.RUNNING, PodPhase.SUCCEEDED]),
new Set([
PodPhase.COMPLETED,
PodPhase.RUNNING,
PodPhase.SUCCEEDED,
PodPhase.FAILED
]),
new Set([PodPhase.PENDING, PodPhase.UNKNOWN])
)
core.debug('Container step is running or complete, pulling logs')
Expand Down
57 changes: 32 additions & 25 deletions packages/k8s/src/k8s/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ import {
PodPhase,
mergePodSpecWithOptions,
mergeObjectMeta,
useKubeScheduler
useKubeScheduler,
fixArgs
} from './utils'

const kc = new k8s.KubeConfig()
Expand Down Expand Up @@ -226,31 +227,37 @@ export async function execPodStep(
stdin?: stream.Readable
): Promise<void> {
const exec = new k8s.Exec(kc)
await new Promise(async function (resolve, reject) {
await exec.exec(
namespace(),
podName,
containerName,
command,
process.stdout,
process.stderr,
stdin ?? null,
false /* tty */,
resp => {
// kube.exec returns an error if exit code is not 0, but we can't actually get the exit code
if (resp.status === 'Success') {
resolve(resp.code)
} else {
core.debug(
JSON.stringify({
message: resp?.message,
details: resp?.details
})
)
reject(resp?.message)
command = fixArgs(command)
// Exec returns a websocket. If websocket fails, we should reject the promise. Otherwise, websocket will call a callback. Since at that point, websocket is not failing, we can safely resolve or reject the promise.
await new Promise(function (resolve, reject) {
exec
.exec(
namespace(),
podName,
containerName,
command,
process.stdout,
process.stderr,
stdin ?? null,
false /* tty */,
resp => {
// kube.exec returns an error if exit code is not 0, but we can't actually get the exit code
if (resp.status === 'Success') {
resolve(resp.code)
} else {
core.debug(
JSON.stringify({
message: resp?.message,
details: resp?.details
})
)
reject(resp?.message)
}
}
}
)
)
// If exec.exec fails, explicitly reject the outer promise
// eslint-disable-next-line github/no-then
.catch(e => reject(e))
})
}

Expand Down
13 changes: 10 additions & 3 deletions packages/k8s/src/k8s/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import { Mount } from 'hooklib'
import * as path from 'path'
import { v1 as uuidv4 } from 'uuid'
import { POD_VOLUME_NAME } from './index'
import { JOB_CONTAINER_EXTENSION_NAME } from '../hooks/constants'
import { CONTAINER_EXTENSION_PREFIX } from '../hooks/constants'
import * as shlex from 'shlex'

export const DEFAULT_CONTAINER_ENTRY_POINT_ARGS = [`-f`, `/dev/null`]
Expand Down Expand Up @@ -41,6 +41,11 @@ export function containerVolumes(
name: POD_VOLUME_NAME,
mountPath: '/github/file_commands',
subPath: '_temp/_runner_file_commands'
},
{
name: POD_VOLUME_NAME,
mountPath: '/github/workflow',
subPath: '_temp/_github_workflow'
}
)
return mounts
Expand Down Expand Up @@ -180,7 +185,7 @@ export function mergeContainerWithOptions(
): void {
for (const [key, value] of Object.entries(from)) {
if (key === 'name') {
if (value !== base.name && value !== JOB_CONTAINER_EXTENSION_NAME) {
if (value !== CONTAINER_EXTENSION_PREFIX + base.name) {
core.warning("Skipping name override: name can't be overwritten")
}
continue
Expand Down Expand Up @@ -209,7 +214,9 @@ export function mergePodSpecWithOptions(
for (const [key, value] of Object.entries(from)) {
if (key === 'containers') {
base.containers.push(
...from.containers.filter(e => !e.name?.startsWith('$'))
...from.containers.filter(
e => !e.name?.startsWith(CONTAINER_EXTENSION_PREFIX)
)
)
} else if (key === 'volumes' && value) {
const volumes = value as k8s.V1Volume[]
Expand Down
17 changes: 15 additions & 2 deletions packages/k8s/tests/k8s-utils-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,20 @@ describe('k8s utils', () => {
expect(volumes.find(e => e.mountPath === '/__w')).toBeTruthy()
})

it('should always have /github/workflow mount if working on container job or container action', () => {
let volumes = containerVolumes([], true, true)
expect(volumes.find(e => e.mountPath === '/github/workflow')).toBeTruthy()
volumes = containerVolumes([], true, false)
expect(volumes.find(e => e.mountPath === '/github/workflow')).toBeTruthy()
volumes = containerVolumes([], false, true)
expect(volumes.find(e => e.mountPath === '/github/workflow')).toBeTruthy()

volumes = containerVolumes([], false, false)
expect(
volumes.find(e => e.mountPath === '/github/workflow')
).toBeUndefined()
})

it('should have container action volumes', () => {
let volumes = containerVolumes([], true, true)
let workspace = volumes.find(e => e.mountPath === '/github/workspace')
Expand All @@ -205,11 +219,10 @@ describe('k8s utils', () => {
expect(fileCommands?.subPath).toBe('_temp/_runner_file_commands')
})

it('should have externals, github home and github workflow mounts if job container', () => {
it('should have externals, github home mounts if job container', () => {
const volumes = containerVolumes()
expect(volumes.find(e => e.mountPath === '/__e')).toBeTruthy()
expect(volumes.find(e => e.mountPath === '/github/home')).toBeTruthy()
expect(volumes.find(e => e.mountPath === '/github/workflow')).toBeTruthy()
})

it('should throw if user volume source volume path is not in workspace', () => {
Expand Down
Loading

0 comments on commit eb68b52

Please sign in to comment.