Added shipper capability to sync compacted blocks once. #731

bwplotka · 2019-01-14T13:23:37Z

Added shipper capability to sync compacted blocks once.

Changes:

Use shipper metrics properly.
Do not add compacted blocks to uploaded arr in thanos shipper meta if they are not uploaded.
Added flag for syncing compacted blocks once.
Enable working on different work dir.
Enable using snapshot API to avoid race conditions.
Ignore empty blocks.

Signed-off-by: Bartek Plotka [email protected]

jojohappy

Nice PR! LGTM! just some minor nits for me!

cmd/thanos/bucket.go

pkg/promclient/promclient.go

pkg/shipper/shipper.go

FUSAKLA

Great, ok from me just few minor suggestions or nits.

FUSAKLA · 2019-01-23T22:08:10Z

cmd/thanos/bucket.go

+		if *dataDir != "" {
+			level.Warn(logger).Log("msg", "is Prometheus operating on specified data dir? If yes, turn it's local compaction or leave dataDir empty to safely snapshot data to avoid races.")
+		}
+		level.Warn(logger).Log("msg", "have you turned off compactor? Currently it is unsafe to run compactor in the same time as this command! (y/N)")


maybe adding flag --force-yes would be good to allow automating?

on other hand, you can do the same with just yes command on unix, so let's skip this for now?

That's true, you are right we can skip it.

cmd/thanos/bucket.go

pkg/shipper/shipper.go

bwplotka · 2019-02-05T17:22:52Z

Moved from command to proper migration handling in sidecar. Tested that during FOSDEM demo. (:

FUSAKLA

Nice :) just few questions

FUSAKLA · 2019-02-06T19:55:57Z

pkg/shipper/shipper.go

@@ -51,13 +57,18 @@ func newMetrics(r prometheus.Registerer) *metrics {
 		Name: "thanos_shipper_upload_failures_total",
 		Help: "Total number of failed object uploads",
 	})
+	m.uploadedCompacted = prometheus.NewGauge(prometheus.GaugeOpts{
+		Name: "thanos_shipper_upload_compacted_done",
+		Help: "If 1 it means shipper uploaded all compacted blocks from the filesystem.",


Just out of curiosity what is the use-case for this metric?
It strikes me that shipping of compacted blocks is one time issue during migration of pure Prometheus setup.
In that case you should see it in the logs because you know it happens in this time and after that you will disable the compacted syncing I suppose?

Maybe I'm just missing the use-case?

Yes, at a minimum, this metric should only be registered if uploadCompacted is true.

Yea, but wanted just more flexible way to tell if that one operation is done or not....will register it only on that flag good point 👍

FUSAKLA · 2019-02-06T21:40:04Z

pkg/testutil/prometheus.go

+		return errors.New("method Start was not invoked.")
+	}
+	return runutil.Retry(time.Second, ctx.Done(), func() error {
+		r, err := http.Get(fmt.Sprintf("http://%s/metrics", p.addr))


Might be better to check the /-/ready endpoint? IIRC /metrics is ready sooner than the instance is ready to answer queries for example.

FUSAKLA · 2019-02-06T21:55:11Z

pkg/testutil/prometheus.go

+		return errors.Wrap(err, "failed to kill Prometheus. Kill it manually")
+	}
+
+	time.Sleep(time.Second / 2)


could we use p.wait() ? https://golang.org/pkg/os/#Process.Wait
in case of lazy stop running two instances on the same data could cause issues.

jojohappy

LGTM! Just a small question.

jojohappy · 2019-02-07T06:59:25Z

pkg/shipper/shipper.go

@@ -295,6 +457,7 @@ func hardlinkBlock(src, dst string) error {
 type Meta struct {
 	Version  int         `json:"version"`
 	Uploaded []ulid.ULID `json:"uploaded"`
+	Ignored  []ulid.ULID `json:"ignored"`


Where is this filed used?

It's not, good point!

jojohappy · 2019-02-07T07:20:18Z

pkg/block/metadata/meta.go

@@ -27,6 +27,7 @@ const (
 	CompactorRepairSource SourceType = "compactor.repair"
 	RulerSource           SourceType = "ruler"
 	BucketRepairSource    SourceType = "bucket.repair"
+	BucketSyncSource      SourceType = "bucket.sync"


Maybe it should be removed?

SuperQ · 2019-02-07T08:38:57Z

cmd/thanos/sidecar.go

@@ -51,6 +51,8 @@ func registerSidecar(m map[string]setupFunc, app *kingpin.Application, name stri

 	objStoreConfig := regCommonObjStoreFlags(cmd, "", false)

+	uploadCompacted := cmd.Flag("shipper.upload-compacted", "[Experimental] If true sidecar will try to upload compacted blocks as well. Useful for migration purposes. Works only if compaction is disabled on Prometheus.").Hidden().Bool()


We should probably be explicit and Default("false") here.

bwplotka · 2019-02-07T14:53:37Z

Addressed all comments @SuperQ @FUSAKLA

domgreen

LGTM - minor naming nits :)

domgreen · 2019-02-08T11:22:12Z

pkg/shipper/shipper.go

+// NewMigrating creates a new shipper that detects new TSDB blocks in dir and uploads them
+// to remote if necessary, including compacted blocks which are already in filesystem.
+// It attaches the Thanos metadata section in each meta JSON file.
+func NewMigrating(


nit - would update to NewWithCompacted its a shipper that also ships compacted blocks.

domgreen · 2019-02-08T12:09:51Z

pkg/shipper/shipper.go

+// If updload
+//
+// It is not concurrency-safe, however it is compactor-safe (running concurrently with compactor is ok)
+func (s *Shipper) Sync(ctx context.Context) (uploaded int, err error) {


nit - uploaded is used a few times in this func ... might be clearer to be something like uploadedCount

Mhm, I would stay with upload int - you can see integer so you can assume this?

domgreen · 2019-02-08T12:12:24Z

pkg/shipper/shipper.go

+	)
+	// Sync non compacted blocks first.
+	if err := s.iterBlockMetas(func(m *metadata.Meta) error {
+		// Do not sync a block if we already uploaded or ignored it. If it is no longer found in the bucket,


nit - remove it

which it? (:

I guess later - mgm not sure

Changes: * Use shipper metrics properly. * Do not add compacted blocks to `uploaded` arr in thanos shipper meta if they are not uploaded. * Added flag for syncing compacted blocks once. * Enable working on different work dir. * Enable using snapshot API to avoid race conditions. * Ignore empty blocks. Signed-off-by: Bartek Plotka <[email protected]>

bwplotka requested a review from domgreen January 14, 2019 13:23

bwplotka force-pushed the promclient-snapshot branch from 767d905 to 75a13c3 Compare January 14, 2019 14:03

bwplotka force-pushed the sync-command branch from f0b17e4 to 44fadad Compare January 14, 2019 14:05

bwplotka force-pushed the promclient-snapshot branch from 75a13c3 to 0e0b864 Compare January 14, 2019 14:10

jojohappy reviewed Jan 18, 2019

View reviewed changes

cmd/thanos/bucket.go Outdated Show resolved Hide resolved

cmd/thanos/bucket.go Outdated Show resolved Hide resolved

pkg/promclient/promclient.go Outdated Show resolved Hide resolved

pkg/shipper/shipper.go Outdated Show resolved Hide resolved

bwplotka changed the title ~~Added shipper capability to sync compacted blocks once. Hooked in a sync command.~~ [sync part4] Added shipper capability to sync compacted blocks once. Hooked in a sync command. Jan 21, 2019

bwplotka added the feature request/improvement label Jan 21, 2019

bwplotka force-pushed the promclient-snapshot branch from 0e0b864 to bbd0b52 Compare January 23, 2019 18:22

FUSAKLA reviewed Jan 23, 2019

View reviewed changes

pkg/shipper/shipper.go Outdated Show resolved Hide resolved

FUSAKLA approved these changes Jan 23, 2019

View reviewed changes

bwplotka force-pushed the promclient-snapshot branch from bbd0b52 to a8391db Compare January 30, 2019 13:32

bwplotka force-pushed the sync-command branch 2 times, most recently from 8c9de1f to d12b918 Compare January 30, 2019 13:48

bwplotka force-pushed the promclient-snapshot branch from a8391db to 8abd8d1 Compare January 30, 2019 13:51

bwplotka force-pushed the sync-command branch from d12b918 to abcc887 Compare January 30, 2019 13:53

bwplotka force-pushed the promclient-snapshot branch 4 times, most recently from 737cf23 to e15f983 Compare February 1, 2019 20:48

bwplotka changed the base branch from promclient-snapshot to master February 1, 2019 21:21

bwplotka force-pushed the sync-command branch 7 times, most recently from ad0c1fa to bbdbe00 Compare February 5, 2019 17:21

bwplotka changed the title ~~[sync part4] Added shipper capability to sync compacted blocks once. Hooked in a sync command.~~ Added shipper capability to sync compacted blocks once. Feb 5, 2019

bwplotka force-pushed the sync-command branch 3 times, most recently from 43fa7f3 to 17c496b Compare February 6, 2019 00:22

bwplotka mentioned this pull request Feb 6, 2019

Cut v0.3.0 release. #797

Merged

3 tasks

bwplotka added the priority: P0 label Feb 6, 2019

bwplotka force-pushed the sync-command branch from 17c496b to ca773c7 Compare February 6, 2019 17:16

FUSAKLA approved these changes Feb 6, 2019

View reviewed changes

jojohappy approved these changes Feb 7, 2019

View reviewed changes

SuperQ reviewed Feb 7, 2019

View reviewed changes

bwplotka force-pushed the sync-command branch from ca773c7 to bc57e25 Compare February 7, 2019 13:02

bwplotka force-pushed the sync-command branch from bc57e25 to 81f9257 Compare February 8, 2019 11:56

domgreen approved these changes Feb 8, 2019

View reviewed changes

bwplotka force-pushed the sync-command branch from 81f9257 to 99ca07f Compare February 8, 2019 12:31

bwplotka merged commit 9eb0944 into master Feb 8, 2019

bwplotka deleted the sync-command branch February 8, 2019 12:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added shipper capability to sync compacted blocks once. #731

Added shipper capability to sync compacted blocks once. #731

bwplotka commented Jan 14, 2019 •

edited

Loading

jojohappy left a comment

FUSAKLA left a comment

FUSAKLA Jan 23, 2019

bwplotka Jan 30, 2019

FUSAKLA Feb 6, 2019

bwplotka commented Feb 5, 2019

FUSAKLA left a comment

FUSAKLA Feb 6, 2019

SuperQ Feb 7, 2019

bwplotka Feb 7, 2019

FUSAKLA Feb 6, 2019

FUSAKLA Feb 6, 2019

jojohappy left a comment

jojohappy Feb 7, 2019

bwplotka Feb 7, 2019 •

edited

Loading

jojohappy Feb 7, 2019

bwplotka Feb 7, 2019

SuperQ Feb 7, 2019

bwplotka Feb 7, 2019

bwplotka commented Feb 7, 2019

domgreen left a comment

domgreen Feb 8, 2019

domgreen Feb 8, 2019

bwplotka Feb 8, 2019

domgreen Feb 8, 2019

bwplotka Feb 8, 2019

bwplotka Feb 8, 2019 •

edited

Loading

		@@ -51,6 +51,8 @@ func registerSidecar(m map[string]setupFunc, app *kingpin.Application, name stri

		objStoreConfig := regCommonObjStoreFlags(cmd, "", false)

		uploadCompacted := cmd.Flag("shipper.upload-compacted", "[Experimental] If true sidecar will try to upload compacted blocks as well. Useful for migration purposes. Works only if compaction is disabled on Prometheus.").Hidden().Bool()

Added shipper capability to sync compacted blocks once. #731

Added shipper capability to sync compacted blocks once. #731

Conversation

bwplotka commented Jan 14, 2019 • edited Loading

jojohappy left a comment

Choose a reason for hiding this comment

FUSAKLA 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

bwplotka commented Feb 5, 2019

FUSAKLA 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

jojohappy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwplotka Feb 7, 2019 • edited Loading

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 Feb 7, 2019

domgreen 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

bwplotka Feb 8, 2019 • edited Loading

Choose a reason for hiding this comment

bwplotka commented Jan 14, 2019 •

edited

Loading

bwplotka Feb 7, 2019 •

edited

Loading

bwplotka Feb 8, 2019 •

edited

Loading