Skip to content

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 mabrikan force-pushed the main branch 2 times, most recently from bba7fbf to 6ae4686 Compare March 4, 2023 23:36
@mabrikan mabrikan closed this Mar 4, 2023
@mabrikan mabrikan reopened this Mar 4, 2023
@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.

2 participants