Vtctld SwitchReads: fix bug where writes were also being switched as …

…part of switching reads when all traffic was switched using SwitchTraffic (#14360) Signed-off-by: Rohit Nayak <[email protected]> Signed-off-by: Matt Lord <[email protected]> Co-authored-by: Matt Lord <[email protected]>
vitessio · Oct 27, 2023 · a4dbece · a4dbece
1 parent 28ceaa8
commit a4dbece
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 9 deletions.
diff --git a/go/vt/vtctl/workflow/server.go b/go/vt/vtctl/workflow/server.go
@@ -2940,9 +2940,18 @@ func (s *Server) WorkflowSwitchTraffic(ctx context.Context, req *vtctldatapb.Wor
 
 // switchReads is a generic way of switching read traffic for a workflow.
 func (s *Server) switchReads(ctx context.Context, req *vtctldatapb.WorkflowSwitchTrafficRequest, ts *trafficSwitcher, state *State, timeout time.Duration, cancel bool, direction TrafficSwitchDirection) (*[]string, error) {
-	roTypesToSwitchStr := topoproto.MakeStringTypeCSV(req.TabletTypes)
+	var roTabletTypes []topodatapb.TabletType
+	// When we are switching all traffic we also get the primary tablet type, which we need to
+	// filter out for switching reads.
+	for _, tabletType := range req.TabletTypes {
+		if tabletType != topodatapb.TabletType_PRIMARY {
+			roTabletTypes = append(roTabletTypes, tabletType)
+		}
+	}
+
+	roTypesToSwitchStr := topoproto.MakeStringTypeCSV(roTabletTypes)
 	var switchReplica, switchRdonly bool
-	for _, roType := range req.TabletTypes {
+	for _, roType := range roTabletTypes {
 		switch roType {
 		case topodatapb.TabletType_REPLICA:
 			switchReplica = true
@@ -2953,14 +2962,13 @@ func (s *Server) switchReads(ctx context.Context, req *vtctldatapb.WorkflowSwitc
 
 	// Consistently handle errors by logging and returning them.
 	handleError := func(message string, err error) (*[]string, error) {
-		werr := vterrors.Errorf(vtrpcpb.Code_INTERNAL, fmt.Sprintf("%s: %v", message, err))
-		ts.Logger().Error(werr)
-		return nil, werr
+		ts.Logger().Error(err)
+		return nil, err
 	}
 
 	log.Infof("Switching reads: %s.%s tablet types: %s, cells: %s, workflow state: %s", ts.targetKeyspace, ts.workflow, roTypesToSwitchStr, ts.optCells, state.String())
 	if !switchReplica && !switchRdonly {
-		return handleError("invalid tablet types", vterrors.Errorf(vtrpcpb.Code_FAILED_PRECONDITION, "tablet types must be REPLICA or RDONLY: %s", roTypesToSwitchStr))
+		return handleError("invalid tablet types", vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "tablet types must be REPLICA or RDONLY: %s", roTypesToSwitchStr))
 	}
 	if !ts.isPartialMigration { // shard level traffic switching is all or nothing
 		if direction == DirectionBackward && switchReplica && len(state.ReplicaCellsSwitched) == 0 {
@@ -2987,7 +2995,7 @@ func (s *Server) switchReads(ctx context.Context, req *vtctldatapb.WorkflowSwitc
 			return nil, err
 		}
 		if !rdonlyTabletsExist {
-			req.TabletTypes = append(req.TabletTypes, topodatapb.TabletType_RDONLY)
+			roTabletTypes = append(roTabletTypes, topodatapb.TabletType_RDONLY)
 		}
 	}
 
@@ -3020,13 +3028,13 @@ func (s *Server) switchReads(ctx context.Context, req *vtctldatapb.WorkflowSwitc
 	if ts.MigrationType() == binlogdatapb.MigrationType_TABLES {
 		if ts.isPartialMigration {
 			ts.Logger().Infof("Partial migration, skipping switchTableReads as traffic is all or nothing per shard and overridden for reads AND writes in the ShardRoutingRule created when switching writes.")
-		} else if err := sw.switchTableReads(ctx, cells, req.TabletTypes, direction); err != nil {
+		} else if err := sw.switchTableReads(ctx, cells, roTabletTypes, direction); err != nil {
 			return handleError("failed to switch read traffic for the tables", err)
 		}
 		return sw.logs(), nil
 	}
 	ts.Logger().Infof("About to switchShardReads: %+v, %+s, %+v", cells, roTypesToSwitchStr, direction)
-	if err := sw.switchShardReads(ctx, cells, req.TabletTypes, direction); err != nil {
+	if err := sw.switchShardReads(ctx, cells, roTabletTypes, direction); err != nil {
 		return handleError("failed to switch read traffic for the shards", err)
 	}
 

diff --git a/go/vt/vtctl/workflow/traffic_switcher.go b/go/vt/vtctl/workflow/traffic_switcher.go
@@ -587,6 +587,10 @@ func (ts *trafficSwitcher) switchTableReads(ctx context.Context, cells []string,
 	// For forward migration, we add tablet type specific rules to redirect traffic to the target.
 	// For backward, we redirect to source.
 	for _, servedType := range servedTypes {
+		if servedType != topodatapb.TabletType_REPLICA && servedType != topodatapb.TabletType_RDONLY {
+			return vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "invalid tablet type specified when switching reads: %v", servedType)
+		}
+
 		tt := strings.ToLower(servedType.String())
 		for _, table := range ts.Tables() {
 			if direction == DirectionForward {