[loki-distributed] Typo in querier HPA template

kovaxur commented 2 years ago

Hi, I think there's a typo in the querier HPA template for loki-distributed, it now only creates the HPA if the indexGateway is enabled:

https://github.com/grafana/helm-charts/blob/bcd11416f10f5ed0b04e9f1b6432f79d1ec1c265/charts/loki-distributed/templates/querier/hpa.yaml#L1

It should be rather:

{{- if and .Values.querier.enabled .Values.querier.autoscaling.enabled }}

kayketeixeira commented 2 years ago

Hi, I think there's a typo in the querier HPA template for loki-distributed, it now only creates the HPA if the indexGateway is enabled:

https://github.com/grafana/helm-charts/blob/bcd11416f10f5ed0b04e9f1b6432f79d1ec1c265/charts/loki-distributed/templates/querier/hpa.yaml#L1

It should be rather:

{{- if and .Values.querier.enabled .Values.querier.autoscaling.enabled }}

Any solution for this case?

renatosis commented 2 years ago

We want to enable HPA but it's attached to index-gateway usage

verejoel commented 2 years ago

It's not a typo, it's definitely a design choice. The current architecture seems to be:

if index gateway is enabled, run the querier as a Deployment and allow autoscaling
if index gateway is disabled, run the querier as a StatefulSet, and assign a static number of replicas with no option of autoscaling.

{{- if and .Values.querier.enabled .Values.querier.autoscaling.enabled }}

This solution doesn't work, as querier is not an optional component - there is no flag to enable or disable it in the Chart values.

What this actually boils down to is a feature request: allow autoscaling ingesters when deployed as a StatefulSet. This should be feasible, unless there is some Loki specific reason why it's not a good idea to autoscale stateful ingesters.

grafana / helm-charts

[loki-distributed] Typo in querier HPA template #1391