[bitnami/thanos] inconsistent caching configuration in values.yaml

[mhulscher]

Which chart:

bitnami/thanos

Is your feature request related to a problem? Please describe.

Recently the query-frontend component has been added to the thanos helm-chart. The query-frontend supports caching query-range results in memcache or in memory. This configuration can be passed in the values.yaml file at queryFrontend.config.

However, other components that consume configmaps for cache configuration are configured in a different way. Whereas the query caching configuration lives under the queryFrontend key, that's not true for the following settings:

key	desc
objstoreConfig	rightfully at top-level because it is shared by multiple components (at least storegateway and compactor)
indexCacheConfig	lives at the top-level but is specific to the storegateway
bucketCacheConfig	lives at the top-level but is specific to the storegateway

Additionally, the next release of thanos seems to support a 2nd caching configuration for the query-frontend component; caching of labels: thanos-io/thanos#3315 . This would make the current key at queryFrontend.config confusing. Am I configuring the query-range result cache or the labels cache?

Describe the solution you'd like

I am curious to know if there has already been discussions regarding this topic.

I can see at least two options to make the caching configurations more consistent.

1. Move all caching configuration to the top-level of the values.yaml and use specific names, for example:

indexCacheConfig:
  foo: bar

queryRangeResultCacheConfig:
  baz: qux

2. Move all caching configuration under their respective component and use specific key names, for example

storegateway:
  indexCacheConfig:
    foo: bar

  bucketCacheConfig:
    baz: qux

queryFrontend:
  queryRangeResultCacheConfig:
    foo: bar

  LabelsResponseCacheConfig:
    baz: qux

Regardless of what would happen, this will likely result in a backwards incompatible change in the format of the values.yaml file.

If there's a preferred direction then I am willing to help author a PR.

Describe alternatives you've considered

N/A

Additional context

N/A

[miguelaeh]

Hi @mhulscher ,
It seems we already support that, see

charts/bitnami/thanos/values.yaml

Line 40 in c1b6bf7

## Objstore Configuration

[mhulscher]

@miguelaeh I am not sure what you are trying to say? The issue that I am raising is not that caching is not supported, I know that it is supported :) The issue is that the several cache configurations are not configured in a consistent manner in the values.yaml.

[miguelaeh]

I am sorry, I mean, each component has its own section to be configured. That section will be a yaml file. Isn't it possible to configure the cache options into that files?

[mhulscher]

@miguelaeh
I am sorry, but I think that you may need to read the issue again. Some of the cache configuration is located at the top-level of the values.yaml, even though they are not shared between any components. These are only for the storegateway.

indexCacheConfig
bucketCacheConfig

But some other cache configuration is located under the specific component:

queryFrontend:
  config:

This cache configuration for queryFrontend.config only specifically configures the caching of query-range calls. In the next release of thanos there is also going to be a labels-response cache configuration.

The issue is that right now, the way in which the caches are configured is different for different components, which doesn't make sense.

[miguelaeh]

Hi @mhulscher ,
I understand, probably we have been adding the options due to request or PR without check that. Thank you for reporting it.
Would you like to create a PR to standardize that between components?

[mhulscher]

I can create a PR, however, like I mentioned in the issue:

Regardless of what would happen, this will likely result in a backwards incompatible change in the format of the values.yaml file.

How acceptable is it that this results in a major, backwards incompatible, release? Could we potentially combine it with #4307 and #4417? Keeping the chart backwards compatible is going to be a real pain and is going to leave a lot of duplicate template code.

[miguelaeh]

Hi @mhulscher ,
A new major version was just released updating to Helm v3. I think, in this case, it would be nice to support both and mark the old ones as deprecated, so the new ones take preference over those. And we can remove them in the next major, so we do not create more major versions right now.

[stale]

This Issue has been automatically marked as "stale" because it has not had recent activity (for 15 days). It will be closed if no further activity occurs. Thanks for the feedback.

[stale]

Due to the lack of activity in the last 5 days since it was marked as "stale", we proceed to close this Issue. Do not hesitate to reopen it later if necessary.