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

Remove replicas if KEDA is enabled #29838

Merged
merged 1 commit into from
Mar 10, 2023
Merged

Remove replicas if KEDA is enabled #29838

merged 1 commit into from
Mar 10, 2023

Conversation

mabrikan
Copy link
Contributor

@mabrikan mabrikan commented Mar 1, 2023

Remove replicas field in workers Deployment/StatefulSet if KEDA is enabled

GitOps operators (e.g. ArgoCD) complain when the manifest is changed due to KEDA/HPA auto-scaling. The good practice is to remove replicas field when auto-scaling is used.

Reference:
https://argo-cd.readthedocs.io/en/stable/user-guide/best_practices/#leaving-room-for-imperativeness

@boring-cyborg boring-cyborg bot added the area:helm-chart Airflow Helm Chart label Mar 1, 2023
@potiuk
Copy link
Member

potiuk commented Mar 4, 2023

needs static checks fixes and rebase.

@mabrikan
Copy link
Contributor Author

mabrikan commented Mar 4, 2023

Done, @potiuk.

@potiuk
Copy link
Member

potiuk commented Mar 5, 2023

Looks like there are some side-effects of that change - the errors look quite related.

@potiuk
Copy link
Member

potiuk commented Mar 5, 2023

Nope. It happens in main - we need to fix it there first.

@potiuk
Copy link
Member

potiuk commented Mar 5, 2023

Can you please rebase @mabrikan -> I think the errors you see should have been fixed in main already, but it's good to double-check it.

@mabrikan
Copy link
Contributor Author

mabrikan commented Mar 5, 2023

Sure thing 👍

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Looks good now. And the explanation is plausible. @jedcunningham @dstandish - any problem you see with removing replicas in this case?

@potiuk
Copy link
Member

potiuk commented Mar 10, 2023

I read a bit more and yeah - that's also official recommendation for HPA to not set replicas, so it should be OK to merge. https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#replicas

@dstandish @jedcunningham - shout if you think otherwise :)

@potiuk potiuk merged commit e60be9e into apache:main Mar 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:helm-chart Airflow Helm Chart
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants