-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
fix: Selenium Grid in case multiple scaler triggers are activate #6437
Open
VietND96
wants to merge
3
commits into
kedacore:main
Choose a base branch
from
NDViet:main
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+1,360
−225
Conversation
This file contains 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
VietND96
force-pushed
the
main
branch
3 times, most recently
from
December 23, 2024 06:07
0153063
to
8615239
Compare
…sion triggers are active Signed-off-by: Viet Nguyen Duc <[email protected]>
1 task
VietND96
changed the title
fix: Selenium Grid in case multiple browserVersion scalers are active
fix: Selenium Grid in case multiple scaler triggers are activate
Dec 23, 2024
Signed-off-by: Viet Nguyen Duc <[email protected]>
Signed-off-by: Viet Nguyen Duc <[email protected]>
/run-e2e selenium |
JorTurFer
approved these changes
Dec 23, 2024
/run-e2e selenium |
Can you try e2e run one more time, I guess pull image could take time or reach pull limit rate...Here is result in my local EXECUTION SUMMARY
##############################################
##############################################
Passed tests:
Execution of tests/scalers/selenium/selenium_test.go, has passed after "one" attempts |
It looks like issue reaching the hub ->
I'm checking another issue in my local, once I finish I'll try to check this too |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Implement strictly the condition to trigger the scaler in complex cases that Selenium Grid includes autoscaling Nodes (with multiple scaledObject with different triggers metadata), non-autoscaling Nodes, relay Nodes, and so on.
In general, update the scaler logic should be aligned with Selenium Grid core in
DefaultSlotMatcher
- which can be inspected https://github.com/SeleniumHQ/selenium/blob/trunk/java/src/org/openqa/selenium/grid/data/DefaultSlotMatcher.javaIt is also recommended to set the
browserName
,browserVersion
, andplatformName
to align across request capabilities, Node stereotype and scaler trigger params to have correct scaling behavior.In terms of
SlotMatcher
in Grid, there is no valuelatest
for the capabilitybrowserVersion
. So, remove that valuelatest
and keep the default value EMPTY.For example, client sends the request (in Python binding)
It is equivalent to scaler trigger params.
For example, client sends the request
The scaler trigger param should be aligned as
Most people are using the popular browser node image from the project docker-selenium. In default Node stereotype, the short browser version is set for
browserVersion
, it might cause breaking backward compatiblity, or request withoutbrowserVersion
could not trigger scaler anymore. Via SeleniumHQ/docker-selenium#2520, you can setSE_NODE_BROWSER_VERSION
as empty to unset the default version (take newer docker image tag from that project, which contains the change onwards).This also avoids multiple scalers, for example same
browserName
combines multiplebrowserVersion
could have issue with overlapping in counting number of ongoing and pending sessions belonging to each scaler trigger.In terms of Selenium Grid v4, it has a kind of Relay Node, which has capabilities to forward commands to the Appium instance. With this Node type, the
browserName
is optional. So, updating the trigger parambrowserName
to optional.Checklist
Fixes #
Relates to #