Skip to content

Port rate limiter to experimental charts #4478

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
nikola-jokic wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Port rate limiter to experimental charts #4478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a3ee606
Fix helm chart validation workflow
nikola-jokic May 2, 2026
8958d25
Port rate limiter to experimental charts
nikola-jokic Apr 30, 2026
238b852
Merge remote-tracking branch 'origin/master' into nikola-jokic/workqu…
Copilot May 11, 2026
1f10ac8
chore: finalize conflict resolution updates
Copilot May 11, 2026
2d42309
Revert unintended go module file changes
Copilot May 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 42 additions & 21 deletions .github/workflows/gha-validate-chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,23 +65,6 @@ jobs:
echo "$changed" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT

- name: Install helm-unittest
if: |
contains(steps.list-changed.outputs.changed_charts, 'charts/gha-runner-scale-set-controller-experimental') ||
contains(steps.list-changed.outputs.changed_charts, 'charts/gha-runner-scale-set-experimental')
run: |
helm plugin install https://github.com/helm-unittest/helm-unittest.git

- name: Run helm-unittest (gha-runner-scale-set-controller-experimental)
if: contains(steps.list-changed.outputs.changed_charts, 'charts/gha-runner-scale-set-controller-experimental')
run: |
helm unittest ./charts/gha-runner-scale-set-controller-experimental/

- name: Run helm-unittest (gha-runner-scale-set-experimental)
if: contains(steps.list-changed.outputs.changed_charts, 'charts/gha-runner-scale-set-experimental')
run: |
helm unittest ./charts/gha-runner-scale-set-experimental/

- name: Run chart-testing (lint)
run: |
ct lint --config charts/.ci/ct-config-gha.yaml
Expand Down Expand Up @@ -132,15 +115,53 @@ jobs:
steps:
- name: Checkout
uses: actions/checkout@v6
with:
fetch-depth: 0

- name: Set up Helm
uses: azure/setup-helm@1a275c3b69536ee54be43f2070a358922e12c8d4
with:
version: ${{ env.HELM_VERSION }}

# python is a requirement for the chart-testing action below (supports yamllint among other tests)
- uses: actions/setup-python@v6
with:
python-version: "3.11"

- name: Set up chart-testing
uses: helm/chart-testing-action@6ec842c01de15ebb84c8627d2744a0c2f2755c9f

- name: Run chart-testing (list-changed)
id: list-changed
run: |
ct version
changed=$(ct list-changed --config charts/.ci/ct-config-gha.yaml)
if [[ -n "$changed" ]]; then
echo "changed=true" >> $GITHUB_OUTPUT
fi
echo "changed_charts<<EOF" >> $GITHUB_OUTPUT
echo "$changed" >> $GITHUB_OUTPUT
echo "EOF" >> $GITHUB_OUTPUT

- name: Install helm-unittest
run: |
helm plugin install https://github.com/helm-unittest/helm-unittest.git

- name: Run helm-unittest (gha-runner-scale-set-controller-experimental)
run: |
helm unittest ./charts/gha-runner-scale-set-controller-experimental/

- name: Run helm-unittest (gha-runner-scale-set-experimental)
run: |
helm unittest ./charts/gha-runner-scale-set-experimental/

- uses: actions/setup-go@v6
with:
go-version-file: "go.mod"
cache: false

- name: Test gha-runner-scale-set
run: go test ./charts/gha-runner-scale-set/...

- name: Test gha-runner-scale-set-controller
run: go test ./charts/gha-runner-scale-set-controller/...
- name: Test gha-runner-scale-set-experimental
run: go test ./charts/gha-runner-scale-set-experimental/...
- name: Test gha-runner-scale-set-controller-experimental
run: go test ./charts/gha-runner-scale-set-controller-experimental/...
2 changes: 2 additions & 0 deletions charts/.ci/ct-config-gha.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ check-version-increment: false # Disable checking that the chart version has bee
charts:
- charts/gha-runner-scale-set-controller
- charts/gha-runner-scale-set
- charts/gha-runner-scale-set-controller-experimental
- charts/gha-runner-scale-set-experimental
skip-clean-up: true
5 changes: 5 additions & 0 deletions charts/gha-runner-scale-set-controller-experimental/templates/_controller_template.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,11 @@ args:
{{- with .Values.controller.manager.config.k8sClientRateLimiterBurst }}
- "--k8s-client-rate-limiter-burst={{ . }}"
{{- end }}
{{- with .Values.controller.manager.config.rateLimiter }}
{{- with .name }}
- "--workqueue-rate-limiter={{ . }}"
{{- end }}
{{- end }}
{{- with .Values.controller.manager.config.healthProbeBindAddress }}
- "--health-probe-bind-address={{ . }}"
{{- end }}
Expand Down
66 changes: 66 additions & 0 deletions ...nner-scale-set-controller-experimental/tests/controller_deployment_rate_limiter_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
suite: "Controller Deployment rate limiter"
templates:
- deployment.yaml
tests:
- it: should omit workqueue-rate-limiter flag by default
release:
name: "test-arc"
namespace: "test-ns"
asserts:
- notContains:
path: spec.template.spec.containers[0].args
content: "--workqueue-rate-limiter=bucket_rate_limiter"
- notContains:
path: spec.template.spec.containers[0].args
content: "--workqueue-rate-limiter=typed_rate_limiter"
Comment on lines +12 to +15
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default-case test only asserts the args list does not contain the two specific values. This would still pass if the chart accidentally rendered something like --workqueue-rate-limiter= (or a future third value). To actually verify the flag is omitted by default, assert that no arg contains the --workqueue-rate-limiter= prefix (e.g., via a single notContains on the prefix or a regex-based assertion, depending on helm-unittest capabilities).

Suggested change
content: "--workqueue-rate-limiter=bucket_rate_limiter"
- notContains:
path: spec.template.spec.containers[0].args
content: "--workqueue-rate-limiter=typed_rate_limiter"
content: "--workqueue-rate-limiter="

Copilot uses AI. Check for mistakes.

- it: should include workqueue-rate-limiter flag when bucket_rate_limiter is configured
set:
controller:
manager:
config:
rateLimiter:
name: "bucket_rate_limiter"
release:
name: "test-arc"
namespace: "test-ns"
asserts:
- contains:
path: spec.template.spec.containers[0].args
content: "--workqueue-rate-limiter=bucket_rate_limiter"

- it: should include workqueue-rate-limiter flag when typed_rate_limiter is configured
set:
controller:
manager:
config:
rateLimiter:
name: "typed_rate_limiter"
release:
name: "test-arc"
namespace: "test-ns"
asserts:
- contains:
path: spec.template.spec.containers[0].args
content: "--workqueue-rate-limiter=typed_rate_limiter"

- it: should render both config and extraArgs in deterministic order
set:
controller:
manager:
config:
rateLimiter:
name: "bucket_rate_limiter"
container:
extraArgs:
- "--workqueue-rate-limiter=typed_rate_limiter"
release:
name: "test-arc"
namespace: "test-ns"
asserts:
- contains:
path: spec.template.spec.containers[0].args
content: "--workqueue-rate-limiter=bucket_rate_limiter"
- contains:
path: spec.template.spec.containers[0].args
content: "--workqueue-rate-limiter=typed_rate_limiter"
Comment on lines +47 to +66
Copy link

Copilot AI Apr 30, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test claims to verify deterministic ordering between the config-generated flag and extraArgs, but it only checks that both values are present. Since duplicate flags are order-sensitive (the last one typically wins), this test should assert the relative order in the rendered args list (or rename the test to avoid implying ordering is verified).

Copilot uses AI. Check for mistakes.
52 changes: 0 additions & 52 deletions charts/gha-runner-scale-set-controller-experimental/tests/template_test.go
View file
Open in desktop

This file was deleted.

7 changes: 7 additions & 0 deletions charts/gha-runner-scale-set-controller-experimental/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,13 @@ controller:
k8sClientRateLimiterQPS: null
k8sClientRateLimiterBurst: null

## Workqueue rate limiter configuration.
## By default, controller-runtime uses a combined rate limiter with both a per-item
## exponential backoff and an overall token bucket (10 QPS, 100 bucket size).
## Valid names: "bucket_rate_limiter" (default), "typed_rate_limiter" (per-item only, no global token bucket).
# rateLimiter:
# name: "bucket_rate_limiter"

# The address the health probe endpoint binds to. Disabled if empty/null.
# When set, liveness and readiness probes are added to the controller pod.
# healthProbeBindAddress: ":8081"
Expand Down
61 changes: 0 additions & 61 deletions charts/gha-runner-scale-set-experimental/tests/template_test.go
View file
Open in desktop

This file was deleted.