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

feat: Horizontal Pod Autoscaler #15

Merged
merged 8 commits into from
Jan 28, 2025
Merged

feat: Horizontal Pod Autoscaler #15

merged 8 commits into from
Jan 28, 2025

Conversation

dvanmali
Copy link
Contributor

Add an optional chart configuration for Horizontal Pod Autoscaler for the Surreal Deployment.

Changes chart from 0.3.6 > 0.3.7

Copy link
Contributor

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

He @dvanmali! We appreciate your contribution!

This looks great, but I believe we need to address a thing correctly to move forward.

That is, this doesn't work, until we omit the deployment's desired replicas when HPA is enabled when HPA is active. Please see

replicas: {{ .Values.replicaCount }}

Note that it's not straightforward when you deploy the chart without HPA and then enabled it in a second deployment- It's not handled by Helm well so you need some manual steps to make it work. Please also see helm/helm#12650 for more details.

It would be great if you could:

  • Fix the chart so that the deployment replicas is omitted when HPA is active
  • Have some guidance in the chart README about how to migrate from non-HPA deployment to HPA-enabled

Thank you in advance for your support!

@dvanmali
Copy link
Contributor Author

Good catch @mumoshu and thanks for reviewing this PR :)

Fixed the HPA file and added docs to state that HPA is an optional feature which can be added/removed at anytime using helm update.

@dvanmali
Copy link
Contributor Author

dvanmali commented Jan 22, 2025

@mumoshu Fixed with an enabled flag now.

I also ran helm-docs locally on my computer and some of the docs did not match, so made minor changes to values.yaml and README.md to match.

Copy link
Contributor

@mumoshu mumoshu left a comment

Choose a reason for hiding this comment

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

I appreciate your effort, @dvanmali!
This LGTM!

I manually verified that it supports turning HPA on/off. I've also added a quick E2E test to prevent it from breaking in the future. I hope you like it.

Having two or more pods works only when you have a distributed backend like TiKV and FoundationDB. We should document this in a separate pull request.

Thank you for your contribution, @dvanmali!

@mumoshu mumoshu merged commit 9ca46e2 into surrealdb:main Jan 28, 2025
10 checks passed
@dvanmali
Copy link
Contributor Author

Much thanks for writing the e2e tests and the merge 🥇 Looks great! 😄

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.

2 participants