Skip to content

resharding: safer MigrateServedTypes #4248

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
sougou merged 2 commits into from
Oct 10, 2018
Merged

resharding: safer MigrateServedTypes #4248

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b9b8cf1
resharding: safer MigrateServedTypes
sougou Oct 7, 2018
9f05597
resharding: address review comments
sougou Oct 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
203 changes: 107 additions & 96 deletions go/vt/proto/topodata/topodata.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

74 changes: 22 additions & 52 deletions go/vt/topo/shard.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,6 @@ func (si *ShardInfo) UpdateDisableQueryService(ctx context.Context, tabletType t
DisableQueryService: true,
BlacklistedTables: nil,
})
} else {
log.Warningf("Trying to remove TabletControl.DisableQueryService for missing type %v for shard %v/%v", tabletType, si.keyspace, si.shardName)
}
return nil
}
Expand All @@ -452,12 +450,16 @@ func (si *ShardInfo) UpdateDisableQueryService(ctx context.Context, tabletType t
return fmt.Errorf("cannot safely alter DisableQueryService as BlacklistedTables is set")
}
if !tc.DisableQueryService {
// This code is unreachable because we always delete the control record when we enable QueryService.
Copy link
Copy Markdown
Member

@rafael rafael Oct 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we remove it in that case?

Copy link
Copy Markdown
Contributor Author

@sougou sougou Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was tempted to. But it's possible some new code could change this without knowing. So, it's good to leave it here as fail-safe.

Copy link
Copy Markdown
Member

@rafael rafael Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case, why don't we panic? If new code gets added then it should just fail bad.

Copy link
Copy Markdown
Contributor Author

@sougou sougou Oct 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've had some heated arguments in the past about this. The problem with panic is that if tests miss the code path, it will happen in production.

I've now settled on returning an error, and a clarifying comment that the code is unreachable.

The one exception is if the function doesn't return an error. In such cases, I still panic.

return fmt.Errorf("cannot safely alter DisableQueryService as DisableQueryService is not set, this record should not be there for shard %v/%v", si.keyspace, si.shardName)
}

if disableQueryService {
tc.Cells = addCells(tc.Cells, cells)
} else {
if tc.Frozen {
return fmt.Errorf("migrate has gone past the point of no return, cannot re-enable serving for %v/%v", si.keyspace, si.shardName)
}
si.removeCellsFromTabletControl(tc, tabletType, cells)
}
return nil
Expand Down Expand Up @@ -501,56 +503,15 @@ func (si *ShardInfo) GetServedTypesPerCell(cell string) []topodatapb.TabletType
return result
}

// CheckServedTypesMigration makes sure the provided migration is possible
func (si *ShardInfo) CheckServedTypesMigration(tabletType topodatapb.TabletType, cells []string, remove bool) error {
// we can't remove a type we don't have
if si.GetServedType(tabletType) == nil && remove {
return fmt.Errorf("supplied type %v cannot be migrated out of the shard because it is not a served type: %v", tabletType, si)
}

// master is a special case with a few extra checks
if tabletType == topodatapb.TabletType_MASTER {
if len(cells) > 0 {
return fmt.Errorf("cannot migrate only some cells for MASTER in shard %v/%v. Do not specify a list of cells", si.keyspace, si.shardName)
}
if remove && len(si.ServedTypes) > 1 {
// Log which types must be migrated first.
var types []string
for _, servedType := range si.ServedTypes {
if servedType.TabletType != topodatapb.TabletType_MASTER {
types = append(types, servedType.TabletType.String())
}
}
return fmt.Errorf("cannot migrate MASTER away from %v/%v until everything else is migrated. Make sure that the following types are migrated first: %v", si.keyspace, si.shardName, strings.Join(types, ", "))
}
}

return nil
}

// UpdateServedTypesMap handles ServedTypesMap. It can add or remove
// records, cells, ...
// UpdateServedTypesMap handles ServedTypesMap. It can add or remove cells.
func (si *ShardInfo) UpdateServedTypesMap(tabletType topodatapb.TabletType, cells []string, remove bool) error {
// check parameters to be sure
if err := si.CheckServedTypesMigration(tabletType, cells, remove); err != nil {
return err
}

sst := si.GetServedType(tabletType)
if sst == nil {
// the record doesn't exist
if remove {
log.Warningf("Trying to remove ShardServedType for missing type %v in shard %v/%v", tabletType, si.keyspace, si.shardName)
} else {
si.ServedTypes = append(si.ServedTypes, &topodatapb.Shard_ServedType{
TabletType: tabletType,
Cells: cells,
})
}
return nil
}

if remove {
if sst == nil {
// nothing to remove
return nil
}
result, emptyList := removeCells(sst.Cells, cells, si.Cells)
if emptyList {
// we don't have any cell left, we need to clear this record
Expand All @@ -561,12 +522,21 @@ func (si *ShardInfo) UpdateServedTypesMap(tabletType topodatapb.TabletType, cell
}
}
si.ServedTypes = servedTypes
} else {
sst.Cells = result
return nil
}
} else {
sst.Cells = addCells(sst.Cells, cells)
sst.Cells = result
return nil
}

// add
if sst == nil {
si.ServedTypes = append(si.ServedTypes, &topodatapb.Shard_ServedType{
TabletType: tabletType,
Cells: cells,
})
return nil
}
sst.Cells = addCells(sst.Cells, cells)
return nil
}

Expand Down
14 changes: 0 additions & 14 deletions go/vt/topo/shard_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ package topo

import (
"reflect"
"strings"
"testing"

"golang.org/x/net/context"
Expand Down Expand Up @@ -329,29 +328,16 @@ func TestUpdateServedTypesMap(t *testing.T) {
t.Fatalf("migrate master failed: %v", err)
}

// try to migrate master away, see it fail
if err := si.UpdateServedTypesMap(topodatapb.TabletType_MASTER, nil, true); err == nil || err.Error() != "cannot migrate MASTER away from ks/sh until everything else is migrated. Make sure that the following types are migrated first: RDONLY, REPLICA" {
t.Fatalf("migrate master away unexpected error: %v", err)
}

// Migrate each serving type away from this shard.
// RDONLY
if err := si.UpdateServedTypesMap(topodatapb.TabletType_RDONLY, nil, true); err != nil {
t.Fatalf("remove master failed: %v", err)
}
// Cannot migrate a type away (here RDONLY) which is not served (anymore).
if err := si.UpdateServedTypesMap(topodatapb.TabletType_RDONLY, nil, true); err == nil || !strings.HasPrefix(err.Error(), "supplied type RDONLY cannot be migrated out of the shard because it is not a served type: ") {
t.Fatalf("migrate rdonly should have failed because it's already migrated: %v", err)
}
// REPLICA
if err := si.UpdateServedTypesMap(topodatapb.TabletType_REPLICA, nil, true); err != nil {
t.Fatalf("remove master failed: %v", err)
}
// MASTER
// Migration fails if a list of cells is specified.
if err := si.UpdateServedTypesMap(topodatapb.TabletType_MASTER, []string{"first", "third"}, true); err == nil || err.Error() != "cannot migrate only some cells for MASTER in shard ks/sh. Do not specify a list of cells" {
t.Fatalf("remove master failed: %v", err)
}
if err := si.UpdateServedTypesMap(topodatapb.TabletType_MASTER, nil, true); err != nil {
t.Fatalf("remove master failed: %v", err)
}
Expand Down
Loading