Repository navigation
fix(ci): Use H100 runner for Specific Tests - #357
Conversation
Summary of ChangesHello @XinyueZhang369, 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 addresses the need for more powerful computational resources for specific, demanding CI tests. It reconfigures the CI pipeline to route resource-intensive benchmarks and E2E tests to dedicated H100 GPU runners. This ensures that these critical tests can execute efficiently and reliably, preventing resource starvation on standard runners. The changes also include the necessary Kubernetes configurations to manage the new runner deployments and their automatic scaling. 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
|
📝 WalkthroughWalkthroughAdds Kubernetes RunnerDeployments (GPU/CPU), autoscalers and RBAC for self-hosted runners, updates the CI workflow to select per-job runners and reference Changes
Sequence DiagramsequenceDiagram
participant Dev as Developer (PR)
participant GH as GitHub Actions
participant WF as pr-test-rust.yml
participant Autoscaler as HorizontalRunnerAutoscaler
participant K8s as Kubernetes (RunnerDeployments)
participant Runner as Self-hosted Runner (arc-runner-*)
participant Bench as Benchmarks
Dev->>GH: Push PR triggers workflow
GH->>WF: Start pr-test-rust.yml
WF->>WF: Evaluate job matrix.runner
WF->>Runner: Schedule job on selected `${{ matrix.runner }}`
Autoscaler->>K8s: Adjust replicas based on PercentageRunnersBusy
K8s->>Runner: Provision runner pods (GPU / CPU)
Runner->>WF: Execute CI jobs (unit, e2e, benchmarks)
Runner->>Bench: Run benchmarks and upload results
WF->>WF: Evaluate `python-unit-tests.result` in final/finish
WF->>GH: Report combined status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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 |
4b2791f to
9bc9c10
Compare
![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
The pull request introduces Kubernetes manifests for GitHub Actions self-hosted runners, including autoscalers, CPU and GPU runner deployments, and associated RBAC, aiming to provide dedicated H100 GPU runners for resource-intensive tasks. However, the RBAC configuration for the runner service account is overly permissive, granting broad access to secrets and the ability to execute commands in other pods within the namespace, which poses a significant security risk if a runner is compromised. Beyond this critical security concern, there are also areas for improvement regarding consistency and clarity in the configurations.
| repository: lightseekorg/smg | ||
| labels: | ||
| - k8s-runner-cpu | ||
| serviceAccountName: argo-runner-arc |
There was a problem hiding this comment.
The serviceAccountName here (argo-runner-arc) is inconsistent with the arc-runner-sa used in the GPU runner deployments and defined in arc-runner-rbac.yaml. This inconsistency could lead to permission issues or confusion during deployment and operation. Please align the service account names for consistency and proper RBAC application.
serviceAccountName: arc-runner-sa| privileged: true # Required for DinD | ||
| env: | ||
| - name: DOCKER_TLS_CERTDIR | ||
| value: "" # Disables TLS for shared socket use |
There was a problem hiding this comment.
Disabling TLS for the Docker socket (DOCKER_TLS_CERTDIR: "") removes a layer of security for communication with the Docker daemon. This is particularly risky when combined with privileged: true as it makes the Docker daemon susceptible to man-in-the-middle attacks if the network is not fully trusted. It's recommended to enable TLS for Docker communication if possible, or ensure the network path is secure.
There was a problem hiding this comment.
Need this for docker in docker
| - 4-gpu-h100 | ||
| - k8s-runner-gpu |
There was a problem hiding this comment.
The podAffinityTerm for arc-runner-gpu-h100 uses runner-deployment-name in its labelSelector. For clarity and explicit control, it would be beneficial to explicitly add this label to the labels section of the runner pod template. This ensures that the affinity rule correctly targets pods belonging to this specific runner deployment.
labels:
- 4-gpu-h100
- k8s-runner-gpu
- runner-deployment-name: arc-runner-gpu-h100| - 4-gpu-a10 | ||
| - k8s-runner-gpu |
There was a problem hiding this comment.
Similar to the H100 runner, the podAffinityTerm for arc-runner-gpu-a10 uses runner-deployment-name in its labelSelector. Please explicitly add this label to the labels section of the runner pod template for clarity and to ensure the affinity rule correctly targets pods belonging to this specific runner deployment.
labels:
- 4-gpu-a10
- k8s-runner-gpu
- runner-deployment-name: arc-runner-gpu-a10![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/pr-test-rust.yml:
- Line 604: The conditional that aggregates downstream job failures is missing
the python-unit-tests job: update the elif condition that currently checks
needs.python-lint.result, build-wheel, unit-tests, gateway-e2e,
gateway-e2e-heavy, go-unit-tests, and go-bindings-e2e to also include
needs.python-unit-tests.result == "failure" so the finish job fails when
python-unit-tests fails; locate the conditional in the workflow (the elif line
shown) and add the check for needs.python-unit-tests.result in the same format
as the other checks.
In `@scripts/k8s-runner-resources/arc-runner-cpu.yaml`:
- Line 13: The Deployment's serviceAccountName is set to the incorrect value
"argo-runner-arc" causing RBAC mismatch; update the serviceAccountName in this
YAML (the field serviceAccountName: argo-runner-arc) to the correct service
account "arc-runner-sa" so it matches the RBAC resource (arc-runner-rbac /
arc-runner-sa) and the GPU deployments.
🧹 Nitpick comments (6)
scripts/k8s-runner-resources/arc-runner-rbac.yaml (1)
13-13: Misleading comment: "Argo Workflows" should be "GitHub Actions Runners".The comment suggests this is for Argo Workflows, but this RBAC is for GitHub Actions Runner Controller (ARC). Consider updating for clarity.
- # Argo Workflows + # GitHub Actions Runner secrets accessscripts/k8s-runner-resources/arc-runner-cpu.yaml (1)
25-26: Docker sidecar missing DinD configuration present in GPU deployments.The GPU deployments include
securityContext: privileged: true,DOCKER_TLS_CERTDIRenv var, and volume mounts for Docker socket and storage. The CPU deployment's docker container lacks these, which may prevent Docker-in-Docker from functioning correctly.Consider aligning the DinD configuration with the GPU deployments if this runner needs Docker capabilities:
♻️ Suggested addition
- name: docker image: fra.ocir.io/idqj093njucb/docker:dind + securityContext: + privileged: true + env: + - name: DOCKER_TLS_CERTDIR + value: "" + volumeMounts: + - name: docker-sock + mountPath: /var/run + - name: docker-storage + mountPath: /var/lib/dockerYou would also need to add corresponding volume definitions in the spec.
scripts/k8s-runner-resources/arc-runner-autoscaler.yaml (1)
40-40: Minor naming inconsistency.The naming pattern differs:
arc-runner-h100-autoscaler,arc-runner-a10-autoscalervsarc-cpu-runner-autoscaler. Consider using consistent naming likearc-runner-cpu-autoscalerfor uniformity.- name: arc-cpu-runner-autoscaler + name: arc-runner-cpu-autoscalerscripts/k8s-runner-resources/arc-runner-gpu.yaml (3)
96-98: Same deprecated label used in A10 deployment.Apply the same fix as the H100 deployment.
nodeSelector: nvidia.com/gpu: "true" - beta.kubernetes.io/instance-type: BM.GPU.A10.4 + node.kubernetes.io/instance-type: BM.GPU.A10.4
52-67: Runner container missing CPU/memory resource requests.The runner container only specifies GPU limits. Adding CPU and memory requests/limits (similar to the CPU runner's 8 CPU / 16Gi) would improve scheduling predictability and prevent resource contention on GPU nodes.
16-18: Replace deprecated node label in both runner deployments.
beta.kubernetes.io/instance-typehas been deprecated since Kubernetes v1.17. Replace withnode.kubernetes.io/instance-typefor forward compatibility.♻️ Fixes required
Line 18 (H100 deployment):
nodeSelector: nvidia.com/gpu: "true" - beta.kubernetes.io/instance-type: BM.GPU.H100.8 + node.kubernetes.io/instance-type: BM.GPU.H100.8Line 98 (A10 deployment):
nodeSelector: nvidia.com/gpu: "true" - beta.kubernetes.io/instance-type: BM.GPU.A10.4 + node.kubernetes.io/instance-type: BM.GPU.A10.4
|
We need to reduce the timeout of the benchmarks test with switching from A10 to H100. For instance, TTFT should be reduced to 0.8 or 0.78. See issue #351 |
f77922e to
7c2b61d
Compare
| test_filter: "" | ||
| setup_trtllm: true | ||
| ignore_opts: "" | ||
| runs-on: k8s-runner-gpu |
There was a problem hiding this comment.
could we use runs-on: ${{ matrix.runner }} here, and in each matrix we can set runner: 4-gpu-h100 for benchamrk and trt test, and runner k8s-runner-gpu for others. This eliminates the duplications of new gateway-e2e-heavy workflow and looks more clear
There was a problem hiding this comment.
Ah that would better! Fixed
There was a problem hiding this comment.
nice!
I'm just thinking can it be more neat to set it something like runs-on: ${{ matrix.runner || 'k8s-runner-gpu' }}, in this way, default will be k8s-runner-gpu, only benchmark and trt need set runner to override
There was a problem hiding this comment.
I see, sure, changed!
ee2a9fe to
ba4b51c
Compare
| scaleUpThreshold: "0.95" | ||
| scaleDownThreshold: "0.25" | ||
| scaleUpFactor: "0.5" | ||
| scaleDownFactor: "0.5" No newline at end of file |
| - name: docker-sock | ||
| mountPath: /var/run | ||
| - name: docker-storage | ||
| mountPath: /var/lib/docker No newline at end of file |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml`:
- Around line 16-19: The nodeSelector uses the deprecated label key
"beta.kubernetes.io/instance-type"; replace that key with the stable
"node.kubernetes.io/instance-type" wherever it appears (e.g., in the
nodeSelector block alongside "nvidia.com/gpu: \"true\"") to ensure scheduling
works on newer clusters—update all other occurrences of
"beta.kubernetes.io/instance-type" in the same manifest as well.
- Around line 69-74: The deployment uses privileged DinD (image:
fra.ocir.io/idqj093njucb/docker:dind) with securityContext.privileged: true and
DOCKER_TLS_CERTDIR set to "", which allows job containers full daemon control;
replace this by either running rootless Docker or a non-privileged build runtime
(e.g., switch to containerd/buildkit-based image and remove
securityContext.privileged), or enable TLS by removing DOCKER_TLS_CERTDIR:"" and
configuring DOCKER_TLS_CERTDIR to a secure path plus mounting scoped TLS
credentials/secrets for the runner; apply the same change for both DinD blocks
that set image: fra.ocir.io/.../dind, securityContext.privileged and env
DOCKER_TLS_CERTDIR.
🧹 Nitpick comments (1)
scripts/k8s-runner-resources/arc-runner-gpu.yaml (1)
52-79: Define CPU/memory requests and limits for runner + docker containers.Without requests/limits, these pods may be best-effort and prone to eviction or noisy-neighbor contention, especially on GPU nodes. Add explicit CPU/memory requests/limits to stabilize scheduling and performance.
Also applies to: 132-159
| nodeSelector: | ||
| nvidia.com/gpu: "true" | ||
| beta.kubernetes.io/instance-type: BM.GPU.H100.8 | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Kubernetes instance-type label beta.kubernetes.io/instance-type deprecated node.kubernetes.io/instance-type
💡 Result:
beta.kubernetes.io/instance-type is a deprecated Node label. Since Kubernetes v1.17, you should use the stable replacement node.kubernetes.io/instance-type instead. [1]
What to change
- Update any nodeSelector, nodeAffinity, Helm charts, or controllers that match on:
beta.kubernetes.io/instance-type=...
- To instead match on:
node.kubernetes.io/instance-type=...[1]
Notes
- The kubelet typically sets
node.kubernetes.io/instance-typebased on the cloud provider’s instance type (when a cloud provider integration is in use). [1] - Kubernetes generally recommends scheduling on capabilities (CPU/GPU/memory, taints/tolerations, topology) rather than pinning to specific instance types. [1]
Source
- Kubernetes “Well-Known Labels, Annotations and Taints” reference. [1]
Replace deprecated instance-type label to avoid scheduling failures on newer clusters.
beta.kubernetes.io/instance-type has been deprecated since Kubernetes v1.17. Use the stable replacement node.kubernetes.io/instance-type instead.
🔧 Suggested update
nodeSelector:
nvidia.com/gpu: "true"
- beta.kubernetes.io/instance-type: BM.GPU.H100.8
+ node.kubernetes.io/instance-type: BM.GPU.H100.8 nodeSelector:
nvidia.com/gpu: "true"
- beta.kubernetes.io/instance-type: BM.GPU.A10.4
+ node.kubernetes.io/instance-type: BM.GPU.A10.4Also applies to: 96-99
🤖 Prompt for AI Agents
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml` around lines 16 - 19, The
nodeSelector uses the deprecated label key "beta.kubernetes.io/instance-type";
replace that key with the stable "node.kubernetes.io/instance-type" wherever it
appears (e.g., in the nodeSelector block alongside "nvidia.com/gpu: \"true\"")
to ensure scheduling works on newer clusters—update all other occurrences of
"beta.kubernetes.io/instance-type" in the same manifest as well.
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| securityContext: | ||
| privileged: true # Required for DinD | ||
| env: | ||
| - name: DOCKER_TLS_CERTDIR | ||
| value: "" # Disables TLS for shared socket use |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the file to understand the context
wc -l scripts/k8s-runner-resources/arc-runner-gpu.yamlRepository: lightseekorg/smg
Length of output: 112
🏁 Script executed:
# Read the relevant sections - lines 69-74 and 149-154
sed -n '65,80p' scripts/k8s-runner-resources/arc-runner-gpu.yaml
echo "---"
sed -n '145,160p' scripts/k8s-runner-resources/arc-runner-gpu.yamlRepository: lightseekorg/smg
Length of output: 1131
🌐 Web query:
Docker-in-Docker DinD Kubernetes security best practices 2025 privileged rootless TLS
💡 Result:
DinD on Kubernetes (2025→2026) security best practices (privileged, rootless, TLS)
1) Prefer not running DinD at all (best risk reduction)
- Use daemonless / “no Docker socket” builders where possible (e.g., BuildKit rootless or Buildx Kubernetes driver with
rootless=true) to avoid giving a pod a Docker daemon with elevated host-like power. Docker’s Buildx Kubernetes driver explicitly supports rootless mode and creates pods withoutsecurityContext.privileged. [1]
2) If you must run DinD, treat it as high risk
- Assume privileged == near-host-level power: Kubernetes notes privileged containers override/ignore key hardening controls (seccomp/AppArmor/SELinux) and effectively gain broad capabilities. Avoid unless there’s no alternative. [2]
- Enforce Pod Security Standards:
- For most namespaces, enforce Baseline or Restricted so
privileged: true, host namespaces, and hostPath mounts are disallowed by policy. [3] - Put the DinD workload in a tightly controlled exception namespace with explicit approvals.
- For most namespaces, enforce Baseline or Restricted so
3) Rootless: helpful, but don’t assume it removes the need for privilege
- Rootless Docker runs the daemon and containers as a non-root user via user namespaces (reduces impact of daemon/runtime bugs). [4]
- However, Docker’s own guidance for “Rootless Docker in Docker” still uses
--privileged(to disable seccomp/AppArmor and mount restrictions), meaning you may still be bypassing important isolation controls even if the daemon UID isn’t 0. [5]
4) Use Kubernetes User Namespaces where available (additional containment)
- Consider opting pods into user namespaces with
spec.hostUsers: falseso “root in the container” maps to an unprivileged host UID/GID, reducing host impact of a breakout. [6] - Note: user namespaces come with constraints/limitations (for example, cannot combine with hostNetwork/hostPID/hostIPC). [6]
5) Never expose an unauthenticated Docker API; use TLS or SSH
- If you expose the Docker daemon beyond a local UNIX socket, Docker recommends securing it with SSH or mutual TLS (
tlsverify). [7]
6) If privileged DinD is unavoidable, reduce blast radius aggressively
- Dedicate a separate node pool (taints/tolerations) for DinD jobs; assume node compromise is possible.
- Use tight NetworkPolicies (only allow what’s required) and short-lived, ephemeral workers.
- Minimize mounts: avoid hostPath; don’t mount the host’s
/var/run/docker.sock(that is effectively root on the node in many setups). - Keep
privilegedas a last resort; otherwise, grant only specific Linux capabilities (Kubernetes recommends capabilities over privileged when possible). [2]
Key takeaway: In Kubernetes, “DinD” usually pushes you toward privileged (or “privileged-like”) operation; the safest pattern is to avoid DinD and use rootless BuildKit/Buildx-on-Kubernetes where possible. [1][2][5]
Sources: Docker docs on Buildx Kubernetes rootless mode [1], Kubernetes on privileged container impacts [2], Kubernetes Pod Security Standards [3], Docker rootless mode [4], Docker rootless DinD tip showing --privileged [5], Kubernetes user namespaces (hostUsers: false) [6], Docker daemon TLS/SSH guidance [7].
Privileged DinD with TLS disabled is a security risk for CI runners.
A privileged Docker daemon plus DOCKER_TLS_CERTDIR="" gives any job container full daemon control. For untrusted PRs, this can lead to host escape. Consider rootless Docker, a non-privileged runtime (containerd/buildkit), or enabling TLS with scoped credentials.
This applies to both locations: lines 69-74 and 149-154.
🤖 Prompt for AI Agents
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml` around lines 69 - 74, The
deployment uses privileged DinD (image: fra.ocir.io/idqj093njucb/docker:dind)
with securityContext.privileged: true and DOCKER_TLS_CERTDIR set to "", which
allows job containers full daemon control; replace this by either running
rootless Docker or a non-privileged build runtime (e.g., switch to
containerd/buildkit-based image and remove securityContext.privileged), or
enable TLS by removing DOCKER_TLS_CERTDIR:"" and configuring DOCKER_TLS_CERTDIR
to a secure path plus mounting scoped TLS credentials/secrets for the runner;
apply the same change for both DinD blocks that set image: fra.ocir.io/.../dind,
securityContext.privileged and env DOCKER_TLS_CERTDIR.
| name: arc-runner-gpu-a10 | ||
| namespace: actions-runner-system | ||
| spec: | ||
| replicas: 2 |
There was a problem hiding this comment.
I thought we have 4 A10, do we only have 2? I also didn't see hpa for a10.
There was a problem hiding this comment.
we probably don't need to provision all 4 A20 all the time, so here is set to 2 for now. For HPA, today the auto scaler somehow kept creating cpu and a10 runner pods that cannot register to the repo regardless the max number, so I deleted all the old auto scalers for cpu, a10 and h100, this one is a new configuration, I want to bake it for some times, since most resources are h100, I only create for h100 for now for baking, once the scaling strategy works stably, I'll create the same for a10 and update this file
Co-authored-by: xinyzzha <xinyue.zhang@oracle.com>
Co-authored-by: xinyzzha <xinyue.zhang@oracle.com> Signed-off-by: ppraneth <pranethparuchuri@gmail.com>
``feat/dense-llama-model-registry`` was the temporary pin added when lightseekorg/tokenspeed#357 was in flight. That PR merged and the source branch was deleted, so the pinned clone now fails with ``Remote branch feat/dense-llama-model-registry not found in upstream origin``. ``main`` includes #357, so we can flip back.
``feat/dense-llama-model-registry`` was the temporary pin added when lightseekorg/tokenspeed#357 was in flight. That PR merged and the source branch was deleted, so the pinned clone now fails with ``Remote branch feat/dense-llama-model-registry not found in upstream origin``. ``main`` includes #357, so we can flip back. Signed-off-by: yetone <yetoneful@gmail.com>
``feat/dense-llama-model-registry`` was the temporary pin added when lightseekorg/tokenspeed#357 was in flight. That PR merged and the source branch was deleted, so the pinned clone now fails with ``Remote branch feat/dense-llama-model-registry not found in upstream origin``. ``main`` includes #357, so we can flip back. Signed-off-by: yetone <yetoneful@gmail.com>
``feat/dense-llama-model-registry`` was the temporary pin added when lightseekorg/tokenspeed#357 was in flight. That PR merged and the source branch was deleted, so the pinned clone now fails with ``Remote branch feat/dense-llama-model-registry not found in upstream origin``. ``main`` includes #357, so we can flip back. Signed-off-by: yetone <yetoneful@gmail.com>
``feat/dense-llama-model-registry`` was the temporary pin added when lightseekorg/tokenspeed#357 was in flight. That PR merged and the source branch was deleted, so the pinned clone now fails with ``Remote branch feat/dense-llama-model-registry not found in upstream origin``. ``main`` includes #357, so we can flip back. Signed-off-by: yetone <yetoneful@gmail.com>
4 participants
Description
Problem
The
benchmarksandchat-completions-trtllmE2E tests require more GPU resources than the standardk8s-runner-gpurunners provide. Additionally, the benchmark TTFT thresholds were calibrated for A10 GPUs and are far too lenient for H100s (see #351).Solution
Move these two matrix entries into a new
gateway-e2e-heavyjob that runs on4-gpu-h100runners, and tighten benchmark thresholds to match H100 performance.Changes
benchmarksandchat-completions-trtllmfrom thegateway-e2ematrix into a newgateway-e2e-heavyjob targeting4-gpu-h100runnersgateway-e2estepsfinishjob to depend on and checkgateway-e2e-heavysummarize-benchmarksto depend ongateway-e2e-heavy(where benchmark artifacts are now produced)test_regular_perf:ttft_mean_max6s → 0.8s (H100 averages ~0.77s)test_pd_perf:ttft_mean_max13s → 5s (H100 PD averages ~4.2-4.6s)Test Plan
The k8s runner resource manifests are all applied on cluster, then make sure this change pass the test workflow
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit