thanosconvert: convert block meta from Thanos to Cortex #3770

milesbxf · 2021-02-01T14:48:00Z

What this PR does:

thanosconvert is a small tool to iterate over a bucket and convert block-level metadata (in meta.json) so that it is readable by Cortex. This tool assumes that users have already copied Thanos blocks over to a destination bucket, with blocks organised by user at the top level.

This is a relatively simple serial implementation - it's pretty quick to run so making it concurrent is probably unnecessary, at least for a v1. I've only tested this with S3, but since it uses github.com/thanos-io/thanos/pkg/objstore it should work with all the other supported block stores. It reuses the same pkg/storage/bucket.Config struct for bucket configuration so should be easy to configure for users who already have the store-gateway/querier running.

It's idempotent so is safe to run multiple times against the same bucket, and leaves Cortex blocks untouched. It will log any errors it encounters but keeps going.

I'm happy that this is mostly complete, but there's a few things left to do - I'd love some feedback before I finish it off!

Which issue(s) this PR fixes:
Fixes #2920

Checklist

Add CLI usage message
Clean up, comment public API methods
Documentation added
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

pracucci

Fantastic job! We wanted this since a long time, thanks! I left few comments here and there, but mostly nits that should be very quick to address.

Other than the comments:

Could you add it to the list of experimental features listed in docs/configuration/v1-guarantees.md, please?
Could you mention the tool in docs/blocks-storage/migrate-storage-from-thanos-and-prometheus.md? There's a sentence like "no tool is provided to migrate TSDB blocks from Thanos" while now there is 😉

pracucci · 2021-02-01T17:03:46Z

cmd/thanosconvert/main.go

+	iterCtx := context.Background()
+	results, err := converter.Run(iterCtx)
+	if err != nil {
+		level.Error(util.Logger).Log("msg", "iterate blocks", "err", err)


Suggested change

level.Error(util.Logger).Log("msg", "iterate blocks", "err", err)

level.Error(util.Logger).Log("msg", "error while iterating blocks", "err", err)

pracucci · 2021-02-01T17:05:19Z

tools/thanosconvert/thanosconvert.go

+type Results map[string]PerUserResults
+
+// NewThanosBlockConverter creates a ThanosBlockConverter
+func NewThanosBlockConverter(ctx context.Context, cfg bucket.Config, dryRun bool, logger log.Logger, reg prometheus.Registerer) (*ThanosBlockConverter, error) {


I would remove reg prometheus.Registerer given this tool doesn't export metrics anyway, and just pass nil as registered to bucket.NewClient() (when nil metrics registration will be skipped).

pracucci · 2021-02-01T17:09:15Z

tools/thanosconvert/thanosconvert.go

+	var users []string
+	err := c.bkt.Iter(ctx, "", func(o string) error {
+		users = append(users, strings.TrimSuffix(o, "/"))
+		return nil
+	})


You may want to reuse the scanner we already have:

scanner := cortex_tsdb.NewUsersScanner(c.bkt, cortex_tsdb.AllUsers) users, _, err := scanner.ScanUsers(ctx)

pracucci · 2021-02-01T17:11:59Z

tools/thanosconvert/thanosconvert.go

+func (c ThanosBlockConverter) convertUser(ctx context.Context, user string) (PerUserResults, error) {
+	results := PerUserResults{}
+
+	err := c.bkt.Iter(ctx, user, func(o string) error {


I would wrap c.bkt with bucket.NewUserBucketClient(). This will mask you the first object prefix segment (which is the <tenant ID>/).

pracucci · 2021-02-01T17:14:10Z

tools/thanosconvert/thanosconvert.go

+	err := c.bkt.Iter(ctx, user, func(o string) error {
+		// get block ID from path
+
+		parts := strings.Split(o, "/")


Once you use the UserBuckerClient, this should just be a matter of:

o = strings.TrimSuffix(o, "/") blockID, err := ulid.Parse(o)

If the parse fail, just skip it (no error) because it means it's not a block. We don't have just blocks in bucket.

pracucci · 2021-02-01T17:23:56Z

tools/thanosconvert/thanosconvert_test.go

+	}
+}
+
+func CortexMeta(user string) metadata.Meta {


No need to export this function.

pracucci · 2021-02-01T17:24:04Z

tools/thanosconvert/thanosconvert_test.go

+	}
+}
+
+func ThanosMeta() metadata.Meta {


No need to export this function.

pracucci · 2021-02-01T17:24:08Z

tools/thanosconvert/thanosconvert_test.go

+	}
+}
+
+func GetLogger() log.Logger {


No need to export this function.

pracucci · 2021-02-01T17:24:11Z

tools/thanosconvert/thanosconvert_test.go

+	return logger
+}
+
+func MockBucket(data fakeBucket) *bucket.ClientMock {


No need to export this function.

pracucci · 2021-02-01T17:26:26Z

tools/thanosconvert/thanosconvert_test.go

+			name: "bucket fully converted is a noop",
+			bucketData: fakeBucket{
+				"user1": map[string]metadata.Meta{
+					"block1": CortexMeta("user1"),


Once you start using ulid.Parse() these tests will fail because "block1" is not a valid block ID. You could just define at the beginning of the test function something like the following, and then use such variables across the test cases:

block1 := ulid.MustNew(1, nil) block2 := ulid.MustNew(2, nil)

milesbxf · 2021-02-02T10:35:24Z

Fantastic job! We wanted this since a long time, thanks! I left few comments here and there, but mostly nits that should be very quick to address.

Other than the comments:

Could you add it to the list of experimental features listed in docs/configuration/v1-guarantees.md, please?

Could you mention the tool in docs/blocks-storage/migrate-storage-from-thanos-and-prometheus.md? There's a sentence like "no tool is provided to migrate TSDB blocks from Thanos" while now there is 😉

@pracucci thanks for the detailed review! I think I've addressed all your comments - I've added a new commit to make it easier to review the changes, but can squash it down once you're happy. I'll make the docs changes today.

thanosconvert is a small tool to iterate over a bucket and convert block-level metadata (in meta.json) so that it is readable by Cortex. This tool assumes that users have already copied Thanos blocks over to a destination bucket, with blocks organised by user at the top level. Signed-off-by: Miles Bryant <milesbryant@monzo.com>

* Remove prometheus.DefaultRegisterer as we don't export metrics * Use cortex_tsdb.UsersScanner instead of custom logic * Use UserBucketClient instead of custom logic * Use ulid.Parse to make sure we're in a valid block * Generate ulids in tests to pass validation * Use metadata.Read and Write instead of decoding/encoding JSON ourselves * Add Metadata.BlockMeta.Version in tests to pass validation in Read * Use cortex_tsdb.TenantIDExternalLabel instead of hardcoded string Signed-off-by: Miles Bryant <milesbryant@monzo.com>

Signed-off-by: Miles Bryant <milesbryant@monzo.com>

milesbxf · 2021-02-04T10:04:44Z

👋 @pracucci I've updated the docs and added a CHANGELOG entry. Is there anything else I need to do?

Signed-off-by: Miles Bryant <milesbryant@monzo.com>

pracucci

Good job, thanks! I left a couple of nits. Can you confirm you manually tested it after changes? I haven't got the time to manually test it.

@pstibrany could you do the 2nd review, please?

pracucci · 2021-02-05T10:47:39Z

tools/thanosconvert/thanosconvert.go

+		metaKey := fmt.Sprintf("%s/%s/meta.json", user, blockID)
+		r, err := c.bkt.Get(ctx, metaKey)


Use the userBucketClient please. It means the path to meta.json will be:
metaKey := fmt.Sprintf("%s/meta.json", blockID)

After using userBucketClient, we can also use block.DownloadMeta() which will download and parse the meta.json file (it doesn't check for Cortex orgid label, so that's fine), to simplify the code here.

pracucci · 2021-02-05T10:49:16Z

tools/thanosconvert/thanosconvert.go

+	var changesRequired []string
+
+	if meta.Thanos.Labels == nil {
+		// TODO: no "thanos" entry could mean this block is corrupt - should we bail out instead?


I would not bail out. If we don't require the Thanos entry, the tool should work with Prometheus blocks too, which I believe is a plus for the community.

pstibrany

Thank you, this is useful tool. I left bunch of comments suggestions if you feel like working on this code bit more, but they are not blocking... the tool already looks useful as-is.

docs/blocks-storage/migrate-storage-from-thanos-and-prometheus.md

pstibrany · 2021-02-05T11:32:39Z

tools/thanosconvert/thanosconvert.go

+
+func NewPerUserResults() PerUserResults {
+	return PerUserResults{
+		FailedBlocks:    []string{},


Nit: this creates 0-sized slice, which is unnecessary. As it is now, PerUserResults{} is perfectly fine default value, and doesn't even need "New" function -- which isn't used anywhere.

(Furthermore, since methods are defined on *PerUserResults, this method should return pointer. But I'd suggest to remove it completely.)

Turns out I wasn't even using this constructor anyway 😄

tools/thanosconvert/thanosconvert.go

pstibrany · 2021-02-05T11:37:26Z

tools/thanosconvert/thanosconvert.go

+	userBucketClient := bucket.NewUserBucketClient(user, c.bkt)
+	err := userBucketClient.Iter(ctx, "", func(o string) error {
+		// get block ID from path
+		o = strings.TrimSuffix(o, "/")


We can shorten the code here by using id, ok := block.IsBlockDir(o).

pstibrany · 2021-02-05T11:40:24Z

tools/thanosconvert/thanosconvert.go

+		metaKey := fmt.Sprintf("%s/%s/meta.json", user, blockID)
+		r, err := c.bkt.Get(ctx, metaKey)


After using userBucketClient, we can also use block.DownloadMeta() which will download and parse the meta.json file (it doesn't check for Cortex orgid label, so that's fine), to simplify the code here.

pstibrany · 2021-02-05T11:42:54Z

tools/thanosconvert/thanosconvert.go

+
+		if len(changesRequired) > 0 {
+			if c.dryRun {
+				level.Debug(c.logger).Log("msg", "Block requires changes, not uploading due to dry run", "block", blockID, "changes_required", strings.Join(changesRequired, ","))


Do we need to repeat "not uploading due to dry run" message for each block? Perhaps just short "(dry-run)" would do.

tools/thanosconvert/thanosconvert.go

pstibrany · 2021-02-05T11:51:48Z

cmd/thanosconvert/main.go

+	logfmt, loglvl := logging.Format{}, logging.Level{}
+	logfmt.RegisterFlags(flag.CommandLine)
+	loglvl.RegisterFlags(flag.CommandLine)
+	flag.StringVar(&configFilename, "config", "", "Path to bucket config YAML")


Nit: I'd suggest supporting command line options too (or perhaps as the only way of configuring). Bucket config needs just a couple of options.

pstibrany · 2021-02-05T11:53:57Z

cmd/thanosconvert/main.go

+
+var (
+	configFilename string
+	dryRun         bool


Nit: no need for global variables, move to main() method.

* Add command line options for bucket configuration * Use github.com/thanos-io/thanos/pkg/block to parse block directory and fetch metadata instead of custom code * Tweak some log lines Co-authored-by: Peter Štibraný <pstibrany@gmail.com> Signed-off-by: Miles Bryant <milesbryant@monzo.com>

milesbxf · 2021-02-05T12:46:44Z

Thank you, this is useful tool. I left bunch of comments suggestions if you feel like working on this code bit more, but they are not blocking... the tool already looks useful as-is.

Thanks for the review - I figured I'd implement a few of your suggestions whilst the PR is still open 🙏

milesbxf · 2021-02-05T12:47:26Z

Good job, thanks! I left a couple of nits. Can you confirm you manually tested it after changes? I haven't got the time to manually test it.

Thanks! Just to confirm, I've manually tested this after the latest batch of changes (again, only with S3)

milesbxf · 2021-02-05T13:55:43Z

Looks like an unrelated test failed:

--- FAIL: TestAlertmanager_ServeHTTP (0.24s)
    multitenant_test.go:90: 
        	Error Trace:	multitenant_test.go:90
        	            				testing.go:846
        	            				testing.go:876
        	            				testing.go:1048
        	Error:      	Received unexpected error:
        	            	unlinkat /tmp/alertmanager319454365: directory not empty
        	Test:       	TestAlertmanager_ServeHTTP
FAIL
coverage: 65.6% of statements in github.com/cortexproject/cortex/pkg/alertmanager
FAIL	github.com/cortexproject/cortex/pkg/alertmanager	32.268s
FAIL

I can report an issue for this if you think it's a flake - in the meantime how do I rerun CI?

pstibrany · 2021-02-05T14:40:55Z

in the meantime how do I rerun CI

There is "Rerun all jobs" button in top-right corner of the workflow run. I've restarted it now, just in case you don't have permissions and don't see that button.

pstibrany

Thanks for addressing my feedback. I've left few more (non-blocking) comments to take a look at, if you want.

pstibrany · 2021-02-09T09:31:49Z

tools/thanosconvert/thanosconvert.go

+
+		if len(changesRequired) > 0 {
+			if c.dryRun {
+				level.Info(c.logger).Log("msg", "Block requires changes (dry-run)", "block", blockID.String(), "changes_required", strings.Join(changesRequired, ","))


I'd suggest to add "user" into all log messages here. (Or use ulogger := log.With(c.logger, "user", user) use wrapped ulogger only in entire method.)

pstibrany · 2021-02-09T09:37:31Z

tools/thanosconvert/thanosconvert.go

+func convertMetadata(meta metadata.Meta, expectedUser string) (metadata.Meta, []string) {
+	var changesRequired []string
+
+	if meta.Thanos.Labels == nil {


No need for this check or creating of empty meta.Thanos.Labels, since it will be replaced anyway. Reading from nil map, or deleting from nil map will work as expected. (I'd suggest adding unit test for that)

pstibrany · 2021-02-09T09:43:12Z

cmd/thanosconvert/main.go

+	logger, err := log.NewPrometheusLogger(loglvl, logfmt)
+	if err != nil {
+		fmt.Printf("failed to create logger: %v\n", err)
+		flag.Usage()


Printing usage hides the error message. I'd suggest removing flag.Usage() calls after print errors.

pstibrany · 2021-02-09T09:44:09Z

cmd/thanosconvert/main.go

+	if configFilename != "" {
+		buf, err := ioutil.ReadFile(configFilename)
+		if err != nil {
+			level.Error(logger).Log("msg", "failed to load config file", "err", err, "filename", configFilename)


This methods uses three different ways to print errors: logger, fmt.Printf (stdout) and fmt.Fprintf(flag.Commandline.Output()) which uses stderr. I'd suggest to print errors followed by os.Exit(1) to stderr.

pstibrany · 2021-02-09T09:47:09Z

cmd/thanosconvert/main.go

+		os.Exit(1)
+	}
+
+	fmt.Println("Results:")


We use logger for logging the progress in Run method. Perhaps we should use logger for logging the result too?

pstibrany · 2021-02-09T09:49:08Z

cmd/thanosconvert/main.go

+
+	iterCtx := context.Background()
+	results, err := converter.Run(iterCtx)
+	if err != nil {


There may be some results already when this error happens. Should this code simply log results first, and error (if any) as last?

milesbxf · 2021-02-10T09:41:28Z

Thanks for addressing my feedback. I've left few more (non-blocking) comments to take a look at, if you want.

Thanks @pstibrany - I've addressed some of these 🙏 I probably won't be able to spend much more time on this, so let me know if there's anything blocking the merge you'd like me to address

* Add user field to log lines during bucket iteration * Remove flag.Usage from errors * Remove logging from main function; Fprintf to stderr instead Signed-off-by: Miles Bryant <milesbryant@monzo.com>

pracucci · 2021-02-10T17:11:23Z

Thanks a lot @milesbxf for your hard work!

I probably won't be able to spend much more time on this, so let me know if there's anything blocking the merge you'd like me to address

Sure. It's a great tool, let's ship it! 🚀

pull-request-size bot added the size/XL label Feb 1, 2021

milesbxf force-pushed the thanosconvert branch from 047a8f2 to d33bf34 Compare February 1, 2021 14:49

milesbxf changed the title ~~thanosconvert: add tool to convert Thanos meta.json~~ thanosconvert: convert block meta from Thanos to Cortex Feb 1, 2021

milesbxf force-pushed the thanosconvert branch from d33bf34 to a8b8f2f Compare February 1, 2021 15:13

milesbxf marked this pull request as ready for review February 1, 2021 15:17

milesbxf force-pushed the thanosconvert branch from a8b8f2f to f55d19c Compare February 1, 2021 15:22

pracucci reviewed Feb 1, 2021

View reviewed changes

milesbxf force-pushed the thanosconvert branch from 7e6f56d to 85057b3 Compare February 2, 2021 10:35

milesbxf requested a review from pracucci February 2, 2021 11:00

milesbxf force-pushed the thanosconvert branch 3 times, most recently from 8659187 to e17a6fb Compare February 4, 2021 10:03

milesbxf added 4 commits February 4, 2021 10:03

Add documentation, fix logging flags and add usage string

e6fef01

Signed-off-by: Miles Bryant <milesbryant@monzo.com>

Add changelog entry

97e872b

Signed-off-by: Miles Bryant <milesbryant@monzo.com>

milesbxf force-pushed the thanosconvert branch from e17a6fb to 97e872b Compare February 4, 2021 10:04

milesbxf force-pushed the thanosconvert branch from 0d46028 to 6c1246e Compare February 4, 2021 15:31

Use moved NewPrometheusLogger

20ee863

Signed-off-by: Miles Bryant <milesbryant@monzo.com>

milesbxf force-pushed the thanosconvert branch from 6c1246e to 20ee863 Compare February 4, 2021 15:37

pracucci approved these changes Feb 5, 2021

View reviewed changes

pracucci requested a review from pstibrany February 5, 2021 10:50

pstibrany approved these changes Feb 5, 2021

View reviewed changes

milesbxf force-pushed the thanosconvert branch from e858b9c to c51e10d Compare February 5, 2021 12:27

milesbxf force-pushed the thanosconvert branch from c51e10d to 1d9bfe3 Compare February 5, 2021 12:29

milesbxf requested a review from pstibrany February 5, 2021 12:46

pstibrany approved these changes Feb 9, 2021

View reviewed changes

Address further comments

ad5bc30

* Add user field to log lines during bucket iteration * Remove flag.Usage from errors * Remove logging from main function; Fprintf to stderr instead Signed-off-by: Miles Bryant <milesbryant@monzo.com>

milesbxf force-pushed the thanosconvert branch from f9f2348 to ad5bc30 Compare February 10, 2021 09:41

pracucci merged commit 532a320 into cortexproject:master Feb 10, 2021

milesbxf deleted the thanosconvert branch February 18, 2021 11:30

	level.Error(util.Logger).Log("msg", "iterate blocks", "err", err)
	level.Error(util.Logger).Log("msg", "error while iterating blocks", "err", err)

		metaKey := fmt.Sprintf("%s/%s/meta.json", user, blockID)
		r, err := c.bkt.Get(ctx, metaKey)

thanosconvert: convert block meta from Thanos to Cortex #3770

thanosconvert: convert block meta from Thanos to Cortex #3770

Uh oh!

Conversation

milesbxf commented Feb 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milesbxf commented Feb 2, 2021

Uh oh!

milesbxf commented Feb 4, 2021

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

milesbxf commented Feb 5, 2021

Uh oh!

milesbxf commented Feb 5, 2021

Uh oh!

milesbxf commented Feb 5, 2021

Uh oh!

pstibrany commented Feb 5, 2021

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

milesbxf commented Feb 1, 2021 •

edited

Loading