Implements PendingJobCount to fix kedacore/keda#1323

[thomas-lamure]

Signed-off-by: Thomas Lamure thomas.lamure@eridanis.com

Provide a description of what has been changed

When using the accurateScalingStrategy, if a job's pod takes time to be created or started, this PR will prevent multiple jobs to be requested instead of one.
The reason for the pod start delay may be multiple, pulling new image, scaling cluster...
This PR is a port of the code from #639 that I have been using in Keda V1 since the mentioned PR.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO)
• Tests have been added
• (NOT NEEDED)A PR is opened to update the documentation on https://github.com/kedacore/keda-docs
• Changelog has been updated

Fixes #1323

[zroubalik]

@TsuyoshiUshio PTAL^

[TsuyoshiUshio]

Hi @thomas-lamure
Thank you for the contribution! This feature is awesome. I reviewed your code I have only one thing to ask. Could you keep the current behavior as a default? For example, we can add an optional property on the type in https://github.com/kedacore/keda/blob/main/api/v1alpha1/scaledjob_types.go#L64 like AccurateEnablePendingJobCount as bool default false. If any other cool way is there I'm ok as well. Just want to keep the current behavior as default to prevent breaking change for users. I believe your fix will be great for most cases.

[thomas-lamure]

Hi @TsuyoshiUshio,
Thanks for the review. As the current behavior, is quite erratic and is a bug forcing multiple jobs for one message if and only if the pod takes more time than the poll interval to mark the message as being handled, I am not sure that the request to keep the current behavior is really needed.
I am ready however to code it like that if you confirm your request and/or if @zroubalik and/or @tomkerkhove come to the same conclusion as you.
My intention here is to start a discussion on the subject to be sure to make the best improvement possible.
What do you think?

[TsuyoshiUshio]

Hi @thomas-lamure

What you said is totally true. However, I'd like to share why I said that. I'm flexible since your code is cool, I think it depend on the current policy of this repo.

1. The accurate strategy is developed with a customer. They are using it in production with a very important cloud backend system that deploys cloud platform. It works perfectly for now.
2. I'm working for Cloud provider product team. I learned how people use it differently. Even if, it looks no problem feature, deployed then cause unexpected outage. For this repo. I might too careful though.
3. That is why I introduce the Strategy concept to adopt a various request.
4. So that I thought, I'd like to keep the behavior is the safer option since we already GA, with a switch like feature flags. Your change is essential change, then in the next major version release, we can consider, the breaking change and make it default.

As I said, I might be too careful. As you did ask @zroubalik and @tomkerkhove might be a very good idea. speed vs consistency.

[zroubalik]

Even though I am generally all in for consistency, in this particular case, I am more inclined towards what @thomas-lamure is suggesting.

@TsuyoshiUshio I see your points, but is the currect solution really the one, that user would expect?

I am not sure if spinning mutliple jobs for one incoming message is ideal.

@tomkerkhove WDYT?

[tomkerkhove]

I am not sure if spinning mutliple jobs for one incoming message is ideal.

@tomkerkhove WDYT?

I don't see this happening as well since they would compete for the message

[TsuyoshiUshio]

Ok. Then Let's merge it as is!

[zroubalik]

Ok. Then Let's merge it as is!

We can add more detailed info and wanening to the changelog about this particular change, would that be better for you @TsuyoshiUshio ?

[fjmacagno]

@thomas-lamure I don't think this is quite correct. i think pendingJobCount needs to be taken into account in the comparison to maxReplicaCount.

If we say

maxScale = 10
runningJobCount = 5
pendingJobCount = 5
maxReplicaCount = 20

we get 5, which is correct.
10 + 5 = 15 < 20 -=> 10 - 5 = 5

However, if we say:

maxScale = 10
runningJobCount = 5
pendingJobCount = 5
maxReplicaCount = 10

then we get the return 5, even though runningJobCount + pendingJobCount = 10, so the maxReplicaCount is already hit.
(10 + 5 = 15) > 10 -=> 10 - 5 = 5

So this should instead be

if (maxScale + runningJobCount) > maxReplicaCount {
     return maxReplicaCount - runningJobCount  - pendingJobCount
}
return maxScale - pendingJobCount

or to simplify

return min(maxReplicaCount - runningJobCount, maxScale) - pendingJobCount

Reference: #1227 (comment)

[thomas-lamure]

Hi @fjmacagno,

I think the problem here is the terminology, runningJobCount is in fact "jobs that are not finished", so in your example the 5 running jobs are the 5 pending jobs. so the result =5 is good, the runner can accept 5 other jobs.
If you subtract again runningJobCount, you will never scale to maxReplicaCount.

[zroubalik]

Hi @fjmacagno I tend to share @thomas-lamure view on this. But I am happy to hear more from you, whether we need to change something, or change docs in some area. Thanks!