[kube-prometheus-stack] allow override of for and severity rules

[ps-xaf]

What this PR does / why we need it

Attemp to address the issue with fixed values "for" and "severity" in the helm chart, to make it more flexible to use and allow to overload those values without forking and customzing the entire chart.

Which issue this PR fixes

• fixes [kube-prometheus-stack] Makes default prometheus rules more flexible #3384

Special notes for your reviewer

$ docker run -it --rm --network host --workdir=/data --volume ~/.kube/config:/root/.kube/config:ro --volume $(pwd):/data --volume ~/.config/helm:/root/.config/helm quay.io/helmpack/chart-testing:v3.10.1 ct lint --chart-dirs . --charts . --validate-maintainers=false                                                                                                                                   Linting charts...
Version increment checking disabled.

------------------------------------------------------------------------------------------------------------------------
 Charts to be processed:
------------------------------------------------------------------------------------------------------------------------
 kube-prometheus-stack => (version: "56.6.3", path: ".")
------------------------------------------------------------------------------------------------------------------------

Hang tight while we grab the latest from your chart repositories...
... 
</>snip</>
...
Update Complete. ⎈Happy Helming!⎈
Saving 5 charts
Dependency crds did not declare a repository. Assuming it exists in the charts directory
Downloading kube-state-metrics from repo https://prometheus-community.github.io/helm-charts
Downloading prometheus-node-exporter from repo https://prometheus-community.github.io/helm-charts
Downloading grafana from repo https://grafana.github.io/helm-charts
Downloading prometheus-windows-exporter from repo https://prometheus-community.github.io/helm-charts
Deleting outdated charts
Linting chart "kube-prometheus-stack => (version: \"56.6.3\", path: \".\")"
Validating /data/Chart.yaml...
Validation success! 👍

Linting chart with values file "ci/01-provision-crds-values.yaml"...

==> Linting .

1 chart(s) linted, 0 chart(s) failed

Linting chart with values file "ci/02-test-without-crds-values.yaml"...

==> Linting .

1 chart(s) linted, 0 chart(s) failed

Linting chart with values file "ci/03-non-defaults-values.yaml"...

==> Linting .

1 chart(s) linted, 0 chart(s) failed

Linting chart with values file "ci/04-prometheus-operator-webhook-values.yaml"...

==> Linting .

1 chart(s) linted, 0 chart(s) failed

Linting chart with values file "ci/05-alertmanager-ingress-host-with-wildcard-values.yaml"...

==> Linting .

1 chart(s) linted, 0 chart(s) failed

------------------------------------------------------------------------------------------------------------------------
 ✔︎ kube-prometheus-stack => (version: "56.6.3", path: ".")
------------------------------------------------------------------------------------------------------------------------
All charts linted successfully

Checklist

• DCO signed
• Chart Version bumped
• Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

[jkroepke]

Hi,

thanks for you contribution. I personally like the idea and I could also benefit from it.

However all files insides rules-1.14 directory are auto-generated by an python script. In case I would merge this, the changes are getting reverted by next person who execution the script.

Please adjust the script https://github.com/prometheus-community/helm-charts/blob/main/charts/kube-prometheus-stack/hack/sync_prometheus_rules.py to include your changes.

[ps-xaf]

However all files insides rules-1.14 directory are auto-generated by an python script. In case I would merge this, the changes are getting reverted by next person who execution the script.

Please adjust the script https://github.com/prometheus-community/helm-charts/blob/main/charts/kube-prometheus-stack/hack/sync_prometheus_rules.py to include your changes.

Thanks for the swift review and reply and pointing me into the right direction. I'll have a look the coming days how I can add it to the sync script.

[jkroepke]

Please solve the merge conflicts, otherwise, CI would not run

[jkroepke]

@GMartinez-Sisti Since this is a larger PR and related to Python, I also would like to hear your opinion. In general, I like the idea, but it also add a lot of complexity.

[GMartinez-Sisti]

Overall LGTM, I think being able to customise these values is a great feature 💪 . Please check the comments.

[GMartinez-Sisti]

This is more a feature than a fix, so we should bump minor:

Suggested change

	version: 56.7.1
	version: 56.8.0

[ps-xaf]

bumped up to 56.9.0

[GMartinez-Sisti]

Is this change intentional? Looks like it's reverting changes applied on #4151 (same for similar changes below)

The result looks like this:

<         description: The PersistentVolume claimed by {{ $labels.persistentvolumeclaim }} in Namespace {{ $labels.namespace }} on Cluster {{ $labels.cluster }} is only {{ $value | humanizePercentage }} free.
---
>         description: The PersistentVolume claimed by {{ $labels.persistentvolumeclaim }} in Namespace {{ $labels.namespace }} {{ with $labels.cluster -}} on Cluster {{ . }} {{- end }} is only {{ $value | humanizePercentage }} free.

[ps-xaf]

Thanks @GMartinez-Sisti, this was unintentional. I'll need to have a look as I get those changes also on main, when running sync_prometheus_rules.py.

A further unintended change is in prometheus.yaml

[ps-xaf]

I've removed those changes. They are from upstream due to PR 2339 as the sync script fetches from main

See also upstreamd diff 1 2 3 4 5

[ps-xaf]

Not intendet.

[ps-xaf]

Same, it's from upstream pr 2344 and rule diff

[jkroepke]

Is this an manual revert? bf74da9 (#4225)

[ps-xaf]

Is this an manual revert? bf74da9 (#4225)

yep, this was manually done.

[jkroepke]

Is this an manual revert? bf74da9 (#4225)

yep, this was manually done.

Huch, the CI should normally fail here.

helm-charts/charts/kube-prometheus-stack/ci/lint.sh

Line 25 in 412f4d3

if ! git diff --exit-code; then

[ps-xaf]

Is this an manual revert? bf74da9 (#4225)

yep, this was manually done.

Huch, the CI should normally fail here.

helm-charts/charts/kube-prometheus-stack/ci/lint.sh

Line 25 in 412f4d3

if ! git diff --exit-code; then

I would need to take a closer look as I am not too familiar with the github ci (more a gitlab person 😉 ).
From what I get, it runs sync_prometheus_rules.py and then checks if changes exists in the diffs, which shouldn't be there.
e.g.

$ if git diff -U1 --exit-code ; then echo "Nodiff: $?" ; else echo "diff: $?";  fi
Nodiff: 0

$ echo "manual edit into a rules file" >> templates/alertmanager/alertmanager.yaml

$ if git diff -U1 --exit-code ; then echo "Nodiff: $?" ; else echo "diff: $?";  fi
diff --git a/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml b/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml
index 52b49fc7..8102bc0f 100644
--- a/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml
+++ b/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml
@@ -188 +188,2 @@ spec:
 {{- end }}
+manual edit into a rules file
diff: 1

Negating the if condition results in true and therefore the check doesn't catch it. IMHO the if condition shouldn't be negated.

$ if ! git diff -U1 --exit-code ; then echo "Nodiff: $?" ; else echo "diff: $?";  fi
diff --git a/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml b/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml
index 52b49fc7..8102bc0f 100644
--- a/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml
+++ b/charts/kube-prometheus-stack/templates/alertmanager/alertmanager.yaml
@@ -188 +188,2 @@ spec:
 {{- end }}
+manual edit into a rules file
Nodiff: 0

Unrelated to the check, an actual run of the script sync_prometheus_rules.py results in more changes than in this pr. Because it fetches upstream files e.g.manifests/prometheus-prometheusRule.yaml from prometheus-operator/kube-prometheus, which results in the changes related to description and expressions.

Please let me know in case I miss something, related to the workflow and this pr

[jkroepke]

Unrelated to the check, an actual run of the script sync_prometheus_rules.py results in more changes than in this pr. Because it fetches upstream files e.g.manifests/prometheus-prometheusRule.yaml from prometheus-operator/kube-prometheus, which results in the changes related to description and expressions.

The easiest thing here would be a separate PR which include only changes from upstream. Then, rebase the PR here.

We can also leave it as-is. I'm fine with both here.

Looked through the generated diff here, and it looks good to me. I have no real opinion about the new python code here. Its an script which lives in a folder called hack.

If @GMartinez-Sisti agree here, too. I guess we can merge this.

[ps-xaf]

Unrelated to the check, an actual run of the script sync_prometheus_rules.py results in more changes than in this pr. Because it fetches upstream files e.g.manifests/prometheus-prometheusRule.yaml from prometheus-operator/kube-prometheus, which results in the changes related to description and expressions.

The easiest thing here would be a separate PR which include only changes from upstream. Then, rebase the PR here.

This would be cleaner but I leave it with you.

Regards my comment related to the lint step. Forget about it, the check looks fine. If I have some time I test it in my repo.

[GMartinez-Sisti]

Unrelated to the check, an actual run of the script sync_prometheus_rules.py results in more changes than in this pr. Because it fetches upstream files e.g.manifests/prometheus-prometheusRule.yaml from prometheus-operator/kube-prometheus, which results in the changes related to description and expressions.

The easiest thing here would be a separate PR which include only changes from upstream. Then, rebase the PR here.

This would be cleaner but I leave it with you.

Regards my comment related to the lint step. Forget about it, the check looks fine. If I have some time I test it in my repo.

Fair enough. It’s now documented here too, so I’m good with it. No need to do a PR just for the sake of it.

[ps-xaf]

fixed the merged conflict and added the upstream changes.

[jkroepke]

Thanks for the PR! I guess its an huge improvement for a lot of users.

[degisftw]

This is amazing! And just in time for when I needed to do just this :) Thanks @ps-xaf !