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

Introduce minReplicaCount for scaledjob #3425

Merged
merged 14 commits into from
Aug 2, 2022

Conversation

FrittenToni
Copy link
Contributor

@FrittenToni FrittenToni commented Jul 26, 2022

Signed-off-by: Simon Oehm [email protected]

Provide a description of what has been changed

This PR introduces a property called minReplicaCount. The purpose of this property is to specify a job amount that should be created by default. The feature is useful if you want to avoid pod startup times and can be used in combination with "custom" strategy and "customScalingQueueLengthDeduction" property.

Without "minReplicaCount" only the following scaling can be achieved:

kedaScalingOld

With "minReplicaCount" we can achieve the following result:
kedaScalingNew

Checklist

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

Fixes #

Relates to #3426
Documentation: kedacore/keda-docs#836

Copy link
Member

@zroubalik zroubalik left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, before I am gonna do a full review, could you please adress these points:

Signed-off-by: Simon Oehm <[email protected]>
Signed-off-by: Simon Oehm <[email protected]>
Signed-off-by: Simon Oehm <[email protected]>
Signed-off-by: Simon Oehm <[email protected]>
@FrittenToni
Copy link
Contributor Author

Hi @zroubalik, I've incorporated your requests. Please have a look.

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.

Looking good,
some nits inline

@@ -111,8 +113,19 @@ func init() {
// MaxReplicaCount returns MaxReplicaCount
func (s ScaledJob) MaxReplicaCount() int64 {
if s.Spec.MaxReplicaCount != nil {
return int64(*s.Spec.MaxReplicaCount)
return int64(*s.Spec.MaxReplicaCount) - s.MinReplicaCount()
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this? Does MaxReplicaCount change based on MinReplicaCount?

Copy link
Contributor Author

@FrittenToni FrittenToni Aug 1, 2022

Choose a reason for hiding this comment

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

Without - s.MinReplicaCount() Keda would scale potentially to maxReplicaCount + minReplicaCount amount of jobs because the actual scaling logic/ scaling strategy knows nothing about the minReplica scaling. This can be seen greatly when removing - s.MinReplicaCount() and running the e2e test. Keda will scale > maxReplicaCount.

@JorTurFer
Copy link
Member

JorTurFer commented Jul 29, 2022

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

Signed-off-by: Simon Oehm <[email protected]>
Signed-off-by: Simon Oehm <[email protected]>
Signed-off-by: Simon Oehm <[email protected]>
}

return isTargetAchieved
}
Copy link
Contributor Author

@FrittenToni FrittenToni Aug 1, 2022

Choose a reason for hiding this comment

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

I've added this function to ensure that no overscaling happens. The waitForJobCount function above in the file returns the result immediately as soon as count == target. This function waits for all iterations before returning the final result.

@FrittenToni
Copy link
Contributor Author

/run-e2e min_replica*

@JorTurFer
Copy link
Member

JorTurFer commented Aug 2, 2022

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

@JorTurFer
Copy link
Member

/run-e2e min_replica*

This will not work, only organization members can run the e2e tests. I have triggered them again, now with the correct regex 😄

CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Zbynek Roubalik <[email protected]>
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.

4 participants