add autoscaling for deployment to helm chart #3221
Conversation
| minReplicas: {{ .minReplicas }} | ||
| maxReplicas: {{ .maxReplicas }} | ||
| metrics: | ||
| {{- with .targetCPUUtilizationPercentage }} |
There was a problem hiding this comment.
Does it always make sense to scale based on both cpu and memory? If we scale by memory, but are not so concerned about cpu, can we do that?
There was a problem hiding this comment.
Yes, we can scale by only memory or CPU. The documentation is updated to reflect that targetCPUUtilizationPercentage or targetMemoryUtilizationPercentage can be diable by setting them to 0. Thanks!
|
If you want to merge your other change in, I think we can make a 0.10.0 release with these. You can update chart.yaml and the changelog for that. |
There was a problem hiding this comment.
Thanks for the PR! I'm a little nervous about this one, actually. I think we definitely want to support HPAs at some point, but I don't think we have a good grip on how to use them effectively when configuring the agent as a whole.
If we're going to merge this, I think it there should probably be a section in the chart's readme which explains how to use the controller.autoscaling section and specifically what use cases it works with. Since you mentioned the app agent receiver, we can probably start there and expand it out as we gain more knowledge.
@captncraig do you think this is something we can merge and support today or should we hold off for a while?
| @@ -0,0 +1,34 @@ | |||
| {{- if and (eq .Values.controller.type "deployment") .Values.controller.autoscaling.enabled }} | |||
There was a problem hiding this comment.
Would it be possible to use an HPA with a StatefulSet too?
There was a problem hiding this comment.
Yes, it is possible to use an HPA with a StatefulSet. But to reduce the nervousness about this change, it is only targeted toward deployment. If we are comfortable with adding HPA for StatefulSet, I can make the change to add it. Thanks!
|
I agree with your concerns, but am still ok merging it, as long as the language is clear. This will not reshard or anything like that, and it may create a lot of redundant work if your config is not suited. This will be extremely useful once we have native clustering though. |
|
@therealmanny would you like to take a shot at adding a header to document this to the chart's readme, or would you prefer for us to do it? |
@rfratto I can take a shot at documenting this to the chart's readme. |
|
Thanks for the contribution! |
github-actions
bot
added
the
frozen-due-to-age
PR Description
This change adds autoscaling value for deployment to the helm chart. Provides the ability to scale app agent receiver deployment.
Which issue(s) this PR fixes
None
Notes to the Reviewer
In this pull request, I am assuming it is a good idea to run more than one app agent receiver integration configured grafana agent replicas forwarding telemetry to the same location. If this is wrong, you can reject this pull request. Thanks!
PR Checklist