Changed -querier.query-ingesters-within and -blocks-storage.tsdb.close-idle-tsdb-timeout defaults from 0 to 13h

[pracucci]

What this PR does:
In this PR I'm proposing to change the defaults of -querier.query-ingesters-within and -blocks-storage.tsdb.close-idle-tsdb-timeout both to 13h. I think this is a relatively safe change to do.

I've checked the docs/ and I think all mentions to -querier.query-ingesters-within and -blocks-storage.tsdb.close-idle-tsdb-timeout still make sense.

Out of scope of this PR:

• I've intentionally not addressed -querier.query-store-after and -blocks-storage.bucket-store.ignore-blocks-within which I consider more sensitive and I would prefer to discuss separately.

Which issue(s) this PR fixes:
Fixes https://github.com/grafana/mimir-squad/issues/497

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[krajorama]

13h comes from ingester keeping data for 12h IIRC, so I'd mention that to show the dependency, or actually use some common constant, otherwise it's hard to understand why we chose exactly 13.

[pracucci]

13h comes from ingester keeping data for 12h IIRC, so I'd mention that to show the dependency, or actually use some common constant, otherwise it's hard to understand why we chose exactly 13.

We're used to keep blocks for a longer period on ingesters (in our jsonnet config), but your comment made me realize that the default is 6h, so we need to increase that too.

[krajorama]

13h comes from ingester keeping data for 12h IIRC, so I'd mention that to show the dependency, or actually use some common constant, otherwise it's hard to understand why we chose exactly 13.

We're used to keep blocks for a longer period on ingesters (in our jsonnet config), but your comment made me realize that the default is 6h, so we need to increase that too.

So you're saying that 13h compares to the 24h retention time? Then that would mean that blocks-storage.tsdb.retention-period needs to be larger then the querier.query-ingesters-within ?

[replay]

If we want it to be idle immediately, why not set it to 0?
I think the answer to that question ^^ should probably be in the comment

[pracucci]

0 means disabled, so that's why I've set it to 1 nanosecond. I will update the comment.

[pstibrany]

Looks good.

[pstibrany]

Isn't this too close to "close on idle" time (also 13h)? Can we run into a race when ingesters decide to close TSDB at the same time when queriers query the block?

[pracucci]

Isn't this too close to "close on idle" time (also 13h)?

That's how we set our jsonnet.

[pstibrany]

But we also set -querier.query-store-after to 12h.

[pracucci]

So you're saying that 13h compares to the 24h retention time? Then that would mean that blocks-storage.tsdb.retention-period needs to be larger then the querier.query-ingesters-within ?

-blocks-storage.tsdb.retention-period is not strictly required to be >= -querier.query-ingesters-within even if doesn't make much sense keep querying ingesters for data that know for sure won't be there. However, it's important that -blocks-storage.tsdb.retention-period and -querier.query-ingesters-within are either disabled or >= -querier.query-store-after, which is what we have in the config description:

  # TSDB blocks retention in the ingester before a block is removed. This should
  # be larger than the -blocks-storage.tsdb.block-ranges-period,
  # -querier.query-store-after and large enough to give store-gateways and
  # queriers enough time to discover newly uploaded blocks.
  # CLI flag: -blocks-storage.tsdb.retention-period
  [retention_period: <duration> | default = 24h]

[pracucci]

13h comes from ingester keeping data for 12h IIRC, so I'd mention that to show the dependency, or actually use some common constant, otherwise it's hard to understand why we chose exactly 13.

We're used to keep blocks for a longer period on ingesters (in our jsonnet config), but your comment made me realize that the default is 6h, so we need to increase that too.

So you're saying that 13h compares to the 24h retention time? Then that would mean that blocks-storage.tsdb.retention-period needs to be larger then the querier.query-ingesters-within ?

Discussed offline with Krajo. He suggests to improve the documentation to mention that our defaults are based on usage patterns seen in real life. I will create an issue about it because I think the discussion involves whether we should change -querier.query-store-after too.