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 scaler exposes sum of pending and ongoing sessions to KDEA #6368

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

VietND96
Copy link
Contributor

@VietND96 VietND96 commented Nov 26, 2024

Provide a description of what has been changed

As KEDA exposes a metric to the HPA Controller, KEDA has to expose the raw value of the queue including all the jobs in-progress and pending, because it's the HPA controller the one which will calculate the right value based on the metric type (Value or AverageValue).

Selenium Grid scaler exposes the the sum of the in-progress sessions plus the pending sessions (after counting nodeMaxSessions to KEDA.

Checklist

Fixes SeleniumHQ/docker-selenium#2464

Relates to #6331

@rickbrouwer
Copy link
Contributor

rickbrouwer commented Nov 26, 2024

I see you are using optional with a default value. I think if something has a default, optional does not need to be passed along. What do you think?

@VietND96
Copy link
Contributor Author

I am not aware on this. So, we can remove optional from all trigger params which has default value already?
If yes, I will proceed the clean up

@rickbrouwer
Copy link
Contributor

I am not aware on this. So, we can remove optional from all trigger params which has default value already? If yes, I will proceed the clean up

Yes, because if it doesn't exist the value is set with the default.
Reference:

if !exists && params.Default != "" {
exists = true
valFromConfig = params.Default
}

@VietND96
Copy link
Contributor Author

Thanks! I updated the same.

@VietND96 VietND96 changed the title fix: Condition of queueLength for Selenium Grid scaler will scale up fix: Selenium Grid scaler exposes sum of pending and ongoing requests to KDEA Dec 1, 2024
@VietND96 VietND96 changed the title fix: Selenium Grid scaler exposes sum of pending and ongoing requests to KDEA fix: Selenium Grid scaler exposes sum of pending and ongoing sessions to KDEA Dec 1, 2024
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for this awesome fix! ❤️

@JorTurFer
Copy link
Member

JorTurFer commented Dec 2, 2024

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

Signed-off-by: Viet Nguyen Duc <[email protected]>
@JorTurFer
Copy link
Member

JorTurFer commented Dec 2, 2024

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

@JorTurFer JorTurFer enabled auto-merge (squash) December 2, 2024 10:44
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.

[🐛 Bug]: Issue with Scaling Already Scaled Deployments
3 participants