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

Fix namespace selector #349

Closed
wants to merge 9 commits into from

Conversation

Richardds
Copy link
Contributor

@Richardds Richardds commented Feb 14, 2023

Pull Request

Description of the change

The current configuration does not work when the monitored service runs outside of the serviceMonitor's namespace.

Other helm charts, such as bitnami/redis use namespaceSelector to fix this [1].

Benefits

The serviceMonitor is now able to find the monitored service.

Possible drawbacks

Applicable issues

Additional information

Example of metrics section from values.yaml:

...
metrics:
  enabled: true
  token: <token>
  serviceMonitor:
    enabled: true
    namespace: monitoring
    labels:
      release: prometheus

Checklist

Signed-off-by: Richard Boldiš <[email protected]>
Signed-off-by: Richard Boldiš <[email protected]>
@Richardds Richardds force-pushed the fix_namespace_selector branch from a4af4d1 to da69b98 Compare February 14, 2023 18:08
@jessebot
Copy link
Collaborator

Could you please bump the helm chart version? Another PR was merged before yours that bumped the version to 3.5.0, so if you could bump to 3.5.1, this would resolve the conflict. Thank you and thanks for this contribution! :)

@Richardds
Copy link
Contributor Author

Hello @jessebot , I bumped the version to 3.5.1.

@jessebot jessebot self-requested a review February 20, 2023 21:09
Copy link
Collaborator

@jessebot jessebot left a comment

Choose a reason for hiding this comment

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

Looks good to me

@Richardds Richardds force-pushed the fix_namespace_selector branch from 84f3ce9 to 133923d Compare March 19, 2023 12:28
- {{ .Values.metrics.serviceMonitor.namespace | quote }}
{{- else }}
- {{ .Release.Namespace | quote }}
{{- end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

with this whole block, it might need to be moved left two spaces to line up with matchNames.

    matchNames:
    {{- if .Values.metrics.serviceMonitor.namespace }}
    - {{ .Values.metrics.serviceMonitor.namespace | quote }}
    {{- else }}
    - {{ .Release.Namespace | quote }}
    {{- end }}

This would match the way we're doing the gotemplates everywhere else like:

    - port: metrics
      path: "/"
      {{- if .Values.metrics.serviceMonitor.interval }}
      interval: {{ .Values.metrics.serviceMonitor.interval }}
      {{- end }}
      {{- if .Values.metrics.serviceMonitor.scrapeTimeout }}
      scrapeTimeout: {{ .Values.metrics.serviceMonitor.scrapeTimeout }}
      {{- end }}

It's been a long week, @provokateurin can you just confirm I'm making sense here? I don't have time to test the helm chart directly for this specific issue right this second, but I will be tackling metrics probably in another month or two for a work project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @jessebot,
I offset the list by two fewer spaces. At the moment, it seems that it is not uniform across the project. I created another MR to tweak this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I missed this ages ago and now your other PR makes sense :) this was also before I realized you have a bit more freedom when using gotemplating :)

Copy link
Collaborator

@wrenix wrenix left a comment

Choose a reason for hiding this comment

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

I miss a documentation in the values.yaml

@@ -19,6 +19,13 @@ metadata:
{{- end }}
spec:
jobLabel: {{ .Values.metrics.serviceMonitor.jobLabel | quote }}
namespaceSelector:
matchNames:
{{- if .Values.metrics.serviceMonitor.namespace }}
Copy link
Collaborator

@wrenix wrenix Nov 21, 2023

Choose a reason for hiding this comment

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

      {{- with .Values.metrics.serviceMonitor.namespaces }}
      {{- toYaml . | nindent 6 }}

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. with is easy reable
  2. so multiple namespaces are configurable

Copy link
Collaborator

Choose a reason for hiding this comment

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

agree

@jessebot
Copy link
Collaborator

please see wrenix's comment, bump the chart version, and sign off your commits and then this can be merged.

@wrenix
Copy link
Collaborator

wrenix commented Feb 5, 2024

i believe we could close this PR now ;)
#524 was merged

@provokateurin
Copy link
Member

You're right

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