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.

Sign up for GitHub

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

Jump to bottom

ScaledJob: introduce rolloutStrategy #2164

Merged
merged 14 commits into from
Nov 1, 2021
Merged

ScaledJob: introduce rolloutStrategy #2164

merged 14 commits into from
Nov 1, 2021

Conversation

etamarw
Copy link
Contributor

@etamarw etamarw commented Oct 6, 2021 •
edited
Loading

This pr should fix / add features as described on this issue: #2098

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • A PR is opened to update the documentation on (repo) (if applicable)
  • Changelog has been updated

Fixes #

I've added the field rolloutStrategy (gradual / immediate - the default). it lets the user to choose whether to redeploy all running jobs created by the previous version of the scalejob or to rollout new changes gradually. means only new jobs will contain the latest changes, and already running jobs wont get deleted.
also, i fixed the bug we had with returning the number of jobs the controller deletes.

its my first contribution over here so i really need a code review :)

@etamarw
add rolloutStrategy to the scaledjob controller
cf241c0 
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw
add rolloutStrategy to the scaledjob types
4d077e2 
Signed-off-by: etamarw <etamarw@wix.com>
zroubalik
zroubalik reviewed Oct 11, 2021
View reviewed changes
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.

Looking good, please run make build & make manifests to also include regenerated CRDs and deepcopy objects.

@zroubalik zroubalik changed the title Etamar/rollout strategy ScaledJob: introduce rolloutStrategy Oct 11, 2021
zroubalik
zroubalik reviewed Oct 12, 2021
View reviewed changes
Copy link
Member

@zroubalik zroubalik left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good, do you think that you can add an unit test for this feature? Similar to ScaledObject one: https://github.com/kedacore/keda/blob/main/controllers/keda/scaledobject_controller_test.go

And update documentation: https://keda.sh/docs/2.5/concepts/scaling-jobs/

config/crd/bases/keda.sh_scaledobjects.yaml Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

@TsuyoshiUshio PTAL :)

@zroubalik zroubalik added this to the v2.5.0 milestone Oct 12, 2021
arschles
arschles suggested changes Oct 12, 2021
View reviewed changes
config/crd/bases/keda.sh_clustertriggerauthentications.yaml Outdated Show resolved Hide resolved
controllers/keda/scaledjob_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledjob_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledjob_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledjob_controller.go Outdated Show resolved Hide resolved
controllers/keda/scaledjob_controller.go Outdated Show resolved Hide resolved
@zroubalik
Copy link
Member

@etamarw any update on this please?

@etamarw
Copy link
Contributor Author

etamarw commented Oct 20, 2021

sorry for the latency, had a really busy week. ill try to push relevant changes in the next few days.

@zroubalik
Copy link
Member

@etamarw no problem :) It would be nice to include this change into the next release which will be in (~3 weeks), so you have some time, no need to rush. Thanks!

etamarw and others added 6 commits October 20, 2021 19:10
@etamarw
add generated crds and deepcopy obj
f636f7c 
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw
fix typo deleteing -> deleting
156df97 
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw @arschles
set logger message according to best practices
15685dd 
Co-authored-by: Aaron Schlesinger <70865+arschles@users.noreply.github.com>
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw @arschles
set logger messages according to best practices
cc1c9df 
Co-authored-by: Aaron Schlesinger <70865+arschles@users.noreply.github.com>
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw
remove redundant files
cb2d68e 
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw
nit: set case before the default
f10cfbc 
Signed-off-by: etamarw <etamarw@wix.com>
TsuyoshiUshio
TsuyoshiUshio approved these changes Oct 20, 2021
View reviewed changes
Copy link
Contributor

@TsuyoshiUshio TsuyoshiUshio left a comment

Choose a reason for hiding this comment

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

Thank you! It is very good contribution! I love your idea! I leave only one comment for @zroubalik check.

config/crd/bases/keda.sh_scaledjobs.yaml Show resolved Hide resolved
@etamarw
restore relevant files
ec1b438 
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw
use ctx in favor of context.TODO()
c973cf3 
Signed-off-by: etamarw <etamarw@wix.com>
@etamarw etamarw requested a review from arschles October 25, 2021 17:42
@etamarw
Copy link
Contributor Author

etamarw commented Oct 25, 2021

@zroubalik please make sure that i restored all of the relevant files.

@zroubalik
Copy link
Member

@etamarw looking good, could you please resolve the conflict in go.mod and add entry to the changelog (Unreleased, New section?

@etamarw
add scaledjob rolloutStrategy to unreleased changelog
3490e77 
Signed-off-by: etamarw <etamarw@wix.com>
arschles
arschles approved these changes Oct 26, 2021
View reviewed changes
Copy link
Contributor

@arschles arschles left a comment

Choose a reason for hiding this comment

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

this looks great @etamarw 👍

zroubalik
zroubalik reviewed Oct 26, 2021
View reviewed changes
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.

@etamarw Looking good, the last thing is missing is to document this feature in here: https://github.com/kedacore/keda-docs/blob/master/content/docs/2.5/concepts/scaling-jobs.md

CHANGELOG.md Outdated Show resolved Hide resolved
@etamarw
Copy link
Contributor Author

etamarw commented Oct 30, 2021

pr to the docs repo: kedacore/keda-docs#563

etamarw and others added 2 commits October 30, 2021 17:28
@etamarw
Merge branch 'main' into etamar/rollout-strategy
3b6ed61
@etamarw @zroubalik
Update CHANGELOG.md
57b5dc6 
Co-authored-by: Zbynek Roubalik <726523+zroubalik@users.noreply.github.com>
Signed-off-by: etamarw <etamarw@wix.com>
@zroubalik zroubalik merged commit 197594d into kedacore:main Nov 1, 2021
@zroubalik zroubalik mentioned this pull request Nov 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zroubalik zroubalik zroubalik approved these changes

@arschles arschles arschles approved these changes

@TsuyoshiUshio TsuyoshiUshio TsuyoshiUshio approved these changes

@ahmelsayed ahmelsayed Awaiting requested review from ahmelsayed

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@etamarw @zroubalik @arschles @TsuyoshiUshio