diff --git a/weed/worker/tasks/balance/plugin_handler.go b/weed/worker/tasks/balance/plugin_handler.go index 813f770c2..d271b0bd2 100644 --- a/weed/worker/tasks/balance/plugin_handler.go +++ b/weed/worker/tasks/balance/plugin_handler.go @@ -12,6 +12,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/admin/topology" "github.com/seaweedfs/seaweedfs/weed/glog" + "github.com/seaweedfs/seaweedfs/weed/pb" "github.com/seaweedfs/seaweedfs/weed/pb/plugin_pb" "github.com/seaweedfs/seaweedfs/weed/pb/worker_pb" pluginworker "github.com/seaweedfs/seaweedfs/weed/plugin/worker" @@ -790,12 +791,17 @@ func (h *VolumeBalanceHandler) checkMoveStillValid(ctx context.Context, masterAd } func checkMovePreconditions(locations []string, volumeID uint32, sourceNode, targetNode string) error { + // Proposal nodes carry the grpc suffix (host:port.grpcPort) while the + // master reports plain host:port urls, so compare in http form. + sourceAddress := pb.ServerAddress(strings.TrimSpace(sourceNode)) + targetAddress := pb.ServerAddress(strings.TrimSpace(targetNode)) sourceFound := false for _, location := range locations { - switch strings.TrimSpace(location) { - case targetNode: + locationAddress := pb.ServerAddress(strings.TrimSpace(location)) + switch { + case locationAddress.Equals(targetAddress): return fmt.Errorf("stale move: volume %d already has a replica on target %s", volumeID, targetNode) - case sourceNode: + case locationAddress.Equals(sourceAddress): sourceFound = true } } diff --git a/weed/worker/tasks/balance/plugin_handler_test.go b/weed/worker/tasks/balance/plugin_handler_test.go index a3c2294b1..2067623fa 100644 --- a/weed/worker/tasks/balance/plugin_handler_test.go +++ b/weed/worker/tasks/balance/plugin_handler_test.go @@ -798,18 +798,22 @@ func (r *recordingDetectionSender) SendActivity(event *plugin_pb.ActivityEvent) func TestCheckMovePreconditions(t *testing.T) { tests := []struct { - name string - locations []string - wantErr string + name string + locations []string + sourceNode string + targetNode string + wantErr string }{ - {"valid move", []string{"10.0.0.1:8080", "10.0.0.3:8080"}, ""}, - {"volume left the source", []string{"10.0.0.3:8080"}, "no longer on source"}, - {"volume gone entirely", nil, "no longer on source"}, - {"target already has a replica", []string{"10.0.0.1:8080", "10.0.0.2:8080"}, "already has a replica on target"}, + {"valid move", []string{"10.0.0.1:8080", "10.0.0.3:8080"}, "10.0.0.1:8080", "10.0.0.2:8080", ""}, + {"volume left the source", []string{"10.0.0.3:8080"}, "10.0.0.1:8080", "10.0.0.2:8080", "no longer on source"}, + {"volume gone entirely", nil, "10.0.0.1:8080", "10.0.0.2:8080", "no longer on source"}, + {"target already has a replica", []string{"10.0.0.1:8080", "10.0.0.2:8080"}, "10.0.0.1:8080", "10.0.0.2:8080", "already has a replica on target"}, + {"proposal nodes carry grpc suffix", []string{"10.0.0.1:8080", "10.0.0.3:8080"}, "10.0.0.1:8080.18080", "10.0.0.2:8080.18080", ""}, + {"grpc-suffixed target already has a replica", []string{"10.0.0.1:8080", "10.0.0.2:8080"}, "10.0.0.1:8080.18080", "10.0.0.2:8080.18080", "already has a replica on target"}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - err := checkMovePreconditions(tt.locations, 5, "10.0.0.1:8080", "10.0.0.2:8080") + err := checkMovePreconditions(tt.locations, 5, tt.sourceNode, tt.targetNode) if tt.wantErr == "" { if err != nil { t.Fatalf("unexpected error: %v", err)