-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Store: *breaking ⚠️* set debug.advertise-compatibility-label t... #8509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
1ea9df6 to
7417cf8
Compare
... by default (thanos-io#8509) In a previous change back in v0.37.0[^1], we stopped explicitely checking for the compatibility label advertised by the store gateway introduced to ensure compatibility with querier <v0.8.0. However, what didn't happen in that PR is that the thanos will still try to add this label by default[^2]. This change modifies the command line argument to default to NOT adding this compatibility label by default anymore, since this is now the assumption made in code. The reason behind this change is that while upgrading from v0.36.x, we quickly realised that querries were fanning out to all our endpoint-groups, and not abiding by our external labels anymore. [^1]: [pull/7645](https://github.com/thanos-io/thanos/pull/7645/files#diff-fb8e209f24ab370a81b6909a08fd58b18b6dd4a7816597879c08cf5590542b60R800) [^2]: [debug.advertise-compatibility-label](https://github.com/thanos-io/thanos/blob/d997eed1749e4d91243efc0137f6a20cb659841b/cmd/thanos/store.go#L182-L183) Signed-off-by: Alexis Lesieur <Xaelias@users.noreply.github.com>
7417cf8 to
4cbca31
Compare
Just out of curiosity - how was the fan-out pruning affected by this? I don't yet fully understand.
Just out of curiosity - how was the fan-out pruning affected by this? I don't yet fully understand.
I'm not gonna lie I'm not the one who found the issue (just the one that was doing the upgrade). But my understanding is that since we went from:
if ls[0].Name == store.CompatibilityTypeLabelName {
continue
(which would effectively "ignore" the compatibility label in this loop) to
if ls.Len() == 0 {
continue
now we have the problem of ls.len() is never 0, because we have the compatibility label by default in there. Which means we required things to match the compatibility label set, which was everything, because it's added by default?
Uh oh!
There was an error while loading. Please reload this page.
Changes
In a previous change back in v0.37.01 , we stopped explicitely checking for the compatibility label advertised by the store gateway introduced to ensure compatibility with querier <v0.8.0.
However, what didn't happen in that PR is that the thanos will still try to add this label by default2 .
This change modifies the command line argument to default to NOT adding this compatibility label by default anymore, since this is now the assumption made in code.
The reason behind this change is that while upgrading from v0.36.x, we quickly realised that querries were fanning out to all our endpoint-groups, and not abiding by our external labels anymore.
Verification
We've been running with
--no-debug.advertise-compatibility-labelin prod and confirmed the behavior is fixed.PS: Hi 👋 first PR on this project. Apologies if anything doesn't look right, let me know if I need to change aything!
Footnotes
pull/7645 ↩
debug.advertise-compatibility-label ↩