Merge pull request #5647 from planetscale/ds-delete-tablet-fake-master

DeleteTablet: allow deletion of old master tablet without -allow_master

go/vt/wrangler/tablet.go

@@ -109,17 +109,18 @@ func (wr *Wrangler) DeleteTablet(ctx context.Context, tabletAlias *topodatapb.Ta
  if err != nil {
  return err
  }
- wasMaster := ti.Type == topodatapb.TabletType_MASTER
- if wasMaster && !allowMaster {
- return fmt.Errorf("cannot delete tablet %v as it is a master, use allow_master flag", topoproto.TabletAliasString(tabletAlias))
- }
 
- // remove the record and its replication graph entry
- if err := topotools.DeleteTablet(ctx, wr.ts, ti.Tablet); err != nil {
+ wasMaster, err := wr.isMasterTablet(ctx, ti)
+ if err != nil {
  return err
  }
 
+ if wasMaster && !allowMaster {
+ return fmt.Errorf("cannot delete tablet %v as it is a master, use allow_master flag", topoproto.TabletAliasString(tabletAlias))
+ }
+
  // update the Shard object if the master was scrapped.
+ // we do this before calling DeleteTablet so that the operation can be retried in case of failure.
  if wasMaster {
  // We lock the shard to not conflict with reparent operations.
  ctx, unlock, lockErr := wr.ts.LockShard(ctx, ti.Keyspace, ti.Shard, fmt.Sprintf("DeleteTablet(%v)", topoproto.TabletAliasString(tabletAlias)))
@@ -129,14 +130,20 @@ func (wr *Wrangler) DeleteTablet(ctx context.Context, tabletAlias *topodatapb.Ta
  defer unlock(&err)
 
  // update the shard record's master
- _, err = wr.ts.UpdateShardFields(ctx, ti.Keyspace, ti.Shard, func(si *topo.ShardInfo) error {
+ if _, err := wr.ts.UpdateShardFields(ctx, ti.Keyspace, ti.Shard, func(si *topo.ShardInfo) error {
  if !topoproto.TabletAliasEqual(si.MasterAlias, tabletAlias) {
  wr.Logger().Warningf("Deleting master %v from shard %v/%v but master in Shard object was %v", topoproto.TabletAliasString(tabletAlias), ti.Keyspace, ti.Shard, topoproto.TabletAliasString(si.MasterAlias))
  return topo.NewError(topo.NoUpdateNeeded, si.Keyspace()+"/"+si.ShardName())
  }
  si.MasterAlias = nil
  return nil
- })
+ }); err != nil {
+ return err
+ }
+ }
+
+ // remove the record and its replication graph entry
+ if err := topotools.DeleteTablet(ctx, wr.ts, ti.Tablet); err != nil {
  return err
  }
 
@@ -201,3 +208,34 @@ func (wr *Wrangler) VReplicationExec(ctx context.Context, tabletAlias *topodatap
  }
  return wr.tmc.VReplicationExec(ctx, ti.Tablet, query)
 }
+
+// isMasterTablet is a shortcut way to determine whether the current tablet
+// is a master before we allow its tablet record to be deleted. The canonical
+// way to determine the only true master in a shard is to list all the tablets
+// and find the one with the highest MasterTermStartTime among the ones that
+// claim to be master.
+// We err on the side of caution here, i.e. we should never return false for
+// a true master tablet, but it is ok to return true for a tablet that isn't
+// the true master. This can occur if someone issues a DeleteTablet while
+// the system is in transition (a reparenting event is in progress and parts of
+// the topo have not yet been updated).
+func (wr *Wrangler) isMasterTablet(ctx context.Context, ti *topo.TabletInfo) (bool, error) {
+ // Tablet record claims to be non-master, we believe it
+ if ti.Type != topodatapb.TabletType_MASTER {
+ return false, nil
+ }
+ si, err := wr.ts.GetShard(ctx, ti.Keyspace, ti.Shard)
+ if err != nil {
+ // strictly speaking it isn't correct to return false here, the tablet status is unknown
+ return false, err
+ }
+ // Tablet record claims to be master, and shard record matches
+ if topoproto.TabletAliasEqual(si.MasterAlias, ti.Tablet.Alias) {
+ return true, nil
+ }
+ // Shard record has another tablet as master, so check MasterTermStartTime
+ // If tablet record's MasterTermStartTime is later than the one in the shard record, then tablet is master
+ tabletMTST := logutil.ProtoToTime(ti.MasterTermStartTime)
+ shardMTST := logutil.ProtoToTime(si.MasterTermStartTime)
+ return tabletMTST.After(shardMTST), nil
+}

go/vt/wrangler/tablet_test.go

@@ -17,11 +17,13 @@ limitations under the License.
 package wrangler
 
 import (
+ "strings"
  "testing"
 
  "golang.org/x/net/context"
  "vitess.io/vitess/go/vt/logutil"
  topodatapb "vitess.io/vitess/go/vt/proto/topodata"
+ "vitess.io/vitess/go/vt/topo"
  "vitess.io/vitess/go/vt/topo/memorytopo"
 )
 
@@ -56,3 +58,123 @@ func TestInitTabletShardConversion(t *testing.T) {
  t.Errorf("Got wrong tablet.KeyRange, got %v expected 80-c0", ti.KeyRange)
  }
 }
+
+// TestDeleteTabletBasic tests delete of non-master tablet
+func TestDeleteTabletBasic(t *testing.T) {
+ cell := "cell1"
+ ts := memorytopo.NewServer(cell)
+ wr := New(logutil.NewConsoleLogger(), ts, nil)
+
+ tablet := &topodatapb.Tablet{
+ Alias: &topodatapb.TabletAlias{
+ Cell: cell,
+ Uid: 1,
+ },
+ Shard: "0",
+ }
+
+ if err := wr.InitTablet(context.Background(), tablet, false /*allowMasterOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
+ t.Fatalf("InitTablet failed: %v", err)
+ }
+
+ if _, err := ts.GetTablet(context.Background(), tablet.Alias); err != nil {
+ t.Fatalf("GetTablet failed: %v", err)
+ }
+
+ if err := wr.DeleteTablet(context.Background(), tablet.Alias, false); err != nil {
+ t.Fatalf("DeleteTablet failed: %v", err)
+ }
+}
+
+// TestDeleteTabletTrueMaster tests that you can delete a true master tablet
+// only if allowMaster is set to true
+func TestDeleteTabletTrueMaster(t *testing.T) {
+ cell := "cell1"
+ ts := memorytopo.NewServer(cell)
+ wr := New(logutil.NewConsoleLogger(), ts, nil)
+
+ tablet := &topodatapb.Tablet{
+ Alias: &topodatapb.TabletAlias{
+ Cell: cell,
+ Uid: 1,
+ },
+ Keyspace: "test",
+ Shard: "0",
+ Type: topodatapb.TabletType_MASTER,
+ }
+
+ if err := wr.InitTablet(context.Background(), tablet, false /*allowMasterOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
+ t.Fatalf("InitTablet failed: %v", err)
+ }
+ if _, err := ts.GetTablet(context.Background(), tablet.Alias); err != nil {
+ t.Fatalf("GetTablet failed: %v", err)
+ }
+
+ // set MasterAlias and MasterTermStartTime on shard to match chosen master tablet
+ if _, err := ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
+ si.MasterAlias = tablet.Alias
+ si.MasterTermStartTime = tablet.MasterTermStartTime
+ return nil
+ }); err != nil {
+ t.Fatalf("UpdateShardFields failed: %v", err)
+ }
+
+ err := wr.DeleteTablet(context.Background(), tablet.Alias, false)
+ wantError := "as it is a master, use allow_master flag"
+ if err == nil || !strings.Contains(err.Error(), wantError) {
+ t.Fatalf("DeleteTablet on master: want error = %v, got error = %v", wantError, err)
+ }
+
+ if err := wr.DeleteTablet(context.Background(), tablet.Alias, true); err != nil {
+ t.Fatalf("DeleteTablet failed: %v", err)
+ }
+}
+
+// TestDeleteTabletFalseMaster tests that you can delete a false master tablet
+// with allowMaster set to false
+func TestDeleteTabletFalseMaster(t *testing.T) {
+ cell := "cell1"
+ ts := memorytopo.NewServer(cell)
+ wr := New(logutil.NewConsoleLogger(), ts, nil)
+
+ tablet1 := &topodatapb.Tablet{
+ Alias: &topodatapb.TabletAlias{
+ Cell: cell,
+ Uid: 1,
+ },
+ Keyspace: "test",
+ Shard: "0",
+ Type: topodatapb.TabletType_MASTER,
+ }
+
+ if err := wr.InitTablet(context.Background(), tablet1, false /*allowMasterOverride*/, true /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
+ t.Fatalf("InitTablet failed: %v", err)
+ }
+
+ tablet2 := &topodatapb.Tablet{
+ Alias: &topodatapb.TabletAlias{
+ Cell: cell,
+ Uid: 2,
+ },
+ Keyspace: "test",
+ Shard: "0",
+ Type: topodatapb.TabletType_MASTER,
+ }
+ if err := wr.InitTablet(context.Background(), tablet2, true /*allowMasterOverride*/, false /*createShardAndKeyspace*/, false /*allowUpdate*/); err != nil {
+ t.Fatalf("InitTablet failed: %v", err)
+ }
+
+ // set MasterAlias and MasterTermStartTime on shard to match chosen master tablet
+ if _, err := ts.UpdateShardFields(context.Background(), "test", "0", func(si *topo.ShardInfo) error {
+ si.MasterAlias = tablet2.Alias
+ si.MasterTermStartTime = tablet2.MasterTermStartTime
+ return nil
+ }); err != nil {
+ t.Fatalf("UpdateShardFields failed: %v", err)
+ }
+
+ // Should be able to delete old (false) master with allowMaster = false
+ if err := wr.DeleteTablet(context.Background(), tablet1.Alias, false); err != nil {
+ t.Fatalf("DeleteTablet failed: %v", err)
+ }
+}