Repository navigation
chore(ci): optimize DinD startup - #551
Conversation
📝 WalkthroughWalkthroughShort CI change: gateway-e2e matrix no longer excludes Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Kubernetes RunnerDeployment configuration to support GitHub Actions runners on H100 GPU nodes. This new deployment enables the provisioning of ephemeral runners with specific resource allocations, node affinities, and a Docker-in-Docker setup, facilitating advanced CI/CD workflows requiring GPU acceleration and containerization. Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new Kubernetes RunnerDeployment resource named arc-runner-gpu-h100-test. While it includes some improvements like resource requests for the docker sidecar, a significant security concern remains regarding the use of privileged containers, which creates a high risk of cluster-wide compromise and host node escape in a CI/CD environment. Furthermore, there are significant gaps in resource management, as the main runner container lacks CPU and memory limits, and the docker container is missing a CPU limit. Specific comments have been provided to address these security and resource management concerns.
…0 test environment - Changed runner name to '4-gpu-h100-test' in the CI workflow. - Added a new RunnerDeployment for 'arc-runner-gpu-h100-test' with specific resource configurations in the Kubernetes setup. - Commented out several test configurations for clarity and future reference. Signed-off-by: key4ng <rukeyang@gmail.com>
- Removed the ignore option for 'test_pd_perf.py' in the CI workflow, allowing it to run alongside other benchmarks. - Maintained existing configurations for Go and nightly benchmarks. Signed-off-by: key4ng <rukeyang@gmail.com>
- Updated the CI workflow to include detailed configurations for various test scenarios, including agentic APIs and chat completions. - Restored previously commented-out test configurations for better clarity and future execution. - Adjusted the runner name back to '4-gpu-h100' for consistency with the updated deployment. Signed-off-by: key4ng <rukeyang@gmail.com>
- Increased CPU limit to 2 for the GPU runner in the Kubernetes setup to enhance performance. - Maintained existing memory limits for consistency. Signed-off-by: key4ng <rukeyang@gmail.com>
57ad8f8 to
3ad4baf
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ad4baf91c
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| limits: | ||
| cpu: "2" | ||
| memory: "4Gi" |
There was a problem hiding this comment.
The new docker-storage volume is a memory-backed emptyDir with an 8Gi limit, but the DinD container is capped at memory: "4Gi"; for memory-backed emptyDir, writes are charged to the writing container, so Docker image/layer data in /var/lib/docker will hit the 4Gi cgroup limit first and can OOM-kill dockerd during larger pulls/builds. This makes the intended 8Gi cache unusable and can cause benchmark jobs on 4-gpu-h100 runners to fail as soon as Docker state exceeds ~4Gi.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/k8s-runner-resources/arc-runner-gpu.yaml (1)
138-139: 🧹 Nitpick | 🔵 TrivialConsider applying consistent optimizations to the A10 runner.
The H100 runner now has tmpfs-backed docker-storage, explicit resource constraints, and
DOCKER_DRIVER=overlay2, while the A10 runner retains the original configuration. If these optimizations prove effective, consider applying them to the A10 runner for consistency and similar performance benefits.Also applies to: 161-171
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml` around lines 138 - 139, Update the A10 runner's pod spec to mirror the H100 optimizations: change the docker-storage volume from an emptyDir to a tmpfs-backed emptyDir (set emptyDir.medium to "Memory"), add the same container resource requests/limits entries (cpu/memory limits and requests) used in the H100 runner to the A10 runner's container spec, and add the environment variable DOCKER_DRIVER=overlay2 to the container env list so the A10 runner uses the same storage driver. Ensure you target the docker-storage volume name and the A10 runner container spec when applying these changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml`:
- Around line 46-49: The tmpfs emptyDir named docker-storage is configured with
medium: Memory and sizeLimit: 8Gi which can exceed the pod's docker container
memory limit; update the Kubernetes spec so the memory-backed tmpfs cannot cause
OOMs by either increasing the docker container memory limit to at least 12Gi (to
accommodate the 8Gi tmpfs plus workload) or reducing docker-storage's sizeLimit
to a value that fits within the current container memory limit (e.g., <=4Gi);
locate the docker-storage emptyDir block and the container resource limits (the
docker container memory limit) and make the corresponding change to ensure tmpfs
size + container usage stay below the memory limit.
---
Outside diff comments:
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml`:
- Around line 138-139: Update the A10 runner's pod spec to mirror the H100
optimizations: change the docker-storage volume from an emptyDir to a
tmpfs-backed emptyDir (set emptyDir.medium to "Memory"), add the same container
resource requests/limits entries (cpu/memory limits and requests) used in the
H100 runner to the A10 runner's container spec, and add the environment variable
DOCKER_DRIVER=overlay2 to the container env list so the A10 runner uses the same
storage driver. Ensure you target the docker-storage volume name and the A10
runner container spec when applying these changes.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/pr-test-rust.ymlscripts/k8s-runner-resources/arc-runner-gpu.yaml
- Decreased the memory size limit from 8Gi to 4Gi for the GPU runner in the Kubernetes setup to optimize resource usage. Signed-off-by: key4ng <rukeyang@gmail.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
scripts/k8s-runner-resources/arc-runner-gpu.yaml (1)
47-49:⚠️ Potential issue | 🟠 MajorLeave headroom between tmpfs cap and DinD memory limit.
On Lines 47-49 and Line 81,
docker-storagetmpfs is capped at4Giwhile the docker container memory limit is also4Gi. This leaves effectively zero headroom fordockerd/runtime memory and can still cause OOM under pull/extract spikes.Suggested fix (pick one)
emptyDir: medium: Memory - sizeLimit: 4Gi + sizeLimit: 3Gilimits: cpu: "2" - memory: "4Gi" + memory: "6Gi"#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path import re, sys path = Path("scripts/k8s-runner-resources/arc-runner-gpu.yaml") text = path.read_text() h100 = text.split("\n---\n", 1)[0] lines = h100.splitlines() def parse_gi(v: str) -> int: m = re.fullmatch(r'"?(\d+)Gi"?', v.strip()) if not m: raise ValueError(f"Unexpected Gi format: {v}") return int(m.group(1)) size_limit = None mem_limit = None # find docker-storage sizeLimit in H100 block for i, line in enumerate(lines): if re.match(r'^\s*-\s*name:\s*docker-storage\s*$', line): for j in range(i + 1, min(i + 12, len(lines))): m = re.match(r'^\s*sizeLimit:\s*("?[\d]+Gi"?)\s*$', lines[j]) if m: size_limit = parse_gi(m.group(1)) break break # find docker container memory limit in H100 block for i, line in enumerate(lines): if re.match(r'^\s*-\s*name:\s*docker\s*$', line): for j in range(i + 1, min(i + 40, len(lines))): m = re.match(r'^\s*memory:\s*("?[\d]+Gi"?)\s*$', lines[j]) if m: mem_limit = parse_gi(m.group(1)) break print(f"tmpfs sizeLimit: {size_limit}Gi") print(f"docker memory limit: {mem_limit}Gi") if size_limit is None or mem_limit is None: print("Unable to parse one or both values.") sys.exit(2) if size_limit >= mem_limit: print("FAIL: tmpfs cap >= memory limit (no headroom).") sys.exit(1) print("PASS: tmpfs cap leaves headroom.") PYAlso applies to: 79-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml` around lines 47 - 49, The docker-storage tmpfs sizeLimit is set equal to the docker container memory limit (both 4Gi), leaving no headroom; update the tmpfs configuration (the - name: docker-storage entry's sizeLimit) so it is strictly less than the docker container memory limit (the - name: docker entry's memory field) — either reduce sizeLimit (e.g., to 3Gi) or increase the docker container memory limit accordingly in both occurrences (the H100 block and the other block around lines ~79-81) so tmpfs < container memory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml`:
- Around line 47-49: The docker-storage tmpfs sizeLimit is set equal to the
docker container memory limit (both 4Gi), leaving no headroom; update the tmpfs
configuration (the - name: docker-storage entry's sizeLimit) so it is strictly
less than the docker container memory limit (the - name: docker entry's memory
field) — either reduce sizeLimit (e.g., to 3Gi) or increase the docker container
memory limit accordingly in both occurrences (the H100 block and the other block
around lines ~79-81) so tmpfs < container memory.
2 participants
Description
Problem
Docker-in-Docker (DinD) containers on H100 GPU runners start slowly due to using the node's root disk for Docker storage, missing resource guarantees, and relying on auto-detected storage drivers.
Solution
Three DinD optimizations are applied to the H100 runner deployment:
Tmpfs-backed Docker storage (
medium: Memory, 8Gi limit): Moves/var/lib/dockerfrom the node's root disk (often slow network-attached or spinning storage on bare-metal GPU nodes) to an in-memory filesystem, eliminating disk I/O for image pulls, layer extraction, and container creation.Explicit resource requests/limits (1 CPU / 2Gi request, 4Gi memory limit): Guarantees the DinD sidecar gets scheduled with sufficient CPU and memory, preventing starvation during Docker daemon initialization when competing with GPU workloads on the same node.
Explicit
DOCKER_DRIVER=overlay2: Skips the storage driver auto-detection phase at startup and avoids potential fallback to thevfsdriver, which performs full layer copies instead of using copy-on-write.Additionally,
test_pd_perf.pyis included in PR benchmark runs for broader benchmark coverage alongsidetest_regular_perf.py.Changes
scripts/k8s-runner-resources/arc-runner-gpu.yaml: Optimizearc-runner-gpu-h100DinD — switchdocker-storageto tmpfs (8Gi), add resource requests/limits to Docker container, setDOCKER_DRIVER=overlay2.github/workflows/pr-test-rust.yml: Removetest_pd_perf.pyfrom benchmark ignore list to include PD benchmarks in PR runsTest Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Release Notes