Repository navigation
refactor(ci): Add actions.summerwind.dev ARC runner deployment option - #797
Conversation
github-actions
Bot
added
the
documentation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds ARC-based GitHub Actions runner manifests and docs: RBAC, CPU and multiple GPU RunnerDeployments, HorizontalRunnerAutoscaler resources, and README instructions for installing actions.summerwind.dev/ARC and applying the manifests. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub
participant ARC_Controller as "actions-runner-controller"
participant K8s_API as "Kubernetes API"
participant RunnerPods as "Runner Pods / DinD"
GitHub->>ARC_Controller: Workflows queued / Runner registration via GitHub App
ARC_Controller->>K8s_API: Create RunnerDeployment & RunnerPod resources
K8s_API->>RunnerPods: Schedule Pod on GPU/CPU node (nodeSelector/tolerations)
RunnerPods->>GitHub: Register ephemeral runner and accept jobs
GitHub->>RunnerPods: Dispatch workflow jobs
RunnerPods->>RunnerPods: Job execution (uses DinD, model cache)
K8s_API->>ARC_Controller: HorizontalRunnerAutoscaler metrics (queued/busy)
ARC_Controller->>K8s_API: Scale RunnerDeployment (update replicas)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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 expands the available options for deploying GitHub Actions self-hosted runners on Kubernetes. It provides a new, fully documented method utilizing the Highlights
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. 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 an alternative deployment method for ARC runners using the actions.summerwind.dev controller, adding new Kubernetes manifests and updating documentation. My review found several critical and high-severity issues in the new manifests. The RBAC role for runner pods is overly permissive, creating a security risk. The CPU runner's Docker-in-Docker configuration is incomplete, lacking proper setup for the Docker socket and requiring DOCKER_TLS_CERTDIR to be explicitly set to an empty string, which will cause Docker-dependent jobs to fail. Additionally, a GPU runner deployment is missing necessary CPU and memory resource definitions and also requires the DOCKER_TLS_CERTDIR environment variable to be set for its Docker-in-Docker setup, potentially causing instability. Finally, the CPU runner's autoscaler configuration contains invalid parameters that will prevent it from functioning correctly. Addressing these points will improve the security, stability, and functionality of the new runner deployments.
| metrics: | ||
| - type: PercentageRunnersBusy | ||
| scaleUpThreshold: "0.95" | ||
| scaleDownThreshold: "0.25" | ||
| scaleUpFactor: "0.5" | ||
| scaleDownFactor: "0.5" |
There was a problem hiding this comment.
The arc-cpu-runner-autoscaler is configured to use the PercentageRunnersBusy metric, but it includes scaleUpFactor and scaleDownFactor fields. These fields are only valid for the TotalNumberOfQueuedAndInProgressWorkflowRuns metric type and will be ignored or cause an error here. This will prevent the autoscaler from functioning as expected.
metrics:
- type: PercentageRunnersBusy
scaleUpThreshold: "0.95"
scaleDownThreshold: "0.25"| spec: | ||
| ephemeral: true | ||
| repository: lightseekorg/smg | ||
| labels: | ||
| - k8s-runner-cpu | ||
| serviceAccountName: arc-runner-sa | ||
|
|
||
| containers: | ||
| - name: runner | ||
| image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1 | ||
| resources: | ||
| requests: | ||
| cpu: "8" | ||
| memory: "16Gi" | ||
| limits: | ||
| cpu: "8" | ||
| memory: "16Gi" | ||
| env: | ||
| - name: HF_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: HUGGINGFACE_API_KEY | ||
| name: huggingface-secret | ||
| - name: OPENAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: OPENAI_API_KEY | ||
| name: openai-api-key | ||
| - name: ANTHROPIC_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: ANTHROPIC_API_KEY | ||
| name: anthropic-api-key | ||
| - name: XAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: XAI_API_KEY | ||
| name: xai-api-key | ||
| - name: docker | ||
| image: fra.ocir.io/idqj093njucb/docker:dind |
There was a problem hiding this comment.
The Docker-in-Docker (dind) configuration for the CPU runner is incomplete. The runner container is missing the DOCKER_HOST environment variable and volume mounts for the Docker socket. The docker sidecar is missing the privileged security context, resource definitions, and volume mounts required for it to function correctly. Additionally, the DOCKER_TLS_CERTDIR environment variable must be set to an empty string in the docker sidecar to disable TLS for the Docker socket, which is necessary for DinD setups. This will cause any Docker operations in workflows on this runner to fail. The configuration should be updated to properly set up the dind sidecar and the communication between the two containers, similar to the GPU runner definitions.
spec:
ephemeral: true
repository: lightseekorg/smg
labels:
- k8s-runner-cpu
serviceAccountName: arc-runner-sa
volumes:
- name: docker-sock
emptyDir: {}
- name: docker-storage
emptyDir: {}
containers:
- name: runner
image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1
resources:
requests:
cpu: "8"
memory: "16Gi"
limits:
cpu: "8"
memory: "16Gi"
volumeMounts:
- name: docker-sock
mountPath: /var/run
env:
- name: DOCKER_HOST
value: unix:///var/run/docker.sock
- name: HF_TOKEN
valueFrom:
secretKeyRef:
key: HUGGINGFACE_API_KEY
name: huggingface-secret
- name: OPENAI_API_KEY
valueFrom:
secretKeyRef:
key: OPENAI_API_KEY
name: openai-api-key
- name: ANTHROPIC_API_KEY
valueFrom:
secretKeyRef:
key: ANTHROPIC_API_KEY
name: anthropic-api-key
- name: XAI_API_KEY
valueFrom:
secretKeyRef:
key: XAI_API_KEY
name: xai-api-key
- name: docker
image: fra.ocir.io/idqj093njucb/docker:dind
securityContext:
privileged: true
resources:
requests:
cpu: "1"
memory: "2Gi"
limits:
cpu: "2"
memory: "4Gi"
env:
- name: DOCKER_TLS_CERTDIR
value: ""
- name: DOCKER_DRIVER
value: overlay2
volumeMounts:
- name: docker-sock
mountPath: /var/run
- name: docker-storage
mountPath: /var/lib/dockerReferences
- 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.
| containers: | ||
| - name: runner | ||
| image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1 | ||
| resources: | ||
| limits: | ||
| nvidia.com/gpu: 4 | ||
| volumeMounts: | ||
| - name: model-cache | ||
| mountPath: /models | ||
| - name: docker-sock | ||
| mountPath: /var/run | ||
| - name: dshm | ||
| mountPath: /dev/shm | ||
| env: | ||
| - name: DOCKER_HOST | ||
| value: unix:///var/run/docker.sock | ||
| - name: HF_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: HUGGINGFACE_API_KEY | ||
| name: huggingface-secret | ||
| - name: OPENAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: OPENAI_API_KEY | ||
| name: openai-api-key | ||
| - name: ANTHROPIC_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: ANTHROPIC_API_KEY | ||
| name: anthropic-api-key | ||
| - name: XAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: XAI_API_KEY | ||
| name: xai-api-key | ||
| - name: docker | ||
| 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 | ||
| volumeMounts: | ||
| - name: docker-sock | ||
| mountPath: /var/run | ||
| - name: docker-storage | ||
| mountPath: /var/lib/docker |
There was a problem hiding this comment.
The arc-runner-gpu-a10 deployment is missing CPU and memory resource requests and limits for both the runner and docker containers. Additionally, for the docker sidecar, the DOCKER_TLS_CERTDIR environment variable must be set to an empty string to disable TLS for the Docker socket, which is necessary for DinD setups. This results in a lower Quality of Service (QoS) class, making the pods more likely to be evicted under node pressure. It is a best practice to explicitly define resources for all containers to ensure predictable performance and scheduling.
containers:
- name: runner
image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1
resources:
requests:
cpu: "16"
memory: "64Gi"
limits:
cpu: "16"
memory: "64Gi"
nvidia.com/gpu: 4
volumeMounts:
- name: model-cache
mountPath: /models
- name: docker-sock
mountPath: /var/run
- name: dshm
mountPath: /dev/shm
env:
- name: DOCKER_HOST
value: unix:///var/run/docker.sock
- name: HF_TOKEN
valueFrom:
secretKeyRef:
key: HUGGINGFACE_API_KEY
name: huggingface-secret
- name: OPENAI_API_KEY
valueFrom:
secretKeyRef:
key: OPENAI_API_KEY
name: openai-api-key
- name: ANTHROPIC_API_KEY
valueFrom:
secretKeyRef:
key: ANTHROPIC_API_KEY
name: anthropic-api-key
- name: XAI_API_KEY
valueFrom:
secretKeyRef:
key: XAI_API_KEY
name: xai-api-key
- name: docker
image: fra.ocir.io/idqj093njucb/docker:dind
securityContext:
privileged: true # Required for DinD
resources:
requests:
cpu: "1"
memory: "2Gi"
limits:
cpu: "2"
memory: "4Gi"
env:
- name: DOCKER_TLS_CERTDIR
value: "" # Disables TLS for shared socket use
- name: DOCKER_DRIVER
value: overlay2
volumeMounts:
- name: docker-sock
mountPath: /var/run
- name: docker-storage
mountPath: /var/lib/dockerReferences
- 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.
| - pods | ||
| - pods/log | ||
| - pods/exec |
There was a problem hiding this comment.
The Role for the runner pods grants permissions for pods/log and pods/exec. This is overly permissive and violates the principle of least privilege. A standard runner pod does not need to execute commands in or view logs of other pods. These permissions could be abused if a workflow is compromised and should be removed to enhance security.
- pods![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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-autoscaler.yaml`:
- Around line 104-109: The PercentageRunnersBusy metric configuration uses scale
factors that decrease capacity on scale-up; update the metric block (metrics /
type: PercentageRunnersBusy) so that scaleUpFactor is greater than 1 (e.g.,
"1.4" or "1.5") to increase runners when busy and scaleDownFactor remains less
than 1 (e.g., "0.7") to reduce runners when underutilized, keeping the existing
thresholds (scaleUpThreshold and scaleDownThreshold) as-is.
In `@scripts/k8s-runner-resources/arc-runner-cpu.yaml`:
- Around line 47-48: The docker sidecar container (name: docker) is missing
critical DinD configuration: add securityContext.privileged: true to the docker
container, add volumeMounts for the docker socket and storage (mounts named
docker-sock and docker-storage) and ensure matching volumes are defined at the
pod level, add environment variables DOCKER_TLS_CERTDIR (empty string) and
DOCKER_DRIVER (e.g., overlay2) to the docker container, and add appropriate
resources.requests and resources.limits (cpu/memory) similar to the GPU runner's
docker sidecar so DinD can run properly and the runner can access the
socket/storage.
- Around line 16-48: The CPU deployment is missing the Docker socket and related
volumes/volumeMounts so the runner container cannot talk to the dind container;
add a top-level volumes block defining docker-sock (hostPath
/var/run/docker.sock), docker-storage (emptyDir) and dshm (emptyDir with medium:
Memory) and update the runner container (name: runner) to include volumeMounts
for docker-sock (mountPath: /var/run/docker.sock), docker-storage (mountPath:
/var/lib/docker) and dshm (mountPath: /dev/shm); ensure the dind container
(name: docker) also mounts those same volumes so DOCKER_HOST:
unix:///var/run/docker.sock works correctly.
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml`:
- Around line 168-215: The arc-runner-gpu-a10 deployment is missing resource
requests for the runner container, resources for the docker sidecar, and a
memory-backed docker-storage volume; update the YAML to match other GPU
deployments by adding a resources block to the runner container (keep
limits.nvidia.com/gpu: 4 and add cpu and memory requests and limits), add a
resources.requests/limits block to the docker sidecar container (name: docker)
similar to other DinD sidecars, and change the docker-storage volume definition
(name: docker-storage) to an emptyDir with medium: Memory and sizeLimit: 4Gi to
make it memory-backed. Ensure you modify the runner and docker container specs
and the docker-storage volume declaration accordingly.
In `@scripts/k8s-runner-resources/arc-runner-rbac.yaml`:
- Around line 13-20: Replace the misleading "# Argo Workflows" comment above the
RBAC block with an accurate label for ARC/GitHub Actions runners (e.g., "#
Actions Runner Controller (ARC) Runners" or "# ARC runners") so the comment
correctly reflects the RBAC purpose; update the comment that immediately
precedes the secrets resource block (the line containing "# Argo Workflows") to
the new text in the same location.
- Around line 22-31: The RBAC rule for the Pods resource includes the pods/exec
subresource but only grants read verbs (get, list, watch), which prevents exec
operations; update the verbs array for the Pods rule that lists "pods" and
"pods/exec" to include "create" if runners should be able to exec into pods, or
add a clear inline comment next to the resources/verbs block (referencing the
Pods rule and the "pods/exec" entry) stating the intent is read-only and that
"create" was intentionally omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d7c51f99-fc49-4531-b171-992bec599165
📒 Files selected for processing (5)
scripts/k8s-runner-resources/README.mdscripts/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.yaml
| metrics: | ||
| - type: PercentageRunnersBusy | ||
| scaleUpThreshold: "0.95" | ||
| scaleDownThreshold: "0.25" | ||
| scaleUpFactor: "0.5" | ||
| scaleDownFactor: "0.5" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
actions-runner-controller PercentageRunnersBusy scaleUpFactor scaleDownFactor configuration
💡 Result:
In Actions Runner Controller (ARC), PercentageRunnersBusy is a HorizontalRunnerAutoscaler.spec.metrics[].type that scales a RunnerDeployment based on the percentage of runners currently busy (running jobs). ARC queries GitHub for the number of busy runners in the RunnerDeployment’s namespace, then adjusts the desired replica count when thresholds are crossed. [1]
Example configuration
apiVersion: actions.summerwind.dev/v1alpha1
kind: HorizontalRunnerAutoscaler
spec:
scaleTargetRef:
name: example-runner-deployment
minReplicas: 1
maxReplicas: 10
metrics:
- type: PercentageRunnersBusy
scaleUpThreshold: "0.75"
scaleDownThreshold: "0.30"
scaleUpFactor: "1.4"
scaleDownFactor: "0.7"[1][2]
What the fields mean (for PercentageRunnersBusy)
scaleUpThreshold: if% busyis greater than this value, ARC re-evaluates desired runners to scale up. [2]scaleDownThreshold: if% busyis less than this value, ARC re-evaluates desired runners to scale down. [2][3]scaleUpFactor: multiplier applied to the current runner count to compute the new desired count on scale-up (e.g., 10 →10 * 1.4→ 14). [1][2]scaleDownFactor: multiplier applied to the current runner count to compute the new desired count on scale-down (e.g., 10 →10 * 0.7→ 7). [1][2]
Notes:
- You typically pair these with
minReplicas/maxReplicasbounds. [1] - ARC supports either factor-based scaling (
scaleUpFactor/scaleDownFactor) or step-based scaling (scaleUpAdjustment/scaleDownAdjustment) for a metric (not both). [3]
Sources:
[1] ARC docs (actions.github.io/actions-runner-controller)
[2] ARC API docs on pkg.go.dev (actions-runner-controller)
[3] HorizontalRunnerAutoscalerMetrics docs (docs.rs)
Correct scaleUpFactor and scaleDownFactor values for PercentageRunnersBusy metric.
The format of scaleUpFactor: "0.5" and scaleDownFactor: "0.5" as strings is correct, but the values are problematic. These fields are multipliers applied to the current runner count:
- When
scaleUpThreshold: "0.95"is exceeded (≥95% runners busy), the system appliesscaleUpFactor: "0.5", which reduces the runner count to 50% of current (e.g., 4 → 2 runners). This is the opposite of the intended behavior—you should add runners when they are busy, not remove them. - Similarly,
scaleDownFactor: "0.5"also reduces runners, making both directions reduce capacity.
Use a factor greater than 1 for scaleUpFactor (e.g., "1.5" or "1.4") to increase runners, and keep scaleDownFactor less than 1 (e.g., "0.7") to decrease runners during low usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/arc-runner-autoscaler.yaml` around lines 104 -
109, The PercentageRunnersBusy metric configuration uses scale factors that
decrease capacity on scale-up; update the metric block (metrics / type:
PercentageRunnersBusy) so that scaleUpFactor is greater than 1 (e.g., "1.4" or
"1.5") to increase runners when busy and scaleDownFactor remains less than 1
(e.g., "0.7") to reduce runners when underutilized, keeping the existing
thresholds (scaleUpThreshold and scaleDownThreshold) as-is.
| containers: | ||
| - name: runner | ||
| image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1 | ||
| resources: | ||
| requests: | ||
| cpu: "8" | ||
| memory: "16Gi" | ||
| limits: | ||
| cpu: "8" | ||
| memory: "16Gi" | ||
| env: | ||
| - name: HF_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: HUGGINGFACE_API_KEY | ||
| name: huggingface-secret | ||
| - name: OPENAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: OPENAI_API_KEY | ||
| name: openai-api-key | ||
| - name: ANTHROPIC_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: ANTHROPIC_API_KEY | ||
| name: anthropic-api-key | ||
| - name: XAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: XAI_API_KEY | ||
| name: xai-api-key | ||
| - name: docker | ||
| image: fra.ocir.io/idqj093njucb/docker:dind |
There was a problem hiding this comment.
Missing volumes and volume mounts for Docker socket sharing.
The runner container references DOCKER_HOST: unix:///var/run/docker.sock in GPU deployments, but this CPU deployment is missing:
- The
volumessection entirely (nodocker-sock,docker-storage,dshmvolumes) - Volume mounts in the runner container
Without shared volumes, the runner and DinD containers cannot communicate.
Proposed fix to add volumes section
serviceAccountName: arc-runner-sa
+
+ volumes:
+ - name: docker-sock
+ emptyDir: {}
+ - name: docker-storage
+ emptyDir:
+ medium: Memory
+ sizeLimit: 4Gi
containers:
- name: runner
image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1
resources:
requests:
cpu: "8"
memory: "16Gi"
limits:
cpu: "8"
memory: "16Gi"
+ volumeMounts:
+ - name: docker-sock
+ mountPath: /var/run
env:
+ - name: DOCKER_HOST
+ value: unix:///var/run/docker.sock
- name: HF_TOKEN🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/arc-runner-cpu.yaml` around lines 16 - 48, The
CPU deployment is missing the Docker socket and related volumes/volumeMounts so
the runner container cannot talk to the dind container; add a top-level volumes
block defining docker-sock (hostPath /var/run/docker.sock), docker-storage
(emptyDir) and dshm (emptyDir with medium: Memory) and update the runner
container (name: runner) to include volumeMounts for docker-sock (mountPath:
/var/run/docker.sock), docker-storage (mountPath: /var/lib/docker) and dshm
(mountPath: /dev/shm); ensure the dind container (name: docker) also mounts
those same volumes so DOCKER_HOST: unix:///var/run/docker.sock works correctly.
| - name: docker | ||
| image: fra.ocir.io/idqj093njucb/docker:dind |
There was a problem hiding this comment.
Docker-in-Docker sidecar is missing critical configuration.
The docker container is incomplete compared to the GPU runner manifests. It's missing:
securityContext.privileged: true(required for DinD)- Volume mounts for
docker-sockanddocker-storage - Environment variables (
DOCKER_TLS_CERTDIR,DOCKER_DRIVER) - Resource requests/limits
Without these, the DinD sidecar will fail to function, and the runner container won't be able to use Docker.
Proposed fix based on GPU runner configuration
- name: docker
image: fra.ocir.io/idqj093njucb/docker:dind
+ securityContext:
+ privileged: true # Required for DinD
+ resources:
+ requests:
+ cpu: "1"
+ memory: "2Gi"
+ limits:
+ cpu: "2"
+ memory: "4Gi"
+ env:
+ - name: DOCKER_TLS_CERTDIR
+ value: "" # Disables TLS for shared socket use
+ - name: DOCKER_DRIVER
+ value: overlay2
+ volumeMounts:
+ - name: docker-sock
+ mountPath: /var/run
+ - name: docker-storage
+ mountPath: /var/lib/docker🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/arc-runner-cpu.yaml` around lines 47 - 48, The
docker sidecar container (name: docker) is missing critical DinD configuration:
add securityContext.privileged: true to the docker container, add volumeMounts
for the docker socket and storage (mounts named docker-sock and docker-storage)
and ensure matching volumes are defined at the pod level, add environment
variables DOCKER_TLS_CERTDIR (empty string) and DOCKER_DRIVER (e.g., overlay2)
to the docker container, and add appropriate resources.requests and
resources.limits (cpu/memory) similar to the GPU runner's docker sidecar so DinD
can run properly and the runner can access the socket/storage.
| containers: | ||
| - name: runner | ||
| image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1 | ||
| resources: | ||
| limits: | ||
| nvidia.com/gpu: 4 | ||
| volumeMounts: | ||
| - name: model-cache | ||
| mountPath: /models | ||
| - name: docker-sock | ||
| mountPath: /var/run | ||
| - name: dshm | ||
| mountPath: /dev/shm | ||
| env: | ||
| - name: DOCKER_HOST | ||
| value: unix:///var/run/docker.sock | ||
| - name: HF_TOKEN | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: HUGGINGFACE_API_KEY | ||
| name: huggingface-secret | ||
| - name: OPENAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: OPENAI_API_KEY | ||
| name: openai-api-key | ||
| - name: ANTHROPIC_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: ANTHROPIC_API_KEY | ||
| name: anthropic-api-key | ||
| - name: XAI_API_KEY | ||
| valueFrom: | ||
| secretKeyRef: | ||
| key: XAI_API_KEY | ||
| name: xai-api-key | ||
| - name: docker | ||
| 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 | ||
| volumeMounts: | ||
| - name: docker-sock | ||
| mountPath: /var/run | ||
| - name: docker-storage | ||
| mountPath: /var/lib/docker |
There was a problem hiding this comment.
Inconsistent configuration in arc-runner-gpu-a10 deployment.
This deployment differs from the other GPU deployments in several ways:
- Missing runner container resource requests (lines 171-173): Only
limits.nvidia.com/gpuis specified; no CPU/memory requests like other deployments have. - Missing docker container resources (lines 204-215): No resource requests/limits for the DinD sidecar.
docker-storagevolume not memory-backed (lines 161-162): Other deployments usemedium: MemorywithsizeLimit: 4Gi.
These inconsistencies may cause scheduling issues or resource contention.
Proposed fix to align with other GPU deployments
containers:
- name: runner
image: fra.ocir.io/idqj093njucb/action-runner:v0.0.1
resources:
+ requests:
+ cpu: "32"
+ memory: "128Gi"
limits:
nvidia.com/gpu: 4 - name: docker-storage
- emptyDir: {}
+ emptyDir:
+ medium: Memory
+ sizeLimit: 4Gi - name: docker
image: fra.ocir.io/idqj093njucb/docker:dind
securityContext:
privileged: true # Required for DinD
+ resources:
+ requests:
+ cpu: "1"
+ memory: "2Gi"
+ limits:
+ cpu: "2"
+ memory: "4Gi"
env:🤖 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 168 - 215, The
arc-runner-gpu-a10 deployment is missing resource requests for the runner
container, resources for the docker sidecar, and a memory-backed docker-storage
volume; update the YAML to match other GPU deployments by adding a resources
block to the runner container (keep limits.nvidia.com/gpu: 4 and add cpu and
memory requests and limits), add a resources.requests/limits block to the docker
sidecar container (name: docker) similar to other DinD sidecars, and change the
docker-storage volume definition (name: docker-storage) to an emptyDir with
medium: Memory and sizeLimit: 4Gi to make it memory-backed. Ensure you modify
the runner and docker container specs and the docker-storage volume declaration
accordingly.
| # Pods | ||
| - apiGroups: [""] | ||
| resources: | ||
| - pods | ||
| - pods/log | ||
| - pods/exec | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other runner RBAC files in the repo grant create verb for pods/exec
rg -n "pods/exec" --type yaml -A 5 -B 2Repository: lightseekorg/smg
Length of output: 585
🌐 Web query:
Kubernetes RBAC pods/exec create verb permission required
💡 Result:
To allow kubectl exec via Kubernetes RBAC, you must grant the create verb on the pods/exec subresource (core API group ""). [1], [2]
Typical minimal Role rules (namespace-scoped):
rules:
- apiGroups: [""]
resources: ["pods"]
verbs: ["get"] # kubectl commonly reads the Pod first
- apiGroups: [""]
resources: ["pods/exec"]
verbs: ["create"] # required for exec (kubectl uses POST -> RBAC "create")Note: some client libraries negotiate exec using an HTTP GET first (e.g., WebSocket), in which case you may also need verbs: ["get","create"] on pods/exec. [2]
Quick check:
kubectl auth can-i create pods/exec -n <ns>[3]
Sources:
[1] Kubernetes RBAC docs (subresources via resource/subresource) https://kubernetes.io/docs/reference/access-authn-authz/rbac/
[2] Stack Overflow: kubectl exec requires create on pods/exec (and sometimes get) https://stackoverflow.com/questions/48118125/kubernetes-rbac-role-verbs-to-exec-to-pod
[3] Kubenomicon: kubectl auth can-i create pods/exec https://kubenomicon.com/Execution/Exec_inside_container.html
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation in the repo about this RBAC config
find . -type f \( -name "*.md" -o -name "*.txt" -o -name "README*" \) | xargs grep -l "rbac\|pods/exec" 2>/dev/null || echo "No documentation found"
# Also check if there are comments in the RBAC file itself
head -50 scripts/k8s-runner-resources/arc-runner-rbac.yamlRepository: lightseekorg/smg
Length of output: 1024
🏁 Script executed:
#!/bin/bash
# Check the README for context about the runner RBAC requirements
cat scripts/k8s-runner-resources/README.mdRepository: lightseekorg/smg
Length of output: 6147
Add create verb to pods/exec or clarify intent with a comment.
Kubernetes RBAC requires the create verb on the pods/exec subresource to enable actual pod exec operations. The current permissions grant only get, list, and watch verbs, which are insufficient for exec functionality. Either add create to the verbs list if runners need to exec into pods, or add an inline comment explaining that this is intentionally limited to read-only access.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/arc-runner-rbac.yaml` around lines 22 - 31, The
RBAC rule for the Pods resource includes the pods/exec subresource but only
grants read verbs (get, list, watch), which prevents exec operations; update the
verbs array for the Pods rule that lists "pods" and "pods/exec" to include
"create" if runners should be able to exec into pods, or add a clear inline
comment next to the resources/verbs block (referencing the Pods rule and the
"pods/exec" entry) stating the intent is read-only and that "create" was
intentionally omitted.
![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: 070ea73062
ℹ️ 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".
| - name: docker | ||
| image: fra.ocir.io/idqj093njucb/docker:dind |
There was a problem hiding this comment.
This RunnerDeployment adds a docker:dind sidecar but does not wire it up for usable Docker access from the runner (no shared /var/run volume, no privileged DinD setup, and no runner-side Docker endpoint wiring). In workflows that run on k8s-runner-cpu and invoke Docker (for example container actions or docker build), jobs will fail because the runner cannot reach a functional daemon.
Useful? React with 👍 / 👎.
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
The Role includes pods/exec but only grants get, list, and watch; exec requests are authorized via the create verb on the pods/exec subresource. As written, any workflow step that uses kubectl exec from these runners will get RBAC forbidden errors even though exec access appears intended.
Useful? React with 👍 / 👎.
|
What about the old files using the github CRD? should we remove them? |
070ea73 to
7f2a061
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
scripts/k8s-runner-resources/arc-runner-autoscaler.yaml (1)
104-109:⚠️ Potential issue | 🔴 CriticalFix the CPU scale-up factor.
On Lines 106-109,
scaleUpFactor: "0.5"halves the pool when busy instead of adding runners, so the CPU HRA cannot scale out under load.Minimal patch
metrics: - type: PercentageRunnersBusy scaleUpThreshold: "0.95" scaleDownThreshold: "0.25" - scaleUpFactor: "0.5" + scaleUpFactor: "1.5" scaleDownFactor: "0.5"In `actions-runner-controller` `HorizontalRunnerAutoscaler` with metric `PercentageRunnersBusy`, how are `scaleUpFactor` and `scaleDownFactor` applied? Must `scaleUpFactor` be greater than 1 to increase replicas?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/k8s-runner-resources/arc-runner-autoscaler.yaml` around lines 104 - 109, The HorizontalRunnerAutoscaler metric block using PercentageRunnersBusy has the scaleUpFactor set to "0.5", which decreases replicas when busy; change scaleUpFactor to a value greater than 1 (for example "1.5") so that the autoscaler increases replicas under load; update the metrics entry that contains PercentageRunnersBusy (the scaleUpFactor and optionally scaleDownFactor fields) to use the correct >1 scaleUpFactor to enable scale-out.scripts/k8s-runner-resources/arc-runner-gpu.yaml (1)
174-224:⚠️ Potential issue | 🟠 MajorBring
arc-runner-gpu-a10back in line with the other GPU pools.On Lines 177-182, the runner requests only ephemeral storage, so CPU and memory are left unreserved, and on Lines 213-224 the DinD sidecar has no resource envelope at all. The other GPU RunnerDeployments pin those values, so this pool will schedule and behave differently under load.
#!/bin/bash set -euo pipefail python -m pip install --quiet pyyaml >/dev/null 2>&1 python - <<'PY' import yaml from pathlib import Path docs = [d for d in yaml.safe_load_all(Path("scripts/k8s-runner-resources/arc-runner-gpu.yaml").read_text()) if d] for name in ("arc-runner-4-gpu-h100", "arc-runner-gpu-a10"): doc = next(d for d in docs if d["metadata"]["name"] == name) spec = doc["spec"]["template"]["spec"] print(f"\n{name}") for c in spec["containers"]: print(c["name"], { "requests": c.get("resources", {}).get("requests"), "limits": c.get("resources", {}).get("limits"), "env": [e["name"] for e in c.get("env", [])], }) PY🤖 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 174 - 224, The "arc-runner-gpu-a10" Deployment's runner container only requests ephemeral-storage and omits CPU/memory requests/limits, and the docker sidecar has no resources at all; update the containers named "runner" and "docker" to include the same resources.requests and resources.limits entries used by the other GPU pools (add CPU and memory request/limit values alongside ephemeral-storage and keep nvidia.com/gpu in limits for "runner"), modifying the resources block under each container (resources.requests and resources.limits) so both containers have explicit CPU and memory reservations and limits to match the other GPU RunnerDeployments.scripts/k8s-runner-resources/arc-runner-cpu.yaml (1)
14-48:⚠️ Potential issue | 🔴 CriticalWire the CPU runner to a real DinD socket.
On Lines 16-48, this pod adds a
dockersidecar but never defines shared/var/runor/var/lib/dockervolumes, and the sidecar on Line 47 is missing the privileged/resource setup used inarc-runner-gpu.yaml. As written, the runner container has no path to a working Docker daemon, so Docker-based jobs on the CPU pool will fail.#!/bin/bash set -euo pipefail python -m pip install --quiet pyyaml >/dev/null 2>&1 python - <<'PY' import yaml from pathlib import Path targets = [ ("scripts/k8s-runner-resources/arc-runner-cpu.yaml", "arc-runner-cpu"), ("scripts/k8s-runner-resources/arc-runner-gpu.yaml", "arc-runner-4-gpu-h100"), ] for file, name in targets: docs = [d for d in yaml.safe_load_all(Path(file).read_text()) if d] doc = next(d for d in docs if d["metadata"]["name"] == name) spec = doc["spec"]["template"]["spec"] print(f"\n{name}") print("volumes:", [v["name"] for v in spec.get("volumes", [])]) for c in spec["containers"]: print(c["name"], { "mounts": [m["name"] for m in c.get("volumeMounts", [])], "privileged": c.get("securityContext", {}).get("privileged"), "env": [e["name"] for e in c.get("env", [])], "resources": c.get("resources"), }) PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/k8s-runner-resources/arc-runner-cpu.yaml` around lines 14 - 48, The pod mounts for the Docker sidecar are missing so the runner cannot reach a Docker daemon; add shared volumes (e.g., volume names docker-sock and docker-graph) to the pod spec and mount them into both containers: mount docker-sock at /var/run/docker.sock in the runner container and the docker sidecar, and mount docker-graph at /var/lib/docker in both containers (use emptyDir for graph storage or match arc-runner-gpu.yaml). Also update the docker sidecar container "docker" to include the same securityContext.privileged: true and the same resources (requests/limits) used in arc-runner-gpu.yaml so it runs as DinD; ensure the runner container either has the socket mount or an appropriate DOCKER_HOST env var to point to the sidecar.
🤖 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-autoscaler.yaml`:
- Around line 14-17: The autoscalers all use the same repo-wide metric
TotalNumberOfQueuedAndInProgressWorkflowRuns for repository lightseekorg/smg,
causing unrelated GPU pools to scale; update each HRA block that references
TotalNumberOfQueuedAndInProgressWorkflowRuns to scope the metric to the intended
pool by adding workflowLabels (or repository/workflow selectors) that match the
target RunnerDeployment labels or use a metric that filters by runner labels, so
each autoscaler only observes queue depth for its specific RunnerDeployment;
locate the metric entries named TotalNumberOfQueuedAndInProgressWorkflowRuns and
replace or augment repositoryNames: - lightseekorg/smg with the appropriate
workflowLabels or label-based selector for the corresponding GPU pool.
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml`:
- Around line 237-239: The GPU runner pools are missing the shared label that
workflows expect; in each pool where labels currently contain entries like
"1-gpu-h100" and "1-gpu" (the label lists at the three GPU pools), add
"k8s-runner-gpu" to the labels array so workflows using runs-on: k8s-runner-gpu
can match these GPU-capable pools; update the label lists for the pools that
currently list "1-gpu-h100" / "1-gpu" (and the other two similar GPU pools) to
include "k8s-runner-gpu".
In `@scripts/k8s-runner-resources/arc-runner-rbac.yaml`:
- Around line 13-20: The Role for arc-runner-sa currently grants namespace-wide
secrets access; remove the entire secrets rule under arc-runner-sa (the
apiGroups: [""], resources: - secrets, verbs: - get - list - watch) or restrict
it to a minimal scoped rule: only "get" and list explicit resourceNames required
by the runner. Locate the Role/ClusterRole definition that mentions
arc-runner-sa in arc-runner-rbac.yaml and either delete that secrets stanza or
replace it with a single-verb "get" rule that enumerates the exact secret names
in resourceNames to avoid namespace-wide secret enumeration.
In `@scripts/k8s-runner-resources/README.md`:
- Around line 175-189: Update the "Apply Runner Resources" section to list the
required cluster prerequisites and/or split CPU/GPU steps: explicitly document
that the manifests (arc-runner-rbac.yaml, arc-runner-cpu.yaml,
arc-runner-gpu.yaml, arc-runner-autoscaler.yaml) require pre-created secrets
huggingface-secret, openai-api-key, anthropic-api-key, xai-api-key and that GPU
runners additionally require the model-cache PersistentVolumeClaim (model-cache
PVC); either add a prerequisites subsection with commands or links to create
those secrets/PVCs before running kubectl apply, or separate the CPU and GPU
apply instructions with the GPU block noting the model-cache PVC dependency.
---
Duplicate comments:
In `@scripts/k8s-runner-resources/arc-runner-autoscaler.yaml`:
- Around line 104-109: The HorizontalRunnerAutoscaler metric block using
PercentageRunnersBusy has the scaleUpFactor set to "0.5", which decreases
replicas when busy; change scaleUpFactor to a value greater than 1 (for example
"1.5") so that the autoscaler increases replicas under load; update the metrics
entry that contains PercentageRunnersBusy (the scaleUpFactor and optionally
scaleDownFactor fields) to use the correct >1 scaleUpFactor to enable scale-out.
In `@scripts/k8s-runner-resources/arc-runner-cpu.yaml`:
- Around line 14-48: The pod mounts for the Docker sidecar are missing so the
runner cannot reach a Docker daemon; add shared volumes (e.g., volume names
docker-sock and docker-graph) to the pod spec and mount them into both
containers: mount docker-sock at /var/run/docker.sock in the runner container
and the docker sidecar, and mount docker-graph at /var/lib/docker in both
containers (use emptyDir for graph storage or match arc-runner-gpu.yaml). Also
update the docker sidecar container "docker" to include the same
securityContext.privileged: true and the same resources (requests/limits) used
in arc-runner-gpu.yaml so it runs as DinD; ensure the runner container either
has the socket mount or an appropriate DOCKER_HOST env var to point to the
sidecar.
In `@scripts/k8s-runner-resources/arc-runner-gpu.yaml`:
- Around line 174-224: The "arc-runner-gpu-a10" Deployment's runner container
only requests ephemeral-storage and omits CPU/memory requests/limits, and the
docker sidecar has no resources at all; update the containers named "runner" and
"docker" to include the same resources.requests and resources.limits entries
used by the other GPU pools (add CPU and memory request/limit values alongside
ephemeral-storage and keep nvidia.com/gpu in limits for "runner"), modifying the
resources block under each container (resources.requests and resources.limits)
so both containers have explicit CPU and memory reservations and limits to match
the other GPU RunnerDeployments.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2a4ee299-4ccc-4a1c-83e7-47fdce08bcfe
📒 Files selected for processing (5)
scripts/k8s-runner-resources/README.mdscripts/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.yaml
| metrics: | ||
| - type: TotalNumberOfQueuedAndInProgressWorkflowRuns | ||
| repositoryNames: | ||
| - lightseekorg/smg |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For actions-runner-controller/actions.summerwind.dev HorizontalRunnerAutoscaler, does the TotalNumberOfQueuedAndInProgressWorkflowRunsmetric filter by the targetRunnerDeployment labels, or does it count all queued/in-progress jobs for the listed repositories? If I create multiple HRAs for the same repository but different runner labels, will they all observe the same queue depth?
💡 Result:
The TotalNumberOfQueuedAndInProgressWorkflowRuns metric counts all queued and in-progress workflow runs across the specified repositoryNames in the HorizontalRunnerAutoscaler (HRA) spec. It does not filter by the labels of the target RunnerDeployment or RunnerSet. The metric polls GitHub's API for pending workflow runs against the listed repositories and uses this total to suggest desired replicas (up to maxReplicas), without considering which specific runners (identified by labels) those jobs are targeted to. If multiple HRAs are created for the same repository(ies) but target different RunnerDeployments with different runner labels, they will all observe the same queue depth because each HRA's metric calculation is independent and based solely on the repository-scoped workflow run counts from GitHub, not on label-matched jobs or current runner availability for those labels. Jobs queued for specific labels will only be assignable to matching runners, but the scaling decision doesn't filter the count by labels—leading to potential over-scaling across HRAs if not managed carefully (e.g., via unique repositories per HRA).
Citations:
- 1: https://github.com/actions/actions-runner-controller/blob/master/docs/automatically-scaling-runners.md
- 2: Cannot scale from zero with TotalNumberOfQueuedAndInProgressWorkflowRuns metric actions/actions-runner-controller#2850
- 3: TotalNumberOfQueuedAndInProgressWorkflowRuns does not scale when workflow run is cancelled due to concurrency rules actions/actions-runner-controller#2623
- 4: https://errorism.dev/issues/actions-actions-runner-controller-metric-totalnumberofqueuedandinprogressworkflowruns-doesnt-seem-to-line-up-with-current-workflowjo
All five GPU autoscalers will scale based on the same repo-wide queue metric, causing unrelated pools to scale unnecessarily.
The TotalNumberOfQueuedAndInProgressWorkflowRuns metric (lines 14–17, 32–35, 50–53, 68–71, 86–89) counts all queued and in-progress workflow runs across lightseekorg/smg without filtering by RunnerDeployment labels. Each GPU autoscaler observes the total repository queue depth and scales independently, so a single job queued for one GPU pool will trigger scale-up across all five pools.
Consider scoping each HRA to a dedicated repository, workflow label, or using a different metric that respects runner labels to prevent over-scaling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/arc-runner-autoscaler.yaml` around lines 14 -
17, The autoscalers all use the same repo-wide metric
TotalNumberOfQueuedAndInProgressWorkflowRuns for repository lightseekorg/smg,
causing unrelated GPU pools to scale; update each HRA block that references
TotalNumberOfQueuedAndInProgressWorkflowRuns to scope the metric to the intended
pool by adding workflowLabels (or repository/workflow selectors) that match the
target RunnerDeployment labels or use a metric that filters by runner labels, so
each autoscaler only observes queue depth for its specific RunnerDeployment;
locate the metric entries named TotalNumberOfQueuedAndInProgressWorkflowRuns and
replace or augment repositoryNames: - lightseekorg/smg with the appropriate
workflowLabels or label-based selector for the corresponding GPU pool.
| labels: | ||
| - 1-gpu-h100 | ||
| - 1-gpu |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python -m pip install --quiet pyyaml >/dev/null 2>&1
python - <<'PY'
from pathlib import Path
import yaml
docs = [d for d in yaml.safe_load_all(Path("scripts/k8s-runner-resources/arc-runner-gpu.yaml").read_text()) if d]
print("Manifest labels:")
for doc in docs:
print(f" {doc['metadata']['name']}: {doc['spec']['template']['spec'].get('labels', [])}")
print("\nWorkflow runs-on labels:")
for path in sorted(Path(".github/workflows").rglob("*.y*ml")):
data = yaml.safe_load(path.read_text()) or {}
for job_name, job in (data.get("jobs") or {}).items():
runs_on = job.get("runs-on")
labels = runs_on if isinstance(runs_on, list) else [runs_on]
labels = [label for label in labels if isinstance(label, str)]
if any("gpu" in label or label.startswith("k8s-runner") for label in labels):
print(f" {path}:{job_name}: {labels}")
PYRepository: lightseekorg/smg
Length of output: 1982
Add the shared k8s-runner-gpu label to all GPU runner pools to match workflow selectors.
The three pools at lines 237-239, 355-356, and 471-472 are missing the k8s-runner-gpu label that workflows explicitly request. Without it, jobs using runs-on: k8s-runner-gpu cannot schedule on these pools, even though they are GPU-capable.
Patch to add the missing label
labels:
- 1-gpu-h100
- 1-gpu
+ - k8s-runner-gpu
@@
labels:
- 1-gpu
+ - k8s-runner-gpu
@@
labels:
- 2-gpu-h100
+ - k8s-runner-gpu📝 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.
| labels: | |
| - 1-gpu-h100 | |
| - 1-gpu | |
| labels: | |
| - 1-gpu-h100 | |
| - 1-gpu | |
| - k8s-runner-gpu |
🤖 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 237 - 239, The
GPU runner pools are missing the shared label that workflows expect; in each
pool where labels currently contain entries like "1-gpu-h100" and "1-gpu" (the
label lists at the three GPU pools), add "k8s-runner-gpu" to the labels array so
workflows using runs-on: k8s-runner-gpu can match these GPU-capable pools;
update the label lists for the pools that currently list "1-gpu-h100" / "1-gpu"
(and the other two similar GPU pools) to include "k8s-runner-gpu".
| # Argo Workflows | ||
| - apiGroups: [""] | ||
| resources: | ||
| - secrets | ||
| verbs: | ||
| - get | ||
| - list | ||
| - watch |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Kubernetes, does a Pod that consumes a Secret via env.valueFrom.secretKeyRefor a Secret volume need the Pod's service account to have RBACget/list/watchpermissions on secrets? What is the security impact of granting a namespaced service accountget, list, and watch on all secrets in that namespace?
💡 Result:
No, a Pod that consumes a Secret via env.valueFrom.secretKeyRef or a Secret volume does not need the Pod's service account to have RBAC get/list/watch permissions on secrets. The kubelet fetches the Secret on behalf of the Pod using its own broad API permissions and mounts it into the container's filesystem or sets the environment variable during Pod startup. RBAC permissions on secrets control only direct API access from within the Pod (e.g., via kubectl or API clients), not the kubelet's mounting mechanism. Granting a namespaced service account get, list, and watch on all secrets in that namespace allows any application running in Pods using that service account to read the contents of all Secrets in the namespace via the Kubernetes API. This violates least privilege, enables compromised Pods to exfiltrate unrelated credentials (e.g., database passwords, API keys), and increases blast radius if the Pod is compromised. Official guidance recommends restricting such access, using resourceNames for specific secrets only, and preferring namespace isolation or external secret stores.
Citations:
- 1: https://kubernetes.io/docs/concepts/security/secrets-good-practices/
- 2: https://kubernetes.io/docs/concepts/configuration/secret/
- 3: https://stackoverflow.com/questions/63119568/how-is-kubernetes-rbac-actually-enforced-for-service-accounts
- 4: https://oneuptime.com/blog/post/2026-02-09-restrict-secret-access-serviceaccounts-rbac/view
- 5: https://kubernetes.io/docs/concepts/security/rbac-good-practices/
Remove namespace-wide secret access from runner jobs.
Lines 13-20 grant arc-runner-sa unrestricted get/list/watch on all secrets in the actions-runner-system namespace. Every workflow pod using this service account can enumerate and read any secret in the namespace, including controller and API-key credentials. Pod secret consumption via secretKeyRef does not require the pod's service account to have RBAC permissions on secrets; the kubelet handles secret retrieval independently. Remove this rule entirely, or if the runner genuinely needs Kubernetes API access, scope it to explicit resourceNames with get only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/arc-runner-rbac.yaml` around lines 13 - 20, The
Role for arc-runner-sa currently grants namespace-wide secrets access; remove
the entire secrets rule under arc-runner-sa (the apiGroups: [""], resources: -
secrets, verbs: - get - list - watch) or restrict it to a minimal scoped rule:
only "get" and list explicit resourceNames required by the runner. Locate the
Role/ClusterRole definition that mentions arc-runner-sa in arc-runner-rbac.yaml
and either delete that secrets stanza or replace it with a single-verb "get"
rule that enumerates the exact secret names in resourceNames to avoid
namespace-wide secret enumeration.
| ### 4. Apply Runner Resources | ||
|
|
||
| ```bash | ||
| # RBAC for runner pods | ||
| kubectl apply -f scripts/k8s-runner-resources/arc-runner-rbac.yaml | ||
|
|
||
| # CPU runner deployment | ||
| kubectl apply -f scripts/k8s-runner-resources/arc-runner-cpu.yaml | ||
|
|
||
| # GPU runner deployment | ||
| kubectl apply -f scripts/k8s-runner-resources/arc-runner-gpu.yaml | ||
|
|
||
| # Autoscaler | ||
| kubectl apply -f scripts/k8s-runner-resources/arc-runner-autoscaler.yaml | ||
| ``` |
There was a problem hiding this comment.
Document the missing cluster prerequisites before kubectl apply.
On Lines 179-188, the referenced manifests depend on preexisting huggingface-secret, openai-api-key, anthropic-api-key, xai-api-key, and, for GPU runners, the model-cache PVC. A fresh install following this section will create RunnerDeployments that either stay Pending or fail to start. Please add those prerequisites here, or split the CPU/GPU steps so the dependency surface is explicit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/k8s-runner-resources/README.md` around lines 175 - 189, Update the
"Apply Runner Resources" section to list the required cluster prerequisites
and/or split CPU/GPU steps: explicitly document that the manifests
(arc-runner-rbac.yaml, arc-runner-cpu.yaml, arc-runner-gpu.yaml,
arc-runner-autoscaler.yaml) require pre-created secrets huggingface-secret,
openai-api-key, anthropic-api-key, xai-api-key and that GPU runners additionally
require the model-cache PersistentVolumeClaim (model-cache PVC); either add a
prerequisites subsection with commands or links to create those secrets/PVCs
before running kubectl apply, or separate the CPU and GPU apply instructions
with the GPU block noting the model-cache PVC dependency.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
github-actions
Bot
added
grpc
|
Hi @XinyueZhang369, 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:
|
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)
model_gateway/src/routers/grpc/harmony/stages/request_building.rs (1)
104-112:⚠️ Potential issue | 🟠 MajorKeep Harmony stop-token injection in one layer.
These new arguments now conflict with the existing append step at Lines 226-257. On the Responses path, SGLang and vLLM already write
harmony_stop_idsinside their builders, and this stage then appends the sameprep.harmony_stop_idsagain, so the final request carries duplicated Harmony stop IDs. TensorRT-LLM still ignores the extra parameter, so backend behavior has also become inconsistent.Please pick a single owner for this wiring: either keep the stage-level injection and stop passing
prep.harmony_stop_idsinto the backend builders, or move the logic fully into each builder and delete the later append step.Also applies to: 151-158, 197-204
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model_gateway/src/routers/grpc/harmony/stages/request_building.rs` around lines 104 - 112, The Responses branch is duplicating Harmony stop-token wiring: you currently pass prep.harmony_stop_ids into sglang_client.build_generate_request_from_responses (and analogous vLLM/TensorRT builders) while the stage later re-appends prep.harmony_stop_ids, causing duplicate IDs and inconsistent backend behavior; choose one owner—either stop passing prep.harmony_stop_ids into the builders (remove the harmony_stop_ids args from build_generate_request_from_responses and other builder calls like the vLLM/TensorRT equivalents) so the stage append remains the single injection point, or move injection entirely into each builder (keep the builder args and delete the later append logic that mutates the request with prep.harmony_stop_ids); update all affected paths (Responses and the other mentioned request branches) consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@model_gateway/src/routers/grpc/harmony/stages/request_building.rs`:
- Around line 104-112: The Responses branch is duplicating Harmony stop-token
wiring: you currently pass prep.harmony_stop_ids into
sglang_client.build_generate_request_from_responses (and analogous vLLM/TensorRT
builders) while the stage later re-appends prep.harmony_stop_ids, causing
duplicate IDs and inconsistent backend behavior; choose one owner—either stop
passing prep.harmony_stop_ids into the builders (remove the harmony_stop_ids
args from build_generate_request_from_responses and other builder calls like the
vLLM/TensorRT equivalents) so the stage append remains the single injection
point, or move injection entirely into each builder (keep the builder args and
delete the later append logic that mutates the request with
prep.harmony_stop_ids); update all affected paths (Responses and the other
mentioned request branches) consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fcebbd40-98ee-4af5-a2dd-614fd791b0a9
📒 Files selected for processing (4)
crates/grpc_client/src/sglang_scheduler.rscrates/grpc_client/src/trtllm_service.rscrates/grpc_client/src/vllm_engine.rsmodel_gateway/src/routers/grpc/harmony/stages/request_building.rs
45fc5d7 to
1281d26
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
github-actions
Bot
removed
grpc
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/arc-runner-rbac.yaml`:
- Line 13: The comment "# Argo Workflows" is incorrect for this RBAC YAML;
locate the literal comment "# Argo Workflows" and replace it with a clear,
accurate comment like "# GitHub Actions runners RBAC" (or remove the comment
entirely) so the RBAC manifest reflects that it configures GitHub Actions runner
permissions rather than Argo Workflows.
In `@scripts/k8s-runner-resources/README.md`:
- Around line 145-157: Update the "Alternative: Using `actions.summerwind.dev`
ARC" section to explicitly distinguish the two ARC implementations: state that
the community controller (actions.summerwind.dev /
summerwind/actions-runner-controller) uses RunnerDeployment CRDs and the
community Helm chart, while the official GitHub ARC referenced earlier uses
GitHub's controller with runner scale sets; adjust the section title/link text
and descriptive lines to reference "RunnerDeployment CRDs" and "runner scale
sets" so readers clearly understand they are choosing between two different
implementations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d88f2f0-6b07-4205-90b9-0c41514b0d61
📒 Files selected for processing (5)
scripts/k8s-runner-resources/README.mdscripts/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.yaml
1281d26 to
698ca05
Compare
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
698ca05 to
f4c59fa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4c59fa4b9
ℹ️ 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".
| - type: PercentageRunnersBusy | ||
| scaleUpThreshold: "0.95" | ||
| scaleDownThreshold: "0.25" | ||
| scaleUpFactor: "0.5" |
There was a problem hiding this comment.
With PercentageRunnersBusy, scaleUpFactor is a multiplier on current replicas, so setting it to "0.5" causes the "scale up" path to reduce desired replicas instead of increasing them. In this config (minReplicas: 4), that means busy periods will keep being clamped back to 4 and the CPU runner pool will not scale out under load, which can stall queued CI jobs.
Useful? React with 👍 / 👎.
Thinking to keep them as another option to set up runners for some time in case we need it again, like after 2 ~ 3 weeks, the CI becomes pretty stable and we don't foresee any reason to replace current one, we can remove them |
Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9366b81c8b
ℹ️ 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".
| volumeMounts: | ||
| - name: docker-sock | ||
| mountPath: /var/run | ||
| - name: docker-storage |
There was a problem hiding this comment.
The DinD sidecar is only given /var/run and /var/lib/docker, but not the runner workspace path (/home/runner/_work). With DOCKER_HOST pointing to this sidecar, bind mounts for container actions and jobs.container are resolved inside the daemon container; without a shared work volume, those mounts point to missing/empty paths and containerized workflow steps fail. The same mount pattern is repeated across the other RunnerDeployments in this file.
Useful? React with 👍 / 👎.
| - type: TotalNumberOfQueuedAndInProgressWorkflowRuns | ||
| repositoryNames: | ||
| - lightseekorg/smg |
There was a problem hiding this comment.
Each autoscaler is configured with TotalNumberOfQueuedAndInProgressWorkflowRuns on the same repository (lightseekorg/smg), which is repo-wide rather than runner-label specific. That means queue pressure from one workload class can scale unrelated RunnerDeployments (e.g., CPU backlog scaling H100 pools), causing unnecessary scale-outs and resource/cost churn. Consider a per-deployment signal such as PercentageRunnersBusy or splitting autoscaler scope.
Useful? React with 👍 / 👎.
…smg-project#797) Signed-off-by: XinyueZhang369 <zoeyzhang369@gmail.com>
2 participants
Description
Problem
The existing ARC deployment guide only covers the official GitHub ARC controller (
ghcr.io/actions/actions-runner-controller-charts). Some clusters require or prefer the communityactions.summerwind.devcontroller which usesRunnerDeploymentCRDs and providesHorizontalRunnerAutoscalersupport.Solution
Add an alternative deployment path using the
actions.summerwind.devARC controller to the README, along with the corresponding Kubernetes manifests for RBAC, CPU runners, GPU runners, and autoscaling.Changes
RunnerDeployment-based runner manifests (arc-runner-cpu.yaml,arc-runner-gpu.yaml,arc-runner-rbac.yaml,arc-runner-autoscaler.yaml)README.mdwith an alternative section documenting theactions.summerwind.devARC installation and deployment stepsHorizontalRunnerAutoscalerTest Plan
actions.summerwind.devcontroller via Helm and verify pods are running inactions-runner-systemRunnerDeploymentandHorizontalRunnerAutoscalerresources are createdChecklist
cargo +nightly fmtpassescargo clippy --all-targets --all-features -- -D warningspassesSummary by CodeRabbit
New Features
Documentation