diff --git a/weed/admin/topology/active_topology_test.go b/weed/admin/topology/active_topology_test.go index 79aef9465..6be5ad798 100644 --- a/weed/admin/topology/active_topology_test.go +++ b/weed/admin/topology/active_topology_test.go @@ -459,6 +459,83 @@ func TestECPlannerSeesEachPhysicalDisk(t *testing.T) { } } +// TestVolumeIndexResolvesPhysicalDiskZero pins that a volume genuinely on +// physical disk 0 resolves to its own disk via GetVolumeLocations even when the +// aggregate DiskInfo.DiskId is seeded from a non-zero sibling (as ToDiskInfo +// does from volumes[0]). Re-deriving the per-record disk id would fold disk 0 +// onto the sibling, then the lookup would find no matching volume and drop it. +func TestVolumeIndexResolvesPhysicalDiskZero(t *testing.T) { + topology := NewActiveTopology(10) + + require.NoError(t, topology.UpdateTopology(&master_pb.TopologyInfo{ + DataCenterInfos: []*master_pb.DataCenterInfo{{ + Id: "dc1", + RackInfos: []*master_pb.RackInfo{{ + Id: "rack1", + DataNodeInfos: []*master_pb.DataNodeInfo{{ + Id: "127.0.0.1:8080", + DiskInfos: map[string]*master_pb.DiskInfo{ + "hdd": { + DiskId: 6, // seeded from volumes[0].DiskId + MaxVolumeCount: 200, + VolumeInfos: []*master_pb.VolumeInformationMessage{ + {Id: 11, DiskId: 6, DiskType: "hdd"}, + {Id: 10, DiskId: 0, DiskType: "hdd"}, + }, + }, + }, + }}, + }}, + }}, + })) + + loc10 := topology.GetVolumeLocations(10, "") + require.Len(t, loc10, 1, "volume on physical disk 0 must resolve to one disk") + assert.Equal(t, uint32(0), loc10[0].DiskID, "volume 10 lives on physical disk 0") + + loc11 := topology.GetVolumeLocations(11, "") + require.Len(t, loc11, 1) + assert.Equal(t, uint32(6), loc11[0].DiskID, "volume 11 lives on physical disk 6") +} + +// TestECShardIndexResolvesPhysicalDiskZero is the EC-shard twin of +// TestVolumeIndexResolvesPhysicalDiskZero: rebuildIndexes builds ecShardIndex +// the same way, so an EC shard on physical disk 0 must resolve via +// GetECShardLocations rather than being folded onto a non-zero sibling. +func TestECShardIndexResolvesPhysicalDiskZero(t *testing.T) { + topology := NewActiveTopology(10) + + require.NoError(t, topology.UpdateTopology(&master_pb.TopologyInfo{ + DataCenterInfos: []*master_pb.DataCenterInfo{{ + Id: "dc1", + RackInfos: []*master_pb.RackInfo{{ + Id: "rack1", + DataNodeInfos: []*master_pb.DataNodeInfo{{ + Id: "127.0.0.1:8080", + DiskInfos: map[string]*master_pb.DiskInfo{ + "hdd": { + DiskId: 6, // seeded from a non-zero sibling + MaxVolumeCount: 200, + EcShardInfos: []*master_pb.VolumeEcShardInformationMessage{ + {Id: 21, DiskId: 6, Collection: "c1", EcIndexBits: 0b1}, + {Id: 20, DiskId: 0, Collection: "c1", EcIndexBits: 0b1}, + }, + }, + }, + }}, + }}, + }}, + })) + + loc20 := topology.GetECShardLocations(20, "c1") + require.Len(t, loc20, 1, "EC shard on physical disk 0 must resolve to one disk") + assert.Equal(t, uint32(0), loc20[0].DiskID, "shard 20 lives on physical disk 0") + + loc21 := topology.GetECShardLocations(21, "c1") + require.Len(t, loc21, 1) + assert.Equal(t, uint32(6), loc21[0].DiskID, "shard 21 lives on physical disk 6") +} + // TestPublicInterfaces tests the public interface methods func TestPublicInterfaces(t *testing.T) { topology := NewActiveTopology(10) diff --git a/weed/admin/topology/topology_management.go b/weed/admin/topology/topology_management.go index 72fad39f5..adc512b82 100644 --- a/weed/admin/topology/topology_management.go +++ b/weed/admin/topology/topology_management.go @@ -216,27 +216,22 @@ func (at *ActiveTopology) rebuildIndexes() { at.volumeIndex = make(map[uint32][]string) at.ecShardIndex = make(map[uint32][]string) - // Index by the per-record DiskId (not the outer DiskInfo.DiskId) so the - // keys match the per-physical-disk activeDisk entries — see #9369. + // Derive index keys from the same SplitByPhysicalDisk reconstruction that + // builds at.disks, so every volume/EC lookup resolves to a real activeDisk + // entry. Re-deriving the per-record disk id here would diverge on physical + // disk 0, which the raw DiskId cannot tell apart from the "unset" default. for _, dc := range at.topologyInfo.DataCenterInfos { for _, rack := range dc.RackInfos { for _, nodeInfo := range rack.DataNodeInfos { for _, diskInfo := range nodeInfo.DiskInfos { - for _, volumeInfo := range diskInfo.VolumeInfos { - diskID := volumeInfo.DiskId - if diskID == 0 && diskInfo.DiskId != 0 { - diskID = diskInfo.DiskId + for _, perDisk := range diskInfo.SplitByPhysicalDisk() { + diskKey := fmt.Sprintf("%s:%d", nodeInfo.Id, perDisk.DiskId) + for _, volumeInfo := range perDisk.VolumeInfos { + at.volumeIndex[volumeInfo.Id] = append(at.volumeIndex[volumeInfo.Id], diskKey) } - diskKey := fmt.Sprintf("%s:%d", nodeInfo.Id, diskID) - at.volumeIndex[volumeInfo.Id] = append(at.volumeIndex[volumeInfo.Id], diskKey) - } - for _, ecShardInfo := range diskInfo.EcShardInfos { - diskID := ecShardInfo.DiskId - if diskID == 0 && diskInfo.DiskId != 0 { - diskID = diskInfo.DiskId + for _, ecShardInfo := range perDisk.EcShardInfos { + at.ecShardIndex[ecShardInfo.Id] = append(at.ecShardIndex[ecShardInfo.Id], diskKey) } - diskKey := fmt.Sprintf("%s:%d", nodeInfo.Id, diskID) - at.ecShardIndex[ecShardInfo.Id] = append(at.ecShardIndex[ecShardInfo.Id], diskKey) } } } diff --git a/weed/pb/master_pb/master_helper.go b/weed/pb/master_pb/master_helper.go index 75e183f3c..75de27c64 100644 --- a/weed/pb/master_pb/master_helper.go +++ b/weed/pb/master_pb/master_helper.go @@ -24,8 +24,30 @@ func (d *DiskInfo) SplitByPhysicalDisk() []*DiskInfo { return nil } + // DiskId 0 is overloaded: it is both the first physical disk (Locations[0]) + // and the protobuf default for "unset". Only treat 0 as unset when every + // record reports 0 (the legacy case where the volume server didn't populate + // DiskId). If any record carries a non-zero DiskId, the reporting is real and + // a 0 means physical disk 0 — keep it distinct instead of folding it onto + // d.DiskId, which would merge two physical disks and drop disk 0 from view. + hasNonZero := false + for _, vi := range d.VolumeInfos { + if vi.DiskId != 0 { + hasNonZero = true + break + } + } + if !hasNonZero { + for _, eci := range d.EcShardInfos { + if eci.DiskId != 0 { + hasNonZero = true + break + } + } + } + normalize := func(id uint32) uint32 { - if id == 0 && d.DiskId != 0 { + if id == 0 && !hasNonZero && d.DiskId != 0 { return d.DiskId } return id diff --git a/weed/pb/master_pb/master_helper_test.go b/weed/pb/master_pb/master_helper_test.go index 1e23189d7..ff202c50c 100644 --- a/weed/pb/master_pb/master_helper_test.go +++ b/weed/pb/master_pb/master_helper_test.go @@ -203,6 +203,56 @@ func TestDiskInfoSplitByPhysicalDisk_normalizesZeroToOuterDiskId(t *testing.T) { } } +// TestDiskInfoSplitByPhysicalDisk_keepsRealDiskZeroWhenMixed reproduces a +// multi-disk node where physical disk 0 (Locations[0]) holds volumes and the +// aggregate DiskId is seeded from a non-zero sibling (volumes[0] landed on +// disk 6). DiskId 0 must stay its own physical disk, not get folded onto disk +// 6 — folding drops a disk from the count and doubles the sibling's volumes. +func TestDiskInfoSplitByPhysicalDisk_keepsRealDiskZeroWhenMixed(t *testing.T) { + d := &DiskInfo{ + Type: "hdd", + MaxVolumeCount: 30, + DiskId: 6, // seeded from volumes[0].DiskId in ToDiskInfo + VolumeInfos: []*VolumeInformationMessage{ + {Id: 60, DiskId: 6}, + {Id: 61, DiskId: 6}, + {Id: 1, DiskId: 0}, // real disk 0, must not be remapped to 6 + {Id: 2, DiskId: 0}, + {Id: 30, DiskId: 3}, + }, + } + + got := d.SplitByPhysicalDisk() + byID := map[uint32]*DiskInfo{} + for _, di := range got { + byID[di.DiskId] = di + } + + if len(got) != 3 { + t.Fatalf("want 3 physical disks (0, 3, 6), got %d: %v", len(got), byID) + } + if _, ok := byID[0]; !ok { + t.Fatalf("physical disk 0 was dropped; got disks %v", byID) + } + if byID[0].VolumeCount != 2 { + t.Errorf("disk 0: want 2 volumes, got %d", byID[0].VolumeCount) + } + if byID[6].VolumeCount != 2 { + t.Errorf("disk 6: want 2 volumes (not merged with disk 0), got %d", byID[6].VolumeCount) + } + if byID[3].VolumeCount != 1 { + t.Errorf("disk 3: want 1 volume, got %d", byID[3].VolumeCount) + } + + var sumMax int64 + for _, di := range got { + sumMax += di.MaxVolumeCount + } + if sumMax != 30 { + t.Errorf("sum of MaxVolumeCount = %d, want 30 (lossless split)", sumMax) + } +} + func TestDiskInfoSplitByPhysicalDisk_nilSafe(t *testing.T) { var d *DiskInfo if got := d.SplitByPhysicalDisk(); got != nil { diff --git a/weed/shell/command_ec_common.go b/weed/shell/command_ec_common.go index 718f20f42..ef12e0d18 100644 --- a/weed/shell/command_ec_common.go +++ b/weed/shell/command_ec_common.go @@ -820,8 +820,10 @@ func pickBestDiskOnNode(ecNode *EcNode, vid needle.VolumeId, diskType types.Disk } } - // Return matching disk type if found, otherwise fallback - if bestDiskId != 0 { + // Return matching disk type if found, otherwise fallback. Gate on bestScore, + // not bestDiskId: physical disk 0 is a valid target and 0 is also the "no + // match" zero value, so testing bestDiskId would never select disk 0. + if bestScore != -1 { return bestDiskId } return fallbackDiskId diff --git a/weed/shell/command_ec_test.go b/weed/shell/command_ec_test.go index 05db28149..63a307446 100644 --- a/weed/shell/command_ec_test.go +++ b/weed/shell/command_ec_test.go @@ -9,6 +9,24 @@ import ( "github.com/seaweedfs/seaweedfs/weed/storage/types" ) +// TestPickBestDiskOnNodeSelectsPhysicalDiskZero verifies physical disk 0 can be +// chosen as the preferred shard target. Gating on bestDiskId != 0 discarded a +// best-scoring disk 0 (0 is also the zero value) and returned the fallback. +func TestPickBestDiskOnNodeSelectsPhysicalDiskZero(t *testing.T) { + ecNode := &EcNode{ + disks: map[uint32]*EcDisk{ + 0: {diskId: 0, diskType: string(types.SsdType), freeEcSlots: 10, ecShards: map[needle.VolumeId]*erasure_coding.ShardsInfo{}}, + 5: {diskId: 5, diskType: string(types.HardDriveType), freeEcSlots: 10, ecShards: map[needle.VolumeId]*erasure_coding.ShardsInfo{}}, + }, + } + + // Disk 0 matches the requested type; it must win over the non-matching + // fallback on disk 5 instead of being treated as "no match". + if got := pickBestDiskOnNode(ecNode, needle.VolumeId(42), types.SsdType, false, 0, 0); got != 0 { + t.Fatalf("want physical disk 0 (matching type), got %d", got) + } +} + func TestCommandEcBalanceSmall(t *testing.T) { ecb := &ecBalancer{ ecNodes: []*EcNode{