From 878378b9e37133d1e84e41ddc2b542624a66b514 Mon Sep 17 00:00:00 2001 From: Rohit Nayak <57520317+rohit-nayak-ps@users.noreply.github.com> Date: Fri, 27 Oct 2023 12:48:50 +0200 Subject: [PATCH] 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 Signed-off-by: Matt Lord Co-authored-by: Matt Lord --- go/vt/vtctl/workflow/server.go | 26 ++++++++++++++++-------- go/vt/vtctl/workflow/traffic_switcher.go | 4 ++++ 2 files changed, 21 insertions(+), 9 deletions(-) diff --git a/go/vt/vtctl/workflow/server.go b/go/vt/vtctl/workflow/server.go index abbb0b770f7..c26a38c4811 100644 --- 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 index d4fe77130ae..35f1d1b966b 100644 --- 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 {