Add Retries to Sync Blocks

[LeviHarrison]

What this PR does:

Adds retries to BucketStore.SyncBlocks with BucketStores.syncUsersBlocksWithRetries.

Although this fixes one part of #3802, I think that the bigger issue may be the initial sync (BucketStores.InitialSync), which also ends up calling BucketStore.SyncBlocks, but after going through BucketStore.InitialSync in Thanos. Maybe retries should be added there in some fashion.

cortex/pkg/storegateway/bucket_stores.go

Lines 138 to 143 in c28d326

	func (u *BucketStores) InitialSync(ctx context.Context) error {
	level.Info(u.logger).Log("msg", "synchronizing TSDB blocks for all users")
	if err := u.syncUsersBlocks(ctx, func(ctx context.Context, s *store.BucketStore) error {
	return s.InitialSync(ctx)
	}); err != nil {

cortex/vendor/github.com/thanos-io/thanos/pkg/store/bucket.go

Lines 463 to 466 in c28d326

	func (s *BucketStore) InitialSync(ctx context.Context) error {
	if err := s.SyncBlocks(ctx); err != nil {
	return errors.Wrap(err, "sync block")
	}

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

Checklist

• Tests updated
• Documentation added
• CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX]

[pstibrany]

This will return error on first failure, and will not do any retry. Better version would be:

for retries.Ongoing() {
    err = s.SyncBlocks(ctx)
    if err == nil {
        return nil
    }
    // log error otherwise
    retries.Wait()
}
return retries.Err()

[pracucci]

I'm not sure about this design. We pass our own function to u.syncUsersBlocks(). I was expecting something like this:

return u. syncUsersBlocksWithRetries(ctx, func(ctx context.Context, s *store.BucketStore) error {
  return s.SyncBlocks(ctx)
}

And then having syncUsersBlocksWithRetries() calling syncUsersBlocks().

Am I missing anything?

[LeviHarrison]

Nope. I'll get on it!

[LeviHarrison]

CI seems to be having some issues:

/usr/bin/docker pull quay.io/cortexproject/build-image:upgrade-build-image-debian-491e60715-WIP
  Error response from daemon: Head https://quay.io/v2/cortexproject/build-image/manifests/upgrade-build-image-debian-491e60715-WIP: received unexpected HTTP status: 502 Bad Gateway
  Warning: Docker pull failed with exit code 1, back off 9.622 seconds before retry.

This might have something to do with it: https://status.quay.io/incidents/vfs19hmq660h

Image pulls from quay.io are currently failing with the code "502 Bad Gateway".

[pracucci]

Thanks for addressing my previous comment! The new design looks way more clear. I just left a comment about the returned error and then we're good to go 🚀

[pracucci]

[nit] it's the same but more clear:

Suggested change

	return err
	return nil

[pracucci]

This would suppress the actual error. I would save the last error returned by u.syncUsersBlocks(ctx, f) and return it once all retries expired (eg. see joinMembersOnStartup() at pkg/ring/kv/memberlist/memberlist_client.go for reference).

[LeviHarrison]

I think this is probably a flakey test, given that I changed almost nothing the latest commit.

alertmanager_test.go:337: 
         	Error Trace:	alertmanager_test.go:337
         	Error:      	        Received unexpected error:
         	            	        metric not found
                                        ...
                Test:       	        TestAlertmanagerSharding/legacy_alertstore

[pracucci]

LGTM, thanks!

[pstibrany]

Nit: Given that maxRetries is 3, we can never reach max backoff of 10 seconds. :)

[LeviHarrison]

Ah ok I originally stole that from here and just modified it along the way. Is a 1-second MaxBackoff ok?

[pstibrany]

It's ok to keep it as is. If you choose to use 1s as MaxBackoff, that's fine too. I think it's more important to do retry here, than how much time it waits.

[pstibrany]

Thank you!