Repository navigation
fix(ci): fix 2-gpu runner for RDMA - #1705
Conversation
📝 WalkthroughWalkthroughDisable the H100 autoscaler by zeroing replica bounds in the autoscaler manifest and update the 2-gpu-h100 Helm values: scale bounds, hostNetwork/podAntiAffinity, runner image/security/env, increased resources, init image, and dind sidecar TLS and resource settings. ChangesH100 GPU Runner Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
XinyueZhang369
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| affinity: | ||
| podAffinity: | ||
| podAntiAffinity: | ||
| preferredDuringSchedulingIgnoredDuringExecution: |
There was a problem hiding this comment.
🟡 Nit: preferredDuringSchedulingIgnoredDuringExecution is a soft constraint — the scheduler can still co-locate two 2-gpu runners on the same node when eligible nodes are scarce. With hostNetwork: true and maxRunners: 8, if the BM.GPU.H100.8 pool has fewer than 8 nodes available, co-scheduling will happen and the host-port collisions the comment warns about become real.
Consider using requiredDuringSchedulingIgnoredDuringExecution instead to make the one-runner-per-node guarantee hard, so the scheduler fails loudly (pod stays Pending) rather than silently causing port conflicts at runtime.
![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 updates the Kubernetes runner configuration for the 2-GPU H100 tier, enabling host networking, configuring pod anti-affinity to prevent port collisions, upgrading the runner image, and adding several API keys and environment variables. It also scales down the legacy autoscaler to zero. Feedback suggests completely removing the obsolete legacy deployment and autoscaler configurations rather than just scaling them to zero. Additionally, the resource limits applied to the dind container (2 CPUs and 4Gi memory) may throttle heavy Docker-based CI workloads, so increasing or removing these limits is recommended.
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.
| resources: | ||
| limits: | ||
| cpu: '2' | ||
| memory: 4Gi | ||
| requests: | ||
| cpu: '1' | ||
| memory: 2Gi |
There was a problem hiding this comment.
The dind container is configured with a memory limit of 4Gi and CPU limit of 2.
In a GitHub Actions runner using Docker-in-Docker (DinD), any workflow steps that run inside Docker containers (e.g., docker run ..., docker build ..., or action steps using docker images) are executed by the dockerd daemon inside this dind container.
Because of this, those steps will be constrained by the dind container's resource limits (2 CPUs and 4Gi memory), rather than the runner container's larger allocation (32 CPUs and 128Gi memory). If your CI workflows run heavy ML workloads or large docker builds inside Docker, they may experience severe CPU throttling or OOM kills.
Consider increasing these limits or removing them to allow dind to utilize the pod's full resource capacity when needed.
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.
References
- 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.
| minReplicas: 0 | ||
| maxReplicas: 0 |
There was a problem hiding this comment.
Since the 2-gpu-h100 runner tier has been migrated to the new gha-runner-scale-set (using runner-values-2-gpu-h100.yaml), the legacy summerwind RunnerDeployment (arc-runner-2-gpu-h100 in arc-runner-gpu.yaml) and this HorizontalRunnerAutoscaler are now obsolete.
Instead of just scaling them to 0, consider completely removing the arc-runner-2-gpu-h100 deployment and autoscaler blocks from the codebase to clean up dead configuration and prevent any accidental scale-ups in the future.
There was a problem hiding this comment.
Clean, well-documented infrastructure change. The RDMA/hostNetwork rationale is sound, comments explain the 'why' clearly, and the migration from summerwind to gha-runner-scale-set is correctly wired (legacy autoscaler zeroed out, image version bumped consistently). One soft nit on the anti-affinity constraint strength — see inline comment.
![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: 6d71e9aae4
ℹ️ 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".
| podAntiAffinity: | ||
| preferredDuringSchedulingIgnoredDuringExecution: |
There was a problem hiding this comment.
This new anti-affinity is only a scheduler preference, so when the scale set grows beyond the number of readily available H100 nodes (or the scheduler prioritizes satisfying the 2-GPU request), Kubernetes may still place multiple hostNetwork 2-GPU runners on the same host. In that case their DinD/PD jobs share the node network namespace and can hit the host-port collisions this block is intended to prevent; use required anti-affinity or otherwise partition the host ports if co-location must be impossible.
Useful? React with 👍 / 👎.
![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-2-gpu-h100.yaml (1)
41-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
preferredDuringSchedulingIgnoredDuringExecutiondoes not enforce the one-runner-per-node guarantee.The comment says these PD runners must be one-per-node to avoid host-port collisions, but
preferredDuringSchedulingIgnoredDuringExecutionis only a soft preference; Kubernetes may still place multiple matching pods on the same hostname when it needs to. That would bring back the same nondeterministic host-network collisions this change is trying to remove. (kubernetes.io)Suggested change
affinity: podAntiAffinity: - preferredDuringSchedulingIgnoredDuringExecution: - - weight: 100 - podAffinityTerm: - labelSelector: - matchExpressions: - - key: actions.github.com/scale-set-name - operator: In - values: - - 2-gpu-h100 - topologyKey: kubernetes.io/hostname + requiredDuringSchedulingIgnoredDuringExecution: + - labelSelector: + matchExpressions: + - key: actions.github.com/scale-set-name + operator: In + values: + - 2-gpu-h100 + topologyKey: kubernetes.io/hostname🤖 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-2-gpu-h100.yaml` around lines 41 - 55, Replace the soft pod anti-affinity rule with a hard one so scheduling enforces one 2-gpu-h100 runner per node: under affinity -> podAntiAffinity, change preferredDuringSchedulingIgnoredDuringExecution to requiredDuringSchedulingIgnoredDuringExecution and move the existing podAffinityTerm (with labelSelector matchExpressions key actions.github.com/scale-set-name operator In values [2-gpu-h100] and topologyKey kubernetes.io/hostname) into that required list so Kubernetes will refuse to place multiple matching pods on the same node.
🤖 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-2-gpu-h100.yaml`:
- Around line 41-55: Replace the soft pod anti-affinity rule with a hard one so
scheduling enforces one 2-gpu-h100 runner per node: under affinity ->
podAntiAffinity, change preferredDuringSchedulingIgnoredDuringExecution to
requiredDuringSchedulingIgnoredDuringExecution and move the existing
podAffinityTerm (with labelSelector matchExpressions key
actions.github.com/scale-set-name operator In values [2-gpu-h100] and
topologyKey kubernetes.io/hostname) into that required list so Kubernetes will
refuse to place multiple matching pods on the same node.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fedd118a-f52d-4ec2-b0a8-2ceedb7383fd
📒 Files selected for processing (2)
scripts/k8s-runner-resources/arc-runner-autoscaler.yamlscripts/k8s-runner-resources/runner-values-2-gpu-h100.yaml
|
Hi @XinyueZhang369, this PR has merge conflicts that must be resolved before it can be merged. Please rebase your branch: git fetch origin main
git rebase origin/main
# resolve any conflicts, then:
git push --force-with-lease |
mergify
Bot
added
the
needs-rebase
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
6d71e9a to
8b2b1b0
Compare
mergify
Bot
removed
the
needs-rebase
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/runner-values-2-gpu-h100.yaml (1)
45-55: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse hard anti-affinity here, not a preference.
preferredDuringSchedulingIgnoredDuringExecutiondoes not guarantee one2-gpu-h100runner per node. When the H100 pool is constrained, the scheduler can still place two host-network pods on the same host and reintroduce the host-port collisions this PR is trying to eliminate. Switch this torequiredDuringSchedulingIgnoredDuringExecutionso capacity shortages show up as Pending pods instead of flaky PD jobs.Suggested change
affinity: podAntiAffinity: - preferredDuringSchedulingIgnoredDuringExecution: - - weight: 100 - podAffinityTerm: - labelSelector: - matchExpressions: - - key: actions.github.com/scale-set-name - operator: In - values: - - 2-gpu-h100 - topologyKey: kubernetes.io/hostname + requiredDuringSchedulingIgnoredDuringExecution: + - labelSelector: + matchExpressions: + - key: actions.github.com/scale-set-name + operator: In + values: + - 2-gpu-h100 + topologyKey: kubernetes.io/hostname🤖 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-2-gpu-h100.yaml` around lines 45 - 55, The current podAntiAffinity uses preferredDuringSchedulingIgnoredDuringExecution which is only a soft preference; change it to requiredDuringSchedulingIgnoredDuringExecution so the scheduler enforces one 2-gpu-h100 runner per node. Locate the podAntiAffinity block containing preferredDuringSchedulingIgnoredDuringExecution and replace that top-level key with requiredDuringSchedulingIgnoredDuringExecution while keeping the existing podAffinityTerm, labelSelector (matchExpressions with key actions.github.com/scale-set-name and value 2-gpu-h100), and topologyKey kubernetes.io/hostname unchanged.
🤖 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.
Inline comments:
In `@scripts/k8s-runner-resources/runner-values-2-gpu-h100.yaml`:
- Around line 79-85: The runner container is marked privileged
(securityContext.privileged: true) which grants node-level access; remove
privileged: true from the runner's securityContext and instead confine elevated
privileges to a narrow helper: add a privileged initContainer or privileged
sidecar (e.g., the existing dind helper) that performs the RDMA/memlock setup,
or grant only the IPC_LOCK capability via securityContext.capabilities.add on
that helper container and raise RLIMIT_MEMLOCK there; update references to the
runner pod spec (securityContext) and any init/sidecar container definitions so
only the helper has privileged or IPC_LOCK rights while the main runner remains
non-privileged.
---
Outside diff comments:
In `@scripts/k8s-runner-resources/runner-values-2-gpu-h100.yaml`:
- Around line 45-55: The current podAntiAffinity uses
preferredDuringSchedulingIgnoredDuringExecution which is only a soft preference;
change it to requiredDuringSchedulingIgnoredDuringExecution so the scheduler
enforces one 2-gpu-h100 runner per node. Locate the podAntiAffinity block
containing preferredDuringSchedulingIgnoredDuringExecution and replace that
top-level key with requiredDuringSchedulingIgnoredDuringExecution while keeping
the existing podAffinityTerm, labelSelector (matchExpressions with key
actions.github.com/scale-set-name and value 2-gpu-h100), and topologyKey
kubernetes.io/hostname unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: da278327-d536-4fa4-b7ca-af26155afc3d
📒 Files selected for processing (2)
scripts/k8s-runner-resources/arc-runner-autoscaler.yamlscripts/k8s-runner-resources/runner-values-2-gpu-h100.yaml
| securityContext: | ||
| # The runner runs as non-root (uid 1001), so capabilities.add: | ||
| # [IPC_LOCK] only reaches the bounding set (not effective) and the | ||
| # default 64KiB RLIMIT_MEMLOCK blocks RDMA completion-queue | ||
| # allocation (ibv_create_cq -> ENOMEM, e.g. sglang/Mooncake). | ||
| # privileged makes IPC_LOCK effective and lifts the memlock cap. | ||
| privileged: true |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Avoid making the runner container itself privileged.
This is broader than the existing privileged dind sidecar: every workflow step scheduled onto this scale set now runs inside a privileged, host-network container, which is effectively node-level access. If the real requirement is memlock/RDMA setup, keep that privilege in the narrowest possible helper/runtime path or otherwise treat this label as trusted-only; leaving the main runner privileged materially weakens the runner isolation model.
🤖 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-2-gpu-h100.yaml` around lines 79 -
85, The runner container is marked privileged (securityContext.privileged: true)
which grants node-level access; remove privileged: true from the runner's
securityContext and instead confine elevated privileges to a narrow helper: add
a privileged initContainer or privileged sidecar (e.g., the existing dind
helper) that performs the RDMA/memlock setup, or grant only the IPC_LOCK
capability via securityContext.capabilities.add on that helper container and
raise RLIMIT_MEMLOCK there; update references to the runner pod spec
(securityContext) and any init/sidecar container definitions so only the helper
has privileged or IPC_LOCK rights while the main runner remains non-privileged.
1 participant
Description
Problem
PD KV transfer (NIXL for vLLM, Mooncake for sglang) moves the KV cache over RDMA on the host mlx5 NICs. On the legacy runners the pod ran in an overlay-CNI network namespace, where the shared host NICs show PORT_ACTIVE but expose
an all-zero GID table inside the pod — so RDMA init died at worker startup (Failed to query GID … No data available, UCX Address not valid). The summerwind RunnerDeployment CRD can't express hostNetwork, so the fix required
migrating this tier to the gha-runner-scale-set, whose template.spec is a full PodSpec.
Solution
Moves the 2-gpu-h100 runner — the only tier that runs PD disaggregation e2e tests — from the legacy summerwind RunnerDeployment to the GitHub gha-runner-scale-set, configured so PD KV transfer works over real RoCE RDMA. This fixes
the frequent e2e-2gpu-pd flakiness.
Changes
(ibv_create_cq → ENOMEM, hit by sglang/Mooncake). privileged makes IPC_LOCK effective and lifts the memlock cap. TODO: replace with node containerd default_ulimits memlock=-1 and drop privileged.
Test Plan
Run 27438486706: all three e2e-2gpu-pd variants pass over real RDMA — vllm (NIXL), vllm-mooncake, and sglang (Mooncake). (The run's other failures — unit-tests, openai-responses — are unrelated pre-existing flakes.)
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit