Skip to content
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
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

VietND96
Copy link
Contributor

@VietND96 VietND96 commented Dec 23, 2024

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.java

It is also recommended to set the browserName, browserVersion, and platformName 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 value latest for the capability browserVersion. So, remove that value latest and keep the default value EMPTY.

For example, client sends the request (in Python binding)

options = ChromeOptions()
options.set_capability('platformName', 'Linux')
driver = webdriver.Remote(options=options, command_executor=SELENIUM_GRID_URL)

It is equivalent to scaler trigger params.

  triggers:
    - type: selenium-grid
      metadata:
        url: 'http://selenium-hub:4444/graphql'
        browserName: 'chrome'
        platformName: 'linux'

For example, client sends the request

options = ChromeOptions()
options.set_capability('platformName', 'Linux')
options.set_capability('browserVersion', '131.0')
driver = webdriver.Remote(options=options, command_executor=SELENIUM_GRID_URL)

The scaler trigger param should be aligned as

  triggers:
    - type: selenium-grid
      metadata:
        url: 'http://selenium-hub:4444/graphql'
        browserName: 'chrome'
        platformName: 'linux'
        browserVersion: '131.0'

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 without browserVersion could not trigger scaler anymore. Via SeleniumHQ/docker-selenium#2520, you can set SE_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 multiple browserVersion 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 param browserName to optional.

Checklist

  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Commits are signed with Developer Certificate of Origin (DCO - learn more)

Fixes #

Relates to #

@VietND96 VietND96 requested a review from a team as a code owner December 23, 2024 01:26
@VietND96 VietND96 force-pushed the main branch 3 times, most recently from 0153063 to 8615239 Compare December 23, 2024 06:07
…sion triggers are active

Signed-off-by: Viet Nguyen Duc <[email protected]>
@VietND96 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]>
@VietND96 VietND96 mentioned this pull request Dec 23, 2024
24 tasks
Signed-off-by: Viet Nguyen Duc <[email protected]>
@JorTurFer
Copy link
Member

JorTurFer commented Dec 23, 2024

/run-e2e selenium
Update: You can check the progress here

@JorTurFer
Copy link
Member

JorTurFer commented Dec 23, 2024

/run-e2e selenium
Update: You can check the progress here

@VietND96
Copy link
Contributor Author

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

@JorTurFer
Copy link
Member

It looks like issue reaching the hub ->

scale_handler	error getting scale decision	***"scaledObject.Namespace": "selenium-test-ns", "scaledObject.Name": "chrome-selenium-test-so", "scaler": "seleniumGridScaler", "error": "error requesting selenium grid endpoint: Post \"[http://selenium-hub.selenium-test-ns:4444/graphql\](http://selenium-hub.selenium-test-ns:4444/graphql/)": dial tcp ***0.0.***5***.***4***:4444: connect: connection refused"***
github.com/***/keda/v***/pkg/scaling.(*scaleHandler).getScalerState
	/workspace/pkg/scaling/scale_handler.go:780
github.com/***/keda/v***/pkg/scaling.(*scaleHandler).getScaledObjectState.func***

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants