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.
2026-06-09 18:32:43 +00:00 · 2026-06-30 13:28:53 -07:00
parent 424cd164e9
commit a653a7f72a
2 changed files with 162 additions and 0 deletions
@@ -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 {
@@ -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)
+			}
+		})
+	}
+}