store+compactor: process index cache during compaction #986

xjewer · 2019-03-28T00:51:30Z

Add few steps during compaction:

Process all index cache in old blocks made by compactor until this version,
updates meta.
Calculate index cache during group compaction.
Calculate index cache during downsampling.

Store downloads index cache from object storage or process it if doesn't exist.

covers first step of #942

cc @bwplotka

bwplotka

Awesome!

I generally like it but there are 3 majors:

I love the idea for versioning the index cache. Thanks for remembering about it! But we need this to be inside indexCache struct/JSON (next to index version) not in meta.
we need to work on metrics, they could be improved I think and we don't that many of them
please do not add the index cache traversing to syncer. Let's keep the abstractions focused on one thing.
Let's add missing index cache ONLY for compaction lvl > 2. Also we can do that concurrently.

Actually 4 things ;p

But anyway, Good work! let's improve store GW 💪

pkg/block/block.go

pkg/block/metadata/meta.go

pkg/compact/compact.go

pkg/compact/downsample/streamed_block_writer.go

pkg/store/bucket.go

bwplotka

Thanks for changes. still some comments. (: But looking good!

cmd/thanos/compact.go

pkg/block/index.go

pkg/compact/compact.go

pkg/compact/index_cache.go

pkg/compact/compact.go

bwplotka

Really Really Awesome! Thanks for this, I like this, some minor suggestions only.

The only worry is that will take ages to complete all of it, so wonder if we should not add the quick code for concurrency on those like we did on Compactor group side. Maybe not on this PR though.

Also, do we have an idea how to test it in integration/unit test? I think e2e would be nice place for it: https://github.com/improbable-eng/thanos/blob/master/test/e2e/store_gateway_test.go

cmd/thanos/compact.go

bwplotka · 2019-04-12T11:35:16Z

cmd/thanos/compact.go

@@ -87,6 +92,9 @@ func registerCompact(m map[string]setupFunc, app *kingpin.Application, name stri
 	wait := cmd.Flag("wait", "Do not exit after all compactions have been processed and wait for new work.").
 		Short('w').Bool()

+	generateMissingIndexCacheFiles := cmd.Flag("index.generate-missing-cache-file", "Process indices' cache, upload them to object store and update metas.").


Suggested change

generateMissingIndexCacheFiles := cmd.Flag("index.generate-missing-cache-file", "Process indices' cache, upload them to object store and update metas.").

generateMissingIndexCacheFiles := cmd.Flag("index.generate-missing-cache-file", "If enabled, on startup compactor runs an on-off job that scans all the blocks to find all blocks with missing index cache file. It generates those if needed and upload.").

cmd/thanos/compact.go

bwplotka · 2019-04-12T11:35:36Z

cmd/thanos/compact.go

@@ -255,6 +266,13 @@ func runCompact(
 	g.Add(func() error {
 		defer runutil.CloseWithLogOnErr(logger, bkt, "bucket client")

+		// Generate index file


missing trailing period ;p

bwplotka · 2019-04-12T11:37:06Z

cmd/thanos/compact.go

@@ -300,3 +318,121 @@ func runCompact(
 	level.Info(logger).Log("msg", "starting compact node")
 	return nil
 }
+
+// genMissingIndexCacheFiles generates missing index cache files and uploads them to object storage.


Suggested change

// genMissingIndexCacheFiles generates missing index cache files and uploads them to object storage.

// genMissingIndexCacheFiles scans over all blocks, generates missing index cache files and uploads them to object storage.

cmd/thanos/compact.go

bwplotka · 2019-04-12T11:40:57Z

cmd/thanos/compact.go

+	}
+
+	for _, meta := range metas {
+		if err := generateIndexCacheFile(ctx, bkt, logger, dir, meta); err != nil {


To be more verbose I would check if index cache is missing first in this loop before going to generateIndexCacheFile?

let's keep where it is, otherwise you have to pass IndexCacheFilename as a parameter to generateIndexCacheFile.
I can rename it generateIndexCacheFile -> checkIfExistsOrGenerateIndexCacheFile

passing another parameter is not that bad ;p

but up to you

not addressed I think

bwplotka · 2019-04-12T11:41:07Z

cmd/thanos/compact.go

+	cachePath := filepath.Join(bdir, block.IndexCacheFilename)
+	cache := path.Join(meta.ULID.String(), block.IndexCacheFilename)
+
+	ok, err := objstore.Exists(ctx, bkt, cache)


let's move it as commented above

bwplotka · 2019-04-12T11:42:21Z

pkg/compact/compact.go

@@ -828,7 +831,7 @@ func (cg *Group) compact(ctx context.Context, dir string, comp tsdb.Compactor) (
 	}

 	// Ensure the output block is valid.
-	if err := block.VerifyIndex(cg.logger, filepath.Join(bdir, block.IndexFilename), newMeta.MinTime, newMeta.MaxTime); !cg.acceptMalformedIndex && err != nil {
+	if err := block.VerifyIndex(cg.logger, index, newMeta.MinTime, newMeta.MaxTime); err != nil {


why !cg.acceptMalformedIndex was dropped?

thanks, didn't notice it during resolving merge conflict.

bwplotka · 2019-04-12T11:44:55Z

pkg/objstore/objstore.go

@@ -170,6 +170,23 @@ func DownloadDir(ctx context.Context, logger log.Logger, bkt BucketReader, src,
 	return nil
 }

+// Exists returns true, if file exists, otherwise false and nil error if presence IsObjNotFoundErr, otherwise false with
+// returning error.
+func Exists(ctx context.Context, bkt Bucket, src string) (bool, error) {


I don't really like this helper. Having more than two ways of doing things is usually an antipattern. It's only used in single place, can we maybe make this quick helper , local function instead? Not a blocker

why not?

https://github.com/improbable-eng/thanos/blob/7466e9091a600fa8029e3dea35fca5a8cc4e03c0/pkg/objstore/objstore.go#L47

you literally suggested to do the same

Yea, but with explicit REMOVING statement ;p

But you are right, let's leave it and remove EXISTS from interface in another PRs, thanks for pointing this (:

bwplotka · 2019-04-12T11:49:04Z

Changelog and docs are required to tell what is happening with this flag.

Add few steps during compaction: 1. Generate index cache for old blocks made by compactor until this version. 2. Generate index cache during group compaction. 3. Generate index cache during downsampling. 4. Add index cache version to cache file. Store downloads index cache files from object store or generate on the fly if they don't exist. Signed-off-by: Aleksei Semiglazov <[email protected]>

bwplotka · 2019-04-12T19:08:26Z

CHANGELOG.md

@@ -35,6 +36,7 @@ New tracing span:
 - [#970](https://github.com/improbable-eng/thanos/pull/970) Added `PartialResponseStrategy` field for `RuleGroups` for `Ruler`.
 - [#1016](https://github.com/improbable-eng/thanos/pull/1016) Added option for another DNS resolver (miekg/dns client). 
 This to have SRV resolution working on [Golang 1.11+ with KubeDNS below v1.14](https://github.com/golang/go/issues/27546)
+- [#986](https://github.com/improbable-eng/thanos/pull/986) Store index cache files in object storage, reduces store start-up time by skipping the generating the index cache for all blocks and only do this for recently created uncompacted blocks. 


Could be reworded, but we can do it later on cut 0.4.0 👍

bwplotka · 2019-04-12T19:09:25Z

cmd/thanos/compact.go

+	}
+
+	for _, meta := range metas {
+		if err := generateIndexCacheFile(ctx, bkt, logger, dir, meta); err != nil {


not addressed I think

GiedriusS · 2019-04-12T20:10:49Z

Wasn't fast enough with a full review before you merged it but it looks really nice 👍 awesome, thank you a lot!

bwplotka · 2019-04-12T20:40:13Z

Feel free to mark your feedback somewhere! We can include it in later PR

…

On Fri, 12 Apr 2019 at 21:10, Giedrius Statkevičius < ***@***.***> wrote: Wasn't fast enough with a full review before you merged it but it looks really nice 👍 awesome, thank you a lot! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#986 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGoNu_ufsNjg5kl7pAAUhgv4BHbKZ863ks5vgOhKgaJpZM4cPDd_> .

* query: cleanup store statuses as they come and go (thanos-io#910) Signed-off-by: Adrien Fillon <[email protected]> * [docs] Example of using official prometheus helm chart to deploy server with sidecar (thanos-io#1003) * update documentation with an example of using official prometheus helm chart Signed-off-by: Ivan Kiselev <[email protected]> * a little formatting to values Signed-off-by: Ivan Kiselev <[email protected]> * satisfy PR comments Signed-off-by: Ivan Kiselev <[email protected]> * Compact: group concurrency (thanos-io#1010) * compact: add concurrency to group compact * add flag to controll the number of goroutines to use when compacting group * update compact.md for group-compact-concurrency * fixed: miss wg.Add() * address CR * regenerate docs * use err group * fix typo in flag description * handle context * set up workers in main loop * move var initialisation * remove debug log * validate concurrency * move comment * warn -> error * remove extra newline * fix typo * dns: Added miekgdns resolver as a hidden option to query and ruler. (thanos-io#1016) Fixes: thanos-io#1015 Signed-off-by: Bartek Plotka <[email protected]> * query: set default evaluation interval (thanos-io#1028) Subqueries allows request with no [specified resolution](https://prometheus.io/blog/2019/01/28/subquery-support/). Set it up and allow to configure default evaluation interval. * store+compactor: pre-compute index cache during compaction (thanos-io#986) Fixes first part of thanos-io#942 This changes allow to safe some startup & sync time in store gateway as it is no longer is needed to compute index-cache from block index on its own. For compatibility store GW still can do it, but it first checks bucket if there is index-cached uploaded already. In the same time, compactor precomputes the index cache file on every compaction. To allow quicker addition of index cache files we added `--index.generate-missing-cache-file` flag, that if enabled precompute missing files on compactor startup. Note that it will take time and it's only one-off step per bucket. Signed-off-by: Aleksei Semiglazov <[email protected]> * Added website for Thanos' docs using Hugo. (thanos-io#807) Hosted in github pages. Signed-off-by: adrien-f <[email protected]> Signed-off-by: Bartek Plotka <[email protected]> * gcs: Fixed scopes for inline ServiceAccount option. (thanos-io#1033) Without this that option was unusable. Signed-off-by: Bartek Plotka <[email protected]> * Fixed root docs and liche is now checking root dir as well. (thanos-io#1040) Signed-off-by: Bartek Plotka <[email protected]> * storage docs: add detail about GCS policies and testing (thanos-io#1037) * add more details about GCS policies and testing * remove fixed names from exec command * Prometheus library updated to v2.8.1 (thanos-io#1009) * compact: group concurrency improvements (thanos-io#1029) * group concurrency improvements * remove unnecessary error check * add to wg in main goroutine * receive: Add block shipping (thanos-io#1011) * receive: Add retention flag for local tsdb storage (thanos-io#1046) * querier: Add /api/v1/labels support (thanos-io#905) * Feature: add /api/v1/labels support Signed-off-by: jojohappy <[email protected]> * Disabled gossip by default, marked all flags as deprecated. (thanos-io#1055) + changed small label. Signed-off-by: Bartek Plotka <[email protected]> * ruler: Fixed Chunk going out or Max Uint16. (thanos-io#1041) Fixes thanos-io#1038 Signed-off-by: Bartek Plotka <[email protected]> * store: azure: allow passing an endpoint parameter for specific regions (thanos-io#980) Fix thanos-io#968 Signed-off-by: Adrien Fillon <[email protected]> * feature: support POST method for series endpoint (thanos-io#1021) Signed-off-by: Joseph Lee <[email protected]> * bucket verify: repair out of order labels (thanos-io#964) * bucket verify: repair out of order labels * verify repair: correctly order series in the index on rewrite When we have label sets that are not in the correct order, fixing that changes the order of the series in the index. So the index must be rewritten in that new order. This makes this repair tool take up a bunch more memory, but produces blocks that verify correctly. * Fix the TSDB block safe-delete function The directory name must be the block ID name exactly to verify. A temp directory or random name will not work here. * verify repair: fix duplicate chunk detection Pointer/reference logic error was eliminating all chunks for a series in a given TSDB block that wasn't the first chunk. Chunks are now referenced correctly via pointers. * PR feedback: use errors.Errorf() instead of fmt.Errorf() * Use errors.New() Some linters catch errors.Errorf() as its not really part of the errors package. * Liberally comment this for loop We're comparing items by pointers, using Go's range variables is misleading here and we need not fall into the same trap. * Take advantage of sort.Interface This prevents us from having to re-implement label sorting. * PR Feedback: Comments are full sentences. * Cut release 0.4.0-rc.0 (thanos-io#1017) * Cut release 0.4.0-rc.0 🎉 🎉 NOTE: This is last release that has gossip. Signed-off-by: Bartek Plotka <[email protected]> Co-Authored-By: povilasv <[email protected]> * Fixed crossbuild. Signed-off-by: Bartek Plotka <[email protected]> * ci: Env fixes. (thanos-io#1058) Signed-off-by: Bartek Plotka <[email protected]> * Removed bzr requirement for make crossbuild. Signed-off-by: Bartek Plotka <[email protected]> * Bump github.com/hashicorp/golang-lru from 0.5.0 to 0.5.1 (thanos-io#1051) Bumps [github.com/hashicorp/golang-lru](https://github.com/hashicorp/golang-lru) from 0.5.0 to 0.5.1. - [Release notes](https://github.com/hashicorp/golang-lru/releases) - [Commits](hashicorp/golang-lru@v0.5.0...v0.5.1) Signed-off-by: dependabot[bot] <[email protected]> * Initialze and correctly register all index cache metrics. (thanos-io#1069) * store/cache: add more tests (thanos-io#1071) * Fixed Downsampling process; Fixed `runutil.CloseAndCaptureErr` (thanos-io#1070) * runutil. Simplified CloseWithErrCapture. Signed-off-by: Bartek Plotka <[email protected]> * Fixed Downsampling process; Fixed runutil.CloseAndCaptureErr Fixes thanos-io#1065 Root cause: * runutil defered capture error function was not passing error properly so unit tests were passing, event though there was bug * streamed block write index cache requires index file which was not closed (saved) properly yet. Closers need to be closed to perform this. Signed-off-by: Bartek Plotka <[email protected]> * objstore: Expose S3 region attribute (thanos-io#1060) Minio is able to autodetect the region for cloud providers like AWS but the logic fails with Scaleway Object Storage solution. Related issue on Minio: minio/mc#2570 * Fixed fetching go-bindata failed (thanos-io#1074) * Fixed bug: - fetching go-bindata failed. - change the repo of go-bindata to github.com/go-bindata/go-bindata, because old repo has been archived. - pin the go-bindata as v3.3.1. Signed-off-by: jojohappy <[email protected]> * Add CHANGELOG Signed-off-by: jojohappy <[email protected]> * Remove CHANGELOG Signed-off-by: jojohappy <[email protected]> * add compare flags func to compare flags between prometheus and sidecar (thanos-io#838) Original message: * update documentation for a max/min block duration add compare flags func to compare flags between prom and sidecar * fix some nits Functional change: now we check the configured flags (if possible) and error out if MinTime != MaxTime. We need to check this always since if that is not true then we will get overlapping blocks. Additionally, an error message is printed out if it is not equal to 2h (the recommended value). * Ensured index cache is best effort, refactored tests, validated edge cases. (thanos-io#1073) Fixes thanos-io#651 Current size also includes slice header. Signed-off-by: Bartek Plotka <[email protected]> * website: Moved to netlify. (thanos-io#1078) Signed-off-by: Bartek Plotka <[email protected]> * website: Fixing netlify. (thanos-io#1080) Signed-off-by: Bartek Plotka <[email protected]> * website: Added "founded by" footer. (thanos-io#1081) Signed-off-by: Bartek Plotka <[email protected]> * store/proxy: properly check if context has ended (thanos-io#1082) How the code was before it could happen that we might receive some series from the stream however by the time we'd send them back to the reader, it would not read it anymore since the deadline would have been exceeded. Properly use a `select` here to get out of the goroutine if the deadline has been exceeded. Might potentially fix a problem where we see one goroutine hanging constantly (and thus blocking from work being done): ``` goroutine profile: total 126 25 @ 0x42f62f 0x40502b 0x405001 0x404de5 0xe7435b 0x45cc41 0xe7435a github.com/improbable-eng/thanos/pkg/store.startStreamSeriesSet.func1+0x18a /go/src/github.com/improbable-eng/thanos/pkg/store/proxy.go:318 ``` * Cut release v0.4.0-rc.1 (thanos-io#1088) Signed-off-by: Bartek Plotka <[email protected]> * website: Removed ghpages handling; fixed docs; and status badge. (thanos-io#1084) Signed-off-by: Bartek Plotka <[email protected]> * Fix readme (thanos-io#1090) * store: Compose indexCache properly allowing injection for testing purposes. (thanos-io#1098) Signed-off-by: Bartek Plotka <[email protected]> * website: add sponsor section on homepage (thanos-io#1062) * website: Adjusted logos sizing and responsiveness. (thanos-io#1105) Signed-off-by: Bartek Plotka <[email protected]> * Add Monzo to "Used by" section 🎉 (thanos-io#1106) * Compactor: remove malformed blocks after delay (thanos-io#1053) * compactor removes malformed blocks after delay * compactor removes malformed blocks after delay * include missing file * reuse existing freshness check * fix comment * remove unused var * fix comment * syncDelay -> consistencyDelay * fix comment * update flag description * address cr * fix dupliacte error handling * minimum value for --consistency-delay * update * docs * add test case * move test to inmem bucket * Add Utility Warehouse to "used by" section (thanos-io#1108) * Add Utility Warehouse logo * Make logo smaller * website: add Adform as users (thanos-io#1109) We use Thanos extensively as well so I have added Adform. * Cut release v0.4.0 (thanos-io#1107) Signed-off-by: Bartek Plotka <[email protected]>

bwplotka added difficulty: medium priority: P0 component: compact component: store help wanted labels Mar 28, 2019

bwplotka requested review from povilasv and bwplotka and removed request for povilasv March 28, 2019 22:02

bwplotka requested changes Mar 28, 2019

View reviewed changes

bwplotka requested changes Apr 7, 2019

View reviewed changes

xjewer force-pushed the feature_index_cache branch 4 times, most recently from e089a5f to 7466e90 Compare April 11, 2019 20:39

xjewer marked this pull request as ready for review April 11, 2019 20:48

bwplotka approved these changes Apr 12, 2019

View reviewed changes

xjewer force-pushed the feature_index_cache branch 2 times, most recently from 9308f7e to 84a03eb Compare April 12, 2019 12:35

xjewer force-pushed the feature_index_cache branch from 84a03eb to 5c32ed5 Compare April 12, 2019 14:15

bwplotka approved these changes Apr 12, 2019

View reviewed changes

bwplotka merged commit e6d5b49 into thanos-io:master Apr 12, 2019

xjewer deleted the feature_index_cache branch April 15, 2019 10:32

bwplotka mentioned this pull request Apr 23, 2019

compactor: downsampler dont generate meta.json #1065

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

store+compactor: process index cache during compaction #986

store+compactor: process index cache during compaction #986

xjewer commented Mar 28, 2019

bwplotka left a comment •

edited

Loading

bwplotka left a comment

bwplotka left a comment

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

xjewer Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

xjewer Apr 12, 2019

bwplotka Apr 12, 2019

xjewer Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka commented Apr 12, 2019

bwplotka Apr 12, 2019

bwplotka Apr 12, 2019

GiedriusS commented Apr 12, 2019

bwplotka commented Apr 12, 2019 via email

	generateMissingIndexCacheFiles := cmd.Flag("index.generate-missing-cache-file", "Process indices' cache, upload them to object store and update metas.").
	generateMissingIndexCacheFiles := cmd.Flag("index.generate-missing-cache-file", "If enabled, on startup compactor runs an on-off job that scans all the blocks to find all blocks with missing index cache file. It generates those if needed and upload.").

	// genMissingIndexCacheFiles generates missing index cache files and uploads them to object storage.
	// genMissingIndexCacheFiles scans over all blocks, generates missing index cache files and uploads them to object storage.

store+compactor: process index cache during compaction #986

store+compactor: process index cache during compaction #986

Conversation

xjewer commented Mar 28, 2019

bwplotka left a comment • edited Loading

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

bwplotka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka commented Apr 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GiedriusS commented Apr 12, 2019

bwplotka commented Apr 12, 2019 via email

bwplotka left a comment •

edited

Loading