8000 add horizontalpodautoscaler by coolbry95 · Pull Request #3276 · nginx/kubernetes-ingress · GitHub
[go: up one dir, main page]
Skip to content

add horizontalpodautoscaler #3276

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
Merged
merged 2 commits into from
Nov 25, 2022
Merged

add horizontalpodautoscaler #3276

merged 2 commits into from
Nov 25, 2022

Conversation

coolbry95
Copy link
Contributor
@coolbry95 coolbry95 commented Nov 18, 2022 •
edited
Loading

Proposed changes

Adds the horizontalpodautoscaler for the ingress controller

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@coolbry95 coolbry95 requested a review from a team as a code owner November 18, 2022 16:27
@github-actions github-actions bot added the documentation Pull requests/issues for documentation label Nov 18, 2022
@coolbry95
Copy link
Contributor Author

Broke the last branch because of merges. I will rebase my branch back on to master. I squash the commits when doing the rebasing.

@shaun-nx
Copy link
Contributor

I'm including a link to the old (closed) PR of this so we don't lose the discussions: #3249

@shaun-nx
Copy link
Contributor

@coolbry95 Can you follow-up this discussion thread? #3249 (comment)

@brianehlert
Copy link
Collaborator

I want everyone to be mindful here.
I see that HPA will be removing a version with 1.26, however that will pull in the supported version of K8s to 1.23 https://kubernetes.io/blog/2022/11/18/upcoming-changes-in-kubernetes-1-26/#removal-of-the-v2beta2-horizontalpodautoscaler-api

Which I think is too close to the current release of K8s for many of our customers.

Pulling in the current supported K8s version to 1.21 is as close as I want to go.
And that is already happening due to EndpointSlices.

@lucacome
Copy link

@brianehlert since autoscaling/v2 is available starting from 1.23 I think it makes sense to use that instead of autoscaling/v2beta2, which will be deprecated when 1.26 comes out in a couple of weeks. This just means that anybody on versions < 1.23 won't be able to use HPA.

@brianehlert
Copy link
Collaborator

@brianehlert since autoscaling/v2 is available starting from 1.23 I think it makes sense to use that instead of autoscaling/v2beta2, which will be deprecated when 1.26 comes out in a couple of weeks. This just means that anybody on versions < 1.23 won't be able to use HPA.

As long as that is somehow non-breaking for customers that might install the next version of NIC on 1.22 or 1.21
I don't want to bring the oldest supported version to 1.23 simply for the convenience of having HPA in the Helm chart. That is my point.

@coolbry95
Copy link
Contributor Author

This won't pull the minimum version to 1.23. The HPA will only be available for clusters in 1.23 and above.

@github-actions github-actions bot added the helm_chart Pull requests that update the Helm Chart label Nov 24, 2022
@shaun-nx
Copy link
Contributor

Confirmed that the HPA resource doesn't get install on a server version less than 1.23 👍
The feature is behaving as expected on k8s server >= 1.23 as well. Looks all good to me, nice work!

@jjngx jjngx self-requested a review November 24, 2022 15:42
@shaun-nx shaun-nx merged commit dcd8ad9 into nginx:main Nov 25, 2022
@coolbry95
Copy link
Contributor Author

Fixes #3174

@brianehlert brianehlert mentioned this pull request Jun 22, 2023
Closed
@coolbry95 coolbry95 mentioned this pull request Oct 10, 2023
Merged
6 tasks
@sindhushiv sindhushiv mentioned this pull request Jun 4, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucacome lucacome Awaiting requested review from lucacome lucacome was automatically assigned from nginxinc/kic

@haywoodsh haywoodsh Awaiting requested review from haywoodsh haywoodsh was automatically assigned from nginxinc/kic

2 more reviewers

@jjngx jjngx jjngx approved these changes

@shaun-nx shaun-nx shaun-nx approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees

@shaun-nx shaun-nx

Labels
documentation Pull requests/issues for documentation helm_chart Pull requests that update the Helm Chart
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@coolbry95 @shaun-nx @brianehlert @lucacome @jjngx @tomasohaodha
0