diff --git a/weed/shell/command_volume_balance.go b/weed/shell/command_volume_balance.go index 36cd38051..7e5a8e760 100644 --- a/weed/shell/command_volume_balance.go +++ b/weed/shell/command_volume_balance.go @@ -215,17 +215,32 @@ func (c *commandVolumeBalance) balanceVolumeServersByDiskType(diskType types.Dis return nil } +// splitCSVSet parses a comma-separated list into a set for exact-match filtering. +// Whitespace around items is trimmed and empty items are skipped, so callers +// can use len(set) > 0 to test whether any filter was specified. +func splitCSVSet(csv string) map[string]bool { + set := make(map[string]bool) + for _, item := range strings.Split(csv, ",") { + if item = strings.TrimSpace(item); item != "" { + set[item] = true + } + } + return set +} + func collectVolumeServersByDcRackNode(t *master_pb.TopologyInfo, selectedDataCenter string, selectedRacks string, selectedNodes string) (nodes []*Node) { + rackSet := splitCSVSet(selectedRacks) + nodeSet := splitCSVSet(selectedNodes) for _, dc := range t.DataCenterInfos { if selectedDataCenter != "" && dc.Id != selectedDataCenter { continue } for _, r := range dc.RackInfos { - if selectedRacks != "" && !strings.Contains(selectedRacks, r.Id) { + if len(rackSet) > 0 && !rackSet[r.Id] { continue } for _, dn := range r.DataNodeInfos { - if selectedNodes != "" && !strings.Contains(selectedNodes, dn.Id) { + if len(nodeSet) > 0 && !nodeSet[dn.Id] { continue } nodes = append(nodes, &Node{ @@ -488,7 +503,7 @@ func maybeMoveOneVolume(commandEnv *CommandEnv, volumeReplicas map[uint32][]*Vol } if candidateVolume.RemoteStorageName != "" { - return false, fmt.Errorf("does not move volume in remove storage") + return false, fmt.Errorf("does not move volume in remote storage") } if candidateVolume.ReplicaPlacement > 0 { diff --git a/weed/shell/command_volume_balance_test.go b/weed/shell/command_volume_balance_test.go index 99fdf5575..65438cc54 100644 --- a/weed/shell/command_volume_balance_test.go +++ b/weed/shell/command_volume_balance_test.go @@ -287,3 +287,87 @@ func TestDeleteEmptySelection(t *testing.T) { }) } + +func TestSplitCSVSet(t *testing.T) { + tests := []struct { + name string + in string + want map[string]bool + }{ + {"empty input is empty set (no filter)", "", map[string]bool{}}, + {"whitespace only is empty set (no filter)", " ", map[string]bool{}}, + {"commas only is empty set (no filter)", ",,,", map[string]bool{}}, + {"whitespace and commas only is empty set (no filter)", " , , ", map[string]bool{}}, + {"single", "rack1", map[string]bool{"rack1": true}}, + {"multi", "rack1,rack2", map[string]bool{"rack1": true, "rack2": true}}, + {"trims whitespace", " rack1 , rack2 ", map[string]bool{"rack1": true, "rack2": true}}, + {"skips empty items", "rack1,,rack2,", map[string]bool{"rack1": true, "rack2": true}}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, splitCSVSet(tc.in)) + }) + } +} + +// Regression test for the rack/node filter that previously used +// strings.Contains, which falsely matched any id that was a substring of the +// user-supplied flag value (e.g. -racks=rack10 also matched rack1). +func TestCollectVolumeServersByDcRackNode_RackFilter(t *testing.T) { + topo := &master_pb.TopologyInfo{ + DataCenterInfos: []*master_pb.DataCenterInfo{{ + Id: "dc1", + RackInfos: []*master_pb.RackInfo{ + {Id: "rack1", DataNodeInfos: []*master_pb.DataNodeInfo{{Id: "n1"}}}, + {Id: "rack10", DataNodeInfos: []*master_pb.DataNodeInfo{{Id: "n10"}}}, + {Id: "rack2", DataNodeInfos: []*master_pb.DataNodeInfo{{Id: "n2"}}}, + }, + }}, + } + + got := collectVolumeServersByDcRackNode(topo, "", "rack10", "") + if assert.Len(t, got, 1, "-racks=rack10 should not match rack1") { + assert.Equal(t, "rack10", got[0].rack) + } + + got = collectVolumeServersByDcRackNode(topo, "", "rack1,rack2", "") + racks := map[string]bool{} + for _, n := range got { + racks[n.rack] = true + } + assert.Equal(t, map[string]bool{"rack1": true, "rack2": true}, racks, + "-racks=rack1,rack2 should match exactly those two, not rack10") +} + +// Regression test for the -nodes filter, mirroring the rack-filter case. +// Uses bare ids (no :port suffix) so that "node1" is a true substring of +// "node10": under the old strings.Contains implementation, +// -nodes=node10 wrongly included node1 as well. +func TestCollectVolumeServersByDcRackNode_NodeFilter(t *testing.T) { + topo := &master_pb.TopologyInfo{ + DataCenterInfos: []*master_pb.DataCenterInfo{{ + Id: "dc1", + RackInfos: []*master_pb.RackInfo{{ + Id: "rack1", + DataNodeInfos: []*master_pb.DataNodeInfo{ + {Id: "node1"}, + {Id: "node10"}, + {Id: "node2"}, + }, + }}, + }}, + } + + got := collectVolumeServersByDcRackNode(topo, "", "", "node10") + if assert.Len(t, got, 1, "-nodes=node10 should not match node1") { + assert.Equal(t, "node10", got[0].info.Id) + } + + got = collectVolumeServersByDcRackNode(topo, "", "", "node1,node2") + nodes := map[string]bool{} + for _, n := range got { + nodes[n.info.Id] = true + } + assert.Equal(t, map[string]bool{"node1": true, "node2": true}, nodes, + "-nodes=node1,node2 should match exactly those two, not node10") +}