-
Notifications
You must be signed in to change notification settings - Fork 1k
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
feat: allow setting service annotations via helm chart #6894
Conversation
✅ Deploy Preview for karpenter-docs-prod canceled.
|
What's preventing you from using the existing |
|
This PR has been inactive for 14 days. StaleBot will close this stale PR after 14 more days of inactivity. |
@jmdeal I would like to move forward with this if my explanation makes sense. |
Sorry this has slipped, could you elaborate on what the consequences of this are?
Unless this is strictly blocking, and no other workarounds exist, we don't want to expand the configuration surface for the helm chart we ship. |
The datadog agent watches for annotations to configure it's checks/metric collection. Using the existing additionalAnnotations would create annotations on more than one resource. This would configure the datadog agent to run multiple collections of the same metrics making it unusable. If anyone wants to use datadog to collect metrics using datadog's autodiscovery they will also have this problem. |
Got it, I assume for most resources this would be a no-op, but you can get duplicate metrics since it would scrape both the Karpenter pods and the service? |
Correct. In summary, using `additionalAnnotations` would duplicate metrics sent to Datadog. Adding `service.annotations` on the service would allow us to create annotations to create only one Datadog metric collection.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed offline with other team members, the use-case seems strong enough. LGTM 🚀
Pull Request Test Coverage Report for Build 11448527366Details
💛 - Coveralls |
Fixes #N/A
Description
Allows setting annotations on the karpenter service. We need this so that we can add annotations to setup datadog to monitor karpenter. There are probably other use cases.
How was this change tested?
Generated a helm chart locally with the additional values:
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.