GitHub Runner Scaler: Fix/7969 respect paging for jobs - #7973
GitHub Runner Scaler: Fix/7969 respect paging for jobs#7973jangraevell-pd wants to merge 11 commits into
Conversation
Signed-off-by: Jan Grävell <jan.graevell@prodyna.com>
Signed-off-by: Jan Grävell <jan.graevell@prodyna.com>
Signed-off-by: Jan Grävell <jan.graevell@prodyna.com>
✅ 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. |
Signed-off-by: Jan Grävell <141423264+jangraevell-pd@users.noreply.github.com>
Signed-off-by: Rick Brouwer <rickbrouwer@gmail.com>
|
Thanks for the fix. I think there's an issue: when enableEtags is on, a 304 on page 1 returns the cached combined job list, but page 1's ETag says nothing about later pages. Once the first 100 jobs are completed, page 1 stops changing while jobs on page 2+ do, so the queue length freezes until the run finishes. Caching per page (keying previousJobs by URL instead of repo name) would solve this and would also make the ETags for pages beyond the first actually useful, since right now a 304 there always leads to dropping the etag and refetching. While you're at it, please use the new githubJobsPerPage constant in the URL instead of the hardcoded per_page=100, and update the test comment that says it's "expected to fail until pagination is implemented", since the fix now lives in the same PR. |
|
Will have a look this evening. Your annotation seems valid. |
…g can be used for all pages instead of only the first one. Signed-off-by: Jan Grävell <jan.graevell@prodyna.com>
…respect-paging-for-jobs
…-github-scaler-respect-paging-for-jobs # Conflicts: # CHANGELOG.md # pkg/scalers/github_runner_scaler.go # pkg/scalers/github_runner_scaler_test.go
Signed-off-by: Jan Grävell <jan.graevell@prodyna.com>
7fded19 to
b1da0b3
Compare
Signed-off-by: Jan Grävell <jan.graevell@prodyna.com>
|
@rickbrouwer how do we continue from here? Do I need to do something? |
…respect-paging-for-jobs
|
Can you check this? With enableEtags, a 304 on page 1 returns cached=true and page 2 is never fetched, but ETags are per page. Once page 1 is fully completed it stops changing, so the queue length freezes. Must the cache be per page instead? Do you know this? |
Fixes the GitHub Runner scaler's
100-job hard limit described in #7969.getWorkflowRunJobsonly ever requested a single page (per_page=100) of aworkflow run's jobs and never followed pagination. For a run with more than
100 jobs, the still-queued/in-progress jobs beyond the 100th were silently
never counted towards the scaler's queue length, causing scaling to stop as
soon as the first 100 jobs of a run finished.
Changes:
getWorkflowRunJobsnow loops through pages (&page=N), accumulating jobs until a page returns fewer thangithubJobsPerPage(100) jobs.previousJobs-cache handling into a newfetchWorkflowRunJobsPage(ctx, workflowRunID, repoName, page)helper.TestNewGitHubRunnerScaler_QueueLength_SingleRepo_150Jobs_100Completed) simulating a 150-job run (100 completed on page 1, 50 still queued on page 2) with a page-aware mock server, asserting the correct queue length of 50.Checklist
Unchecked items are not applicable (no new scaler, no scaler schema/metadata changes, no Helm/manifest or docs/UX changes).
Fixes #7969