-
Notifications
You must be signed in to change notification settings - Fork 832
Shuffle-sharding of queriers in the query-frontend #3113
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
Merged
pstibrany
merged 29 commits into
cortexproject:master
from
pstibrany:query-frontend-sharding
Sep 17, 2020
Merged
Changes from all commits
Commits
Show all changes
29 commits
Select commit
Hold shift + click to select a range
9008e16
Added ID to the querier.
pstibrany 6947054
Extended frontend protocol to add request type.
pstibrany 78c3713
Frontend now asks querier for its ID before running process loop.
pstibrany 55d98fe
Close gRPC connection when stopping manager.
pstibrany 2629b98
Shuffle shard queriers between users.
pstibrany 96924dc
Added MaxQueriersPerUser to overrides.
pstibrany 2b9dcab
Fixes.
pstibrany d23a825
Fix querier.id default value.
pstibrany 813f2f2
CHANGELOG.md
pstibrany dd81676
Make lint happy.
pstibrany 31cc2d2
Fix protos.
pstibrany 0f0b343
Fixed bug in getNextRequestForQuerier, modified benchmarks to add que…
pstibrany 6ee9f37
Fixed spelling.
pstibrany 36caae2
Move metrics increment in register/unregister querier connection.
pstibrany b90323a
When select queriers for tenant, use shuffling.
pstibrany 736ca66
Use rand numbers to find queriers.
pstibrany cb9b616
Fixed docs.
pstibrany 45a1f7f
Add integration test for sharding queriers.
pstibrany c66e3da
Use shuffling for selecting queriers per user.
pstibrany 35b81e8
Updated documentation.
pstibrany 0927013
Fixed docs after rebase.
pstibrany 32820ec
Unify seed computation with subring PR.
pstibrany fde0808
Added unit test to verify that selected queriers are unique and come …
pstibrany 84217e6
Review feedback.
pstibrany 874a548
Mention shuffle sharding in v1 guarantees.
pstibrany 729cc27
Merge branch 'master' into query-frontend-sharding
pracucci 6501e26
Review feedback.
pstibrany 2058c50
Fix flag after master merge.
pstibrany 78cf6b0
Fixed TestQueuesConsistency test.
pstibrany File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,155 @@ | ||
// +build requires_docker | ||
|
||
package integration | ||
|
||
import ( | ||
"strconv" | ||
"sync" | ||
"testing" | ||
"time" | ||
|
||
"github.com/prometheus/common/model" | ||
"github.com/prometheus/prometheus/prompb" | ||
"github.com/stretchr/testify/assert" | ||
"github.com/stretchr/testify/require" | ||
|
||
"github.com/cortexproject/cortex/integration/e2e" | ||
e2ecache "github.com/cortexproject/cortex/integration/e2e/cache" | ||
e2edb "github.com/cortexproject/cortex/integration/e2e/db" | ||
"github.com/cortexproject/cortex/integration/e2ecortex" | ||
) | ||
|
||
func TestQuerierSharding(t *testing.T) { | ||
runQuerierShardingTest(t, true) | ||
} | ||
|
||
func TestQuerierNoSharding(t *testing.T) { | ||
runQuerierShardingTest(t, false) | ||
} | ||
|
||
func runQuerierShardingTest(t *testing.T, sharding bool) { | ||
// Going to high starts hitting filedescriptor limit, since we run all queriers concurrently. | ||
const numQueries = 100 | ||
|
||
s, err := e2e.NewScenario(networkName) | ||
require.NoError(t, err) | ||
defer s.Close() | ||
|
||
memcached := e2ecache.NewMemcached() | ||
consul := e2edb.NewConsul() | ||
require.NoError(t, s.StartAndWaitReady(consul, memcached)) | ||
|
||
minio := e2edb.NewMinio(9000, BlocksStorageFlags["-blocks-storage.s3.bucket-name"]) | ||
require.NoError(t, s.StartAndWaitReady(minio)) | ||
|
||
flags := BlocksStorageFlags | ||
|
||
flags = mergeFlags(flags, map[string]string{ | ||
"-querier.cache-results": "true", | ||
"-querier.split-queries-by-interval": "24h", | ||
"-querier.query-ingesters-within": "12h", // Required by the test on query /series out of ingesters time range | ||
"-frontend.memcached.addresses": "dns+" + memcached.NetworkEndpoint(e2ecache.MemcachedPort), | ||
"-querier.max-outstanding-requests-per-tenant": strconv.Itoa(numQueries), // To avoid getting errors. | ||
}) | ||
|
||
if sharding { | ||
// Use only single querier for each user. | ||
flags["-frontend.max-queriers-per-user"] = "1" | ||
} | ||
|
||
// Start Cortex components. | ||
queryFrontend := e2ecortex.NewQueryFrontendWithConfigFile("query-frontend", "", flags, "") | ||
ingester := e2ecortex.NewIngesterWithConfigFile("ingester", consul.NetworkHTTPEndpoint(), "", flags, "") | ||
distributor := e2ecortex.NewDistributorWithConfigFile("distributor", consul.NetworkHTTPEndpoint(), "", flags, "") | ||
|
||
require.NoError(t, s.Start(queryFrontend)) | ||
|
||
querier1 := e2ecortex.NewQuerierWithConfigFile("querier-1", consul.NetworkHTTPEndpoint(), "", mergeFlags(flags, map[string]string{ | ||
"-querier.frontend-address": queryFrontend.NetworkGRPCEndpoint(), | ||
}), "") | ||
querier2 := e2ecortex.NewQuerierWithConfigFile("querier-2", consul.NetworkHTTPEndpoint(), "", mergeFlags(flags, map[string]string{ | ||
"-querier.frontend-address": queryFrontend.NetworkGRPCEndpoint(), | ||
}), "") | ||
|
||
require.NoError(t, s.StartAndWaitReady(querier1, querier2, ingester, distributor)) | ||
require.NoError(t, s.WaitReady(queryFrontend)) | ||
|
||
// Wait until distributor and queriers have updated the ring. | ||
require.NoError(t, distributor.WaitSumMetrics(e2e.Equals(512), "cortex_ring_tokens_total")) | ||
require.NoError(t, querier1.WaitSumMetrics(e2e.Equals(512), "cortex_ring_tokens_total")) | ||
require.NoError(t, querier2.WaitSumMetrics(e2e.Equals(512), "cortex_ring_tokens_total")) | ||
|
||
// Push a series for each user to Cortex. | ||
now := time.Now() | ||
|
||
distClient, err := e2ecortex.NewClient(distributor.HTTPEndpoint(), "", "", "", userID) | ||
require.NoError(t, err) | ||
|
||
var series []prompb.TimeSeries | ||
series, expectedVector := generateSeries("series_1", now) | ||
|
||
res, err := distClient.Push(series) | ||
require.NoError(t, err) | ||
require.Equal(t, 200, res.StatusCode) | ||
|
||
// Send both queriers a single query, so that they both initialize their cortex_querier_request_duration_seconds metrics. | ||
for _, q := range []*e2ecortex.CortexService{querier1, querier2} { | ||
c, err := e2ecortex.NewClient("", q.HTTPEndpoint(), "", "", userID) | ||
require.NoError(t, err) | ||
|
||
_, err = c.Query("series_1", now) | ||
require.NoError(t, err) | ||
} | ||
|
||
wg := sync.WaitGroup{} | ||
|
||
// Run all queries concurrently to get better distribution of requests between queriers. | ||
for i := 0; i < numQueries; i++ { | ||
wg.Add(1) | ||
|
||
go func() { | ||
defer wg.Done() | ||
c, err := e2ecortex.NewClient("", queryFrontend.HTTPEndpoint(), "", "", userID) | ||
require.NoError(t, err) | ||
|
||
result, err := c.Query("series_1", now) | ||
require.NoError(t, err) | ||
require.Equal(t, model.ValVector, result.Type()) | ||
assert.Equal(t, expectedVector, result.(model.Vector)) | ||
}() | ||
} | ||
|
||
wg.Wait() | ||
|
||
require.NoError(t, queryFrontend.WaitSumMetrics(e2e.Equals(numQueries), "cortex_query_frontend_queries_total")) | ||
|
||
// Verify that only single querier handled all the queries when sharding is enabled, otherwise queries have been fairly distributed across queriers. | ||
q1Values, err := querier1.SumMetrics([]string{"cortex_querier_request_duration_seconds"}, e2e.WithMetricCount) | ||
require.NoError(t, err) | ||
require.Len(t, q1Values, 1) | ||
|
||
q2Values, err := querier2.SumMetrics([]string{"cortex_querier_request_duration_seconds"}, e2e.WithMetricCount) | ||
require.NoError(t, err) | ||
require.Len(t, q2Values, 1) | ||
|
||
total := q1Values[0] + q2Values[0] | ||
diff := q1Values[0] - q2Values[0] | ||
if diff < 0 { | ||
diff = -diff | ||
} | ||
|
||
require.Equal(t, float64(numQueries), total-2) // Remove 2 requests used for metrics initialization. | ||
|
||
if sharding { | ||
require.Equal(t, float64(numQueries), diff) | ||
} else { | ||
require.InDelta(t, 0, diff, numQueries*0.20) // Both queriers should have roughly equal number of requests, with possible delta. | ||
} | ||
|
||
// Ensure no service-specific metrics prefix is used by the wrong service. | ||
assertServiceMetricsPrefixes(t, Distributor, distributor) | ||
assertServiceMetricsPrefixes(t, Ingester, ingester) | ||
assertServiceMetricsPrefixes(t, Querier, querier1) | ||
assertServiceMetricsPrefixes(t, Querier, querier2) | ||
assertServiceMetricsPrefixes(t, QueryFrontend, queryFrontend) | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR for store-gateway shuffle sharding, a config called
sharding_strategy
was introduced. Is that something to be added here as well?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Store-gateways can use different sharding strategies. In case of queriers, no sharding was done before (which corresponds to using
max_queriers_per_user: 0
), and only shuffle-sharding is available (ifmax_queriers_per_user
is greater than 0). We can introduce sharding_strategy option too, but personally I don't see a need for it here.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Food for thought (not having a strong opinion): I agree it's not required here, but could help with config consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to be consistent with other config options, we should also support "default" (non-shuffle-sharding) strategy with non-zero shard size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep it as is for now. We marked this feature as experimental, which will allow us to eventually fine-tune the config before marking it stable.