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

chore: helm - add servicemonitor api condition #7695

Merged
merged 3 commits into from
Feb 10, 2025

Conversation

splichy
Copy link
Contributor

@splichy splichy commented Feb 5, 2025

Fixes #N/A

Description
Add servicemonitor condition check if monitoring.coreos.com/v1 is available
This will prevent failure when Prometheus CRDs are not installed yet, e.g. during initial cluster installation, where we usually want to install Karpenter before other services

How was this change tested?

$ helm template karpenter . --namespace karpenter --set serviceMonitor.enabled=true -s templates/servicemonitor.yaml --api-versions monitoring.coreos.com/v1
---
# Source: karpenter/templates/servicemonitor.yaml
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  name: karpenter
  namespace: karpenter
  labels:
    helm.sh/chart: karpenter-1.2.1
    app.kubernetes.io/name: karpenter
    app.kubernetes.io/instance: karpenter
    app.kubernetes.io/version: "1.2.1"
    app.kubernetes.io/managed-by: Helm
spec:
  namespaceSelector:
    matchNames:
      - karpenter
  selector:
    matchLabels:
      app.kubernetes.io/name: karpenter
      app.kubernetes.io/instance: karpenter
  endpoints:
    - port: http-metrics
      path: /metrics
      
 $ helm template karpenter . --namespace karpenter --set serviceMonitor.enabled=true -s templates/servicemonitor.yaml
 Error: could not find template templates/servicemonitor.yaml in chart

Does this change impact docs?

  • Yes, PR includes docs updates
  • Yes, issue opened: #
  • No

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@splichy splichy requested a review from a team as a code owner February 5, 2025 12:48
@splichy splichy requested a review from danxwang22 February 5, 2025 12:48
Copy link

netlify bot commented Feb 5, 2025

Deploy Preview for karpenter-docs-prod canceled.

Name Link
🔨 Latest commit 275ec59
🔍 Latest deploy log https://app.netlify.com/sites/karpenter-docs-prod/deploys/67a9f2072c20c1000869e0e9

@jonathan-innis
Copy link
Contributor

@splichy I don't have a huge issue with the change, but I'm curious why you can't just disable the use of the serviceMonitor if you don't have Prometheus enabled and then just enable the serviceMonitor once it gets installed

@splichy
Copy link
Contributor Author

splichy commented Feb 7, 2025

@jonathan-innis Yes, I can set servicemonitor.enabled=false, but that just adds an unnecessary manual step into deployment, as Prometheus is deployed in the next wave.
So this api availability check allows me to deploy Karpenter even if Prometheus is not installed yet, it's quite common practice across various charts.

@jonathan-innis
Copy link
Contributor

So this api availability check allows me to deploy Karpenter even if Prometheus is not installed yet, it's quite common practice across various charts

But the question is that this CR won't then be installed once you enable Prometheus, right? You're going to have to some new deployment of Karpenter anyways, so why not just use that deployment to enable the serivceMonitor?

@splichy
Copy link
Contributor Author

splichy commented Feb 8, 2025

So this api availability check allows me to deploy Karpenter even if Prometheus is not installed yet, it's quite common practice across various charts

But the question is that this CR won't then be installed once you enable Prometheus, right? You're going to have to some new deployment of Karpenter anyways, so why not just use that deployment to enable the serivceMonitor?

We are using ArgoCD, so with auto-sync enabled it will install service monitor right after Prometheus, we are using sync waves where karpenter has higher priority than prometheus - it's a little bit chicken egg problem

@jonathan-innis
Copy link
Contributor

we are using sync waves where karpenter has higher priority than prometheus

Ah, I got you -- that's interesting -- makes sense to me -- didn't even think about the GitOps auto-remediation use-case!

Copy link
Contributor

@jonathan-innis jonathan-innis left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@jonathan-innis jonathan-innis enabled auto-merge (squash) February 8, 2025 21:44
@coveralls
Copy link

coveralls commented Feb 8, 2025

Pull Request Test Coverage Report for Build 13240939870

Warning: 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

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 64.766%

Totals Coverage Status
Change from base Build 13213523731: 0.0%
Covered Lines: 5803
Relevant Lines: 8960

💛 - Coveralls

@jonathan-innis jonathan-innis merged commit 344301e into aws:main Feb 10, 2025
29 checks passed
edibble21 pushed a commit to edibble21/karpenter-provider-aws that referenced this pull request Feb 19, 2025
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.

4 participants