Repository navigation
ci: Change to use github runner set for ci runners - #659
Conversation
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
github-actions
Bot
added
documentation
📝 WalkthroughWalkthroughReplaces multi-target runner selection in CI workflow with a single Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant ARC as ARC Helm/Values
participant K8s as Kubernetes API
participant Pod as Runner Pod (Init + DIND)
GH->>ARC: request runner with label (workflow dispatch)
ARC->>K8s: create/scale RunnerScaleSet / Pod via ARC controller
K8s->>Pod: schedule Pod on matching GPU/CPU node
Pod->>Pod: init-dind-externals (copy externals)
Pod->>Pod: dind init (dockerd startup)
Pod->>GH: runner registers and executes job
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
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 modernizes the GitHub Actions self-hosted runner infrastructure by transitioning from a legacy CRD-based setup to the officially supported Actions Runner Controller (ARC) runner scale set model. This change leverages Helm charts to define and manage various runner configurations, enhancing the scalability and maintainability of CI/CD workflows within Kubernetes environments. Highlights
Changelog
Ignored Files
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
|
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
![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: faacd1fb5f
ℹ️ 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".
| needs: build-wheel | ||
| if: ${{ !cancelled() }} | ||
| runs-on: ["k8s-runner-gpu", "4-gpu-h100"] | ||
| runs-on: 4-gpu-h100" |
There was a problem hiding this comment.
The multi-worker job now targets runs-on: 4-gpu-h100", which GitHub Actions parses as a literal label containing the trailing quote. With ARC scale sets named 4-gpu-h100, no runner will match this label, so all multi-worker nightly benchmark jobs will stay queued after this change.
Useful? React with 👍 / 👎.
| kind: Secret | ||
| metadata: | ||
| name: github-arc-secret | ||
| namespace: actions-runner-systemapp |
There was a problem hiding this comment.
The secret template sets metadata.namespace to actions-runner-systemapp, but the deployment guide creates and installs runner scale sets in actions-runner-system. Following the documented flow, kubectl apply will fail because that namespace does not exist (or the secret will land in the wrong namespace), and ARC will not be able to resolve githubConfigSecret for the runner scale sets.
Useful? React with 👍 / 👎.
![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 migrates the CI runners to the new GitHub Actions Runner Controller (ARC) scale set model, which is a great improvement, enhancing scalability and aligning with official GitHub support. However, several significant security and configuration concerns have been identified. The Docker-in-Docker (dind) sidecar containers are configured to run in privileged: true mode, granting excessive permissions and posing a substantial risk. They are also incorrectly configured as initContainers, which will prevent runner pods from starting. Furthermore, for DinD setups, it is crucial to disable TLS for the Docker socket by setting DOCKER_TLS_CERTDIR to an empty string. Additionally, there's a namespace typo and feedback to enhance the clarity of the new deployment guide. It is recommended to address these DinD configuration issues and explore more secure alternatives for Docker-in-Docker to minimize risk.
| github_app_id: "2703284" | ||
| github_app_installation_id: "105440663" | ||
| github_app_private_key: "github-app-private-key" |
There was a problem hiding this comment.
This template file contains hardcoded values for github_app_id and github_app_installation_id. Committing secrets or sensitive IDs to the repository is a major security risk, even if they are example values. The file should use placeholders as documented in scripts/k8s-runner-resources/README.md.
github_app_id: "<your-app-id>"
github_app_installation_id: "<your-installation-id>"
github_app_private_key: "<contents-of-your-pem-file>"| kind: Secret | ||
| metadata: | ||
| name: github-arc-secret | ||
| namespace: actions-runner-systemapp |
There was a problem hiding this comment.
There is a typo in the namespace. It is actions-runner-systemapp, but all other configuration files and the instructions in README.md use actions-runner-system. This will cause the secret to be created in the wrong namespace, and the ARC controller will not be able to find it.
namespace: actions-runner-system| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| restartPolicy: Always | ||
| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
The dind container is configured with privileged: true, which is a significant security risk as it grants excessive permissions and could lead to container escape, especially for CI runners executing untrusted code. It is also incorrectly configured as an initContainer. initContainers must run to completion, but dockerd is a long-lived process, causing the pod to get stuck in the Init state. The dind container should be moved to the containers section to run as a sidecar. Additionally, restartPolicy: Always is an invalid field for a container specification and should be removed. 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. Consider using a more secure alternative for Docker-in-Docker, such as rootless mode or a specialized runtime like Sysbox, to avoid privileged: true.
References
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the
DOCKER_TLS_CERTDIRenvironment variable to an empty string.
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| restartPolicy: Always | ||
| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
The dind container is configured with privileged: true, which is a significant security risk as it grants excessive permissions and could lead to container escape, especially for CI runners executing untrusted code. It is also incorrectly configured as an initContainer. initContainers must run to completion, but dockerd is a long-lived process, causing the pod to get stuck in the Init state. The dind container should be moved to the containers section to run as a sidecar. Additionally, restartPolicy: Always is an invalid field for a container specification and should be removed. 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. Consider using a more secure alternative for Docker-in-Docker, such as rootless mode or a specialized runtime like Sysbox, to avoid privileged: true.
References
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the
DOCKER_TLS_CERTDIRenvironment variable to an empty string.
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| restartPolicy: Always | ||
| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
The dind container is configured with privileged: true, which is a significant security risk as it grants excessive permissions and could lead to container escape, especially for CI runners executing untrusted code. It is also incorrectly configured as an initContainer. initContainers must run to completion, but dockerd is a long-lived process, causing the pod to get stuck in the Init state. The dind container should be moved to the containers section to run as a sidecar. Additionally, restartPolicy: Always is an invalid field for a container specification and should be removed. 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. Consider using a more secure alternative for Docker-in-Docker, such as rootless mode or a specialized runtime like Sysbox, to avoid privileged: true.
References
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the
DOCKER_TLS_CERTDIRenvironment variable to an empty string.
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| restartPolicy: Always | ||
| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
The dind container is configured with privileged: true, which is a significant security risk as it grants excessive permissions and could lead to container escape, especially for CI runners executing untrusted code. It is also incorrectly configured as an initContainer. initContainers must run to completion, but dockerd is a long-lived process, causing the pod to get stuck in the Init state. The dind container should be moved to the containers section to run as a sidecar. Additionally, restartPolicy: Always is an invalid field for a container specification and should be removed. 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. Consider using a more secure alternative for Docker-in-Docker, such as rootless mode or a specialized runtime like Sysbox, to avoid privileged: true.
References
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the
DOCKER_TLS_CERTDIRenvironment variable to an empty string.
| image: fra.ocir.io/idqj093njucb/docker:dind | ||
| restartPolicy: Always | ||
| securityContext: | ||
| privileged: true |
There was a problem hiding this comment.
The dind container is configured with privileged: true, which is a significant security risk as it grants excessive permissions and could lead to container escape, especially for CI runners executing untrusted code. It is also incorrectly configured as an initContainer. initContainers must run to completion, but dockerd is a long-lived process, causing the pod to get stuck in the Init state. The dind container should be moved to the containers section to run as a sidecar. Additionally, restartPolicy: Always is an invalid field for a container specification and should be removed. 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. Consider using a more secure alternative for Docker-in-Docker, such as rootless mode or a specialized runtime like Sysbox, to avoid privileged: true.
References
- When using a Docker-in-Docker (DinD) setup, it is necessary to disable TLS for the Docker socket by setting the
DOCKER_TLS_CERTDIRenvironment variable to an empty string.
|
|
||
| ```bash | ||
| kubectl create namespace actions-runner-system | ||
| kubectl apply -f scripts/k8s-runner-resources/arc-secret-template.yaml |
There was a problem hiding this comment.
The file path scripts/k8s-runner-resources/arc-secret-template.yaml is relative to the repository root. This also applies to the paths used in the helm install commands below. It would be clearer to instruct the user to run these commands from the root of the repository to avoid 'file not found' errors. Consider adding a note at the beginning of the section, for example: Note: All commands should be run from the root of the repository.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly-benchmark.yml:
- Line 191: The runs-on value in the GitHub Actions workflow is malformed: the
string for the runner label in the runs-on key (runs-on) contains an extra
trailing double-quote (`4-gpu-h100"`). Edit the workflow so the runs-on entry
uses the correct label without the stray quote (e.g., change the runs-on value
to 4-gpu-h100 or a properly quoted "self-hosted", "4-gpu-h100" as appropriate)
to restore proper runner matching for the job.
In `@scripts/k8s-runner-resources/arc-secret-template.yaml`:
- Line 5: The Secret's namespace is wrong: change the metadata namespace value
from "actions-runner-systemapp" to "actions-runner-system" so the
github-arc-secret is created in the same namespace used by ARC components and
runner scale sets; update the Namespace field (the "namespace:" entry /
metadata.namespace for the Secret) accordingly to match the documented
actions-runner-system namespace.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9dbe75b-96b9-41f4-96fc-9b20c1daa033
📒 Files selected for processing (8)
.github/workflows/nightly-benchmark.ymlscripts/k8s-runner-resources/README.mdscripts/k8s-runner-resources/arc-secret-template.yamlscripts/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-general.yamlscripts/k8s-runner-resources/runner-values-4-gpu-h100.yamlscripts/k8s-runner-resources/runner-values-cpu.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-secret-template.yaml`:
- Around line 8-10: Replace the concrete-looking values for the GitHub app
fields with explicit placeholders: change github_app_id and
github_app_installation_id to clear tokens like <GITHUB_APP_ID> and
<GITHUB_APP_INSTALLATION_ID>, and ensure github_app_private_key is a descriptive
placeholder (e.g. <GITHUB_APP_PRIVATE_KEY>) so the template’s keys
(github_app_id, github_app_installation_id, github_app_private_key) cannot be
mistaken for real IDs across environments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5751b111-7b26-485f-a9d1-f95efb1fd16f
📒 Files selected for processing (6)
.github/workflows/nightly-benchmark.ymlscripts/k8s-runner-resources/arc-runner-autoscaler.yamlscripts/k8s-runner-resources/arc-runner-cpu.yamlscripts/k8s-runner-resources/arc-runner-gpu.yamlscripts/k8s-runner-resources/arc-runner-rbac.yamlscripts/k8s-runner-resources/arc-secret-template.yaml
💤 Files with no reviewable changes (4)
- scripts/k8s-runner-resources/arc-runner-cpu.yaml
- scripts/k8s-runner-resources/arc-runner-rbac.yaml
- scripts/k8s-runner-resources/arc-runner-gpu.yaml
- scripts/k8s-runner-resources/arc-runner-autoscaler.yaml
| github_app_id: "2703284" | ||
| github_app_installation_id: "105440663" | ||
| github_app_private_key: "github-app-private-key" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Use explicit placeholders instead of concrete IDs in the template.
For a reusable template, concrete-looking IDs on Lines 8-9 can cause accidental misconfiguration across environments. Prefer obvious placeholders (for example <GITHUB_APP_ID> and <GITHUB_APP_INSTALLATION_ID>).
Suggested template tweak
stringData:
- github_app_id: "2703284"
- github_app_installation_id: "105440663"
- github_app_private_key: "github-app-private-key"
+ github_app_id: "<GITHUB_APP_ID>"
+ github_app_installation_id: "<GITHUB_APP_INSTALLATION_ID>"
+ github_app_private_key: "<GITHUB_APP_PRIVATE_KEY_PEM>"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| github_app_id: "2703284" | |
| github_app_installation_id: "105440663" | |
| github_app_private_key: "github-app-private-key" | |
| github_app_id: "<GITHUB_APP_ID>" | |
| github_app_installation_id: "<GITHUB_APP_INSTALLATION_ID>" | |
| github_app_private_key: "<GITHUB_APP_PRIVATE_KEY_PEM>" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/arc-secret-template.yaml` around lines 8 - 10,
Replace the concrete-looking values for the GitHub app fields with explicit
placeholders: change github_app_id and github_app_installation_id to clear
tokens like <GITHUB_APP_ID> and <GITHUB_APP_INSTALLATION_ID>, and ensure
github_app_private_key is a descriptive placeholder (e.g.
<GITHUB_APP_PRIVATE_KEY>) so the template’s keys (github_app_id,
github_app_installation_id, github_app_private_key) cannot be mistaken for real
IDs across environments.
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/runner-values-1-gpu-h100.yaml`:
- Around line 62-75: The runner pod only sets a GPU limit and lacks
CPU/memory/ephemeral-storage requests/limits (and the dind container has no
resource bounds), causing scheduling/eviction risk; update the resources block
for the runner container (container name "runner") to include explicit requests
and limits for cpu, memory and ephemeral-storage, and add a similar resources
block for the dind container (container name "dind") with sane request/limit
values for CPU, memory and ephemeral-storage; ensure both containers include
both requests and limits so the kube-scheduler and eviction logic can operate
predictably.
- Around line 59-60: The emptyDir used for the volume named "docker-storage" is
unbounded and can let Docker layer storage consume all node ephemeral disk;
update the volume spec for name: docker-storage to bound ephemeral storage
(e.g., add emptyDir.sizeLimit with an appropriate value such as "20Gi") or
switch to a disk-backed alternative if required, ensuring the change is made on
the volume named "docker-storage" in the manifest so runner nodes cannot be
destabilized by unbounded image layers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e865404-c242-428f-b872-913477c7455b
📒 Files selected for processing (1)
scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml
| - name: docker-storage | ||
| emptyDir: {} |
There was a problem hiding this comment.
Bound docker-storage to prevent node disk exhaustion.
emptyDir for Docker layer storage is unbounded here; heavy image builds can fill node ephemeral disk and destabilize runner nodes.
Suggested fix
- name: docker-storage
- emptyDir: {}
+ emptyDir:
+ sizeLimit: 200Gi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - name: docker-storage | |
| emptyDir: {} | |
| - name: docker-storage | |
| emptyDir: | |
| sizeLimit: 200Gi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml` around lines 59 -
60, The emptyDir used for the volume named "docker-storage" is unbounded and can
let Docker layer storage consume all node ephemeral disk; update the volume spec
for name: docker-storage to bound ephemeral storage (e.g., add
emptyDir.sizeLimit with an appropriate value such as "20Gi") or switch to a
disk-backed alternative if required, ensuring the change is made on the volume
named "docker-storage" in the manifest so runner nodes cannot be destabilized by
unbounded image layers.
| containers: | ||
| - name: runner | ||
| image: fra.ocir.io/idqj093njucb/action-runner:v0.0.2 | ||
| command: | ||
| - /home/runner/run.sh | ||
| env: | ||
| - name: DOCKER_HOST | ||
| value: unix:///var/run/docker.sock | ||
| - name: RUNNER_WAIT_FOR_DOCKER_IN_SECONDS | ||
| value: '120' | ||
| resources: | ||
| limits: | ||
| nvidia.com/gpu: 1 | ||
| volumeMounts: |
There was a problem hiding this comment.
Add CPU/memory/ephemeral-storage requests and limits for runner + dind.
Currently only GPU limit is set on the runner, and dind has no resource bounds. This risks scheduling contention and evictions under parallel CI load.
Suggested fix
containers:
- name: runner
image: fra.ocir.io/idqj093njucb/action-runner:v0.0.2
@@
resources:
+ requests:
+ cpu: "4"
+ memory: "16Gi"
+ ephemeral-storage: "40Gi"
limits:
+ cpu: "8"
+ memory: "32Gi"
+ ephemeral-storage: "120Gi"
nvidia.com/gpu: 1
@@
initContainers:
@@
- name: dind
@@
+ resources:
+ requests:
+ cpu: "2"
+ memory: "8Gi"
+ ephemeral-storage: "40Gi"
+ limits:
+ cpu: "4"
+ memory: "16Gi"
+ ephemeral-storage: "120Gi"Also applies to: 99-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/runner-values-1-gpu-h100.yaml` around lines 62 -
75, The runner pod only sets a GPU limit and lacks CPU/memory/ephemeral-storage
requests/limits (and the dind container has no resource bounds), causing
scheduling/eviction risk; update the resources block for the runner container
(container name "runner") to include explicit requests and limits for cpu,
memory and ephemeral-storage, and add a similar resources block for the dind
container (container name "dind") with sane request/limit values for CPU, memory
and ephemeral-storage; ensure both containers include both requests and limits
so the kube-scheduler and eviction logic can operate predictably.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 159bbd0a06
ℹ️ 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".
| stringData: | ||
| github_app_id: "2703284" | ||
| github_app_installation_id: "105440663" | ||
| github_app_private_key: "github-app-private-key" |
There was a problem hiding this comment.
The template stores github_app_private_key as a double-quoted scalar, but this key is a multi-line PEM; if operators follow the README and paste the PEM contents here, YAML folds embedded line breaks into spaces, so ARC receives an invalid private key and cannot authenticate to GitHub to create runner registration tokens. Use a literal block scalar (e.g. |) or escaped \n formatting so the original line breaks are preserved.
Useful? React with 👍 / 👎.
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
2 participants
Description
Problem
The GitHub Actions self-hosted runners are using the legacy summerwind/actions-runner-controller CRD-based configuration (RunnerDeployment). This needs to be migrated to the newer actions.github.com/v1alpha1 ARC (Actions Runner Controller) runner scale set approach, which uses Helm-based gha-runner-scale-set charts for better scalability and official GitHub support.
Solution
Migrate to the official ARC runner scale set model by:
Changes
names (4-gpu-h100)
set installation, upgrading, and uninstalling
Test Plan
Checklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
Documentation
Chores
Workflow