Wait for owned series recomputation before lowering local series limit #7411

pr00se · 2024-02-16T21:57:04Z

What this PR does

The existing owned series implementation prevents discards when the local series limit is lowered due to a shard size increase. It does this by caching the current shard size until after the owned series are recomputed, only using the new (higher) shard size once the computation successfully completes.

However, it does not prevent discards when the lowered local limit is due to an increase in ingester count (e.g.: when shuffle sharding is disabled); the lower local limit takes effect immediately, and samples are discarded until the next owned series computation (at most ingester.owned-series-update-interval seconds later). This is due to the limiter fetching the ingester count from the ring lifecycler directly, meaning it picks up changes before the owned series service has a change to run and update the owned series count.

To address this, this PR caches the calculated local series limit on each tenant's userDB whenever owned series are recomputed, and uses this value as a minimum when calculating local limits during push requests. Because the cached value functions as a minimum, local limits are able to increase instantaneously, but can't be lowered until after the next owned series computation.

Summary of changes:

pkg/ingester/limiter.go: limiter takes minLocalLimit as a parameter instead of userShardSize when calculating local limits. This is 0 for every limit except for the user series limit.
pkg/ingester/user_tsdb.go: when we recompute the owned series count, we get the local series limit from the limiter and cache this value on the userDB. This value is passed as a minimum to the limiter when checking the series limit in PreCreation.
pkg/ingester/owned_series.go: added a new owned series recompute reason, recomputeOwnedSeriesReasonLocalLimitChanged, which is used when the user's calculated local series limit changes without any change to ingester or shard counts. This means that we'll recompute the owned series whenever ONLY the global series limit for a user changes, but this is necessary to correctly enforce lowered series limits (without this it wouldn't be possible to have a lowered series limit take effect until the user's actual subring changed).

The bulk of the changes, by line count, are to tests:

added the option to pass the ingesterRing to the ingester creation helpers in ingester_test.go
added two e2e tests for owned series, checking for discards when local limits are lowered by shard and ingester count increases
added an array of tests for the owned series service, both for when recomputation is triggered, and the limiting behavior

Which issue(s) this PR fixes or relates to

Fixes #5578

Checklist

Tests updated.
Documentation added.
CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
about-versioning.md updated with experimental features.

jdbaldry

Documentation changes look good to me.

pracucci

Good job! The logic changes make sense to me, but I haven't reviewed tests yet. Also @pstibrany is more familiar with all such work, so would be great if you could review it too. Tomorrow I will review tests.

pkg/ingester/user_tsdb.go

pracucci · 2024-02-19T17:40:26Z

pkg/ingester/limiter.go

@@ -135,7 +137,7 @@ func (l *Limiter) convertGlobalToLocalLimit(userShardSize int, globalLimit int)
 	// Global limit is equally distributed among all the active zones.
 	// The portion of global limit related to each zone is then equally distributed
 	// among all the ingesters belonging to that zone.
-	return int((float64(globalLimit*l.replicationFactor) / float64(zonesCount)) / float64(ingestersInZoneCount))
+	return max(int((float64(globalLimit*l.replicationFactor)/float64(zonesCount))/float64(ingestersInZoneCount)), minLocalLimit)


[nit] May be slightly easier to read:

Suggested change

return max(int((float64(globalLimit*l.replicationFactor)/float64(zonesCount))/float64(ingestersInZoneCount)), minLocalLimit)

desiredLocalLimit := int((float64(globalLimit*l.replicationFactor)/float64(zonesCount))/float64(ingestersInZoneCount))

return max(desiredLocalLimit, minLocalLimit)

I would actually suggest moving this into convertGlobalToLocalLimitOrUnlimited. This is not part of "conversion", and in my PR #7424 I'm introducing different conversion when partition-ring is used.

Moved the max up a level in 8ce33a75fdf7a0e5b0801e6be16fa6e392defdb5

pracucci · 2024-02-19T18:00:11Z

pkg/ingester/user_tsdb.go

@@ -289,8 +294,8 @@ func (u *userTSDB) PreCreation(metric labels.Labels) error {
 	}

 	// Total series limit.
-	series, shards := u.getSeriesAndShardsForSeriesLimit()
-	if !u.limiter.IsWithinMaxSeriesPerUser(u.userID, series, shards) {
+	series, minLimit := u.getSeriesAndMinForSeriesLimit()


[nit] I suggest to be consistent with naming. It's called minLocalLimit in other places and I suggest to call it minLocalLimit here as well (it's clearer and keep consistency).

changed in 82e5e8229ddce202770927c7eda43a06a502ac63

pracucci · 2024-02-19T18:00:40Z

pkg/ingester/user_tsdb.go

 // series limit.
-func (u *userTSDB) getSeriesAndShardsForSeriesLimit() (int, int) {
+func (u *userTSDB) getSeriesAndMinForSeriesLimit() (int, int) {


[nit] WDYT?

Suggested change

func (u *userTSDB) getSeriesAndMinForSeriesLimit() (int, int) {

func (u *userTSDB) getSeriesCountAndMinLocalLimit() (int, int) {

renamed in 5e79268c2ca3ab4227e0ff563420f507342c5dd0

pracucci

I also reviewed the tests. Nice job on unit tests! Not super excited about all the sleeps in the integration tests and I'm wondering if these new tests are really necessary (looks your unit tests are relatively high level anyway) or we can improve the integration tests to remove some of the sleeps.

pracucci · 2024-02-20T08:29:12Z

pkg/ingester/limiter_test.go

@@ -419,6 +419,7 @@ func TestLimiter_AssertMaxSeriesPerUser(t *testing.T) {
 		ringReplicationFactor  int


[nit] Can you rename the test function to TestLimiter_IsWithinMaxSeriesPerUser() given that's what we test here?

done in cbd21dcd289e38b358f65326a809a4b1ef992849. I also renamed the other tests that were similarly misnamed.

pracucci · 2024-02-20T09:15:21Z

pkg/ingester/owned_series_test.go

+				// initial limit
+				c.checkCalculatedLocalLimit(t, ownedServiceTestUserSeriesLimit, "")
+
+				// we'd normally scale up all zones at once, but doing it one by one lets us


we'd normally scale up all zones at once

Scaling is never atomic. Ingesters are added to the ring at different times. So even if we increase the replicas count of different zones at different times, the new ingesters will register to the ring at slightly different time. That's just to say that your test LGTM, but I would clarify this comment to match what happens in reality.

Same comment applies below.

These tests were deleted

pracucci · 2024-02-20T09:21:54Z

pkg/ingester/owned_series_test.go

+	for name, tc := range testCases {
+		t.Run(name, func(t *testing.T) {


These tests are a bit slow. WDYT parallelising them?

Suggested change

for name, tc := range testCases {

t.Run(name, func(t *testing.T) {

for name, tc := range testCases {

tc := tc

t.Run(name, func(t *testing.T) {

t.Parallel()

done in 53836caef913ecb8305c79dd22d18c675a2ce8d6

pracucci · 2024-02-20T09:30:07Z

integration/ingester_limits_test.go

+	gen.Wait()
+
+	// Wait for owned series service to run and metrics to get updated
+	// Metrics are updated only every 15 seconds :/


Where is the 15s hardcoded?

Is that referring to 15s default value in -ingester.owned-series-update-interval?

We hard-code it here:

mimir/pkg/ingester/ingester.go

Line 667 in 16877ec

limitMetricsUpdateTicker := time.NewTicker(time.Second * 15)

pstibrany

Great work, I've left some minor comments and suggestions.

CHANGELOG.md

pkg/ingester/user_tsdb.go

pstibrany · 2024-02-20T09:53:09Z

pkg/ingester/limiter.go

@@ -135,7 +137,7 @@ func (l *Limiter) convertGlobalToLocalLimit(userShardSize int, globalLimit int)
 	// Global limit is equally distributed among all the active zones.
 	// The portion of global limit related to each zone is then equally distributed
 	// among all the ingesters belonging to that zone.
-	return int((float64(globalLimit*l.replicationFactor) / float64(zonesCount)) / float64(ingestersInZoneCount))
+	return max(int((float64(globalLimit*l.replicationFactor)/float64(zonesCount))/float64(ingestersInZoneCount)), minLocalLimit)


I would actually suggest moving this into convertGlobalToLocalLimitOrUnlimited. This is not part of "conversion", and in my PR #7424 I'm introducing different conversion when partition-ring is used.

pkg/ingester/owned_series.go

pkg/ingester/owned_series_test.go

pstibrany · 2024-02-20T11:04:30Z

pkg/ingester/owned_series_test.go

+				// wait for the ingester to see the updated ring
+				test.Poll(t, 2*c.ringHeartbeatPeriod, 2, func() interface{} {
+					return c.ing.lifecycler.InstancesCount()
+				})


Can we add these waits into register methods? Simple idea is to check instances count before registration, and after, and check if it's before+1. It would simplify tests a bit.

Removed these checks entirely, since the limiter no longer uses the lifecycler

pstibrany · 2024-02-20T11:06:10Z

pkg/ingester/owned_series_test.go

-			c.updateOwnedSeriesAndCheckResult(t, false, 1, recomputeOwnedSeriesReasonShardSizeChanged)
-			c.checkUpdateReasonForUser(t, "")
-			c.checkUserSeriesOwnedAndShardsByTestedIngester(t, ownedServiceSeriesCount/2, 2)
+		"shard size = 0, scale ingesters up and down": {


or shard size = 100

Should i run a separate test with shard size >> number of ingesters?

Maybe just for completeness, to make sure it doesn't break in the future.

pkg/ingester/owned_series_test.go

pstibrany · 2024-02-20T11:21:35Z

pkg/ingester/owned_series_test.go

@@ -374,6 +756,529 @@ func TestOwnedSeriesRingChanged(t *testing.T) {
 	})
 }

+func TestOwnedSeriesLimiting(t *testing.T) {


Is this test useful? Limiter already has its own limits, and we also check computed min-local limit in TestOwnedSeriesService test.

Agreed -- deleted the test

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

…iesPerUser_WithPartitionsRing

Dismissing my own review because of recent changes that I still have to review

pstibrany

Great work, thank you!

pstibrany · 2024-02-28T09:17:22Z

pkg/ingester/limiter_test.go

 			require.NoError(t, err)

-			strategy := newPartitionRingLimiterStrategy(&partitionRingHolder{pr: pr}, limits.IngestionTenantShardSize)
+			strategy := newPartitionRingLimiterStrategy(&partitionRingHolder{pr: pr}, limits.IngestionPartitionsTenantShardSize)


Thanks for catching this bug.

pr00se force-pushed the owned-series-limiting branch 4 times, most recently from e9e1b92 to 4edbf9b Compare February 17, 2024 17:11

pr00se changed the title ~~Owned series limiting~~ Wait for owned series recomputation before lowering local series limit Feb 18, 2024

pr00se marked this pull request as ready for review February 18, 2024 23:13

pr00se requested review from a team and jdbaldry as code owners February 18, 2024 23:13

jdbaldry approved these changes Feb 19, 2024

View reviewed changes

pracucci reviewed Feb 19, 2024

View reviewed changes

pracucci previously approved these changes Feb 20, 2024

View reviewed changes

pstibrany reviewed Feb 20, 2024

View reviewed changes

This was referenced Feb 20, 2024

Modify ingester's limiter to work with ingest store. #7424

Merged

Expose InstancesInZoneCount and ZonesCount from ring.ReadRing. grafana/dskit#494

Merged

pr00se mentioned this pull request Feb 21, 2024

Switch ingester limiter to use ingesterRing rather than lifecycler #7440

Merged

4 tasks

pstibrany force-pushed the owned-series-limiting branch from cbd21dc to 8a02111 Compare February 23, 2024 17:09

pr00se force-pushed the owned-series-limiting branch from 47144ff to 1a953b5 Compare February 27, 2024 07:55

pr00se added 13 commits February 27, 2024 10:31

Owned series limits e2e tests

a7ec018

Owned series limits unit tests

0e45c15

Cache the local limit when calculating owned series

9f9398f

Recompute owned series when local limit changes

5a9a7b2

Multi-zone tests

d93bbc5

Shorten heartbeat polling timeout

5f39b37

Add skipped tests for known error condition

42922af

Lint for the lint god

eb3379d

Address flusher tests where limiter is nil

218136b

Move unnecessary mock limiter from tests

9a001cc

Update experimental features

02f72fb

Update CHANGELOG

4fb7ba9

getSeriesAndMinForSeriesLimit -> getSeriesCountAndMinLocalLimit

9906ca4

pr00se and others added 15 commits February 27, 2024 10:32

Apply minimum local limit in convertGlobalToLocalLimitOrUnlimited

6a06d5a

Address code review feedback

aa7d1fb

Rename struct fields

c8ca012

Rename limiter tests

7dff1d8

Fix nil rings.

19bd6da

Signed-off-by: Peter Štibraný <pstibrany@gmail.com>

User IngestionPartitionsTenantShardSize in TestLimiter_IsWithinMaxSer…

76a5b1b

…iesPerUser_WithPartitionsRing

Fix lint error

1520b86

Instantiate owned series service after limiter

002dd87

Remove unused function

e6f516d

Don't wait for lifecycler in owned series tests

3a9d0a6

Update limiter tests

1a636f6

Consolidate some owned series tests

e443d6a

Add test for WAL replay trigger

3a73d02

Run tests in parallel

85e65bf

Drop multi-zone tests

acc9668

pr00se force-pushed the owned-series-limiting branch 2 times, most recently from d3d975d to acc9668 Compare February 27, 2024 15:38

pr00se added 3 commits February 27, 2024 16:55

Make integration tests faster

2611ad5

Remove integration tests because they're flaky

179c5e2

Run tests serially to avoid race condition

8aab11f

pracucci self-requested a review February 28, 2024 07:19

pstibrany approved these changes Feb 28, 2024

View reviewed changes

pr00se merged commit 6190ed0 into main Feb 29, 2024
29 checks passed

pr00se deleted the owned-series-limiting branch February 29, 2024 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for owned series recomputation before lowering local series limit #7411

Wait for owned series recomputation before lowering local series limit #7411

pr00se commented Feb 16, 2024 •

edited

jdbaldry left a comment

pracucci left a comment

pracucci Feb 19, 2024

pstibrany Feb 20, 2024

pr00se Feb 20, 2024

pracucci Feb 19, 2024

pr00se Feb 21, 2024

pracucci Feb 19, 2024

pr00se Feb 20, 2024

pracucci left a comment

pracucci Feb 20, 2024

pr00se Feb 21, 2024

pracucci Feb 20, 2024

pracucci Feb 20, 2024

pr00se Feb 27, 2024

pracucci Feb 20, 2024

pr00se Feb 27, 2024

pracucci Feb 20, 2024

pstibrany Feb 20, 2024

pr00se Feb 20, 2024

pstibrany left a comment

pstibrany Feb 20, 2024

pstibrany Feb 20, 2024

pr00se Feb 27, 2024

pstibrany Feb 20, 2024

pr00se Feb 27, 2024

pstibrany Feb 28, 2024

pstibrany Feb 20, 2024

pr00se Feb 27, 2024

pstibrany left a comment

pstibrany Feb 28, 2024

	return max(int((float64(globalLimit*l.replicationFactor)/float64(zonesCount))/float64(ingestersInZoneCount)), minLocalLimit)
	desiredLocalLimit := int((float64(globalLimit*l.replicationFactor)/float64(zonesCount))/float64(ingestersInZoneCount))
	return max(desiredLocalLimit, minLocalLimit)

	func (u *userTSDB) getSeriesAndMinForSeriesLimit() (int, int) {
	func (u *userTSDB) getSeriesCountAndMinLocalLimit() (int, int) {

		@@ -419,6 +419,7 @@ func TestLimiter_AssertMaxSeriesPerUser(t *testing.T) {
		ringReplicationFactor int

		for name, tc := range testCases {
		t.Run(name, func(t *testing.T) {

Wait for owned series recomputation before lowering local series limit #7411

Wait for owned series recomputation before lowering local series limit #7411

Conversation

pr00se commented Feb 16, 2024 • edited

What this PR does

Which issue(s) this PR fixes or relates to

Checklist

jdbaldry left a comment

Choose a reason for hiding this comment

pracucci 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pracucci 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

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

pstibrany 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pstibrany left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pr00se commented Feb 16, 2024 •

edited