wdclient: keep the location of a volume reported added and removed at once (#10635)

* wdclient: keep the location of a volume reported added and removed at once

A volume moved between a server's disks arrives in both lists of one message,
and the server still has it. Additions were applied before removals, so the
removal won and the client was left with no location for a volume that never
went anywhere.

Reordering would swap the bug for a window where the volume resolves nowhere,
since the two updates take the lock separately. Skip the removal instead, so
the order the lists are applied in stops mattering.

* wdclient: build each ec update explicitly in the move test

Reusing one response object and adding the deletion to it left the overlap the
test turns on implicit, and reading it as a delete-only update is the natural
mistake.

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)
 	}

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)
+	}
+}