From 378f9a64ff58a2a3b5cbe23c867d6644d7885cdc Mon Sep 17 00:00:00 2001 From: qzhello <951012707@qq.com> Date: Fri, 26 Jun 2026 15:48:29 +0800 Subject: [PATCH] fix: apply collectionPattern during detection in volume.fix.replication (#10115) * fix(shell): correct volume.list -writable filter unit and comparison * fix(shell): correct volume.list -writable filter unit and comparison * chore(shell): fix typo in EC shard helper param names * fix(shell): use exact match for volume.balance -racks/-nodes filter The old strings.Contains-based filter quietly included any id that was a substring of the user-supplied flag value (e.g. -racks=rack10 also matched rack1). Replace it with an exact-match set parsed from the comma-separated flag value, and add regression tests for both -racks and -nodes paths. Also fix a small typo in the "remote storage" error returned by maybeMoveOneVolume. * fix(shell): use exact match for volume.balance -racks/-nodes filter The old strings.Contains-based filter quietly included any id that was a substring of the user-supplied flag value (e.g. -racks=rack10 also matched rack1). Replace it with an exact-match set parsed from the comma-separated flag value, and add regression tests for both -racks and -nodes paths. Also fix a small typo in the "remote storage" error returned by maybeMoveOneVolume. * refactor(shell): drop nil sentinel in splitCSVSet, use len() in callers * fix: apply collectionPattern during detection in volume.fix.replication * use existing wildcard.MatchesWildcard for collection matching It returns a plain bool, so drop the up-front filepath.Match validation and the path/filepath import that only existed to handle its error. * trim verbose comments to terse one-liners * drop redundant per-path collection guards Detection already filters by replicas[0].info.Collection. The repair guard re-checked pickOneReplicaToCopyFrom's collection (a different replica), so a mixed-collection volume could pass detection yet be skipped in repair without decrementing the counter, spinning the -apply loop. deleteOneVolume keeps its collectionIsMismatch safety. --------- Co-authored-by: Chris Lu --- weed/shell/command_volume_fix_replication.go | 58 +++++++------------ ..._volume_fix_replication_collection_test.go | 34 +++++++++++ 2 files changed, 54 insertions(+), 38 deletions(-) create mode 100644 weed/shell/command_volume_fix_replication_collection_test.go diff --git a/weed/shell/command_volume_fix_replication.go b/weed/shell/command_volume_fix_replication.go index 73497e530..6476b41d5 100644 --- a/weed/shell/command_volume_fix_replication.go +++ b/weed/shell/command_volume_fix_replication.go @@ -4,7 +4,6 @@ import ( "flag" "fmt" "io" - "path/filepath" "strconv" "strings" "time" @@ -15,6 +14,7 @@ import ( "github.com/seaweedfs/seaweedfs/weed/storage/needle" "github.com/seaweedfs/seaweedfs/weed/storage/needle_map" "github.com/seaweedfs/seaweedfs/weed/storage/types" + "github.com/seaweedfs/seaweedfs/weed/util/wildcard" "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" "github.com/seaweedfs/seaweedfs/weed/storage/super_block" @@ -114,6 +114,12 @@ func (c *commandVolumeFixReplication) Do(args []string, commandEnv *CommandEnv, var underReplicatedVolumeIds, overReplicatedVolumeIds, misplacedVolumeIds []uint32 for vid, replicas := range volumeReplicas { replica := replicas[0] + + // Filter here so the termination counter matches what gets fixed; else -apply loops forever. + if !c.matchCollectionPattern(replica.info.Collection) { + continue + } + replicaPlacement, _ := super_block.NewReplicaPlacementFromByte(byte(replica.info.ReplicaPlacement)) // build locations list for optional verbose output @@ -255,6 +261,18 @@ func checkOneVolume(a *VolumeReplica, b *VolumeReplica, writer io.Writer, comman return } +// matchCollectionPattern reports whether collection matches -collectionPattern: +// empty matches everything, CollectionDefault matches the unnamed collection. +func (c *commandVolumeFixReplication) matchCollectionPattern(collection string) bool { + if *c.collectionPattern == "" { + return true + } + if *c.collectionPattern == CollectionDefault { + return collection == "" + } + return wildcard.MatchesWildcard(*c.collectionPattern, collection) +} + func (c *commandVolumeFixReplication) deleteOneVolume(commandEnv *CommandEnv, writer io.Writer, applyChanges bool, doCheck bool, volumeIds []uint32, volumeReplicas map[uint32][]*VolumeReplica, allLocations []location, selectOneVolumeFn SelectOneVolumeFunc) error { if len(volumeIds) == 0 { // nothing to do @@ -271,23 +289,6 @@ func (c *commandVolumeFixReplication) deleteOneVolume(commandEnv *CommandEnv, wr continue } - // check collection name pattern - if *c.collectionPattern != "" { - var matched bool - if *c.collectionPattern == CollectionDefault { - matched = replica.info.Collection == "" - } else { - var err error - matched, err = filepath.Match(*c.collectionPattern, replica.info.Collection) - if err != nil { - return fmt.Errorf("match pattern %s with collection %s: %v", *c.collectionPattern, replica.info.Collection, err) - } - } - if !matched { - continue - } - } - collectionIsMismatch := false for _, volumeReplica := range replicas { if volumeReplica.info.Collection != replica.info.Collection { @@ -365,30 +366,11 @@ func (c *commandVolumeFixReplication) fixOneUnderReplicatedVolume(commandEnv *Co replica := pickOneReplicaToCopyFrom(replicas) replicaPlacement, _ := super_block.NewReplicaPlacementFromByte(byte(replica.info.ReplicaPlacement)) foundNewLocation := false - hasSkippedCollection := false keepDataNodesSorted(allLocations, types.ToDiskType(replica.info.DiskType)) fn := capacityByFreeVolumeCount(types.ToDiskType(replica.info.DiskType)) for _, dst := range allLocations { // check whether data nodes satisfy the constraints if fn(dst.dataNode) > 0 && satisfyReplicaPlacement(replicaPlacement, replicas, dst) { - // check collection name pattern - if *c.collectionPattern != "" { - var matched bool - if *c.collectionPattern == CollectionDefault { - matched = replica.info.Collection == "" - } else { - var err error - matched, err = filepath.Match(*c.collectionPattern, replica.info.Collection) - if err != nil { - return false, fmt.Errorf("match pattern %s with collection %s: %v", *c.collectionPattern, replica.info.Collection, err) - } - } - if !matched { - hasSkippedCollection = true - break - } - } - // ask the volume server to replicate the volume foundNewLocation = true fmt.Fprintf(writer, "replicating volume %d %s from %s to dataNode %s ...\n", replica.info.Id, replicaPlacement, replica.location.dataNode.Id, dst.dataNode.Id) @@ -414,7 +396,7 @@ func (c *commandVolumeFixReplication) fixOneUnderReplicatedVolume(commandEnv *Co } } - if !foundNewLocation && !hasSkippedCollection { + if !foundNewLocation { fmt.Fprintf(writer, "failed to place volume %d replica as %s, existing:%+v\n", replica.info.Id, replicaPlacement, len(replicas)) } return false, nil diff --git a/weed/shell/command_volume_fix_replication_collection_test.go b/weed/shell/command_volume_fix_replication_collection_test.go new file mode 100644 index 000000000..c304a9e57 --- /dev/null +++ b/weed/shell/command_volume_fix_replication_collection_test.go @@ -0,0 +1,34 @@ +package shell + +import "testing" + +func TestMatchCollectionPattern(t *testing.T) { + tests := []struct { + name string + pattern string + collection string + expected bool + }{ + {name: "empty pattern matches any collection", pattern: "", collection: "jfs-hdfs-test", expected: true}, + {name: "empty pattern matches empty collection", pattern: "", collection: "", expected: true}, + {name: "default pattern matches empty collection", pattern: CollectionDefault, collection: "", expected: true}, + {name: "default pattern rejects named collection", pattern: CollectionDefault, collection: "jfs-hdfs-test", expected: false}, + {name: "exact match", pattern: "smart-highlevel-test", collection: "smart-highlevel-test", expected: true}, + {name: "exact mismatch", pattern: "smart-highlevel-test", collection: "jfs-hdfs-test", expected: false}, + {name: "prefix wildcard match", pattern: "smart*", collection: "smart-highlevel-test", expected: true}, + {name: "prefix wildcard mismatch", pattern: "smart*", collection: "jfs-hdfs-test", expected: false}, + {name: "single char wildcard match", pattern: "vol?", collection: "vol1", expected: true}, + {name: "single char wildcard mismatch", pattern: "vol?", collection: "vol42", expected: false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pattern := tt.pattern + c := &commandVolumeFixReplication{collectionPattern: &pattern} + if got := c.matchCollectionPattern(tt.collection); got != tt.expected { + t.Errorf("matchCollectionPattern(pattern=%q, collection=%q) = %v, want %v", + tt.pattern, tt.collection, got, tt.expected) + } + }) + } +}