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

only set spec.replicas in Nextcloud Deployment if .Values.hpa.enabled is set to false #596

Merged

Conversation

jessebot
Copy link
Collaborator

@jessebot jessebot commented Jul 23, 2024

Description of the change

  • Setsspec.replicas in Nextcloud Deployment only if hpa.enabled in values.yaml is set to false.
  • update hpa ci test to use hpa.minPods=2

Benefits

This should unbreak the environments that use hpa.enabled in their values.yaml that also use Argo CD?

Possible drawbacks

None that I can think of after #598 was merged! Open to discussion on this one as always :)

Applicable issues

Checklist

@jessebot jessebot self-assigned this Jul 23, 2024
@provokateurin
Copy link
Member

Some kind of test would be nice, the change makes sense but I can not validate it.

@jessebot
Copy link
Collaborator Author

I pinged the original person who opened the Issue that this is a fix for, but in the meantime, I do see that all our normal tests passed, so at least without hpa, it's working fine :)

But wait, does hpa or multi-replicas actually work right now to begin with? 🤔 To test, I'd have to first see if I can get it working with 2 pods to begin with. I'll take some backups and try at some point today, and based on that, will try to create a ci test for this that triggers on tag of hpa.

@jessebot jessebot added the hpa horizontal pod autoscaling - triggers a testing workflow label Jul 23, 2024
@jessebot
Copy link
Collaborator Author

Oh, I also found a Kubernetes sanctioned HPA walkthrough here:
https://kubernetes.io/docs/tasks/run-application/horizontal-pod-autoscale-walkthrough/#run-and-expose-php-apache-server

It doesn't show including a spec.replicas in the Deployment, so I think we're actually in the clear on this :) Also, the new ci test I put in to run on hpa labels also passed, so we've done something right, probably 😅

@jessebot jessebot marked this pull request as ready for review July 23, 2024 11:04
@provokateurin
Copy link
Member

I think we need a summary job, otherwise this is not going to work when the tests are skipped because there were no changes.

@jessebot jessebot force-pushed the fix/dont-set-replicas-in-pod-if-hpa-enabled branch 2 times, most recently from 95c92ae to 2e85081 Compare July 24, 2024 06:05
@jessebot
Copy link
Collaborator Author

Rebased and updated description since #598 was merged. Will wait for tests and then we can merge this one :)

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

I didn't want to drag out the other PR even further, but I think the test should set hpa.minPods to 2 to ensure it works with multiple pods. I'd assume in the testing only 1 pod gets created which doesn't test the feature well.

charts/nextcloud/Chart.yaml Outdated Show resolved Hide resolved
@jessebot
Copy link
Collaborator Author

I didn't want to drag out the other PR even further, but I think the test should set hpa.minPods to 2 to ensure it works with multiple pods. I'd assume in the testing only 1 pod gets created which doesn't test the feature well.

Sounds good! I'll push up a commit to test this and if everything passes, we can squash it all into one commit like we did the last PR and be on our way :)

Copy link
Member

@provokateurin provokateurin left a comment

Choose a reason for hiding this comment

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

HPA works 🎉

… is set to false

update hpa ci test to use a minimum of 2 pods

Signed-off-by: jessebot <jessebot@linux.com>
@jessebot jessebot force-pushed the fix/dont-set-replicas-in-pod-if-hpa-enabled branch from f528d24 to 9e28608 Compare July 24, 2024 06:31
@jessebot jessebot merged commit 34fc2df into nextcloud:main Jul 24, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hpa horizontal pod autoscaling - triggers a testing workflow
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HPA kills new pods instantly after creation
2 participants