From a653a7f72ad6cedfa0cb5d555d3d61d514b7234c Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 30 Jun 2026 13:28:53 -0700 Subject: [PATCH] fix(shell): honor explicit fs.mergeVolumes from/to direction (#10159) * fix(shell): honor explicit fs.mergeVolumes from/to direction mergeVolumes only ever merged a smaller volume into a larger one. When the user named both -fromVolumeId and -toVolumeId with the source larger than the target, the planner produced an empty plan and the command printed just "max volume size: N MB" and moved nothing. Build the requested pair directly when both ids are given, instead of routing through the size-descending heuristic. Read-only, empty, and wrong-collection endpoints are rejected with a clear error rather than a silent no-op. * fix(shell): allow fs.mergeVolumes into an empty target volume Merging chunks into an empty volume is valid, e.g. consolidating data into a freshly created or recently vacuumed volume. Only reject an empty source, which has nothing to move. * fix(shell): reject self-map in directed mergeVolumes planner createMergePlan with from == to returned a {vid: vid} self-merge when called directly. Guard it in the planner so it is correct independent of the Do entrypoint. --- weed/shell/command_fs_merge_volumes.go | 37 ++++++ weed/shell/command_fs_merge_volumes_test.go | 125 ++++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 weed/shell/command_fs_merge_volumes_test.go diff --git a/weed/shell/command_fs_merge_volumes.go b/weed/shell/command_fs_merge_volumes.go index 59c1650b3..08288c0c7 100644 --- a/weed/shell/command_fs_merge_volumes.go +++ b/weed/shell/command_fs_merge_volumes.go @@ -357,6 +357,14 @@ func (c *commandFsMergeVolumes) reloadVolumesInfo(masterClient *wdclient.MasterC } func (c *commandFsMergeVolumes) createMergePlan(collection string, toVolumeId needle.VolumeId, fromVolumeId needle.VolumeId) (map[needle.VolumeId]needle.VolumeId, error) { + // When the user names both endpoints, honor that exact direction. The + // heuristic below only ever merges a smaller volume into a larger one, so + // an explicit "merge larger into smaller" request would otherwise yield an + // empty plan and silently do nothing. + if fromVolumeId != 0 && toVolumeId != 0 { + return c.createDirectedMergePlan(collection, fromVolumeId, toVolumeId) + } + plan := make(map[needle.VolumeId]needle.VolumeId) volumeIds := maps.Keys(c.volumes) sort.Slice(volumeIds, func(a, b int) bool { @@ -418,6 +426,35 @@ func (c *commandFsMergeVolumes) createMergePlan(collection string, toVolumeId ne return plan, nil } +// createDirectedMergePlan builds the single-pair plan {from: to} exactly as the +// user requested, skipping the smaller-into-larger ordering the heuristic +// planner uses. Compatibility and the combined size limit are already checked +// by Do before this runs; here we only reject endpoints that cannot +// participate (read-only, empty, or outside the requested collection). +func (c *commandFsMergeVolumes) createDirectedMergePlan(collection string, from, to needle.VolumeId) (map[needle.VolumeId]needle.VolumeId, error) { + if from == to { + return nil, fmt.Errorf("no volume id changes, %d == %d", from, to) + } + for _, vid := range []needle.VolumeId{from, to} { + volume, err := c.getVolumeInfoById(vid) + if err != nil { + return nil, err + } + if volume.GetReadOnly() { + return nil, fmt.Errorf("volume %d is readonly", vid) + } + if collection != "*" && collection != volume.GetCollection() { + return nil, fmt.Errorf("volume %d is not in collection %q", vid, collection) + } + // Merging into an empty target is valid (e.g. a freshly vacuumed + // volume); only an empty source has nothing to move. + if vid == from && c.getVolumeSize(volume) == 0 { + return nil, fmt.Errorf("volume %d is empty", vid) + } + } + return map[needle.VolumeId]needle.VolumeId{from: to}, nil +} + func (c *commandFsMergeVolumes) getVolumeSizeBasedOnPlan(plan map[needle.VolumeId]needle.VolumeId, vid needle.VolumeId) uint64 { size := c.getVolumeSizeById(vid) for src, dest := range plan { diff --git a/weed/shell/command_fs_merge_volumes_test.go b/weed/shell/command_fs_merge_volumes_test.go new file mode 100644 index 000000000..e368f1343 --- /dev/null +++ b/weed/shell/command_fs_merge_volumes_test.go @@ -0,0 +1,125 @@ +package shell + +import ( + "strings" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" + "github.com/seaweedfs/seaweedfs/weed/storage/needle" +) + +func newMergeCmd(limitMB uint64, vols ...*master_pb.VolumeInformationMessage) *commandFsMergeVolumes { + c := &commandFsMergeVolumes{ + volumes: make(map[needle.VolumeId]*master_pb.VolumeInformationMessage), + volumeSizeLimit: limitMB * 1024 * 1024, + } + for _, v := range vols { + c.volumes[needle.VolumeId(v.Id)] = v + } + return c +} + +// An explicit -fromVolumeId/-toVolumeId pair must be honored as given, even when +// the source is larger than the target. The heuristic planner only ever merges +// a smaller volume into a larger one, which used to make this request a silent +// no-op. +func TestCreateMergePlan_HonorsExplicitDirection(t *testing.T) { + larger := &master_pb.VolumeInformationMessage{Id: 87, Size: 7192590976} + smaller := &master_pb.VolumeInformationMessage{Id: 83, Size: 7088822248} + c := newMergeCmd(250000, larger, smaller) + + plan, err := c.createMergePlan("*", needle.VolumeId(83), needle.VolumeId(87)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := plan[needle.VolumeId(87)]; got != needle.VolumeId(83) { + t.Fatalf("expected 87->83, got plan=%v", plan) + } + + // The reverse direction keeps working too. + plan, err = c.createMergePlan("*", needle.VolumeId(87), needle.VolumeId(83)) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := plan[needle.VolumeId(83)]; got != needle.VolumeId(87) { + t.Fatalf("expected 83->87, got plan=%v", plan) + } +} + +// Merging into an empty target is valid (e.g. consolidating into a freshly +// vacuumed volume); only an empty source should be rejected. +func TestCreateMergePlan_DirectedAllowsEmptyTarget(t *testing.T) { + from := &master_pb.VolumeInformationMessage{Id: 87, Size: 100} + emptyTo := &master_pb.VolumeInformationMessage{Id: 83, Size: 0} + c := newMergeCmd(250000, from, emptyTo) + + plan, err := c.createMergePlan("*", needle.VolumeId(83), needle.VolumeId(87)) + if err != nil { + t.Fatalf("unexpected error merging into empty target: %v", err) + } + if got := plan[needle.VolumeId(87)]; got != needle.VolumeId(83) { + t.Fatalf("expected 87->83, got plan=%v", plan) + } +} + +// The directed planner rejects a self-map on its own, not just via Do, since it +// is exercised directly. +func TestCreateMergePlan_DirectedRejectsSelfMap(t *testing.T) { + v := &master_pb.VolumeInformationMessage{Id: 87, Size: 100} + c := newMergeCmd(250000, v) + + _, err := c.createMergePlan("*", needle.VolumeId(87), needle.VolumeId(87)) + if err == nil || !strings.Contains(err.Error(), "no volume id changes") { + t.Fatalf("expected self-map rejection, got %v", err) + } +} + +func TestCreateMergePlan_DirectedRejectsIneligible(t *testing.T) { + cases := []struct { + name string + from, to *master_pb.VolumeInformationMessage + coll string + wantErr string + }{ + { + name: "readonly source", + from: &master_pb.VolumeInformationMessage{Id: 87, Size: 100, ReadOnly: true}, + to: &master_pb.VolumeInformationMessage{Id: 83, Size: 100}, + coll: "*", + wantErr: "volume 87 is readonly", + }, + { + name: "readonly target", + from: &master_pb.VolumeInformationMessage{Id: 87, Size: 100}, + to: &master_pb.VolumeInformationMessage{Id: 83, Size: 100, ReadOnly: true}, + coll: "*", + wantErr: "volume 83 is readonly", + }, + { + name: "empty source", + from: &master_pb.VolumeInformationMessage{Id: 87, Size: 0}, + to: &master_pb.VolumeInformationMessage{Id: 83, Size: 100}, + coll: "*", + wantErr: "volume 87 is empty", + }, + { + name: "wrong collection", + from: &master_pb.VolumeInformationMessage{Id: 87, Size: 100, Collection: "other"}, + to: &master_pb.VolumeInformationMessage{Id: 83, Size: 100, Collection: "other"}, + coll: "wanted", + wantErr: "volume 87 is not in collection", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := newMergeCmd(250000, tc.from, tc.to) + _, err := c.createMergePlan(tc.coll, needle.VolumeId(tc.to.Id), needle.VolumeId(tc.from.Id)) + if err == nil { + t.Fatalf("expected error %q, got nil", tc.wantErr) + } + if got := err.Error(); !strings.Contains(got, tc.wantErr) { + t.Fatalf("expected error containing %q, got %q", tc.wantErr, got) + } + }) + } +}