A-1547: Add --job-context-dir to configure job coordination file locations - #4142
Conversation
The agent creates several files to coordinate with the processes running a job: the job env files (BUILDKITE_ENV_FILE / BUILDKITE_ENV_JSON_FILE), the job timeout marker file (BUILDKITE_AGENT_JOB_TIMEOUT_FILE), and, in Kubernetes mode, the socket that kubernetes-bootstrap connects to. The directory for these was previously hardcoded: os.TempDir() normally, and /workspace in Kubernetes mode. This adds a --job-context-dir flag (BUILDKITE_JOB_CONTEXT_DIR) to both `agent start` and `kubernetes-bootstrap`. All coordination files are created in that directory, with the socket at a predictable name within it (buildkite.sock). Only the directory needs to be agreed upon between containers; the env/timeout file paths continue to flow to the other containers through the registration env. Defaults are unchanged (os.TempDir(), or /workspace under --kubernetes-exec), so existing stacks are unaffected. A stack can set BUILDKITE_JOB_CONTEXT_DIR pod-wide to a small dedicated shared volume, and drop the workspace mount from the agent container. The env var is included in kubernetes-bootstrap's existingEnvPriority so that each container's own value survives the registration-env overlay (the shared volume may be mounted at different paths per container), and in protectedEnv so job-level env cannot redirect it. Signed-off-by: Adam Eijdenberg <aeijdenberg@canva.com>
![buildsworth-bk-app[bot]](https://avatars.githubusercontent.com/in/3010600?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one blocking issue in the different-mount-path flow; details inline.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 9964, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
About buildsworth
Model: gpt-5.6-sol with xhigh thinking.
How to request a review: Comment @buildsworth-bk review on the PR, or request buildsworth-bk as a reviewer.
Risk labels (how buildsworth classifies risk) — buildsworth classifies risk itself from the diff. To let it approve, grant L2 approval by mentioning @buildsworth-bk (see L2 approval grant):
- L1 — Low risk (dep bumps, docs/copy, lockfiles, small presentational fixes). buildsworth may approve by default.
- L2 — Standard risk (new UI, additive API fields, refactors). Approved only with an L2 grant; otherwise comment-only.
- L3 — High risk (auth, migrations, payments, secrets, perf-critical paths). Human review always required.
2fe056b to
e49027a
Compare
e49027a to
fafc938
Compare
Follow-ups to the previous commit: Reuse the contextDir already computed in NewJobRunner for the Kubernetes socket path instead of recomputing it, and rename the tempDir parameters to contextDir since the directory is no longer necessarily the system temp dir. The registration env sends BUILDKITE_ENV_FILE, BUILDKITE_ENV_JSON_FILE and BUILDKITE_AGENT_JOB_TIMEOUT_FILE as absolute paths computed in the agent container's mount namespace. When the shared volume is mounted at a different path in a command container, those paths are unusable there. kubernetes-bootstrap now rebases them onto its own job context directory by base name, since the agent creates all of these files directly in its context directory.
fafc938 to
eebc5a1
Compare
zhming0
changed the title
Previous concerns have been addressed.
There was a problem hiding this comment.
The previous different-mount-path concern is fixed, and I found no new issues. Because this changes Kubernetes coordination behavior across containers, I’m leaving this as a comment rather than approving.
Want to dig deeper?
Paste this into your agent to explore the findings from this review's Buildkite build:
Download the buildsworth logs from build 10337, then answer my questions about the findings.
Install the reading-buildsworth-logs skill to run this.
|
|
||
| // tempDir is not guaranteed to exist | ||
| if _, err := os.Stat(tempDir); os.IsNotExist(err) { | ||
| func createJobEnvFiles(l logger.Logger, jobID, contextDir string) (shellFile, jsonFile *os.File, err error) { |
There was a problem hiding this comment.
Nit: because createJobEnvFiles creates an explicit context directory when absent, forgetting the shared-volume mount results in a directory and socket being created only in the agent container.
It just means users will take more time to debug a misconfiguration. Maybe worth adding some explicit logging?
There was a problem hiding this comment.
I think this behavior is pre-existing. If there is a misconfiguration the agent will fail loudly.
If are to add logging, I struggle to find wording that adds value, because creating the directory is not wrong in itself (it is normal outside Kubernetes mode). Something like "Creating job context directory because it does not exist" would log on healthy runs too, and since the job context directory is not a prominent concept, I suspect it would be more distracting than helpful.
3 participants
Adds a
--job-context-dirflag (BUILDKITE_JOB_CONTEXT_DIR) toagent startandkubernetes-bootstrapto configure the directory for job coordination files. Based on #4090 by @aeijdenberg-canva, thanks Adam for the contribution. Tracked as A-1547.Closes #4090.
Context
The agent and the processes running a job coordinate through a small set of files: the job env snapshot (
BUILDKITE_ENV_FILE/BUILDKITE_ENV_JSON_FILE), the job timeout marker (BUILDKITE_AGENT_JOB_TIMEOUT_FILE), and, in Kubernetes mode, the socket thatkubernetes-bootstrapconnects to (buildkite.sock). Their directory was hardcoded:/workspacein Kubernetes mode, the system temp dir otherwise.Who this is for
/workspacevolume that Kubernetes mode defaults to; leave the flag unset.--kubernetes-execorchestration: setBUILDKITE_JOB_CONTEXT_DIRon every container to the path where the shared volume is mounted in that container. This removes the last hardcoded/workspacein the agent binary, which can collide with images that claim that path.TMPDIR).Defaults are unchanged in all modes.
Changes
Two commits:
env/protected.goand the container exit status change inkubernetes/runner.gowere resolved during the rebase.kubernetes-bootstrapnow rebasesBUILDKITE_ENV_FILE,BUILDKITE_ENV_JSON_FILEandBUILDKITE_AGENT_JOB_TIMEOUT_FILEonto its own context directory. The agent sends these as absolute paths computed in its own mount namespace, so a container mounting the shared volume at a different path would connect to the socket fine and then fail on unreachable env file paths.contextDircomputed inNewJobRunnerfor the socket path, and rename thetempDirparameters tocontextDirsince the directory is no longer necessarily the system temp dir.existingEnvPrioritycomment (agent-stack-k8s does not setBUILDKITE_JOB_CONTEXT_DIR).Note on scope: mounting the shared volume at the same path in every container remains the supported configuration, since
BUILDKITE_BUILD_PATHalso crosses containers as an absolute path. The env file rebasing is defence in depth and a no-op when paths match.Testing
TestRebaseJobContextPathscovers differing mount paths, same path no-op, and absent varsgo test ./...)go tool gofumpt -extra -w .) andgolangci-lint runreports no issues on the touched packages