From a9c0ed91b5807800ec90302a9c77b8791d7fe818 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Tue, 30 Jun 2026 15:35:27 -0700 Subject: [PATCH] fix(topology): keep physical disk 0 distinct in SplitByPhysicalDisk (#10161) * fix(topology): keep physical disk 0 distinct in SplitByPhysicalDisk DiskId 0 doubles as the first physical disk (Locations[0]) and the protobuf "unset" default. SplitByPhysicalDisk folded every DiskId-0 record onto the aggregate DiskId whenever that was non-zero, so on a multi-disk node the first disk's volumes merged into whichever disk held volumes[0]: the node reported one fewer disk, the sibling showed ~2x volumes, and per-disk max was smeared across the survivors. This surfaced as cluster.status and volume.list undercounting disks. Only treat 0 as unset when no record carries a non-zero DiskId; with a mix, 0 is a real disk and keeps its own entry. * fix(admin): resolve physical disk 0 in active-topology indexes rebuildIndexes re-derived each volume/EC record's physical disk id with the same "DiskId 0 means unset" heuristic SplitByPhysicalDisk used, so the two agreed only by sharing the bug. Now that SplitByPhysicalDisk keeps disk 0 distinct, the duplicated heuristic would fold disk-0 records onto a sibling while at.disks kept them on disk 0; GetVolumeLocations and GetECShardLocations then matched no record and silently dropped every volume and EC shard on the first disk, starving balance and EC tasks. Build the indexes from the same SplitByPhysicalDisk reconstruction that builds at.disks, so the keys always resolve. One source of truth instead of a parallel normalize. * fix(ec): allow physical disk 0 as preferred EC shard target pickBestDiskOnNode gated its result on bestDiskId != 0, but 0 is both a valid physical disk and the uint32 zero value, so a best-scoring disk 0 was discarded and the non-matching fallback returned instead. Gate on bestScore. * test(admin): cover EC-shard index resolution for physical disk 0 rebuildIndexes builds ecShardIndex the same way as volumeIndex; pin the EC path too so a shard on disk 0 keeps resolving via GetECShardLocations. --- weed/admin/topology/active_topology_test.go | 77 +++++++++++++++++++++ weed/admin/topology/topology_management.go | 25 +++---- weed/pb/master_pb/master_helper.go | 24 ++++++- weed/pb/master_pb/master_helper_test.go | 50 +++++++++++++ weed/shell/command_ec_common.go | 6 +- weed/shell/command_ec_test.go | 18 +++++ 6 files changed, 182 insertions(+), 18 deletions(-) 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{