From a6effe3cfbf4c85e8d0aa88022173f7d91f0899e Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Fri, 3 Jul 2026 14:42:33 -0700 Subject: [PATCH] master: repair the lookup index after a vacuum that raced a disconnect (#10226) * master: re-create a vacuum-committed volume the index has lost SetVolumeAvailable dereferenced vid2location[vid] with no nil check. A disconnect during a long vacuum can drop a single-replica volume from the lookup index while it stays on the node; the commit then panics the master instead of re-registering it. Re-create the entry, and seed its size tracking so assigns are counted right away rather than after the next heartbeat. * master: re-register a vacuumed volume the index lost on mark-writable The maintenance worker re-enables a volume by marking it writable after the vacuum commit, but VolumeMarkReadonly only updated nodes the lookup index already knew. If a disconnect race dropped the volume from the index during the vacuum, that path was a no-op and the volume stayed "not found" until the next full heartbeat healed it. Re-register it from the node that still holds it. --- weed/server/master_grpc_server_volume.go | 22 ++++++++-- weed/topology/topology_test.go | 51 ++++++++++++++++++++++++ weed/topology/volume_layout.go | 43 ++++++++++++++------ 3 files changed, 100 insertions(+), 16 deletions(-) diff --git a/weed/server/master_grpc_server_volume.go b/weed/server/master_grpc_server_volume.go index 07d05d7b9..184a6c214 100644 --- a/weed/server/master_grpc_server_volume.go +++ b/weed/server/master_grpc_server_volume.go @@ -15,6 +15,7 @@ import ( "github.com/seaweedfs/raft" "github.com/seaweedfs/seaweedfs/weed/glog" + "github.com/seaweedfs/seaweedfs/weed/pb" "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" "github.com/seaweedfs/seaweedfs/weed/security" "github.com/seaweedfs/seaweedfs/weed/storage/needle" @@ -323,18 +324,33 @@ func (ms *MasterServer) VolumeMarkReadonly(ctx context.Context, req *master_pb.V replicaPlacement, _ := super_block.NewReplicaPlacementFromByte(byte(req.ReplicaPlacement)) vl := ms.Topo.GetVolumeLayout(req.Collection, replicaPlacement, needle.LoadTTLFromUint32(req.Ttl), types.ToDiskType(req.DiskType)) - dataNodes := ms.Topo.Lookup(req.Collection, needle.VolumeId(req.VolumeId)) + vid := needle.VolumeId(req.VolumeId) + dataNodes := ms.Topo.Lookup(req.Collection, vid) + found := false for _, dn := range dataNodes { if dn.Ip == req.Ip && dn.Port == int(req.Port) { + found = true if req.IsReadonly { - vid := needle.VolumeId(req.VolumeId) vl.SetVolumeReadOnly(dn, vid) if pending := vl.GetPendingSize(vid); pending > 0 { glog.V(0).Infof("volume %d marked readonly with %d pending bytes", vid, pending) } } else { - vl.SetVolumeWritable(dn, needle.VolumeId(req.VolumeId)) + vl.SetVolumeWritable(dn, vid) + } + } + } + + // A vacuum worker marks the volume writable after a successful commit. If the + // index lost it meanwhile (e.g. a disconnect race during the vacuum), the loop + // found nothing to update; re-register it from the node that still holds it so + // LookupVolume stops returning "not found". + if !found && !req.IsReadonly { + if dn := ms.Topo.LookupDataNodeByAddress(pb.NewServerAddress(req.Ip, int(req.Port), 0)); dn != nil { + if vi, err := dn.GetVolumesById(vid); err == nil { + ms.Topo.RegisterVolumeLayout(vi, dn) + glog.V(0).Infof("volume %d re-registered from %s:%d, was missing from the lookup index at mark-writable", vid, req.Ip, req.Port) } } } diff --git a/weed/topology/topology_test.go b/weed/topology/topology_test.go index 99124a600..32c3ad7e4 100644 --- a/weed/topology/topology_test.go +++ b/weed/topology/topology_test.go @@ -626,3 +626,54 @@ func TestSyncDataNodeRegistrationReRegistersMissingVolume(t *testing.T) { t.Fatalf("after self-heal: lookup %d got %v, want 1 location", vid, got) } } + +// TestSetVolumeAvailableRepairsMissingVolume covers the vacuum-commit variant of +// the same divergence. A disconnect during a long vacuum can drop a +// single-replica volume from the lookup index while it stays on the node; the +// commit then calls SetVolumeAvailable on it. That used to dereference a nil +// location and panic; it now re-creates the entry and repairs the split. +func TestSetVolumeAvailableRepairsMissingVolume(t *testing.T) { + topo := NewTopology("weedfs", sequence.NewMemorySequencer(), 32*1024, 5, false) + + dc := topo.GetOrCreateDataCenter("dc1") + rack := dc.GetOrCreateRack("rack1") + dn := rack.GetOrCreateDataNode("127.0.0.1", 34534, 0, "127.0.0.1", "", map[string]uint32{"": 25}) + + vid := needle.VolumeId(2640) + volumeMessage := &master_pb.VolumeInformationMessage{ + Id: uint32(vid), + Size: 100, + Collection: "drr", + ReplicaPlacement: uint32(0), + Version: uint32(needle.GetCurrentVersion()), + Ttl: 0, + } + + topo.SyncDataNodeRegistration([]*master_pb.VolumeInformationMessage{volumeMessage}, dn) + + rp, _ := super_block.NewReplicaPlacementFromString("000") + vl := topo.GetVolumeLayout("drr", rp, needle.EMPTY_TTL, types.HardDriveType) + + // Disconnect drops the volume from the index but leaves it on the node. + vl.SetVolumeUnavailable(dn, vid) + if got := topo.Lookup("", vid); got != nil { + t.Fatalf("after SetVolumeUnavailable: expected lookup miss, got %v", got) + } + if _, err := dn.GetVolumesById(vid); err != nil { + t.Fatalf("volume %d should still be in the data node disk map: %v", vid, err) + } + + // The vacuum commit re-marks the volume available; it must re-register it. + vl.SetVolumeAvailable(dn, vid, false, false) + if got := topo.Lookup("", vid); len(got) != 1 { + t.Fatalf("after SetVolumeAvailable: lookup %d got %v, want 1 location", vid, got) + } + // Size tracking must be seeded too, or assigns go uncounted until the next + // heartbeat and the volume can overfill. + vl.accessLock.RLock() + _, tracked := vl.sizeTracking[vid] + vl.accessLock.RUnlock() + if !tracked { + t.Fatalf("after SetVolumeAvailable: size tracking for %d not seeded", vid) + } +} diff --git a/weed/topology/volume_layout.go b/weed/topology/volume_layout.go index 3f47c0110..cab8510c7 100644 --- a/weed/topology/volume_layout.go +++ b/weed/topology/volume_layout.go @@ -164,24 +164,37 @@ func (vl *VolumeLayout) String() string { return fmt.Sprintf("rp:%v, ttl:%v, writables:%v, volumeSizeLimit:%v", vl.rp, vl.ttl, vl.writables, vl.volumeSizeLimit) } +// getOrCreateLocationList returns the vid's location list, creating an empty one +// if the index has none. Callers hold accessLock. +func (vl *VolumeLayout) getOrCreateLocationList(vid needle.VolumeId) *VolumeLocationList { + location, ok := vl.vid2location[vid] + if !ok { + location = NewVolumeLocationList() + vl.vid2location[vid] = location + } + return location +} + +// initSizeTracking seeds a vid's size tracking from a reported size if it has +// none yet. Callers hold accessLock. +func (vl *VolumeLayout) initSizeTracking(vid needle.VolumeId, size uint64, compactRevision uint32) { + if _, exists := vl.sizeTracking[vid]; !exists { + vl.sizeTracking[vid] = &volumeSizeTracking{ + effectiveSize: size, + reportedSize: size, + compactRevision: compactRevision, + } + } +} + func (vl *VolumeLayout) RegisterVolume(v *storage.VolumeInfo, dn *DataNode) { vl.accessLock.Lock() defer vl.accessLock.Unlock() defer vl.rememberOversizedVolume(v, dn) - if _, ok := vl.vid2location[v.Id]; !ok { - vl.vid2location[v.Id] = NewVolumeLocationList() - } - vl.vid2location[v.Id].Set(dn) - // For new volumes, initialize size tracking from reported size. - if _, exists := vl.sizeTracking[v.Id]; !exists { - vl.sizeTracking[v.Id] = &volumeSizeTracking{ - effectiveSize: v.Size, - reportedSize: v.Size, - compactRevision: v.CompactRevision, - } - } + vl.getOrCreateLocationList(v.Id).Set(dn) + vl.initSizeTracking(v.Id, v.Size, v.CompactRevision) // glog.V(4).Infof("volume %d added to %s len %d copy %d", v.Id, dn.Id(), vl.vid2location[v.Id].Length(), v.ReplicaPlacement.GetCopyCount()) for _, dn := range vl.vid2location[v.Id].list { if vInfo, err := dn.GetVolumesById(v.Id); err == nil { @@ -886,7 +899,11 @@ func (vl *VolumeLayout) SetVolumeAvailable(dn *DataNode, vid needle.VolumeId, is return false } - vl.vid2location[vid].Set(dn) + // A disconnect during a long vacuum can drop the entry while the volume is + // still on the node; re-create it (and seed size tracking) instead of + // dereferencing a nil location, so the commit also repairs the split. + vl.getOrCreateLocationList(vid).Set(dn) + vl.initSizeTracking(vid, vInfo.Size, vInfo.CompactRevision) if vInfo.ReadOnly || isReadOnly || isFullCapacity { return false