Add topology aware read #3414

MichelHollands · 2020-10-28T15:32:05Z

What this PR does:
Add zone awareness to the read path. A MaxUnavailableZones field is added to the ReplicationSet struct. The value of that field is derived from the number of zones.

In ReplicationSet.Do() the zones of the of the failing requests is taken into account for zone aware requests. If too many zones have failures then the request fails.

Unit tests for the non-zone aware invocations of ReplicationSet.Do() are added as well.

Which issue(s) this PR fixes:
Fixes #

Checklist

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

CHANGELOG.md

pracucci

Good job @MichelHollands! I think we're on the good track. I left you many comments, but most are small, while GetAll() will need a bit more work.

That being said, I would be glad if you could add 1 integration test. I would test this scenario:

Start a cortex cluster with 6 ingesters and zone-awareness enabled
Push 100 series
Query back 100 series > all good
SIGKILL 1 ingester in 1 zone
Query back 100 series > all good
SIGKILL 1 more ingester in the same zone
Query back 100 series > all good
SIGKILL 1 more ingester in a different zone
Query back 100 series > fail

pkg/ring/replication_set.go

pkg/ring/ring.go

CHANGELOG.md

pracucci

Thanks @MichelHollands for addressing my feedback! I left few more comments. Please make sure that any fix you do is also covered by a unit test to avoid any regression (either in this PR or future changes).

pkg/ring/replication_set.go

pkg/ring/ring.go

pracucci

I just realised that there are places where triggering the zone-awareness logic in the GetAll() is not correct, because it will filter out even healthy instances (eg. NewRingServiceDiscovery(), distributor.GetAll(), etc..). Let's talk offline about this.

pkg/ring/ring.go

pstibrany

First review pass. I need to get better understanding of the tests.

pkg/ring/replication_set.go

pkg/ring/ring.go

pkg/ring/replication_set_test.go

pkg/ring/replication_set_tracker_test.go

pstibrany

Some initial comments. I still need to review TestReplicationSet_Do and integration test.

pstibrany · 2020-11-06T18:41:50Z

pkg/ring/ring.go

If RF=2 and there are two zones, then minSuccessZones will end up being 2. I would expect that we can actually tolerate one failing zone in such case.

Added a special case for rf=2 and nr of zones =2/

I'm actually not sure anymore if my previous statement is correct. Reason is that during write, we would accept it if we get single OK. But during read, we don't know which single zone has received the sample, so we need to ask both.

I'd check with @pracucci to be 100% sure. But now I think my previous comment here was incorrect.

What's the current status of this conversation?

DefaultReplicationStrategy.Filter() calculate minSuccess := (replicationFactor / 2) + 1. The minSuccess is the write quorum. If RF=2, then we need to write to both ingesters in order to succeed. Because of this, it's safe to have maxUnavailableZones=1 when RF=2.

I suggest changing the logic as follow:

maxUnavailableZones = minSuccessZones - 1

Tests should be updated accordingly.

What's the current status of this conversation?

I convinced myself that with RF=2, it is enough to write to single ingester. But that's not a majority 🤦

pkg/ring/ring.go

pkg/ring/replication_set.go

pkg/ring/replication_set_tracker.go

pkg/ring/ring_test.go

pkg/ring/replication_set_test.go

Signed-off-by: Michel Hollands <[email protected]>

Co-authored-by: Peter Štibraný <[email protected]> Signed-off-by: Michel Hollands <[email protected]>

Signed-off-by: Michel Hollands <[email protected]>

Signed-off-by: Marco Pracucci <[email protected]>

Signed-off-by: Michel Hollands <[email protected]>

pkg/ring/replication_set_test.go

Signed-off-by: Michel Hollands <[email protected]>

pstibrany

Thank you!

docs/guides/zone-replication.md

pracucci · 2020-11-10T12:55:41Z

pkg/ring/ring.go

What's the current status of this conversation?

DefaultReplicationStrategy.Filter() calculate minSuccess := (replicationFactor / 2) + 1. The minSuccess is the write quorum. If RF=2, then we need to write to both ingesters in order to succeed. Because of this, it's safe to have maxUnavailableZones=1 when RF=2.

I suggest changing the logic as follow:

maxUnavailableZones = minSuccessZones - 1

Tests should be updated accordingly.

Signed-off-by: Michel Hollands <[email protected]>

pracucci

Thanks @MichelHollands for patiently addressing my feedback. Well done, LGTM! 👏

ranton256 · 2020-11-13T01:55:33Z

integration/e2e/service.go

 	logger.Log("Killing", s.name)

-	if out, err := RunCommandAndGetOutput("docker", "stop", "--time=0", s.containerName()); err != nil {
+	if out, err := RunCommandAndGetOutput("docker", "kill", s.containerName()); err != nil {


Why is this changing from stop to kill?

The Kill() function, used by end-to-end tests, is expected to send a SIGKILL. We were using docker stop --time=0, which is expected to send a SIGTERM and then a SIGKILL after time 0, but we wondered if there could be some timing issues and a graceful shutdown of the stopped process could actually happen (or at least start). To make it more obvious we're going to send a SIGKILL, we decided to just use docker kill.

ranton256 · 2020-11-13T02:11:39Z

pkg/ring/ring.go

+			// 1 failing ingester. Due to how replication works when zone-awareness is
+			// enabled (data is replicated to RF different zones), there's no benefit in
+			// querying healthy instances from "failing zones". A zone is considered
+			// failed if there is single error.


I don't follow the logic behind this.
If I have 1 failed replica in zone A, and 1 failed in zone B, and none failed in zone C, won't this cause an outage for some timeseries when there would have been none in some cases(i.e. the instance in A and the instance in B didn't host a replica of any of the same series). With this logic of removing everything from a zone if there is a failure, won't this make it more likely to have an outage in some cases?

It depends.

Assuming RF=3, if -distributor.shard-by-all-labels is disabled, then this function is not used and we only query the 3 ingesters holding all the series for the queried metric (and we just need 2 success out of 3 queried ingesters).

However, if -distributor.shard-by-all-labels is enabled, we don't know which ingesters hold all the series for a given metric name, so we need to query all of them. When zone-awareness is disabled, we need to query all ingester minus 1, while when zone-awareness is enabled we need to query all ingesters in all zones minus 1 (thus tolerating all ingesters in 1 zone failing).

About shuffle sharding, this function is called after the subring has been selected.

Am I missing anything?

Okay, I follow now, and am convinced the logic is correct, but maybe less enthusiastic about some of the implications, but since the value of all ingesters when -distributor.shard-by-all-labels is enabled is the subring seems more reasonable.

pkg/ring/ring.go

* Add unit tests Signed-off-by: Michel Hollands <[email protected]> * Add lock to avoid race condition in test Signed-off-by: Michel Hollands <[email protected]> * Add zone aware to GetAll Signed-off-by: Michel Hollands <[email protected]> * Remove debug print statements Signed-off-by: Michel Hollands <[email protected]> * Add changelog entry Signed-off-by: Michel Hollands <[email protected]> * Fix comment Signed-off-by: Michel Hollands <[email protected]> * Address replication set review comments Signed-off-by: Michel Hollands <[email protected]> * Reword and change changelong entry to enhancement Signed-off-by: Michel Hollands <[email protected]> * Address review comments in ring code Signed-off-by: Michel Hollands <[email protected]> * Do not return early and add more test cases Signed-off-by: Michel Hollands <[email protected]> * Rename ingesters to instances Signed-off-by: Michel Hollands <[email protected]> * Add one more test Signed-off-by: Michel Hollands <[email protected]> * Update pkg/ring/replication_set.go Co-authored-by: Marco Pracucci <[email protected]> Signed-off-by: Michel Hollands <[email protected]> * Update pkg/ring/replication_set.go : add sign off Signed-off-by: Michel Hollands <[email protected]> * Add integration test Signed-off-by: Michel Hollands <[email protected]> * Fix imports as per goimports Signed-off-by: Michel Hollands <[email protected]> * Address review comments and add extra tests Signed-off-by: Michel Hollands <[email protected]> * Fix rebase Signed-off-by: Michel Hollands <[email protected]> * Fix rebase in test Signed-off-by: Michel Hollands <[email protected]> * Add lock around mockIngester call Signed-off-by: Michel Hollands <[email protected]> * Add lock around mockIngester call at correct place Signed-off-by: Michel Hollands <[email protected]> * Handle nr of zones > replication factor Signed-off-by: Michel Hollands <[email protected]> * Use util.Min instead of if statement Signed-off-by: Michel Hollands <[email protected]> * Update pkg/ring/replication_set_test.go Co-authored-by: Peter Štibraný <[email protected]> Signed-off-by: Michel Hollands <[email protected]> * Use atomic and sets Signed-off-by: Michel Hollands <[email protected]> * Fixed integration test and ReplicationSet.Do() Signed-off-by: Marco Pracucci <[email protected]> * Added tracker unit tests Signed-off-by: Marco Pracucci <[email protected]> * Fixed TestReplicationSet_Do Signed-off-by: Marco Pracucci <[email protected]> * Commented ReplicationSet max errors and max unavailable zones Signed-off-by: Marco Pracucci <[email protected]> * Fixed GetReplicationSetForOperation() logic and improved unit tests Signed-off-by: Marco Pracucci <[email protected]> * Improved tests Signed-off-by: Marco Pracucci <[email protected]> * Fixed tests flakyness Signed-off-by: Marco Pracucci <[email protected]> * Fixed test Signed-off-by: Marco Pracucci <[email protected]> * Update documentation Signed-off-by: Michel Hollands <[email protected]> * Add note about reads from zone aware clusters Signed-off-by: Michel Hollands <[email protected]> * Remove extra space Signed-off-by: Michel Hollands <[email protected]> * Address some of Peter's review comment Signed-off-by: Michel Hollands <[email protected]> * Add special case for rf=2 Signed-off-by: Michel Hollands <[email protected]> * Address review comments Signed-off-by: Michel Hollands <[email protected]> * Fix comment Signed-off-by: Michel Hollands <[email protected]> * Update docs with review comments Signed-off-by: Michel Hollands <[email protected]> * Set maxUnavailableZones and change tests Signed-off-by: Michel Hollands <[email protected]> Co-authored-by: Marco Pracucci <[email protected]> Co-authored-by: Peter Štibraný <[email protected]>

pull-request-size bot added the size/L label Oct 28, 2020

csmarchbanks reviewed Oct 28, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

MichelHollands force-pushed the add_topology_aware_read branch from 9deb837 to 0bf804a Compare October 29, 2020 09:28

MichelHollands marked this pull request as ready for review October 29, 2020 10:11

pracucci reviewed Oct 30, 2020

View reviewed changes

pull-request-size bot added size/XL and removed size/L labels Nov 3, 2020

MichelHollands force-pushed the add_topology_aware_read branch from ebf35f9 to 6cb4418 Compare November 3, 2020 16:49

pracucci reviewed Nov 3, 2020

View reviewed changes

MichelHollands force-pushed the add_topology_aware_read branch 6 times, most recently from 56f66cb to 05dad8c Compare November 4, 2020 09:26

pracucci reviewed Nov 4, 2020

View reviewed changes

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

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

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

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

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

pracucci mentioned this pull request Nov 4, 2020

Split ReadRing.GetAll() into two functions #3460

Merged

3 tasks

MichelHollands force-pushed the add_topology_aware_read branch from 51e087e to fed940f Compare November 4, 2020 17:10

pstibrany mentioned this pull request Nov 5, 2020

Add read-path zone aware replication support #3138

Closed

pstibrany reviewed Nov 5, 2020

View reviewed changes

pkg/ring/replication_set.go Outdated Show resolved Hide resolved

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

pkg/ring/replication_set_test.go Outdated Show resolved Hide resolved

MichelHollands force-pushed the add_topology_aware_read branch from 5ddb6d6 to fe09cca Compare November 5, 2020 15:39

pull-request-size bot added size/XXL and removed size/XL labels Nov 6, 2020

MichelHollands commented Nov 6, 2020

View reviewed changes

pkg/ring/replication_set_tracker_test.go Outdated Show resolved Hide resolved

pstibrany reviewed Nov 6, 2020

View reviewed changes

MichelHollands added 5 commits November 9, 2020 09:01

Add unit tests

741f9b2

Signed-off-by: Michel Hollands <[email protected]>

Add lock to avoid race condition in test

91917fd

Signed-off-by: Michel Hollands <[email protected]>

Add zone aware to GetAll

ba3d860

Signed-off-by: Michel Hollands <[email protected]>

Remove debug print statements

90d9013

Signed-off-by: Michel Hollands <[email protected]>

Add changelog entry

440d0fc

Signed-off-by: Michel Hollands <[email protected]>

MichelHollands and others added 13 commits November 9, 2020 09:01

Use util.Min instead of if statement

7f08749

Signed-off-by: Michel Hollands <[email protected]>

Update pkg/ring/replication_set_test.go

847dd71

Co-authored-by: Peter Štibraný <[email protected]> Signed-off-by: Michel Hollands <[email protected]>

Use atomic and sets

7886450

Signed-off-by: Michel Hollands <[email protected]>

Fixed integration test and ReplicationSet.Do()

ccdb908

Signed-off-by: Marco Pracucci <[email protected]>

Added tracker unit tests

1259ed9

Signed-off-by: Marco Pracucci <[email protected]>

Fixed TestReplicationSet_Do

3facb35

Signed-off-by: Marco Pracucci <[email protected]>

Commented ReplicationSet max errors and max unavailable zones

c788f17

Signed-off-by: Marco Pracucci <[email protected]>

Fixed GetReplicationSetForOperation() logic and improved unit tests

833eb89

Signed-off-by: Marco Pracucci <[email protected]>

Improved tests

3351321

Signed-off-by: Marco Pracucci <[email protected]>

Fixed tests flakyness

a87411b

Signed-off-by: Marco Pracucci <[email protected]>

Fixed test

9902101

Signed-off-by: Marco Pracucci <[email protected]>

Update documentation

c7d39e9

Signed-off-by: Michel Hollands <[email protected]>

Add note about reads from zone aware clusters

75a5dc6

Signed-off-by: Michel Hollands <[email protected]>

MichelHollands force-pushed the add_topology_aware_read branch from 9d50812 to 75a5dc6 Compare November 9, 2020 09:02

MichelHollands added 3 commits November 9, 2020 09:28

Remove extra space

2647294

Signed-off-by: Michel Hollands <[email protected]>

Address some of Peter's review comment

ee08cab

Signed-off-by: Michel Hollands <[email protected]>

Add special case for rf=2

dd18b80

Signed-off-by: Michel Hollands <[email protected]>

pstibrany reviewed Nov 9, 2020

View reviewed changes

pkg/ring/replication_set_test.go Outdated Show resolved Hide resolved

MichelHollands added 2 commits November 9, 2020 17:34

Address review comments

97ea59b

Signed-off-by: Michel Hollands <[email protected]>

Fix comment

474168f

Signed-off-by: Michel Hollands <[email protected]>

pstibrany approved these changes Nov 10, 2020

View reviewed changes

pracucci reviewed Nov 10, 2020

View reviewed changes

MichelHollands added 2 commits November 10, 2020 13:50

Update docs with review comments

1e50276

Signed-off-by: Michel Hollands <[email protected]>

Set maxUnavailableZones and change tests

af5ff2a

Signed-off-by: Michel Hollands <[email protected]>

pracucci approved these changes Nov 10, 2020

View reviewed changes

ranton256 reviewed Nov 13, 2020

View reviewed changes

ranton256 approved these changes Nov 17, 2020

View reviewed changes

pracucci merged commit a04b547 into cortexproject:master Nov 18, 2020

pracucci mentioned this pull request Jun 18, 2021

Zone-aware ring checks can fail when an entire zone is unavailable #4291

Open

Add topology aware read #3414

Add topology aware read #3414

Uh oh!

Conversation

MichelHollands commented Oct 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pracucci left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pracucci Nov 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstibrany left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

MichelHollands commented Oct 28, 2020 •

edited

Loading

pstibrany left a comment •

edited

Loading

pracucci Nov 10, 2020 •

edited

Loading

pracucci Nov 10, 2020 •

edited

Loading