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

feat: Migrate to autoscalingv2 #3606

Merged
merged 8 commits into from
Sep 14, 2022
Merged

feat: Migrate to autoscalingv2 #3606

merged 8 commits into from
Sep 14, 2022

Conversation

JorTurFer
Copy link
Member

@JorTurFer JorTurFer commented Aug 27, 2022 •
edited
Loading

Signed-off-by: Jorge Turrado [email protected]

Provide a description of what has been changed

Checklist

  • Commits are signed with Developer Certificate of Origin (DCO - learn more)
  • Tests have been added
  • Changelog has been updated and is aligned with our changelog requirements

Fixes #2462
Related to kedacore/charts#315

zroubalik
zroubalik reviewed Aug 29, 2022
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.

In the scaledobject_controller.go around line 133 you should check this as well:

Owns(&autoscalingv2beta2.HorizontalPodAutoscaler{}).

I haven't reviewed the whole PR yet.

@JorTurFer
Copy link
Member Author

In the scaledobject_controller.go around line 133 you should check this as well:

Owns(&autoscalingv2beta2.HorizontalPodAutoscaler{}).

I haven't reviewed the whole PR yet.

Nice catch! Updated

@JorTurFer JorTurFer changed the title feat: Add support to autoscalingv2 feat: Migrate to autoscalingv2 Aug 31, 2022
@JorTurFer JorTurFer force-pushed the autoscaling-v2 branch 2 times, most recently from 6bf1b09 to 7e9ae89 Compare August 31, 2022 19:14
@JorTurFer
Copy link
Member Author

I have redone the PR with to migrate to v2 instead of supporting both. I'll prepare the PR to docs ASAP

@JorTurFer
Copy link
Member Author

JorTurFer commented Aug 31, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer force-pushed the autoscaling-v2 branch 2 times, most recently from 6974da8 to ff89ecf Compare September 2, 2022 08:15
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 2, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer marked this pull request as ready for review September 2, 2022 08:24
@JorTurFer JorTurFer requested a review from a team as a code owner September 2, 2022 08:24
tomkerkhove
tomkerkhove reviewed Sep 13, 2022
View reviewed changes
Copy link
Member

@tomkerkhove tomkerkhove left a comment

Choose a reason for hiding this comment

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

Added some remarks but implementation is OK.

Would you mind sending a PR to update our Helm chart to enforce 1.23 or above please?

Did you verify that the new API version doesn't have breaking changes that impact us? (on the phone)

CHANGELOG.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
RELEASE-PROCESS.md Outdated Show resolved Hide resolved
@JorTurFer
Copy link
Member Author

Would you mind sending a PR to update our Helm chart to enforce 1.23 or above please?

I'm not sure about enforce 1.23 in the chart because users can modify the tag so new chart could use old images. But maybe it doesn't make sense. I'll open it.

Did you verify that the new API version doesn't have breaking changes that impact us? (on the phone)

I have tried running the e2e/smoke test against clusters with autoscaling/v2 and they pass, I'd be nice if I had a k8s v1.26 but atm IDK how to test that :/

@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 13, 2022 •
edited by github-actions bot
Loading

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

@JorTurFer JorTurFer mentioned this pull request Sep 13, 2022
Merged
1 task
@tomkerkhove
Copy link
Member

Would you mind sending a PR to update our Helm chart to enforce 1.23 or above please?

I'm not sure about enforce 1.23 in the chart because users can modify the tag so new chart could use old images. But maybe it doesn't make sense. I'll open it.

That's a valid point but think it's best to guide/force end-users that follow the usual path; if they need the latest chart it typically means they need a feature that is in 2.9. Otherwise they can use our current Helm chart.

Did you verify that the new API version doesn't have breaking changes that impact us? (on the phone)

I have tried running the e2e/smoke test against clusters with autoscaling/v2 and they pass, I'd be nice if I had a k8s v1.26 but atm IDK how to test that :/

I guess the breaking changes docs from Kubernetes but Iguess it's OK, no worries

@JorTurFer
feat: migrate to autoscalingv2
321209b 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
Add missing changes
1810095 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
remove unsupported versions
168d7bd 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
add k8s v1.25 to versions test
769766c 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
add a compatibility matrixs
74de3eb 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer
apply feedback
05f14a3 
Signed-off-by: Jorge Turrado <[email protected]>
@JorTurFer JorTurFer mentioned this pull request Sep 14, 2022
Merged
1 task
@JorTurFer
Copy link
Member Author

JorTurFer commented Sep 14, 2022 •
edited by github-actions bot
Loading

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

zroubalik
zroubalik approved these changes Sep 14, 2022
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.

LGTM

@JorTurFer
Copy link
Member Author

How does it look to you @tomkerkhove ? Can I merge it?

tomkerkhove
tomkerkhove approved these changes Sep 14, 2022
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@JorTurFer @tomkerkhove
apply feedback
00f627f 
Co-authored-by: Tom Kerkhove <[email protected]>
Signed-off-by: Jorge Turrado Ferrero <[email protected]>
@JorTurFer JorTurFer merged commit 23bc75f into kedacore:main Sep 14, 2022
@JorTurFer JorTurFer deleted the autoscaling-v2 branch September 14, 2022 09:06
@JorTurFer JorTurFer mentioned this pull request Oct 14, 2022
Closed
joelsmith pushed a commit to joelsmith/keda that referenced this pull request Feb 9, 2023
@JorTurFer @tomkerkhove @joelsmith
UPSTREAM: 3606: feat: Migrate to autoscalingv2 (kedacore#3606)
6f48778 
* feat: migrate to autoscalingv2

Signed-off-by: Jorge Turrado Ferrero <[email protected]>
Co-authored-by: Tom Kerkhove <[email protected]>
@joelsmith joelsmith mentioned this pull request Feb 9, 2023
Merged
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

@tomkerkhove tomkerkhove tomkerkhove approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for autoscaling/v2-based HorizontalPodAutoscaler on Kubernetes v1.23+
3 participants
@JorTurFer @tomkerkhove @zroubalik