Revocation phase using durability policies #9659

GuptaManan100 · 2022-02-09T17:40:53Z

Description

This PR adds the ability to check for revocation using the durability policies in EmergencyReparentShard. What that means is that instead of exiting incase of more than 1 failure, ERS will first try and find whether it is safe to proceed even when we have multiple failures!
So, if the durability_policies allow for more than 1 failure, ERS will succeed!

Consider an example for the same. Let us say we are using semi_sync as our durability policy and we have four servers, A, B, C and D. A is our current primary, B and C are replicas and D is a rdonly tablet.
Lets say that A and D, both go down.

In the older version, ERS would have failed since there are more than 1 failures
But now, ERS will see that even with A and D down, we can proceed safely. Safety, here means that we can guarantee that the unreachable (equivalent to down) tablets cannot make forward progress. In this case, only A is eligible to be a primary and D cannot provide Semi_Sync ACKs, which means that A and D cannot make progress, so it is safe to continue with the ERS.

Related Issue(s)

RFC - Durability and consensus in Vtorc #8975

Checklist

Should this PR be backported? No
Tests were added or are not required
Documentation was added or is not required

Deployment Notes

…emiSyncAcks for a given primary Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…event a tablet from making forward progress Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…o know that no primary capable tablet can accept any new write Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…ds additional tests Signed-off-by: Manan Gupta <manan@planetscale.com>

…ds when 2 tablets are failing Signed-off-by: Manan Gupta <manan@planetscale.com>

…lures Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

…rwise they start overriding each others durability policies configuration Signed-off-by: Manan Gupta <manan@planetscale.com>

Signed-off-by: Manan Gupta <manan@planetscale.com>

deepthi

Comments from a first pass. I still need to understand the new Revoke funcs.

go/test/endtoend/reparent/utils/utils.go

go/vt/vtctl/reparentutil/durability.go

go/vt/vtctl/reparentutil/durability_policies.go

go/vt/vtctl/reparentutil/durability.go

go/vt/vtctl/reparentutil/replication.go

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 · 2022-02-21T04:10:34Z

@deepthi I have renamed the files back so it is easier to review and added the requested comments in the durabler interface.

go/vt/vtctl/reparentutil/durability.go

go/vt/vtctl/reparentutil/durability_funcs.go

go/vt/vtctl/reparentutil/replication.go

Signed-off-by: Manan Gupta <manan@planetscale.com>

ajm188

just some naming/testing suggestions! i think the rest is looking good :)

go/vt/topo/topoproto/tablet.go

go/vt/topo/topoproto/tablet_test.go

ajm188 · 2022-02-24T19:13:04Z

go/vt/vtctl/reparentutil/durability_funcs.go

+}
+
+// RevokeForTablet checks whether we have reached enough tablets such that the given primary eligible tablet cannot accept any new writes
+func RevokeForTablet(primaryEligible *topodatapb.Tablet, tabletsReached []*topodatapb.Tablet, allTablets []*topodatapb.Tablet) bool {


for these functions that check and return bools, i'd suggest renaming to from <Thing> to Is<Thing> to make them read a little nicer where they are used

How do you feel about, haveRevokedForTablet? instead of isRevokedForTablet?

I have made the change for haveRevokedForTablet. If you feel the other name is better, I'll rename it

fine by me! as long as it implies the boolean/question nature, and we're consistent with what we use for both, i'm happy :)

ajm188 · 2022-02-24T19:13:44Z

go/vt/vtctl/reparentutil/durability_funcs.go

+}
+
+// Revoked checks whether we have reached enough tablets to guarantee that no tablet eligible to become a primary can accept any write
+func Revoked(tabletsReached []*topodatapb.Tablet, allTablets []*topodatapb.Tablet) bool {


same naming suggestion here

Changed it to haveRevoked

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 · 2022-02-25T07:12:43Z

Thankyou @deepthi and @ajm188 for the reviews! I have addressed all the review comments, could you take a look once, again?

ajm188 · 2022-02-25T14:32:30Z

lgtm, but let's get deepthi's approval (and clean up the commit history length) before merging

deepthi · 2022-02-25T20:04:30Z

lgtm, but let's get deepthi's approval (and clean up the commit history length) before merging

Do you mean squash some of the commits together?
If so, I recommend squashing only the last 5 (since we started review) either into one, or into whatever is the most relevant older commit. Typically we don't squash at all (though I'm well aware that this makes certain things like reverts and bisects that much harder).

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added 10 commits February 8, 2022 15:49

feat: implement a function which returns the tablets which can send S…

310f618

…emiSyncAcks for a given primary Signed-off-by: Manan Gupta <manan@planetscale.com>

test: add test for SemiSyncACKersForPrimary

df6783c

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add a function to check if we have reached enough tablets to pr…

928321f

…event a tablet from making forward progress Signed-off-by: Manan Gupta <manan@planetscale.com>

refactor: rename variables to match previous code pattern

9713e0d

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: add a function which checks if we have reached enough tablets t…

f4178d7

…o know that no primary capable tablet can accept any new write Signed-off-by: Manan Gupta <manan@planetscale.com>

refactor: fix comments to mention writes

65252c6

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: prevent ERS from failing in case we are able to revoke successfuly

83de3e6

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: fix existing tests for StopReplicationAndBuildStatusMaps and ad…

6b9dc88

…ds additional tests Signed-off-by: Manan Gupta <manan@planetscale.com>

test: add a unit test for reparentShardLocked checking that it succee…

5584b05

…ds when 2 tablets are failing Signed-off-by: Manan Gupta <manan@planetscale.com>

test: add an end to end test running ers even in case of multiple fai…

5236dc4

…lures Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added Type: Enhancement Logical improvement (somewhere between a bug and feature) Component: Cluster management Component: VTorc Vitess Orchestrator integration release notes labels Feb 9, 2022

GuptaManan100 requested review from deepthi and rafael as code owners February 9, 2022 17:40

GuptaManan100 requested review from ajm188, harshit-gangal and sougou February 9, 2022 17:41

GuptaManan100 force-pushed the durability-policy branch from c18aef5 to 5236dc4 Compare February 11, 2022 05:23

GuptaManan100 added 5 commits February 11, 2022 12:29

test: print the mysqlctl log files in case of failure in startup

e34038e

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: add teardown to tests missing it

1fb5940

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: cannot run tests that use durability policies in parallel, othe…

f3cd986

…rwise they start overriding each others durability policies configuration Signed-off-by: Manan Gupta <manan@planetscale.com>

Merge remote-tracking branch 'upstream/main' into durability-policy

5faa597

Signed-off-by: Manan Gupta <manan@planetscale.com>

Merge remote-tracking branch 'upstream/main' into durability-policy

50e3a1c

Signed-off-by: Manan Gupta <manan@planetscale.com>

deepthi reviewed Feb 18, 2022

View reviewed changes

GuptaManan100 added 4 commits February 18, 2022 14:55

Merge remote-tracking branch 'upstream/main' into durability-policy

ac6f8c2

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: fix merge issues

ce96bb1

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: move panic prevention code to higher level of abstraction

2aea514

Signed-off-by: Manan Gupta <manan@planetscale.com>

docs: fix comment

c0bf71d

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 added 3 commits February 18, 2022 17:27

feat: improve error message

5db6040

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: fix unit tests

2c5d3c1

Signed-off-by: Manan Gupta <manan@planetscale.com>

test: remove the usage of t.Fatalf

5b8faad

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 force-pushed the durability-policy branch from bd7f315 to 5b8faad Compare February 18, 2022 12:02

GuptaManan100 added 2 commits February 21, 2022 09:32

refactor: rename files back to make reviewing easier

cdff5c1

Signed-off-by: Manan Gupta <manan@planetscale.com>

docs: add comments in the durabler interface explaining the functions

f7dad7d

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 requested a review from deepthi February 21, 2022 04:11

deepthi reviewed Feb 22, 2022

View reviewed changes

docs: fix comments

8021678

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 requested a review from deepthi February 23, 2022 05:26

GuptaManan100 mentioned this pull request Feb 23, 2022

Filter candidates which cannot make forward progress after ERS #9765

Merged

3 tasks

ajm188 reviewed Feb 24, 2022

View reviewed changes

GuptaManan100 added 2 commits February 25, 2022 12:04

Merge remote-tracking branch 'upstream/main' into durability-policy

5dffcb1

Signed-off-by: Manan Gupta <manan@planetscale.com>

feat: address review comments and rename functions

daee352

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 requested a review from ajm188 February 25, 2022 07:11

deepthi approved these changes Feb 25, 2022

View reviewed changes

docs: fix typing error in comments

9ecfb30

Signed-off-by: Manan Gupta <manan@planetscale.com>

GuptaManan100 merged commit 2024c03 into vitessio:main Mar 1, 2022

GuptaManan100 deleted the durability-policy branch March 1, 2022 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revocation phase using durability policies #9659

Revocation phase using durability policies #9659

GuptaManan100 commented Feb 9, 2022

deepthi left a comment

GuptaManan100 commented Feb 21, 2022

ajm188 left a comment

ajm188 Feb 24, 2022

GuptaManan100 Feb 25, 2022

GuptaManan100 Feb 25, 2022

ajm188 Feb 25, 2022

ajm188 Feb 24, 2022

GuptaManan100 Feb 25, 2022

GuptaManan100 commented Feb 25, 2022

ajm188 commented Feb 25, 2022

deepthi commented Feb 25, 2022

Revocation phase using durability policies #9659

Revocation phase using durability policies #9659

Conversation

GuptaManan100 commented Feb 9, 2022

Description

Related Issue(s)

Checklist

Deployment Notes

deepthi left a comment

Choose a reason for hiding this comment

GuptaManan100 commented Feb 21, 2022

ajm188 left a comment

Choose a reason for hiding this comment

ajm188 Feb 24, 2022

Choose a reason for hiding this comment

GuptaManan100 Feb 25, 2022

Choose a reason for hiding this comment

GuptaManan100 Feb 25, 2022

Choose a reason for hiding this comment

ajm188 Feb 25, 2022

Choose a reason for hiding this comment

ajm188 Feb 24, 2022

Choose a reason for hiding this comment

GuptaManan100 Feb 25, 2022

Choose a reason for hiding this comment

GuptaManan100 commented Feb 25, 2022

ajm188 commented Feb 25, 2022

deepthi commented Feb 25, 2022