ServiceMonitor: Specify monitored namespace

[SchoolGuy]

Pull Request

Description of the change

This Pull Request should utilize the Helm Release Namespace as a target for the ServiceMonitor this Helmchart is optionally deploying.

Benefits

The metrics actually show up in Prometheus.

Possible drawbacks

I don't see any.

Applicable issues

• fixes Feature Request: Add ServiceMonitor namespace selector #312

Additional information

None

Checklist

• DCO has been signed off on the commit.
• Chart version bumped in Chart.yaml according to semver.
• (optional) Variables are documented in the README.md

[SchoolGuy]

This PR should be ready for review. I never worked with Helm templates. The desired value that I want to put in there is the namespace Nextcloud is deployed into, I hope I used the right expression for that.

[jessebot]

@SchoolGuy thanks for your contribution! :) It looks ok to me, but I haven't had a chance to test it. Have you tested this helm template and installed it?

[SchoolGuy]

@jessebot Sadly not yet. If this is a desired test, I can try to do so tomorrow.

[jessebot]

@SchoolGuy, yes please 🙏 We still need to add more tests to the continuous integration for this helm chart, so some of the tests are a bit manual. I will put in an issue to add some contributing docs to cover the gaps. In the meantime, thanks so much for your patience and help on this 💙

[SchoolGuy]

@jessebot If you want I can also add a PR with k3s in GitHub Actions. Or at least I can try... Then we could execute and run the HelmChart here in the repo...

Edit: I just saw that the CI already includes a step where the chart is installed. What is missing then to validate the functionality of the chart? More test cases?

[SchoolGuy]

So you of course need to trust me but I did the following locally:

values-override.yml:

metrics:
  enabled: true
  https: true
  resources:
    requests:
      cpu: 100m
      memory: 256Mi
    limits:
      memory: 512Mi
  service:
    type: ClusterIP
    annotations:
      prometheus.io/scrape: "true"
      prometheus.io/port: "9205"
    labels: {}
  serviceMonitor:
    enabled: true
    namespace: "monitoring"
    jobLabel: "nextcloud"
    labels:
      release: kube-prom-stack

Output:

┬─[enno@tower:~/S/O/n/c/nextcloud]─[11:31:55]─[G:feature/service-monitor-namespace-selector=]
╰─>$ helm template . -n nextcloud-4seul -f values-override.yaml
---
...
---
# Source: nextcloud/templates/metrics-servicemonitor.yaml
apiVersion: monitoring.coreos.com/v1
kind: ServiceMonitor
metadata:
  name: release-name-nextcloud
  namespace: "monitoring"
  labels:
    app.kubernetes.io/name: nextcloud
    helm.sh/chart: nextcloud-3.5.17
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/component: metrics
    release: kube-prom-stack
spec:
  jobLabel: "nextcloud"
  selector:
    matchLabels:
      app.kubernetes.io/name: nextcloud
      app.kubernetes.io/instance: release-name
      app.kubernetes.io/component: metrics
  namespaceSelector:
    matchNames:
      - "nextcloud-4seul"
  endpoints:
    - port: metrics
      path: "/"
      interval: 30s

[jessebot]

The current GHA CI will do a lint and install, but it doesn't check to make sure the app is actually healthy and reachable, to my knowledge. We did want to add k3s in addition to kind though! (testing more k8s distros is always helpful :) ) Mastodon has an example of that here if you'd like to submit a PR.

[jessebot]

Looks good to me. :)

It might makes sense to make this configurable down the line if you'd like to either modify this PR or submit a second one to add a parameter to values.yaml and update the README to reflect it, but if we do that, we need a conditional in the helm template to default to the release namespace if the value is not provided.

[SchoolGuy]

@jessebot What would then the content of the setting be? If we have multiple deployments of this HelmChart in the same cluster we want to either:

• Skip deployment of the ServiceMonitor since one is responsible for all instances.
• Ship one ServiceMonitor per installation (I guess the desired one) in which case I can't find the case where we would want to change this.

But my proposal would be that you open an issue for this and we discuss the details in there. :)

[jessebot]

Sounds good, we can always wait till someone else needs this feature :) Thanks for your help!