feat: Add annotations only to the karpenter service #6065
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
| @@ -5,7 +5,7 @@ metadata: | |||
| namespace: {{ .Release.Namespace }} | |||
| labels: | |||
| {{- include "karpenter.labels" . | nindent 4 }} | |||
| {{- with .Values.additionalAnnotations }} | |||
There was a problem hiding this comment.
This is a breaking change for anyone who is currently expecting that these additionalAnnotations are going to be added to their service. Just to understand your use-case -- which annotations are you trying to add to the service that you don't want to add to the other components?
There was a problem hiding this comment.
Yeah, maybe we can put both additional annotations AND service annotations to the service ?
I want to put the prometheus annotations only on my service and not on my pods for example.
There was a problem hiding this comment.
I want to put the prometheus annotations only on my service and not on my pods for example.
Can you clarify a bit on the use-case here? I'd like to avoid getting building bespoke configuration for each resource so I think we need a pretty common use-case.
Is there a possibility that you could leverage helm template and then use an overlay to update the Service to be what you want? Or maybe use jq/yq to update the configuration directly?
There was a problem hiding this comment.
Most of the time, people have prometheus as a resource discovery. So it looks for every pods / services that have the prometheus annotations to allow scraping. If I put my annotations inside the additionalAnnotations, my pods AND my service would be discovered by prometheus and be scrapped what I want to avoid.
I don't want to customize my way to deploy / do some hotfix like that.
I think we can just combine additionalAnnotations AND serviceAnnotations here what do you think ?
There was a problem hiding this comment.
my service would be discovered by prometheus and be scrapped what I want to avoid
I thought the mechanism that Prometheus discovered services and pods were different and you would be able to filter out services that contain a given label through your prometheus config. Do you mind sharing your prometheus config?
I also don't see this pattern just from glancing around at a few helm charts. Do you have some examples of helm charts out in the community that implement this pattern of sectioning out their service and pod annotations?
There was a problem hiding this comment.
Yes it's indeed possible but my config is scraping every pods AND service that have these annotations without any differences. So yeah maybe this is a bad usage on my side but for example, see node exporter from prometheus :
https://github.com/prometheus-community/helm-charts/blob/a1a6d92b04d4ac3eb553348b127ab7871045c934/charts/prometheus-node-exporter/values.yaml#L95
There was a problem hiding this comment.
@afreyermuth98 I'm just realizing that we have a podAnnotations value. Can you just use this to add the annotations you're targeting to the pods? https://github.com/aws/karpenter-provider-aws/blob/main/charts/karpenter/templates/deployment.yaml#L30
There was a problem hiding this comment.
Hey @jonathan-innis my use case is the contrary. I want to annotate only the service ^^
There was a problem hiding this comment.
So, if you use the service here, I think you are going to run into problems. The service is going to randomly pick pods from the selector and that's going to cause issues with something that is running with leader election. Sometimes, you're going to get the thing that's actually firing metrics. Sometimes, you're going to get the thing that isn't firing metrics. We added the service mainly for: 1) Endpoint discovery for the ServiceMonitor and 2) Serving the webhook endpoint.
Can you just scrape the pods here?
Pull Request Test Coverage Report for Build 8755536921Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
|
This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity. |
Description
Allow to put some annotations only for the service. It's useful when we have some prometheus operator and I don't want to put my prometheus annotations on every resources.
How was this change tested?
I deployed it using the chart locally. As it's a very small change it's ok :)
Does this change impact docs?
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.