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

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

Merged

Conversation

alex5517
Copy link
Contributor

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

Signed-off-by: Alexander Soelberg Heidarsson <[email protected]>
@Sheikh-Abubaker
Copy link
Collaborator

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

Signed-off-by: Alexander Soelberg Heidarsson <[email protected]>
@alex5517
Copy link
Contributor Author

@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: grafana/rollout-operator#15

@alex5517 alex5517 changed the title Add required name label to ingesters [tempo-distributed] Add required name label to ingesters Sep 17, 2024
Copy link
Member

@mapno mapno left a comment

Choose a reason for hiding this comment

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

LGTM

@Sheikh-Abubaker
Copy link
Collaborator

Sheikh-Abubaker commented Sep 23, 2024

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
Copy link
Contributor Author

@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

Copy link
Collaborator

@Sheikh-Abubaker Sheikh-Abubaker left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution @alex5517, keep them coming!

@Sheikh-Abubaker Sheikh-Abubaker merged commit b435f2b into grafana:main Sep 24, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants