-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Store,Compactor: Thanos sharding #1583
Conversation
Nice! It looks awesome generally. Will try to look on it ASAP. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, generally it looks like a good start! Some suggestions (: Thanks for doing this!
pkg/compact/compact.go
Outdated
@@ -275,6 +280,15 @@ func (c *Syncer) downloadMeta(ctx context.Context, id ulid.ULID) (*metadata.Meta | |||
return nil, errors.Wrapf(err, "downloading meta.json for %s", id) | |||
} | |||
|
|||
// Check for block labels by relabeling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. However I would move it out of downloadMeta
as we do more than method suggests which might be confusing. Let's do it after meta is downloaded? Also it's nice to do it after downloadMeta method as we check if it's not too fresh first.
pkg/query/storeset.go
Outdated
// Append the storeType to the end of list, because the store gateway will be exposed external labels, | ||
// we allow the duplicate external labels with different components. | ||
if store.storeType != nil { | ||
tsdbLabelSetStrings = append(tsdbLabelSetStrings, store.storeType.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this sounds like a hack, there is no way to check if this guy is a storeGW
inside checking deduplicated stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, what do you mean?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are adding some artificial labels to the labelset. Why? This sounds wrong? To allow duplicated store gateway labels we just need to avoid detecting this if store.storeType
is Gateway, right? NOT
pkg/store/bucket.go
Outdated
level.Debug(s.logger).Log("msg", "dropping block(drop in relabeling)", "block", id) | ||
return os.RemoveAll(dir) | ||
} | ||
b.labels = labels.FromMap(processedLabels.Map()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is important because actually we get the labels after relabelling, which we agreed not doing in the first iteration (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we expose the original labels from meta.json
? Maybe the action of relabeling will be labeldrop
or labelkeep
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!
It may yes. But what's the use case to drop only some labels?
In fact it would be more useful to add new labels! This is great but there are some limitiation for it (e.g relabelling cannot generate 2 lablesets) . For now in this PR I would say we care ONLY if it exists or not exists. Does it sounds fair? (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree.
pkg/store/bucket.go
Outdated
}, nil | ||
} | ||
|
||
if len(s.relabelConfig) != 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would vote to precompute this every sync instead of computing this on every Info which is frequent (at least every 5s)
… gateway Signed-off-by: jojohappy <sarahdj0917@gmail.com>
…mpty labels when no relabel configuration Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
d420725
to
a210289
Compare
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
It would really nice to add tests particulary for sharding compactor and store GW. Maybe in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! It looks amazing. Hopefully last things to fix!
Thanks for doing it!
CHANGELOG.md
Outdated
@@ -20,6 +20,10 @@ We use *breaking* word for marking changes that are not backward compatible (rel | |||
- [#1534](https://github.com/thanos-io/thanos/pull/1534) Thanos Query Added `/-/ready` and `/-/healthy` endpoints. | |||
- [#1533](https://github.com/thanos-io/thanos/pull/1533) Thanos inspect now supports the timeout flag. | |||
- [#1362](https://github.com/thanos-io/thanos/pull/1362) Optional `replicaLabels` param for `/query` and `/query_range` querier endpoints. When provided overwrite the `query.replica-label` cli flags. | |||
- [#1583](https://github.com/thanos-io/thanos/pull/1583) Thanos sharding: | |||
- Add relabel config (`--selector.relabel-config-file` and `selector.relabel-config`) into Thanos |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth to note exact components (:
CHANGELOG.md
Outdated
- [#1583](https://github.com/thanos-io/thanos/pull/1583) Thanos sharding: | ||
- Add relabel config (`--selector.relabel-config-file` and `selector.relabel-config`) into Thanos | ||
- Selecting blocks to serve depends on the result of block labels relabeling. | ||
- For store gateway, expose advertise labels after relabeling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- For store gateway, expose advertise labels after relabeling. | |
- For store gateway, advertise labels from "approved" blocks. |
@@ -20,6 +20,10 @@ We use *breaking* word for marking changes that are not backward compatible (rel | |||
- [#1534](https://github.com/thanos-io/thanos/pull/1534) Thanos Query Added `/-/ready` and `/-/healthy` endpoints. | |||
- [#1533](https://github.com/thanos-io/thanos/pull/1533) Thanos inspect now supports the timeout flag. | |||
- [#1362](https://github.com/thanos-io/thanos/pull/1362) Optional `replicaLabels` param for `/query` and `/query_range` querier endpoints. When provided overwrite the `query.replica-label` cli flags. | |||
- [#1583](https://github.com/thanos-io/thanos/pull/1583) Thanos sharding: | |||
- Add relabel config (`--selector.relabel-config-file` and `selector.relabel-config`) into Thanos | |||
- Selecting blocks to serve depends on the result of block labels relabeling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be part of the last sentence.
cmd/thanos/flags.go
Outdated
return extflag.RegisterPathOrContent( | ||
cmd, | ||
"selector.relabel-config", | ||
"YAML file that contains seletor relabeling configuration. It follows native Prometheus relabel-config syntax. See format details: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config ", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"YAML file that contains seletor relabeling configuration. It follows native Prometheus relabel-config syntax. See format details: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config ", | |
"YAML file that contains relabeling configuration that allows selecting blocks. It follows native Prometheus relabel-config syntax. See format details: https://prometheus.io/docs/prometheus/latest/configuration/configuration/#relabel_config ", |
pkg/query/storeset.go
Outdated
@@ -218,7 +218,10 @@ func (s *StoreSet) Update(ctx context.Context) { | |||
// Record the number of occurrences of external label combinations for current store slice. | |||
externalLabelOccurrencesInStores := map[string]int{} | |||
for _, st := range healthyStores { | |||
externalLabelOccurrencesInStores[externalLabelsFromStore(st)]++ | |||
if st.storeType != nil && (st.storeType.ToProto() == storepb.StoreType_QUERY || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we changing this? Let's maybe focus on line 258 as you did instead (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Done!
pkg/query/storeset.go
Outdated
if len(store.LabelSets()) > 0 && externalLabelOccurrencesInStores[externalLabels] != 1 { | ||
if len(store.LabelSets()) > 0 && | ||
store.storeType != nil && | ||
(store.storeType.ToProto() == storepb.StoreType_QUERY || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just exclude StoreType_STORE
? It will be much safe. It's the only thing that we change here - store GW handing, right?
pkg/store/bucket.go
Outdated
}, nil | ||
} | ||
|
||
labelSets := make(map[uint64][]storepb.Label, len(s.blocks)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again as I mentioned before - we can build that every sync instead of every Info call. Can we do that? (:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Info should be as fast as possible as it works as healthiness/heartbeat check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will update today. Thanks!
It would be also awesome to have some tests for compactor filtering and store GW exposing info + filtering. Maybe in next PR (: |
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
295b12d
to
de9a47f
Compare
Tests fails ): |
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
ecf1456
to
148828d
Compare
@bwplotka That's green! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Good work, LGTM
* Add selector.relabel-config flag && expose advertised label for store gateway Signed-off-by: jojohappy <sarahdj0917@gmail.com> Signed-off-by: Giedrius Statkevičius <giedriuswork@gmail.com>
Hi @jojohappy @bwplotka |
Changes
Implement partial parts of proposal #1501 :
Note: pre-filter has been implemented before this PR.
TODO:
@bwplotka @brancz @povilasv @GiedriusS PTAL, thanks!