Skip to content
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

Merged
merged 13 commits into from
Oct 9, 2019
Merged

Conversation

jojohappy
Copy link
Member

@jojohappy jojohappy commented Sep 30, 2019

  • CHANGELOG entry if change is relevant to the end user.

Changes

Implement partial parts of proposal #1501 :

  • Add relabel config into Thanos, store gateway and compactor are supported now.
  • Selecting blocks to serve depends on the result of block labels relabeling.
  • For store gateway, expose advertise labels after relabeling.
  • Allow the duplicate external labels for store gateway components.

Note: pre-filter has been implemented before this PR.

TODO:

  • Add unit test.
  • Add documents for relabeling.

@bwplotka @brancz @povilasv @GiedriusS PTAL, thanks!

@bwplotka
Copy link
Member

bwplotka commented Oct 3, 2019

Nice! It looks awesome generally. Will try to look on it ASAP.

Copy link
Member

@bwplotka bwplotka left a 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!

@@ -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.
Copy link
Member

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.

// 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())
Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

level.Debug(s.logger).Log("msg", "dropping block(drop in relabeling)", "block", id)
return os.RemoveAll(dir)
}
b.labels = labels.FromMap(processedLabels.Map())
Copy link
Member

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 (:

Copy link
Member Author

@jojohappy jojohappy Oct 6, 2019

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.

Copy link
Member

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? (:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

}, nil
}

if len(s.relabelConfig) != 0 {
Copy link
Member

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>
@jojohappy jojohappy changed the title [WIP]: Store,Compactor: Thanos sharding Store,Compactor: Thanos sharding Oct 8, 2019
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
Signed-off-by: jojohappy <sarahdj0917@gmail.com>
@bwplotka
Copy link
Member

bwplotka commented Oct 8, 2019

It would really nice to add tests particulary for sharding compactor and store GW. Maybe in /test/e2e/?

Copy link
Member

@bwplotka bwplotka left a 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
Copy link
Member

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- 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.
Copy link
Member

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.

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 ",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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 ",

@@ -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 ||
Copy link
Member

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 (:

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Done!

if len(store.LabelSets()) > 0 && externalLabelOccurrencesInStores[externalLabels] != 1 {
if len(store.LabelSets()) > 0 &&
store.storeType != nil &&
(store.storeType.ToProto() == storepb.StoreType_QUERY ||
Copy link
Member

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?

}, nil
}

labelSets := make(map[uint64][]storepb.Label, len(s.blocks))
Copy link
Member

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? (:

Copy link
Member

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.

Copy link
Member Author

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!

@bwplotka
Copy link
Member

bwplotka commented Oct 8, 2019

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>
@bwplotka
Copy link
Member

bwplotka commented Oct 9, 2019

Tests fails ):

Signed-off-by: jojohappy <sarahdj0917@gmail.com>
@jojohappy
Copy link
Member Author

@bwplotka That's green!

Copy link
Member

@bwplotka bwplotka left a 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

@bwplotka bwplotka merged commit e954fe7 into thanos-io:master Oct 9, 2019
GiedriusS pushed a commit that referenced this pull request Oct 28, 2019
* 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>
@xiaozongyang
Copy link

Hi @jojohappy @bwplotka
Do we have some examples of selecting blocks with relabel configs here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants