Allow to demote master vttablet to SPARE instead of REPLICA #5520

kalfonso · 2019-12-06T02:12:59Z

In our setups we use SPARE tablets to take over MASTER role during reparents or failures. Currently shard sync and external reparent demote MASTER to REPLICA. This change allows to configure the tablet type of a demoted master. SPARE and REPLICA are the only ones allowed. REPLICA is the default one if flag is not set. This can also be achieved with a boolean flag if only SPARE and REPLICA make sense as the only tablet types that can be promoted to master

Signed-off-by: Karel Alfonso Sague kalfonso@squareup.com

go/vt/vttablet/tabletmanager/action_agent.go

deepthi · 2019-12-06T04:13:28Z

go/vt/vttablet/tabletmanager/rpc_external_reparent.go

@@ -171,7 +171,7 @@ func (agent *ActionAgent) finalizeTabletExternallyReparented(ctx context.Context
 			// Call UpdateTabletFields instead of ChangeType so that we can check the type
 			// before changing it and avoid unnecessary topo updates
 			var err error
-			oldMasterTablet, err = topotools.ChangeType(ctx, agent.TopoServer, oldMasterAlias, topodatapb.TabletType_REPLICA, nil)
+			oldMasterTablet, err = topotools.ChangeType(ctx, agent.TopoServer, oldMasterAlias, agent.DemoteMasterType, nil)


Once all components are upgraded to 4.0.0+, this RPC doesn't get called any more. It is here for backwards compatibility during rollouts. It's best to not make any changes to it.

Done, reverted changes in this RPC path

deepthi · 2019-12-06T04:14:12Z

go/vt/wrangler/testlib/rpc_reparent_external_test.go

@@ -589,6 +592,50 @@ func TestRPCTabletExternallyReparentedRerun(t *testing.T) {

 }

+func TestRPCTabletExternallyReparentedDemotesMasterToConfiguredTabletType(t *testing.T) {


This test becomes obsolete along with the RPC. The new test should go into https://github.com/vitessio/vitess/blob/master/go/vt/wrangler/testlib/external_reparent_test.go

deepthi

LGTM. We'll let @enisoc take a look before approving.

enisoc

@deepthi Do we not need to update any other demotion-related code paths? Does this really cover all types of internal/external reparent? This almost feels too easy. :)

enisoc · 2019-12-06T22:14:28Z

go/vt/vttablet/tabletmanager/action_agent.go

@@ -252,6 +254,11 @@ func NewActionAgent(
 		return nil, err
 	}

+	tabletType, err := validateDemoteMasterType(*demoteMasterType)


The var name tabletType might mislead a reader to think it's the type that will be used at startup (since this is inside NewActionAgent). Let's call it something like demoteMasterTabletType.

makes sense, done!

enisoc · 2019-12-06T22:35:36Z

go/vt/vttablet/tabletmanager/action_agent.go

@@ -849,3 +870,12 @@ func (agent *ActionAgent) withRetry(ctx context.Context, description string, wor
 		}
 	}
 }
+
+func validateDemoteMasterType(tabletTypeParam string) (*topodatapb.TabletType, error) {


We treat topodatapb.TabletType as a scalar type, so we usually pass it by value:

Suggested change

func validateDemoteMasterType(tabletTypeParam string) (*topodatapb.TabletType, error) {

func validateDemoteMasterType(tabletTypeParam string) (topodatapb.TabletType, error) {

enisoc · 2019-12-06T22:40:13Z

go/vt/vttablet/tabletmanager/action_agent.go

+
+func validateDemoteMasterType(tabletTypeParam string) (*topodatapb.TabletType, error) {
+	tabletType, err := topoproto.ParseTabletType(tabletTypeParam)
+	if err != nil && (tabletType != topodatapb.TabletType_SPARE && tabletType != topodatapb.TabletType_REPLICA) {


It looks like you meant for that first && to be an ||. However, even then I don't think it will do the right thing. That might return a nil err when there's actually a problem.

I think what we want is:

if err != nil { return topodatapb.TabletType_UNKNOWN, err } if tabletType != topodatapb.TabletType_SPARE && tabletType != topodatapb.TabletType_REPLICA { return topodatapb.TabletType_UNKNOWN, fmt.Errorf("invalid demote_master_type %v; can only be REPLICA or SPARE", tabletType) } return tabletType, nil

oh, fixed. Using unknown tablet type as suggested.

enisoc · 2019-12-06T22:42:31Z

go/vt/wrangler/testlib/external_reparent_test.go

+			}
+			t.Fatalf("old master (%v) should be replica but is: %v", topoproto.TabletAliasString(oldMaster.Tablet.Alias), tablet.Type)
+		}
+		// check the old master was converted to replica


Suggested change

// check the old master was converted to replica

// check the old master was converted to spare

kalfonso · 2019-12-08T23:35:15Z

@enisoc @deepthi: One other area that will be impacted by this change is when checking whether a tablet is eligible to become master rpc_replication.go:781, especially when fixing semi sync replication.

I think under semi sync replication with two set of tablets SPARE and REPLICAS replicating but only allowing SPARE to become master, there could be a situation where a lagging SPARE (with respect to a REPLICA) will be promoted to MASTER causing a replica read to return must up-to-date value than the new master.

This is not a problem in our case as we use AWS Aurora and we run vttablets with semi sync disabled (default). I think demoting to a SPARE should be only allowed if semi sync is disabled to avoid the problem above. I might be missing more, please let me know.

tirsen · 2019-12-09T04:54:47Z

This flag is only safe to use together with --disable_active_reparents. You should add code that checks we are also setting that flag.

tirsen · 2019-12-11T00:14:12Z

go/vt/vttablet/tabletmanager/action_agent.go

+	if tabletType != topodatapb.TabletType_SPARE && tabletType != topodatapb.TabletType_REPLICA {
+		return topodatapb.TabletType_UNKNOWN, fmt.Errorf("invalid demote_master_type %v; can only be REPLICA or SPARE", tabletType)
+	}
+	if tabletType == topodatapb.TabletType_SPARE && !*mysqlctl.DisableActiveReparents {


I think you should check for any value that is not REPLICA. I.e. you should not able to use this flag at all unless you also set --disable_active_reparents

Can you check if it has been passed at all in the Golang flag API?

Done. Refactored to parse flag inside validation

In our setups we use SPARE tablets to take over MASTER role during reparents or failures. Currently shard sync and external reparent demote MASTER to REPLICA. This change allows to configure the tablet type of a demoted master. SPARE and REPLICA are the only ones allowed. REPLICA is the default one if flag is not set. This can also be achieved with a boolean flag if only SPARE and REPLICA make sense as the only tablet types that can be promoted to master Signed-off-by: Karel Alfonso Sague <kalfonso@squareup.com>

deepthi · 2019-12-18T03:53:50Z

@enisoc @deepthi: One other area that will be impacted by this change is when checking whether a tablet is eligible to become master rpc_replication.go:781, especially when fixing semi sync replication.

I think under semi sync replication with two set of tablets SPARE and REPLICAS replicating but only allowing SPARE to become master, there could be a situation where a lagging SPARE (with respect to a REPLICA) will be promoted to MASTER causing a replica read to return must up-to-date value than the new master.

This is not a problem in our case as we use AWS Aurora and we run vttablets with semi sync disabled (default). I think demoting to a SPARE should be only allowed if semi sync is disabled to avoid the problem above. I might be missing more, please let me know.

This problem existed even before we refactored the reparenting. Do you want to handle it in this PR, or leave it for another day? If you don't include it here, could you please create an issue?

deepthi

LGTM

deepthi · 2019-12-18T03:55:12Z

@morgo how do we get the github action statuses to update? they seem to be stuck.

sougou · 2019-12-20T03:42:28Z

Build is borked. Forcing this through.

kalfonso requested a review from sougou as a code owner December 6, 2019 02:12

kalfonso force-pushed the kalfonso.191205-demote-master-type branch 3 times, most recently from 78b0b2d to 12d46db Compare December 6, 2019 04:35

deepthi reviewed Dec 6, 2019

View reviewed changes

deepthi requested a review from enisoc December 6, 2019 04:37

kalfonso force-pushed the kalfonso.191205-demote-master-type branch 2 times, most recently from f4d0535 to 0822e6f Compare December 6, 2019 06:04

deepthi reviewed Dec 6, 2019

View reviewed changes

enisoc reviewed Dec 6, 2019

View reviewed changes

kalfonso force-pushed the kalfonso.191205-demote-master-type branch from 0822e6f to 8d674bc Compare December 8, 2019 21:24

kalfonso force-pushed the kalfonso.191205-demote-master-type branch from 8d674bc to bb93ade Compare December 11, 2019 00:06

tirsen reviewed Dec 11, 2019

View reviewed changes

kalfonso force-pushed the kalfonso.191205-demote-master-type branch from bb93ade to c2eec14 Compare December 11, 2019 00:36

deepthi approved these changes Dec 18, 2019

View reviewed changes

sougou merged commit 89199f2 into vitessio:master Dec 20, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to demote master vttablet to SPARE instead of REPLICA #5520

Allow to demote master vttablet to SPARE instead of REPLICA #5520

kalfonso commented Dec 6, 2019

deepthi Dec 6, 2019

kalfonso Dec 6, 2019

deepthi Dec 6, 2019

kalfonso Dec 6, 2019

deepthi left a comment

enisoc left a comment

enisoc Dec 6, 2019

kalfonso Dec 8, 2019

enisoc Dec 6, 2019

kalfonso Dec 8, 2019

enisoc Dec 6, 2019

kalfonso Dec 8, 2019

enisoc Dec 6, 2019

kalfonso Dec 8, 2019

kalfonso commented Dec 8, 2019

tirsen commented Dec 9, 2019

tirsen Dec 11, 2019

tirsen Dec 11, 2019

kalfonso Dec 11, 2019

deepthi commented Dec 18, 2019

deepthi left a comment

deepthi commented Dec 18, 2019

sougou commented Dec 20, 2019

		@@ -589,6 +592,50 @@ func TestRPCTabletExternallyReparentedRerun(t *testing.T) {

		}

		func TestRPCTabletExternallyReparentedDemotesMasterToConfiguredTabletType(t *testing.T) {

	func validateDemoteMasterType(tabletTypeParam string) (*topodatapb.TabletType, error) {
	func validateDemoteMasterType(tabletTypeParam string) (topodatapb.TabletType, error) {

	// check the old master was converted to replica
	// check the old master was converted to spare

Allow to demote master vttablet to SPARE instead of REPLICA #5520

Allow to demote master vttablet to SPARE instead of REPLICA #5520

Conversation

kalfonso commented Dec 6, 2019

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

deepthi left a comment

Choose a reason for hiding this comment

enisoc 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

kalfonso commented Dec 8, 2019

tirsen commented Dec 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepthi commented Dec 18, 2019

deepthi left a comment

Choose a reason for hiding this comment

deepthi commented Dec 18, 2019

sougou commented Dec 20, 2019