Repository navigation
revert(ci): use NVMe storage for H100 runner workspaces - #1704
Conversation
github-actions
Bot
added
the
documentation
📝 WalkthroughWalkthroughSwitches H100 runner pod storage from hostPath-mounted per-pod NVMe to in-pod emptyDir, removes POD_NAME-based per-pod wiring and dind preStop cleanup, updates runner and dind volume mounts, and deletes the README section describing NVMe setup. ChangesH100 GPU Runner Storage Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Hi @key4ng, the DCO sign-off check has failed. All commits must include a To fix existing commits: # Sign off the last N commits (replace N with the number of unsigned commits)
git rebase HEAD~N --signoff
git push --force-with-leaseTo sign off future commits automatically:
|
key4ng
changed the title
|
👋 The PR description doesn't fully follow
Please update the PR description so reviewers have the context they need. |
This reverts commit 63b532a. Signed-off-by: Keyang Ru <rukeyang@gmail.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| mountPath: /var/lib/docker | ||
| subPathExpr: $(POD_NAME)/docker | ||
|
|
||
| initContainers: |
There was a problem hiding this comment.
🟡 Nit: The removed prepare-runner-nvme init container also initialized model-cache PVC directories (/models/.locks, /models/hub, /models/hub/.locks) with correct ownership (1001:1001) and permissions (1777 on lock dirs). This side-effect is lost in the revert.
If the PVC already has these directories from previous pod runs this is fine, but if the PVC is ever re-provisioned (or a new cluster is set up), model caching may fail on first run due to missing directories or wrong permissions. Worth confirming this is covered elsewhere, or extracting the model-cache init into its own init container independent of the NVMe setup. Same applies to the 2-gpu and 4-gpu files.
7a838f6 to
6551395
Compare
There was a problem hiding this comment.
Clean revert of #1627. The switch from NVMe hostPath back to emptyDir is consistent across all three H100 runner value files, and the associated cleanup infrastructure (subPathExpr, preStop hook, POD_NAME env vars, prepare-runner-nvme init container) is correctly removed.
One 🟡 nit posted about the model-cache PVC directory initialization that was a side-effect of the removed init container.
Summary: 0 🔴 Important · 1 🟡 Nit · 0 🟣 Pre-existing
![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: 7a838f671a
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mountPath: /var/lib/docker | ||
| subPathExpr: $(POD_NAME)/docker | ||
|
|
||
| initContainers: |
There was a problem hiding this comment.
When these H100 scale-set pods start with a fresh or root-owned model-cache PVC, dropping the prepare-runner-nvme init container also drops the only setup that created and chowned /models/.locks and /models/hub/.locks for uid 1001. The H100 workflows call scripts/ci_download_model.sh, which immediately does mkdir -p "$LOCK_DIR" and opens lock files under /models/.locks before downloading models, so those jobs can fail with permission denied even though the workspace/docker storage revert itself is intentional; keep a small model-cache init step or equivalent fsGroup/ownership setup in each H100 values file.
Useful? React with 👍 / 👎.
key4ng
changed the title
![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 removes hostPath-based NVMe storage configurations and associated cleanup logic across the H100 runner values files, replacing them with emptyDir volumes. However, removing the prepare-runner-nvme init container also deleted the permission and directory initialization for the shared model-cache volume. This will cause permission denied errors for the non-root runner container. It is recommended to add a dedicated prepare-model-cache init container to set up the necessary directories and permissions, and to disable TLS for the Docker socket in the DinD setup.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| initContainers: | ||
| - name: prepare-runner-nvme | ||
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| command: | ||
| - sh | ||
| - -c | ||
| - mkdir -p /runner-nvme/${POD_NAME}/_work /runner-nvme/${POD_NAME}/.cache /runner-nvme/${POD_NAME}/tmp /runner-nvme/${POD_NAME}/docker /models/.locks /models/hub /models/hub/.locks && chown -R 1001:1001 /runner-nvme/${POD_NAME} && chown 1001:1001 /models/.locks /models/hub /models/hub/.locks && chmod 1777 /runner-nvme/${POD_NAME}/tmp /models/.locks /models/hub/.locks | ||
| env: | ||
| - name: POD_NAME | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.name | ||
| volumeMounts: | ||
| - mountPath: /runner-nvme | ||
| name: runner-nvme | ||
| - mountPath: /models | ||
| name: model-cache | ||
| - name: init-dind-externals |
There was a problem hiding this comment.
Removing the prepare-runner-nvme init container also removed the initialization and permission setup for the model-cache volume (/models/.locks, /models/hub, /models/hub/.locks). Since the runner container runs as a non-root user (UID 1001) and standard PVCs are often mounted as root-owned, the runner will fail to write to the model cache, leading to permission denied errors. We should add a dedicated prepare-model-cache init container to handle this setup. Additionally, when using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the DOCKER_TLS_CERTDIR environment variable to an empty string.
initContainers:
- name: prepare-model-cache
image: fra.ocir.io/idqj093njucb/docker:dind
env:
- name: DOCKER_TLS_CERTDIR
value: ""
command:
- sh
- -c
- mkdir -p /models/.locks /models/hub /models/hub/.locks && chown 1001:1001 /models/.locks /models/hub /models/hub/.locks && chmod 1777 /models/.locks /models/hub/.locks
volumeMounts:
- mountPath: /models
name: model-cache
- name: init-dind-externalsReferences
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the DOCKER_TLS_CERTDIR environment variable to an empty string.
| initContainers: | ||
| - name: prepare-runner-nvme | ||
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| command: | ||
| - sh | ||
| - -c | ||
| - mkdir -p /runner-nvme/${POD_NAME}/_work /runner-nvme/${POD_NAME}/.cache /runner-nvme/${POD_NAME}/tmp /runner-nvme/${POD_NAME}/docker /models/.locks /models/hub /models/hub/.locks && chown -R 1001:1001 /runner-nvme/${POD_NAME} && chown 1001:1001 /models/.locks /models/hub /models/hub/.locks && chmod 1777 /runner-nvme/${POD_NAME}/tmp /models/.locks /models/hub/.locks | ||
| env: | ||
| - name: POD_NAME | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.name | ||
| volumeMounts: | ||
| - mountPath: /runner-nvme | ||
| name: runner-nvme | ||
| - mountPath: /models | ||
| name: model-cache | ||
| - name: init-dind-externals |
There was a problem hiding this comment.
Removing the prepare-runner-nvme init container also removed the initialization and permission setup for the model-cache volume (/models/.locks, /models/hub, /models/hub/.locks). Since the runner container runs as a non-root user (UID 1001) and standard PVCs are often mounted as root-owned, the runner will fail to write to the model cache, leading to permission denied errors. We should add a dedicated prepare-model-cache init container to handle this setup. Additionally, when using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the DOCKER_TLS_CERTDIR environment variable to an empty string.
initContainers:
- name: prepare-model-cache
image: fra.ocir.io/idqj093njucb/docker:dind
env:
- name: DOCKER_TLS_CERTDIR
value: ""
command:
- sh
- -c
- mkdir -p /models/.locks /models/hub /models/hub/.locks && chown 1001:1001 /models/.locks /models/hub /models/hub/.locks && chmod 1777 /models/.locks /models/hub/.locks
volumeMounts:
- mountPath: /models
name: model-cache
- name: init-dind-externalsReferences
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the DOCKER_TLS_CERTDIR environment variable to an empty string.
| initContainers: | ||
| - name: prepare-runner-nvme | ||
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| command: | ||
| - sh | ||
| - -c | ||
| - mkdir -p /runner-nvme/${POD_NAME}/_work /runner-nvme/${POD_NAME}/.cache /runner-nvme/${POD_NAME}/tmp /runner-nvme/${POD_NAME}/docker /models/.locks /models/hub /models/hub/.locks && chown -R 1001:1001 /runner-nvme/${POD_NAME} && chown 1001:1001 /models/.locks /models/hub /models/hub/.locks && chmod 1777 /runner-nvme/${POD_NAME}/tmp /models/.locks /models/hub/.locks | ||
| env: | ||
| - name: POD_NAME | ||
| valueFrom: | ||
| fieldRef: | ||
| fieldPath: metadata.name | ||
| volumeMounts: | ||
| - mountPath: /runner-nvme | ||
| name: runner-nvme | ||
| - mountPath: /models | ||
| name: model-cache | ||
| - name: init-dind-externals |
There was a problem hiding this comment.
Removing the prepare-runner-nvme init container also removed the initialization and permission setup for the model-cache volume (/models/.locks, /models/hub, /models/hub/.locks). Since the runner container runs as a non-root user (UID 1001) and standard PVCs are often mounted as root-owned, the runner will fail to write to the model cache, leading to permission denied errors. We should add a dedicated prepare-model-cache init container to handle this setup. Additionally, when using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the DOCKER_TLS_CERTDIR environment variable to an empty string.
initContainers:
- name: prepare-model-cache
image: fra.ocir.io/idqj093njucb/docker:dind
env:
- name: DOCKER_TLS_CERTDIR
value: ""
command:
- sh
- -c
- mkdir -p /models/.locks /models/hub /models/hub/.locks && chown 1001:1001 /models/.locks /models/hub /models/hub/.locks && chmod 1777 /models/.locks /models/hub/.locks
volumeMounts:
- mountPath: /models
name: model-cache
- name: init-dind-externalsReferences
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the DOCKER_TLS_CERTDIR environment variable to an empty string.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/runner-values-1-gpu-h100.yaml (1)
47-62:⚠️ Potential issue | 🟠 MajorAdd
ephemeral-storagerequests/limits (and/oremptyDir.sizeLimit) forworkanddocker-storagein the H100 runner pods.In
scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml,runner-values-2-gpu-h100.yaml, andrunner-values-4-gpu-h100.yaml, theworkanddocker-storagevolumes areemptyDir: {}(nosizeLimit), and therunner/dindcontainers do not defineresources.requests/limits.ephemeral-storage. Setephemeral-storagerequests/limits for the relevant containers (and/oremptyDir.sizeLimitfor those volumes) so large workspaces and Docker pulls/layers don’t push the node intoDiskPressureand cause evictions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml` around lines 47 - 62, The work and docker-storage emptyDir volumes currently lack size limits and the runner/dind containers lack ephemeral-storage resource requests/limits; add emptyDir.sizeLimit for the volumes named "work" and "docker-storage" and/or set resources.requests.ephemeral-storage and resources.limits.ephemeral-storage on the runner and dind containers (the containers that mount those volumes) in runner-values-1-gpu-h100.yaml (and mirror the same changes to runner-values-2-gpu-h100.yaml and runner-values-4-gpu-h100.yaml) so large workspaces and Docker layers cannot exhaust node disk and cause DiskPressure evictions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml`:
- Around line 47-62: The work and docker-storage emptyDir volumes currently lack
size limits and the runner/dind containers lack ephemeral-storage resource
requests/limits; add emptyDir.sizeLimit for the volumes named "work" and
"docker-storage" and/or set resources.requests.ephemeral-storage and
resources.limits.ephemeral-storage on the runner and dind containers (the
containers that mount those volumes) in runner-values-1-gpu-h100.yaml (and
mirror the same changes to runner-values-2-gpu-h100.yaml and
runner-values-4-gpu-h100.yaml) so large workspaces and Docker layers cannot
exhaust node disk and cause DiskPressure evictions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37a064c6-4929-42d1-af90-f4c64396ddfb
📒 Files selected for processing (4)
scripts/k8s-runner-resources/README.mdscripts/k8s-runner-resources/runner-values-1-gpu-h100.yamlscripts/k8s-runner-resources/runner-values-2-gpu-h100.yamlscripts/k8s-runner-resources/runner-values-4-gpu-h100.yaml
💤 Files with no reviewable changes (1)
- scripts/k8s-runner-resources/README.md
There was a problem hiding this comment.
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/runner-values-1-gpu-h100.yaml (1)
47-62: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider adding
sizeLimitto unbounded emptyDir volumes.The
workanddocker-storagevolumes useemptyDir: {}without asizeLimit. While the PR description indicates the 360GB boot volume should be sufficient, unbounded emptyDir volumes can cause node eviction if storage is exhausted during large builds or Docker image accumulation.Adding explicit limits provides a safety guardrail against runaway storage consumption.
🛡️ Optional: Add sizeLimit for defensive storage management
volumes: - name: work - emptyDir: {} + emptyDir: + sizeLimit: 100Gi - name: model-cache persistentVolumeClaim: claimName: model-cache - name: dshm emptyDir: medium: Memory sizeLimit: 16Gi - name: dind-sock emptyDir: {} - name: dind-externals emptyDir: {} - name: docker-storage - emptyDir: {} + emptyDir: + sizeLimit: 150Gi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml` around lines 47 - 62, Add explicit sizeLimit values for the unbounded emptyDir volumes to prevent node storage exhaustion: update the volumes named "work" and "docker-storage" to include a sensible sizeLimit (e.g., "sizeLimit: 50Gi" or another value appropriate for your workloads). Modify the YAML entries for the volumes with emptyDir: {} to include an object with medium if needed and a sizeLimit field, keeping consistency with the existing "dshm" volume pattern; ensure the chosen limits align with the 360GB boot volume and cluster resource policies.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml`:
- Around line 47-62: Add explicit sizeLimit values for the unbounded emptyDir
volumes to prevent node storage exhaustion: update the volumes named "work" and
"docker-storage" to include a sensible sizeLimit (e.g., "sizeLimit: 50Gi" or
another value appropriate for your workloads). Modify the YAML entries for the
volumes with emptyDir: {} to include an object with medium if needed and a
sizeLimit field, keeping consistency with the existing "dshm" volume pattern;
ensure the chosen limits align with the 360GB boot volume and cluster resource
policies.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: df5d2b15-ca0b-462f-b59a-7a4c9652b6b1
📒 Files selected for processing (4)
scripts/k8s-runner-resources/README.mdscripts/k8s-runner-resources/runner-values-1-gpu-h100.yamlscripts/k8s-runner-resources/runner-values-2-gpu-h100.yamlscripts/k8s-runner-resources/runner-values-4-gpu-h100.yaml
💤 Files with no reviewable changes (1)
- scripts/k8s-runner-resources/README.md
1 participant
Reverts #1627 since the boot volume extends from 100GB and 360GB which have enough runner space
Summary by CodeRabbit
Documentation
Chores