[kube-prometheus-stack] Change AlertManager and Prometheus CR naming.

[ksa-real]

This is to avoid weird dependent object names. AlertManager instance is
named releasename-alertmanager, which results in most dependent
objects being named alertmanager-releasename-alertname. Same for
Prometheus CR. Generated PersistentVolume is particularly bad:
prometheus-releasename-prometheus-db-prometheus-releasename-prometheus-0.
If .Release.Name is main and we are not using .Values.nameoverride
then we get insane
prometheus-main-kube-prometheus-stack-prometheus-db-prometheus-main-kube-prometheus-stack-prometheus-0.

Signed-off-by: Sergei Kuzmin sergeikuzmin@gmail.com

What this PR does / why we need it:

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

• fixes #

Special notes for your reviewer:

Checklist

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

[ksa-real]

@andrewgkew @gianrubio @gkarthiks @anandsinghkunwar @monotek @mrueg Pinging some guys and some recently reviewed the PRs. Would someone take a look? It looks to me that for workflows to run an approval is needed.

[monotek]

Please sign your commits, to get the dco check work.

Have you tested what happens if you update from 30.2.0 and the names change?

[ksa-real]

@monotek I've now signed the follow-up commits. Thanks for the heads up.
I haven't done the right testing originally, now done. I have set up a Prometheus stack with this chart and have ensured all dashboards are working and Prometheus reports errors to Alertmanager correctly.
The issue with renaming is that PVC name has changed from (e.g. in my case) prometheus-kps-prometheus-db-prometheus-kps-prometheus-0 to prometheus-kps-db-prometheus-kps-0. So either data migration is needed (e.g. copying from old PV to a new PV) or data is lost.
While I think better naming is still desirable, probably we shall discuss a better path to do so, as people will be likely unhappy with that.

[monotek]

So I guess the naming would not justify a new major version with likely rather complicated data migration path.

[ksa-real]

How about merging this when there is something that justifies a major version change?
I propose the following. I'll add a cleanPrometheusOperatorObjectNames: false value to kube-prometheus-stack values. This will control how kube-prometheus-stack.alertmanager.fullname and kube-prometheus-stack.prometheus.fullname templates render: with or without the suffix. By default, the current behavior is preserved, i.e. longer names with duplicates.
Once there is a major change, that justifies the migration, we can switch the value and later deprecate it.
A more complex thing is K8s should likely have a way to deal with such situations. It is not the first time I have an issue with data migration tied to immutable names.
One more note: Prometheus operator itself is naming dependent objects inconsistently with what helm does: putting prefixes instead of suffixes: e.g. StatefulSet named alertmanager-{{Alertmanager CR name}} instead of {{Alertmanager CR name}}-alertmanager. Same for prometheus.

[ksa-real]

@monotek I went ahead and created name templates. Now the change is really minor until turned on by default.

[ksa-real]

@monotek Can you please take a look? Or who may be a good person to ping?

[monotek]

I would like to see some more reviews here.
I'm not sure if its realy needed.

[ksa-real]

Ping:
@andrewgkew @bismarck @desaintmartin

[ahgraber]

Just wanted to chime in supporting this change. Currently the pv/pvc created names are too long for my storageClass and therefore I cannot persist any data.

[ksa-real]

Ping:
@andrewgkew @bismarck @desaintmartin @gianrubio @gkarthiks @GMartinez-Sisti @scottrigby @Xtigyro

[agorgl]

Waiting for this!

[kimxogus]

Waiting for this too

[ksa-real]

I've rebased on the latest main

[ksa-real]

@monotek Would you reconsider? I see a few more people showed interest. Other code owners didn't express opinions.

[monotek]

The maintainers of the chart should decide: https://github.com/prometheus-community/helm-charts/blob/main/.github/CODEOWNERS#L10

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Any further update will cause the issue/pull request to no longer be considered stale. Thank you for your contributions.

[ksa-real]

Ping

[ksa-real]

@monotek Can you unblock?

[zanhsieh]

@ksa-real
I would suggest you close this and start another PR. The current kube-prometheus-stack already favor thanos rules that I can't help to merge / resolving conflicts.