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

Support for Custom Annotations on Linkerd-Viz Prometheus Deployment #11374

Merged
merged 7 commits into from
Oct 3, 2023
Merged

Support for Custom Annotations on Linkerd-Viz Prometheus Deployment #11374

merged 7 commits into from
Oct 3, 2023

Conversation

cemenson
Copy link
Contributor

Support for Custom Annotations on Linkerd-Viz Prometheus Deployment

Currently, adding custom annotations to the prometheus deployment isn't supported. This can't be done either with --set helm flag or by adding relevant config to values.yaml.

By adding additional helm values by way of a with keyword within the prometheus deployment yaml, we can support custom annotations. Setting the value of the reverenced value (.Values.prometheusAnnotations) to empty will ensure this doesn't break or modify any existing deployments.

Once merged, setting prometheusAnnotations in the values.yaml or via a --set helm flag will result in specified annotations being included in the prometheus deployment and pod spec.

Fixes #11365

I agree to the DCO for all the commits in this PR.

@cemenson cemenson requested a review from a team as a code owner September 14, 2023 08:39
@cemenson
Copy link
Contributor Author

@alpeb is this good to merge?

@cemenson cemenson requested a review from alpeb September 18, 2023 22:46
Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

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

Thanks @cemenson, this looks good to me.

@cemenson
Copy link
Contributor Author

@adleong could I trouble you for a review of this?

@adleong
Copy link
Member

adleong commented Sep 25, 2023

Hi @cemenson, can you tell me more about the use case? What kinds of annotations do you need to add to the Prometheus deployment and why? I'll note that we already have a podAnnotations value which can be used to set annotations on the pods themselves. It would be great to understand in what scenarios annotations need to be added to the deployement vs added to the pod template.

@cemenson
Copy link
Contributor Author

cemenson commented Sep 25, 2023

Hi @cemenson, can you tell me more about the use case? What kinds of annotations do you need to add to the Prometheus deployment and why? I'll note that we already have a podAnnotations value which can be used to set annotations on the pods themselves. It would be great to understand in what scenarios annotations need to be added to the deployement vs added to the pod template.

Hi @adleong. For our specific case, we need to set an annotation config.linkerd.io/skip-inbound-ports on the prometheus pod. There is no way to do this as part of the helm install, other than as you said setting the podAnnotations value. Whilst this would work, it would also apply (as I understand from the chart) the annotation to all the other pods that are part of the linkerd-viz chart (tap, web, etc.), which is not desired.

@adleong
Copy link
Member

adleong commented Sep 25, 2023

thanks @cemenson, that context is super helpful!

How would you feel about renaming the value to prometheus.podAnnotations and having it apply just to the prometheus pod template and not the deployment itself?

@cemenson
Copy link
Contributor Author

thanks @cemenson, that context is super helpful!

How would you feel about renaming the value to prometheus.podAnnotations and having it apply just to the prometheus pod template and not the deployment itself?

Yes that's fine @adleong. I've made those changes now.

@adleong
Copy link
Member

adleong commented Oct 2, 2023

Thanks, @cemenson. I'm seeing some weird CI failures that I can't reproduce locally. Could you try pulling the latest main and merging it into this branch?

Edit: I've merged main, let's see if that resolves the CI issues.

@adleong
Copy link
Member

adleong commented Oct 3, 2023

🎉 🎉 🎉

@adleong adleong merged commit eecf11a into linkerd:main Oct 3, 2023
33 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.

Support for Custom Annotations on Linkerd-Viz Prometheus Deployment
3 participants