K8s: Replace external scaler jobScalingStrategy with includeOngoingSessions - #3218
Merged
Conversation
…ssions Aligns the Selenium Grid KEDA external scaler with the trigger param merged into the built-in scaler upstream (kedacore/keda#8107), so the two implementations stay a mechanical swap of trigger `type`. The external scaler previously took `jobScalingStrategy` (default|custom|accurate|eager) and derived the count convention from it. Upstream settled on a plain boolean instead, matching how other scalers expose the same choice (SQS `scaleOnInFlight`, RabbitMQ `excludeUnacknowledged`, Beanstalkd `includeDelayed`, Temporal `includeRunningWorkflowCount`). - metadata: `JobScalingStrategy string` -> `IncludeOngoingSessions bool` (default true); enum validation replaced by strconv.ParseBool. - server: scrapeAndCount adds onGoingSessions only when the flag is set, matching upstream GetMetricsAndActivity. - chart: `_helpers.tpl` derives `includeOngoingSessions` from `autoscaling.scaledJobOptions.scalingStrategy.strategy` - false for `accurate`/`eager`, true otherwise. Override key is now `<node>.hpa.includeOngoingSessions`. The chart default strategy is `accurate`, so the default render now emits "false", which is the fix for the runaway Node creation in #3167. `jobScalingStrategy` is removed rather than deprecated; it only ever existed on the external scaler and never reached the built-in one. Note the patched built-in scaler copy in `.keda/scalers/` is deliberately left tracking upstream main; it picks the param up when #8107 merges. Relates to #3167 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hwsdd5D5hWWXiPJ1hdPBzh
Contributor
|
ⓘ Qodo reviews are paused because the subscription is no longer active. Ask your workspace admin to reactivate the subscription to resume reviews. Manage billing |
Previous-version Nodes in the TEST_MULTIPLE_VERSIONS scale test pulled from Docker Hub, which hits the anonymous pull rate limit when a scale test brings many of them up at once (observed in https://github.com/SeleniumHQ/docker-selenium/actions/runs/33747779946/attempts/2). Rather than editing charts/selenium-grid/multiple-nodes-platform-version.yaml - shipped chart documentation/example content that should stay on the default (Docker Hub) registry for real users - control this from tests/: - Add tests/charts/ci/multiple-nodes-platform-version-values.yaml, a test-only overlay that pins previous-version Nodes to ghcr.io/seleniumhq (anonymous, unmetered for public images). - Prune its version range to what tests/SeleniumTests/__init__.py actually requests (LIST_CHROMIUM_VERSIONS / LIST_FIREFOX_VERSIONS = 144.0-147.0), instead of mirroring the shipped file's full historical list. Versions outside that range never receive a session request during the test, so they never scale up and never pull an image anyway; keeping them only added scaler triggers with no coverage benefit. - Point chart_test.sh's TEST_MULTIPLE_VERSIONS branch at this overlay instead of the chart's file. All ten pinned tags (chrome/edge 144-146, firefox 144-147, each -20260808) were confirmed present on ghcr.io/seleniumhq before this change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hwsdd5D5hWWXiPJ1hdPBzh
Rolling maintenance bump: every k8s-version and helm-version entry across the K8s test matrices advances by one minor release, each to its latest published patch: k8s: 1.29->1.30, 1.30->1.31, 1.31->1.32, 1.32->1.33, 1.33->1.34, 1.34->1.35 helm: 3.14->3.15, 3.15->3.16, 3.16->3.17, 3.17->3.18, 3.18->3.19, 3.19->3.20, 4.0->4.1 Applies to helm-chart-test.yml, k8s-scaling-test.yml, and k8s-dynamic-grid-test.yml (k8s-version only, no helm-version there). docker-version/python-version are untouched. Note: this does not fully clear the isCIDR/Calico CRD failure seen on https://github.com/SeleniumHQ/docker-selenium/actions/runs/33747779946/attempts/2 - job_https now lands on k8s v1.30.14, the same version that failed as job_hostname before this bump (v1.31.14 is the confirmed-working floor for Calico's current CRD). Tracked as a separate follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hwsdd5D5hWWXiPJ1hdPBzh
…n pod-count mismatch Scale testing only ever exercised the "default" ScaledJob strategy, even though the shipped chart default is "accurate" - the exact strategy #3167 and the includeOngoingSessions fix (PR #3218) target. Add matching coverage, and turn the existing (unused) pod-count check into a real assertion instead of a warn-only log. Makefile: - Add test_k8s_autoscaling_job_count_strategy_accurate(_in_chaos| _with_node_max_sessions), mirroring the "default" targets by reusing test_k8s_autoscaling_job_count_strategy_default with SCALING_STRATEGY=accurate (already forwarded to autoscaling.scaledJobOptions.scalingStrategy.strategy by chart_test.sh - no new plumbing needed). .github/workflows/k8s-scaling-test.yml: - Add 3 matrix entries for the new "accurate" targets, each pinned to the same k8s/helm/docker/python versions as its "default" counterpart so a difference in results reflects the strategy, not the environment. tests/AutoscalingTests/common.py: - wait_for_count_matches() was dead code (never called) and, even if called, only warned on mismatch. It also compared pod count to raw session count, which is wrong once sessions span multiple browser types Chrome/Firefox/Edge scale independently, so the correct expected count is the sum of per-browser ceil(sessions/node_max_sessions), not one ceiling over the total. - Rewritten to compute that per-browser expected count and raise AssertionError immediately if the poll times out without matching - a hard failure, not a log line. This makes it a direct regression guard for #3167: a strategy that double-counts on-going sessions never converges pod count to the expected value, so the poll times out and the test fails. - create_session/create_sessions_in_parallel now track (driver, browser_name) tuples so per-browser counts are available; close_all_sessions/randomly_quit_sessions updated accordingly. tests/AutoscalingTests/test_scale_up.py, test_scale_chaos.py: - Call wait_for_count_matches(SESSIONS, TEST_NODE_MAX_SESSIONS) right after each iteration's scale-up, before the next create/quit cycle. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hwsdd5D5hWWXiPJ1hdPBzh
https://github.com/SeleniumHQ/docker-selenium/actions/runs/33781949421/job/100765325753 Confirms the residual risk flagged in 55011ea: after the +1 minor bump, job_https landed on k8s v1.30.14 - the same version that failed as job_hostname before that bump. Calico's ClusterNetworkPolicy CRD uses the CEL builtin isCIDR(self), which v1.30's CEL library doesn't support ("undeclared reference to 'isCIDR'"), so `minikube start` fails applying the CNI manifest every retry. Move job_https to v1.31.14 (matching helm v3.16.4) - the confirmed working floor, since job_hostname already runs there successfully. Every k8s-version across all three workflows is now >= v1.31.14. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hwsdd5D5hWWXiPJ1hdPBzh
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Aligns the Selenium Grid KEDA external scaler with the trigger param merged into the built-in scaler upstream (kedacore/keda#8107), so migrating between the two stays a mechanical swap of trigger
type.Why
The external scaler shipped with
jobScalingStrategy(default|custom|accurate|eager) and derived the count convention from it. Upstream settled on a plain boolean instead, matching how other scalers expose the same choice — AWS SQSscaleOnInFlight, RabbitMQexcludeUnacknowledged, BeanstalkdincludeDelayed, TemporalincludeRunningWorkflowCount.The underlying behaviour is unchanged: whether on-going sessions belong in the metric depends on which Job count KEDA's executor deducts for you (
pkg/scaling/executor/scale_jobs.go).includeOngoingSessionsdefaulttrue(default)customcustomScalingRunningJobPercentageof running Job counttrue(default)accuratefalseeagermaxScale)falsetrue(default)Changes
External scaler (
.keda-external-scaler/)internal/gridscaler/metadata.go—JobScalingStrategy string→IncludeOngoingSessions bool(defaulttrue); enum validation replaced bystrconv.ParseBool.internal/gridscaler/server.go—scrapeAndCountaddsonGoingSessionsonly when the flag is set, mirroring upstreamGetMetricsAndActivity.TestServer_GetMetrics_JobScalingStrategy→TestServer_GetMetrics_IncludeOngoingSessions(unset → 3,true→ 3,false→ 2 on the same fixture KEDA uses); metadata table covers the new default,"false", and an invalid-bool error.README.md(metadata table + new On-going sessions section),SPEC.md,PLAN.md,TASKS.md,deploy/scaledjob-example.yaml,deploy/scaledobject-example.yaml.Helm chart
templates/_helpers.tplderivesincludeOngoingSessionsfromautoscaling.scaledJobOptions.scalingStrategy.strategy:falseforaccurate/eager,trueotherwise. Override key is now<node>.hpa.includeOngoingSessions.accurate, so the default render now emitsincludeOngoingSessions: "false"— this is the fix for the runaway Node creation in [🐛 Bug]: ScaledJob default scalingStrategy "accurate" (since chart 0.56.0) causes runaway node job creation; "default" no longer selectable on KEDA 2.20+ #3167.charts/selenium-grid/README.mddocuments the param and the derivation.Notes
jobScalingStrategyis removed rather than deprecated. It only ever existed on the external scaler and was never read by the built-in one, so nothing else in the chart or upstream consumes it..keda/scalers/selenium_grid_scaler.gois deliberately left tracking upstreammain; it picks the param up when Selenium Grid Scaler: add includeOngoingSessions trigger param kedacore/keda#8107 merges. Until then the built-in trigger path ignores the emitted key (KEDA ignores unknown trigger metadata), i.e. unchanged behaviour.Verification
go mod verify,go mod tidy(clean),go vet,go build,go test ./... -race -covermode=atomicall pass; coverage excluding generated stubs is 97.1% (gate is 90%).gofmt -lis clean module-wide — this also picks up a pre-existing comment-alignment nit incmd/scaler/main_test.go.helm lintclean. Rendered and checked:accurate/eager→"false",default/custom→"true",scalingType=deployment→ key omitted (scaler defaulttrueis correct for HPA), built-in trigger path, and<node>.hpa.includeOngoingSessionsoverride.tests/charts/templates/test_scaled_job.pypasses.test.pyhas one failure (selenium-grid-overviewdashboard ConfigMaps mismatch) that reproduces identically on unmodifiedtrunk— pre-existing and unrelated.Relates to #3167
🤖 Generated with Claude Code
https://claude.ai/code/session_01Hwsdd5D5hWWXiPJ1hdPBzh