search

grafana / helm-charts

Apache License 2.0
1.67k stars 2.28k forks source link

[tempo-distributed] Add required name label to ingesters #3315

Closed alex5517 closed 2 months ago

alex5517 commented 2 months ago

name label is required on pods that should be managed by the rollout-operator this pr ensure that it is added. https://github.com/grafana/rollout-operator/issues/15

Sheikh-Abubaker commented 2 months ago

@alex5517 is this PR intended for rollout-operator helm charts ?

alex5517 commented 2 months ago

@Sheikh-Abubaker

Sorry if the PR was a bit slim on information...

The PR is for the Tempo-Distributed chart. "Recently" the chart got support for deploying the the Tempo ingesters with zone awareness and to manage these new statefulsets it leverage the Grafana Rollout-Operator, but this is not working since the resulting pods from the statefulsets does not have the currently required name label. The issue for removing this requirement was the link i referred to: https://github.com/grafana/rollout-operator/issues/15

Sheikh-Abubaker commented 2 months ago

The PR is for the Tempo-Distributed chart. "Recently" the chart got support for deploying the the Tempo ingesters with zone awareness and to manage these new statefulsets it leverage the Grafana Rollout-Operator, but this is not working since the resulting pods from the statefulsets does not have the currently required name label. The issue for removing this requirement was the link i referred to: grafana/rollout-operator#15

@alex5517 If I didn't get it wrong the scope of this change is to include name label within the ingester pods created when tempo-distributed chart is deployed ?

Like the ingester pods below right ?

$ kubectl get pods
NAME                                      READY   STATUS    RESTARTS     AGE
my-tempo-compactor-9c7968d5b-rmr6c        1/1     Running   0            76s
my-tempo-distributor-5ffbf5d5cc-ktggf     1/1     Running   0            76s
my-tempo-ingester-zone-a-0                1/1     Running   0            76s
my-tempo-ingester-zone-b-0                1/1     Running   0            76s
my-tempo-ingester-zone-c-0                1/1     Running   0            76s
my-tempo-memcached-0                      1/1     Running   0            76s
my-tempo-querier-669f584d9c-jgrxv         1/1     Running   0            76s
my-tempo-query-frontend-59bcfb9f9-kfzwt   1/1     Running   0            76s
alex5517 commented 2 months ago

@Sheikh-Abubaker,

Yes that is correct, it is the same that is done with the Mimir-distributed chart. https://github.com/grafana/mimir/blob/main/operations/helm/charts/mimir-distributed/templates/_helpers.tpl#L296