diff --git a/weed/wdclient/masterclient.go b/weed/wdclient/masterclient.go index d6ca48b38..b06dffad0 100644 --- a/weed/wdclient/masterclient.go +++ b/weed/wdclient/masterclient.go @@ -351,6 +351,22 @@ func (mc *MasterClient) tryConnectToMaster(ctx context.Context, master pb.Server return nextHintedLeader } +// addedVids indexes added ids that are also being removed. A volume moved +// between a server's disks is reported both ways in one message, and the server +// still has it -- acting on the removal would drop a good location, whichever +// order the two lists happen to be applied in. Empty unless both lists are +// non-empty, so the full id list a client gets on connect costs nothing. +func addedVids(added, removed []uint32) map[uint32]struct{} { + if len(added) == 0 || len(removed) == 0 { + return nil + } + index := make(map[uint32]struct{}, len(added)) + for _, vid := range added { + index[vid] = struct{}{} + } + return index +} + func (mc *MasterClient) updateVidMap(resp *master_pb.KeepConnectedResponse) { if resp.VolumeLocation.IsEmptyUrl() { glog.V(0).Infof("updateVidMap ignore short heartbeat: %+v", resp) @@ -363,19 +379,27 @@ func (mc *MasterClient) updateVidMap(resp *master_pb.KeepConnectedResponse) { DataCenter: resp.VolumeLocation.DataCenter, GrpcPort: int(resp.VolumeLocation.GrpcPort), } + stillOnServer := addedVids(resp.VolumeLocation.NewVids, resp.VolumeLocation.DeletedVids) for _, newVid := range resp.VolumeLocation.NewVids { glog.V(2).Infof("%s.%s: %s masterClient adds volume %d", mc.FilerGroup, mc.clientType, loc.Url, newVid) mc.addLocation(newVid, loc) } for _, deletedVid := range resp.VolumeLocation.DeletedVids { + if _, moved := stillOnServer[deletedVid]; moved { + continue + } glog.V(2).Infof("%s.%s: %s masterClient removes volume %d", mc.FilerGroup, mc.clientType, loc.Url, deletedVid) mc.deleteLocation(deletedVid, loc) } + stillOnServerEc := addedVids(resp.VolumeLocation.NewEcVids, resp.VolumeLocation.DeletedEcVids) for _, newEcVid := range resp.VolumeLocation.NewEcVids { glog.V(2).Infof("%s.%s: %s masterClient adds ec volume %d", mc.FilerGroup, mc.clientType, loc.Url, newEcVid) mc.addEcLocation(newEcVid, loc) } for _, deletedEcVid := range resp.VolumeLocation.DeletedEcVids { + if _, moved := stillOnServerEc[deletedEcVid]; moved { + continue + } glog.V(2).Infof("%s.%s: %s masterClient removes ec volume %d", mc.FilerGroup, mc.clientType, loc.Url, deletedEcVid) mc.deleteEcLocation(deletedEcVid, loc) } diff --git a/weed/wdclient/masterclient_move_test.go b/weed/wdclient/masterclient_move_test.go new file mode 100644 index 000000000..b5f70a2be --- /dev/null +++ b/weed/wdclient/masterclient_move_test.go @@ -0,0 +1,80 @@ +package wdclient + +import ( + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" +) + +func moveClient() *MasterClient { + return &MasterClient{vidMapClient: newVidMapClient(nil, "", 0)} +} + +func moveResponse(newVids, deletedVids []uint32) *master_pb.KeepConnectedResponse { + return &master_pb.KeepConnectedResponse{VolumeLocation: &master_pb.VolumeLocation{ + Url: "server:8080", PublicUrl: "server:8080", NewVids: newVids, DeletedVids: deletedVids, + }} +} + +// A volume moved between a server's disks arrives added and removed at once, +// and the server still has it. +func TestMovedVolumeKeepsItsLocation(t *testing.T) { + mc := moveClient() + mc.updateVidMap(moveResponse([]uint32{1}, nil)) + mc.updateVidMap(moveResponse([]uint32{1}, []uint32{1})) + + locations, found := mc.GetLocations(1) + if !found || len(locations) != 1 { + t.Errorf("a volume that moved between its server's disks lost its location: found=%v %v", found, locations) + } +} + +func TestRemovedVolumeLosesItsLocation(t *testing.T) { + mc := moveClient() + mc.updateVidMap(moveResponse([]uint32{1}, nil)) + mc.updateVidMap(moveResponse(nil, []uint32{1})) + + if locations, found := mc.GetLocations(1); found && len(locations) > 0 { + t.Errorf("a volume that left the server kept its location: %v", locations) + } +} + +// Removals of other volumes in the same message must still apply. +func TestRemovalsAlongsideAMoveStillApply(t *testing.T) { + mc := moveClient() + mc.updateVidMap(moveResponse([]uint32{1, 2}, nil)) + mc.updateVidMap(moveResponse([]uint32{1}, []uint32{1, 2})) + + if locations, found := mc.GetLocations(1); !found || len(locations) != 1 { + t.Errorf("the moved volume lost its location: found=%v %v", found, locations) + } + if locations, found := mc.GetLocations(2); found && len(locations) > 0 { + t.Errorf("a volume that left the server kept its location: %v", locations) + } +} + +func moveEcResponse(newEcVids, deletedEcVids []uint32) *master_pb.KeepConnectedResponse { + return &master_pb.KeepConnectedResponse{VolumeLocation: &master_pb.VolumeLocation{ + Url: "server:8080", PublicUrl: "server:8080", NewEcVids: newEcVids, DeletedEcVids: deletedEcVids, + }} +} + +func TestEcVolumeMovedKeepsItsLocation(t *testing.T) { + mc := moveClient() + mc.updateVidMap(moveEcResponse([]uint32{7}, nil)) + mc.updateVidMap(moveEcResponse([]uint32{7}, []uint32{7})) + + if locations, found := mc.GetLocations(7); !found || len(locations) != 1 { + t.Errorf("an ec volume reported both ways lost its location: found=%v %v", found, locations) + } +} + +func TestEcVolumeRemovedLosesItsLocation(t *testing.T) { + mc := moveClient() + mc.updateVidMap(moveEcResponse([]uint32{7}, nil)) + mc.updateVidMap(moveEcResponse(nil, []uint32{7})) + + if locations, found := mc.GetLocations(7); found && len(locations) > 0 { + t.Errorf("an ec volume that left the server kept its location: %v", locations) + } +}