Selenium Grid Scaler | Max Sessions implementation issue

[Wolfe1]

Report

The MaxSessions implementation in #2774 is producing incorrect results. I ran into this while trying to create a fix for #2709.

Expected Behavior

We need to take into consideration the count of the nodes we have in this calculation.

Something like:
count = count / (gridMaxSession / nodeCount)

1. 2 sessions running on 2 nodes, 3 sessions in the queue, each node accepts 1 session max:
   a. count = 5 / (2 / 2)
   b. This equals out to be 5 which is how many nodes we need.
2. 10 sessions running on 2 nodes, 7 sessions in the queue, each node accepts 5 sessions max:
   a. count = 17 / (10 / 2)
   b. This equals out to be 3.4 (rounded to 4 I assume?) which is what we need.

Actual Behavior

https://github.com/kedacore/keda/blob/main/pkg/scalers/selenium_grid_scaler.go#L252 :
count = (count + gridMaxSession - 1) / gridMaxSession is the current equation.

This equation does not take into account the number of nodes available and thus will consistently produce an incorrect result, for example:

1. 2 sessions running on 2 nodes, 3 sessions in the queue, each node accepts 1 session max:
   a. count = (5 + 2 - 1) / 2
   b. This equals out to be 3 when it should be 5
2. 10 sessions running on 2 nodes, 7 sessions in the queue, each node accepts 5 sessions max:
   a. count = (17 + 10 - 1) / 10
   b. This equals out to be 2.6 (rounded to 3 I assume?) when it should be 4

Steps to Reproduce the Problem

Simply using the scaler with a version higher than 2.6.1 will suffice. Items will be left in the queue while we still have room to scale.

Logs from KEDA operator

No response

KEDA Version

2.7.1

Kubernetes Version

1.23

Platform

Microsoft Azure

Scaler Details

Selenium Grid

Anything else?

No response

[JorTurFer]

@adborroto does it make sense to you? I remember we talked about the edge case of having only 1 pending message with high gridMaxSession.
It's true that right now, 1 pending job is enough to scale from zero to 1 and we are working to change this adding an extra parameter, so maybe to change the formula from current to the suggested formula is good idea.
@kedacore/keda-core-contributors ?

[Wolfe1]

@JorTurFer @adborroto I think the confusion here (from what I can see in the tests) is that gridMaxSession is being treated as a variable representing the max session each node can take when in reality it is the max session count for the entire grid.

When running a grid where every node only can run one session at a time it looks like so:

[JorTurFer]

That makes sense totally!
Could you update it during the changes you are already doing?

[Wolfe1]

@JorTurFer that is the plan, should have something soon just making sure im covering my bases with tests and docs.

[AmmarRami]

Hello,
When this fix will be released please? It's opened on 2.7.1 and it's the last release.

Thanks

[zroubalik]

@AmmarRami it will be part of KEDA 2.8 coming in a week or so.