Add Pyrra as (optional) component

[metalmatze]

Description

As per discussion in one of our last Office Hours here is a Pull Request to add Pyrra as component to kube-prometheus.
I've added a new component that imports the CustomResourceDefinition via jsonnet-bundler and commented out the usage of Pyrra in the example.jsonnet so only users who explicitly want Pyrra can enable it.

Type of change

What type of changes does your code introduce to the kube-prometheus? Put an x in the box that apply.

• CHANGE (fix or feature that would cause existing functionality to not work as expected)
• FEATURE (non-breaking change which adds functionality)
• BUGFIX (non-breaking change which fixes an issue)
• ENHANCEMENT (non-breaking change which improves existing functionality)
• NONE (if none of the other choices apply. Example, tooling, build system, CI, docs, etc.)

Changelog entry

Add Pyrra (for SLO management) as component that can manually be enabled

[metalmatze]

Seems like the nested approach I took might not work. In that case we'll probably have to use a api and kubernetes prefix for the objects...

[ArthurSens]

Awesome Matthias, thanks a lot for opening this PR!

Yeah, since you've added Pyrra to main.libsonnet, the manifest generation, as configured here and here, is not expecting the way you've structured pyrra.libsonnet.

By directly putting Pyrra under main.libsonnet, I believe you haven't made the deployment of Pyrra optional to our CI... Maybe you could create a file under jsonnet/kube-prometheus/addons like this:

local pyrra = import '../components/pyrra.libsonnet';

{
  values+:: {
    pyrra: {
      namespace: $.values.common.namespace,
      version: $.values.common.versions.pyrra,
      image: $.values.common.images.pyrra,
    },
  },

  pyrra: pyrra($.values.pyrra),
}

I did that and tried running ./build.sh example.jsonnet with my example.jsonnet looking like this and it seems to work 🙂:

local kp =
  (import 'kube-prometheus/main.libsonnet') +
  (import 'kube-prometheus/addons/pyrra-support.libsonnet') +
  {
    values+:: {
      common+: {
        namespace: 'monitoring',
      },
    },
  };

{ 'setup/0namespace-namespace': kp.kubePrometheus.namespace } +
{
  ['setup/prometheus-operator-' + name]: kp.prometheusOperator[name]
  for name in std.filter((function(name) name != 'serviceMonitor' && name != 'prometheusRule'), std.objectFields(kp.prometheusOperator))
} +
// serviceMonitor and prometheusRule are separated so that they can be created after the CRDs are ready
{ 'prometheus-operator-serviceMonitor': kp.prometheusOperator.serviceMonitor } +
{ 'prometheus-operator-prometheusRule': kp.prometheusOperator.prometheusRule } +
{ 'kube-prometheus-prometheusRule': kp.kubePrometheus.prometheusRule } +
{ ['alertmanager-' + name]: kp.alertmanager[name] for name in std.objectFields(kp.alertmanager) } +
{ ['blackbox-exporter-' + name]: kp.blackboxExporter[name] for name in std.objectFields(kp.blackboxExporter) } +
{ ['grafana-' + name]: kp.grafana[name] for name in std.objectFields(kp.grafana) } +
{ ['kube-state-metrics-' + name]: kp.kubeStateMetrics[name] for name in std.objectFields(kp.kubeStateMetrics) } +
{ ['kubernetes-' + name]: kp.kubernetesControlPlane[name] for name in std.objectFields(kp.kubernetesControlPlane) }
{ ['node-exporter-' + name]: kp.nodeExporter[name] for name in std.objectFields(kp.nodeExporter) } +
{ ['prometheus-' + name]: kp.prometheus[name] for name in std.objectFields(kp.prometheus) } +
{ ['prometheus-adapter-' + name]: kp.prometheusAdapter[name] for name in std.objectFields(kp.prometheusAdapter) }
{ 'setup/pyrra-CustomResourceDefinition': kp.pyrra.crd } +
{ ['pyrra-api-' + name]: kp.pyrra.api[name] for name in std.objectFields(kp.pyrra.api) } +
{ ['pyrra-kubernetes-' + name]: kp.pyrra.kubernetes[name] for name in std.objectFields(kp.pyrra.kubernetes) }
{ ['slos-' + name]: kp.pyrra.SLOs[name] for name in std.objectFields(kp.pyrra.SLOs) }

[ArthurSens]

Not using this nested object approach would make the code more aligned with what we have on other components... not sure how relevant that is 🤔

[metalmatze]

PTAL @ArthurSens 😊

[ArthurSens]

Nice, just tested locally and got it running in no time 🙂

I'm fine with merging as is since you've added it as an addon, just some questions about what needs to be done to improve the experience of using kube-prometheus with Pyrra.

I've noticed that we now have SLO-based alerts for kubernetes coming from both kubernetes-mixin and also from Pyrra, I believe we could extend the addon to drop the alerts coming from kubernetes-mixin, is that correct? We could create an issue for this and improve in another PR of course....

Did you find any other issues when running kube-prometheus and Pyrra at the same cluster/environment?

---

@paulfantom @philipgough, any other comments are also welcome :)

[metalmatze]

Yeah, you're right. They somewhat conflict. I'm happy to follow up with another PR that filters out the kubernetes-mixin alert. At the same time, it might not even be too bad to have both alerts for a while and see how they compare in user experience for users?
Other than that there are a few similar problems where we have alerts that fire, if above a certain threshold like for the prometheus-operator reconcile errors. Don't think that's a big problem either.

Personally, I would have said let's keep both and have people try out Pyrra and the given SLOs. Hopefully we can figure out if it works good enough for people and what to adjust for the SLOs. Eventually, and that's my dream, we would throw out all "old" alerts and continue with just the SLOs. Obviously that's a bigger discussion to have.

[ArthurSens]

Yeah, you're right. They somewhat conflict. I'm happy to follow up with another PR that filters out the kubernetes-mixin alert. At the same time, it might not even be too bad to have both alerts for a while and see how they compare in user experience for users?

I'm just concerned at people using different targets between the mixin and pyrra and getting confused by getting alerts for one but not the other

[ArthurSens]

Letting this snippet in comments was intended? 🤔

[metalmatze]

Good catch. It conflicted with the name. I think it was a copy-paste mistake. I need to fix it. 👍

[metalmatze]

I'm just concerned at people using different targets between the mixin and pyrra and getting confused by getting alerts for one but not the other

Yes. Totally get that. Let's get this in and then I'll work on filtering out some alerts if the Pyrra addon is enabled.

[metalmatze]

Updated the PR to not include the commented SLO that turned out to simply be a copy-paste-mistake

[paulfantom]

LGTM