fix(github-runner): bound etag and per-repo caches#7698
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
Thank you for your contribution! 🙏 Please understand that we will do our best to review your PR and give you feedback as soon as possible, but please bear with us if it takes a little longer as expected. While you are waiting, make sure to:
Once the initial tests are successful, a KEDA member will ensure that the e2e tests are run. Once the e2e tests have been successfully completed, the PR may be merged at a later date. Please be patient. Learn more about our contribution guide. |
There was a problem hiding this comment.
Pull request overview
This PR mitigates unbounded memory growth in the GitHub Runner scaler when enableEtags is enabled by pruning and bounding internal per-URL/per-repo caches.
Changes:
- Add
pruneCaches+evictExcessto cap cache maps (etags/previousJobs/previousWfrs) and droppreviousWfrsentries for repos no longer returned. - Call cache pruning from
GetWorkflowQueueLengthwhenEnableEtagsis on. - Add unit tests for cache pruning and update the changelog entry.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/scalers/github_runner_scaler.go | Introduces cache size cap + pruning logic; invoked during queue-length polling when ETags are enabled. |
| pkg/scalers/github_runner_scaler_test.go | Adds tests covering pruning of absent repos and cache size bounding. |
| CHANGELOG.md | Documents the fix for bounding ETag/per-repo caches (Issue #7685). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When enableEtags is on, three caches in the GitHub Runner scaler grow indefinitely for the lifetime of the operator pod: - etags is keyed by apiURL, and workflow run job URLs include the run ID, so each new workflow run leaks an entry. - previousJobs is keyed by the workflow run repository name, so a misbehaving or malicious API returning rotating repo names accumulates entries. - previousWfrs is keyed by repo from getRepositories, with the same rotation risk. Add pruneCaches that drops previousWfrs entries for repos no longer in the current list and caps all three maps at a fixed size. Called once per scaling cycle from GetWorkflowQueueLength when enableEtags is on, matching the gate that populates the maps. Signed-off-by: Mateen Anjum <[email protected]>
addresses review feedback. when pruneCaches evicts a previousJobs or previousWfrs entry but its etag stays, the next request returns 304 and the scaler errored with "previous ... is not set". drop the stale etag and retry without If-None-Match so the response repopulates the cache. also corrected the comment on githubScalerMaxCacheEntries to mention previousWfrs alongside etags and previousJobs. Signed-off-by: Mateen Anjum <[email protected]>
mateenali66
force-pushed
the
fix/7685-github-runner-cache-bounds
branch
from
0b70fe5 to
3a817e9
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
addresses race-detector feedback. the httptest handler goroutine writes saw* flags that the test goroutine reads, with no explicit happens-before edge, so make test-race could flag it. swap plain bools for go.uber.org/atomic.Bool (KEDA's depguard rejects sync/atomic). Signed-off-by: Mateen Anjum <[email protected]>
reverts the go.mod change. the go.uber.org/atomic indirect-to-direct promotion tripped Snyk's manifest-change scan (0 vulnerabilities, but the policy gate marked the check incomplete). plain sync.Mutex around the saw* flags satisfies the race detector without touching go.mod. Signed-off-by: Mateen Anjum <[email protected]>
|
/run-e2e github* |
rickbrouwer
added
Awaiting/2nd-approval
JorTurFer
left a comment
JorTurFer
left a comment
There was a problem hiding this comment.
Looks really nice!
Does it make sense to expose the githubScalerMaxCacheEntries parameter to bring the option to users?
wozniakjan
left a comment
wozniakjan
left a comment
There was a problem hiding this comment.
Does it make sense to expose the githubScalerMaxCacheEntries parameter to bring the option to users?
for sure can be added, alternatively we can also consider refactoring this to use https://github.com/hashicorp/golang-lru.
Given this PR already has two approvals, it looks in a good shape addressing the unbounded growth, I propose we merge for 2.20 release and iteratively improve if desired with follow-up PRs.
rickbrouwer
removed
the
Awaiting/2nd-approval
* fix(github-runner): bound etag and per-repo caches When enableEtags is on, three caches in the GitHub Runner scaler grow indefinitely for the lifetime of the operator pod: - etags is keyed by apiURL, and workflow run job URLs include the run ID, so each new workflow run leaks an entry. - previousJobs is keyed by the workflow run repository name, so a misbehaving or malicious API returning rotating repo names accumulates entries. - previousWfrs is keyed by repo from getRepositories, with the same rotation risk. Add pruneCaches that drops previousWfrs entries for repos no longer in the current list and caps all three maps at a fixed size. Called once per scaling cycle from GetWorkflowQueueLength when enableEtags is on, matching the gate that populates the maps. Signed-off-by: Mateen Anjum <[email protected]> * fix(github-runner): treat 304 with missing previous as cache miss addresses review feedback. when pruneCaches evicts a previousJobs or previousWfrs entry but its etag stays, the next request returns 304 and the scaler errored with "previous ... is not set". drop the stale etag and retry without If-None-Match so the response repopulates the cache. also corrected the comment on githubScalerMaxCacheEntries to mention previousWfrs alongside etags and previousJobs. Signed-off-by: Mateen Anjum <[email protected]> * test(github-runner): use atomic.Bool in stale-etag retry tests addresses race-detector feedback. the httptest handler goroutine writes saw* flags that the test goroutine reads, with no explicit happens-before edge, so make test-race could flag it. swap plain bools for go.uber.org/atomic.Bool (KEDA's depguard rejects sync/atomic). Signed-off-by: Mateen Anjum <[email protected]> * test(github-runner): use sync.Mutex instead of atomic.Bool reverts the go.mod change. the go.uber.org/atomic indirect-to-direct promotion tripped Snyk's manifest-change scan (0 vulnerabilities, but the policy gate marked the check incomplete). plain sync.Mutex around the saw* flags satisfies the race detector without touching go.mod. Signed-off-by: Mateen Anjum <[email protected]> --------- Signed-off-by: Mateen Anjum <[email protected]> Signed-off-by: Yurii Shcherbak <[email protected]>
the etags map is keyed by the request URL, and workflow run job URLs include the run ID, so the cache grows once per workflow run for the lifetime of the operator pod. previousJobs and previousWfrs are keyed by repo names returned from the API, so a misbehaving or malicious server that rotates repo names accumulates entries the same way.
added pruneCaches that drops previousWfrs entries for repos no longer in the current list and caps all three maps at 5000 entries. called from GetWorkflowQueueLength when enableEtags is on, matching the gate that populates the maps in the first place.
Fixes #7685