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

Update Thanos to latest main #4585

Merged
merged 2 commits into from
Dec 14, 2021
Merged

Update Thanos to latest main #4585

merged 2 commits into from
Dec 14, 2021

Conversation

siggy
Copy link
Contributor

@siggy siggy commented Dec 9, 2021

Update Thanos dependency to include thanos-io/thanos#4928, to conserve memory.

Signed-off-by: Andrew Seigner andrew@sig.gy

What this PR does:

Bumps Thanos dependency to the latest main: thanos-io/thanos@d1acaea

Most notably this pulls in thanos-io/thanos#4928, which gives a 2x~4x memory reduction when using Azure blocks storage:

cortex thanos bump

This PR, combined with #4581, brings Cortex's memory usage back inline with v1.10.0.

Relates to:

Checklist

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

@siggy siggy changed the title Update Thanos to latest main. Update Thanos to latest main Dec 9, 2021
Update Thanos dependency to include thanos-io/thanos#4928, to conserve
memory.

Signed-off-by: Andrew Seigner <andrew@sig.gy>
@siggy siggy marked this pull request as ready for review December 9, 2021 23:38
@bboreham
Copy link
Contributor

Thanks for doing this. It's best to cover all important changes when updating a dependency, so I checked
thanos-io/thanos@de0e384...d1acaea
and I see that Redis is now a cache option. This sounds quite interesting; is it usable from Cortex now or does it need plumbing?

Please can you check the list and summarise all user-visible changes in the CHANGELOG. Just in a few words - it's not necessary to link to individual PRs besides this one where a keen reader can find all the details. Readers of CHANGELOG are assumed to be admins who don't want code-level details.

@@ -86,8 +83,8 @@ replace google.golang.org/grpc => google.golang.org/grpc v1.38.0
// We only pin this version to avoid problems with running go get: github.com/thanos-io/thanos@main. That
// currently fails because Thanos isn't merging release branches to main branch, and Go modules system is then
// confused about which version is the latest one. v0.22.0 was released in July, but latest tag reachable from main
// is v0.19.1. We pin version from late september here. Feel free to remove when updating to later version.
replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211122085937-de0e3848ff60
// is v0.19.1. We pin version from early December here. Feel free to remove when updating to later version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: is the pin still necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanos release tags are still not on the main branch. This replace pattern allowed me to simply put main on this line, and then go mod vendor correctly resolved it to v0.19.1-0.20211122085937-de0e3848ff60.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.
The comment above says it's about merging release branches, but this kinda looks like a merge of the release branch: thanos-io/thanos#4738
Did it not work? Not what Go is expecting?

Copy link
Contributor

Choose a reason for hiding this comment

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

That was a squash merge, not merge commit. It didn't make tag v0.23.1 reachable from main branch.

Copy link
Contributor

Choose a reason for hiding this comment

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

I tried to update release process instructions in Thanos at thanos-io/thanos#4770 to hopefully fix this situation in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for filing thanos-io/thanos#4770 @pstibrany!

Did it not work? Not what Go is expecting?

So we can do a go get on a release tag, and it works:

$ go get github.com/thanos-io/thanos@v0.24.0-rc.1
$ go mod vendor
$ git diff
...
-       github.com/thanos-io/thanos v0.22.0
+       github.com/thanos-io/thanos v0.24.0-rc.1
-replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211208205607-d1acaea2a11a
+// replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211208205607-d1acaea2a11a

... I don't know how important it is that we be able to do go get github.com/thanos-io/thanos@main, but that still doesn't work:

$ go get github.com/thanos-io/thanos@main
go: downloading github.com/thanos-io/thanos v0.19.1-0.20211213141611-c7a44e23febe
go get: github.com/thanos-io/thanos@main (v0.19.1-0.20211213141611-c7a44e23febe) requires github.com/thanos-io/thanos@v0.22.0, not github.com/thanos-io/thanos@main (v0.19.1-0.20211213141611-c7a44e23febe)

So I guess we have a few options:

  1. Leave as-is, wait for Add step for merging release branch back to main. thanos-io/thanos#4770.
  2. Remove the replace and the comment, require v0.24.0-rc.1 directly.
  3. Remove the replace and the comment, wait for v0.24.0 and require it directly.

@siggy
Copy link
Contributor Author

siggy commented Dec 13, 2021

I see that Redis is now a cache option. This sounds quite interesting; is it usable from Cortex now or does it need plumbing?

I think this would need plumbing. Cache instantation in Cortex:

func createCache(cacheName string, backend string, memcached MemcachedClientConfig, logger log.Logger, reg prometheus.Registerer) (cache.Cache, error) {
switch backend {
case "":
// No caching.
return nil, nil
case CacheBackendMemcached:
var client cacheutil.MemcachedClient
client, err := cacheutil.NewMemcachedClientWithConfig(logger, cacheName, memcached.ToMemcachedClientConfig(), reg)
if err != nil {
return nil, errors.Wrapf(err, "failed to create memcached client")
}
return cache.NewMemcachedCache(cacheName, logger, client, reg), nil
default:
return nil, errors.Errorf("unsupported cache type for cache %s: %s", cacheName, backend)
}
}

// NewIndexCache creates a new index cache based on the input configuration.
func NewIndexCache(cfg IndexCacheConfig, logger log.Logger, registerer prometheus.Registerer) (storecache.IndexCache, error) {
switch cfg.Backend {
case IndexCacheBackendInMemory:
return newInMemoryIndexCache(cfg.InMemory, logger, registerer)
case IndexCacheBackendMemcached:
return newMemcachedIndexCache(cfg.Memcached, logger, registerer)
default:
return nil, errUnsupportedIndexCacheBackend
}
}

... would need to call cacheutil.NewRedisClient in Thanos:
https://github.com/thanos-io/thanos/blob/9a26b0edee19a06c6e99a09e33ebceca734c91f9/pkg/cacheutil/redis_client.go#L122-L133

Interestingly the Thanos PR mentions Cortex's redis support as a reference implementation: thanos-io/thanos#4888

Signed-off-by: Andrew Seigner <andrew@sig.gy>
Copy link
Contributor

@bboreham bboreham left a comment

Choose a reason for hiding this comment

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

Thanks; LGTM

@@ -86,8 +83,8 @@ replace google.golang.org/grpc => google.golang.org/grpc v1.38.0
// We only pin this version to avoid problems with running go get: github.com/thanos-io/thanos@main. That
// currently fails because Thanos isn't merging release branches to main branch, and Go modules system is then
// confused about which version is the latest one. v0.22.0 was released in July, but latest tag reachable from main
// is v0.19.1. We pin version from late september here. Feel free to remove when updating to later version.
replace github.com/thanos-io/thanos v0.22.0 => github.com/thanos-io/thanos v0.19.1-0.20211122085937-de0e3848ff60
// is v0.19.1. We pin version from early December here. Feel free to remove when updating to later version.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.
The comment above says it's about merging release branches, but this kinda looks like a merge of the release branch: thanos-io/thanos#4738
Did it not work? Not what Go is expecting?

Copy link
Contributor

@pracucci pracucci left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@pracucci pracucci merged commit 07d6cfc into cortexproject:master Dec 14, 2021
alvinlin123 pushed a commit to ac1214/cortex that referenced this pull request Jan 14, 2022
* Update Thanos to latest main

Update Thanos dependency to include thanos-io/thanos#4928, to conserve
memory.

Signed-off-by: Andrew Seigner <andrew@sig.gy>

* Update changelog to summarize user-facing changes

Signed-off-by: Andrew Seigner <andrew@sig.gy>
Signed-off-by: Alvin Lin <alvinlin@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants