feat!: add operator-metrics port

[fstr]

Description

Expose Pyrra Kubernetes operator container metrics on port 8080. With these metrics, we can also get kube-builder metrics like controller_runtime_reconcile_errors_total on which we can build an alert.

The alert can be optionally enabled. Due to the reconciliation loop interval in the operator, we use a 20 minute interval on the rate function, as it is long enough to avoid dropping to 0 and having a flapping alert while the operator is not reconciling (and reporting a reconciliation error). The alert with resolve slightly delayed because of this, but it is much better than having no alert.

This feature is especially useful, as the Pyrra WebUI currently breaks and shows nothing if a broken SLO has been applied to your Kubernetes cluster.

How can this be tested

Enable the PyrraReconciliationError alert via prometheusRule.enabled: true and deploy a broken/invalid SLO. Even if the ValidatingWebhook is active, this SLO will be accepted but won't reconcile, as the errors.metric is not a valid Vector Selector due to the or expr clause.

---
apiVersion: pyrra.dev/v1alpha1
kind: ServiceLevelObjective
metadata:
  name: broken-slo-test
  namespace: monitoring
spec:
  description: ""
  alerting:
    absent: false
  indicator:
    ratio:
      errors:
        # This (or clause) is not supported and will lead to an error
        metric: prometheus_notifications_errors_total{job="prometheus-k8s"} or up{job="prometheus-k8s"} == 0
      total:
        metric: prometheus_notifications_sent_total{job="prometheus-k8s"}
  target: "99"
  window: 4w

BREAKING CHANGE: This is a breaking change, as both containers expose the default Golang metrics. Users that built monitoring on the existing metrics now have to separate them by container label.

[rlex]

Might be worth bumping version in Chart.yml (major one?) and re-running helm-docs.

[fstr]

What are your thoughts on the "shared" ServiceMonitor? We now have a single ServiceMonitor for the pyrra-server and the pyrra-operator. Both containers run as part of a single pod. I think it's acceptable, but the jobLabel is pyrra-server which doesn't fully match anymore, as it also has the pyrra-operator metrics.

Changing the jobLabel to something generic like pyrra would be another backwards breaking change. Introducing a second ServiceMonitor just for the operator could have the jobLabel: pyrra-operator.

[rlex]

maybe we should just split configuration between operator and pyrra?

ie pyrra.serviceMonitor, pyrra.prometheusUrl, pyrraOperator.serviceMonitor

[sebastiangaiser]

maybe we should just split configuration between operator and pyrra?

ie pyrra.serviceMonitor, pyrra.prometheusUrl, pyrraOperator.serviceMonitor

I think this is a good idea. But when doing this we could also split them up into 2 deployments. On the other hand this might unnecessarily expand the original problem and should be done in another PR. What do you think?

[fstr]

I can split the ServiceMonitor as part of this PR. This will already allow separate configuration and jobNames. In a follow-up PR it can be changed to use two deployments.

[fstr]

I finally came back to this and split the ServiceMonitor into one for operator and one for the server. I moved the config properties under serviceMonitorOperator for now to keep the value file flat.

If you want to move forward with the idea to split the operator and server into separate deployments, which I think it the right way to do, then most properties of the value file can be duplicated moved under server: and operator: respectively.

[sebastiangaiser]

I would either use the operatorMetricsAddress or operatorMetricsPort as they have to align. If you want you can make this possible to overwrite, something like if operatorMetricsPort is "" then use {{ include "pyrra.operatorMetricsPort" . }}. Could possibly be done in the helpers.

[fstr]

Right now, the service port can be configured independently of the container port via .Values.service.operatorMetricsPort. The container port is taken from operatorMetricsAddress, so they are aligned.

The service port and the container port do not have to align.

[sebastiangaiser]

My idea was to extract operatorMetricsPort from operatorMetricsAddress but should be also fine to leave it for now like it is

[sebastiangaiser]

Thank you for pushing this @fstr . I added two small nits, can you please have a look.

[sebastiangaiser]

LGTM

[sebastiangaiser]

@rlex what do you think?

[rlex]

So far so good i think, but it's probably worth bumping it to 1.0.0 since it's pretty major change.
Also, now CI is failing because CRDs aren't present during install.

[fstr]

As we've discussed also splitting the Deployments and subsequently the Services, should we wait with the bump to 1.0.0 until that is done?

Then we have separated them fully.

Operator: ServiceMonitor -> Service -> Deployment
Server: ServiceMonitor -> Service -> Deployment

[sebastiangaiser]

@fstr can you please fix the docs as stated in the CI.
For me bumping a minor should be fine.

[sebastiangaiser]

@rlex do you have anything to add?

[sebastiangaiser]

@rlex can we get this merged?

[rlex]

Sorry for delay! Will merge as soon as CI passes.