[tempo-distributed] refactor and introduce Enterprise Traces support

[zalegrala]

With this change, I aim to align the helpers used in the tempo-distributed chart with those of the other database teams, namely the mimir-distributed chart for improved consistency and long term maintainability. This is also in prep to implement enterprise-traces into the tempo-distributed chart.

As part of this alignment, the following structural changes have been implemented.

• import mimir helpers and begin alignment
• centralize image reference handling
• centralize name reference handling
• BREAKING CHANGE centralize selector label handling -- users who wish to keep old values should still be able to use the nameOverride and fullNameOverride top level keys in their values.
• centralize pod label handling
• centralize service monitor around metamonitoring
• BREAKING CHANGE serviceMonitor has been nested under metaMonitoring -- metamonitoring can be used scrape services as well as install the operator with the following values. Note also that the port names have changed from http to http-metrics.

metaMonitoring:
  serviceMonitor:
    enabled: true
  grafanaAgent:
    enabled: true
    installOperator: true

• minio can now be enabled as part of this chart using the following values

minio:
  enabled: true

As part of the above changes, many helpers now expect that a context is passed. This pattern is meant to be used in the following way.

First declare the dictionay for the given component, and optionally set memberlist to true based on the component needs.

{{ $dict := dict "ctx" . "component" "compactor" "memberlist" true }}

Then you can pass the $dict when rendering a named template.

name: {{ template "tempo.resourceName" $dict }}

This allows us to use one template and centralize the logic passing in the
variables that change.

Additionally, the configuration can now be stored in a secret. See the documentation for useExternalConfig and configStorageType in the values file for more details.

[krajorama]

The original services in the same file use a string here and template it with tpl, this is a bit inconsistent now

[zalegrala]

I've made an additional breaking change to align. How's that look?

[zalegrala]

After some discussion in slack, I expect to roll this back the other direction to template strings for the affinity.

[zalegrala]

This has been rolled back. Now we use a template for all affinity.

[krajorama]

Optional: this is subtle stuff, but in mimir-distributed I reordered the yaml in config template alphabetically because the processing we do will output the result in an ordered way.

[zalegrala]

Ah very interesting.

[krajorama]

This is a bit weird. Don't you reload overrides.yaml automatically in tempo? If it's automatically reloaded from a file, then you shouldn't have it next to the static config, because any change to the overrides will cause the config checksum to change and restart the services for no reason.

[zalegrala]

I don't believe we reload on config change, but I could be wrong.

Aside from Tempo, are you saying a configmap updates the running pod without restart? I'd no idea if that's true. Interesting. I'd been thinking that the annotations were in place specifically to restart the pod on change, but now I wonder.

[zalegrala]

Marty confirmed that we will reload the overrides config. I'll come back to tidy that up. Thanks for the pointer.

[zalegrala]

Perhaps we could follow up in another PR though, what do you think @krajorama ?

[krajorama]

Sounds ok to me as long as it's not forgotten

[krajorama]

shouldn't this be

Suggested change

	{{- if .Values.gateway.enabled }}
	{{- if and .Values.gateway.enabled (not .Values.enterprise.enabled) }}

Because now it seems like you run both gateways when enterprise is enabled

[zalegrala]

I don't suppose a user would want to run both, but since both gateways default to false, I thought just switching in the specific component for each one reduced the logic. Users who want to run only one gateway will need to enable only one gateway in the config.

Alternatively, this could be the kind of thing we put in a validation template, but is not implemented in this pr.

[trevorwhitney]

This looks great! Just a few small nits, and I'll leave the ✔️ for @krajorama as I see he requested some changes.

[krajorama]

FYI We've in Mimir recently made the breaking change to only support kube >= 1.20. Since we are not actually testing on older versions - so we could get rid of this check altogether.

[zalegrala]

This sounds good to me. Perhaps a follow up breaking change to upgrade the required version for the chart as well as drop the api version comparisons.

[krajorama]

Recently we got a feature request from a customer to make migrating from Mimir OSS to Enterprise Metrics easier, so now we're working on merging our "nginx" and "gateway" configuration under the "gateway" section.

So in case you want to add "nginx" for OSS users later, the "enterpriseGateway" naming will bite you. I'd consider calling it just "gateway".

See for ref: grafana/mimir#3181

[zalegrala]

Hmm, I think we'll need to follow up here. We've currently got a gateway: section for the nginx gateway configuration. The linked PR here is quite a large change. Perhaps we can follow up.

[krajorama]

In Mimir we ended up making this include validation, a map and returning the full section instead of the name. https://github.com/grafana/mimir/blob/8962290b64ebcc4c9ba91a8ff1455f7a600c3a29/operations/helm/charts/mimir-distributed/templates/_helpers.tpl#L378

[cajual]

3534217#r88097126

[joshuasimon-taulia]

should minio really be enabled by default?

[zalegrala]

Fair question. After spending a bit more time with it, I think no. Would you like to put up a PR for it @joshuasimon-taulia ?