From 5f787a25c36a915f4f593ff18940ea02259d4597 Mon Sep 17 00:00:00 2001 From: Chris Lu Date: Wed, 2 Sep 2026 11:50:48 -0700 Subject: [PATCH] master: survive a volume layout deleted twice (#11098) * master: survive a layout deleted twice Two volume servers dropping the last replica of volumes that share a layout both find it empty and both delete it. The loser's lookup misses, and the single-value type assertion on the result crashed the master before the caller could look at the found flag. Claude-Session: https://claude.ai/code/session_01WmX6Rchx298NQksHDXg7sk * master: remove a layout and read it back in one step DeleteVolumeLayout looked the layout up and then deleted it, so two deleters could each release the lookup ownership of the same layout, or one could find nothing to release at all. Have the map hand back what it removed. Claude-Session: https://claude.ai/code/session_01WmX6Rchx298NQksHDXg7sk --- weed/topology/collection.go | 11 +-- .../topology/collection_layout_delete_test.go | 72 +++++++++++++++++++ weed/topology/topology.go | 5 +- weed/util/concurrent_read_map.go | 6 +- 4 files changed, 85 insertions(+), 9 deletions(-) create mode 100644 weed/topology/collection_layout_delete_test.go diff --git a/weed/topology/collection.go b/weed/topology/collection.go index 52149308f..9fe355a08 100644 --- a/weed/topology/collection.go +++ b/weed/topology/collection.go @@ -54,7 +54,10 @@ func (c *Collection) GetVolumeLayout(rp *super_block.ReplicaPlacement, ttl *need keyString += string(diskType) } vl, ok := c.storageType2VolumeLayout.Find(keyString) - return vl.(*VolumeLayout), ok + if !ok { + return nil, false + } + return vl.(*VolumeLayout), true } func (c *Collection) GetAllVolumeLayouts() []*VolumeLayout { @@ -76,10 +79,8 @@ func (c *Collection) DeleteVolumeLayout(rp *super_block.ReplicaPlacement, ttl *n keyString += string(diskType) } // Unpublish first so a racing registration re-resolves into a fresh layout. - vl, found := c.GetVolumeLayout(rp, ttl, diskType) - c.storageType2VolumeLayout.Delete(keyString) - if found { - vl.releaseLookupOwnership() + if vl, found := c.storageType2VolumeLayout.Delete(keyString); found { + vl.(*VolumeLayout).releaseLookupOwnership() } } diff --git a/weed/topology/collection_layout_delete_test.go b/weed/topology/collection_layout_delete_test.go new file mode 100644 index 000000000..7b348e878 --- /dev/null +++ b/weed/topology/collection_layout_delete_test.go @@ -0,0 +1,72 @@ +package topology + +import ( + "sync" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb/master_pb" + "github.com/seaweedfs/seaweedfs/weed/sequence" + "github.com/seaweedfs/seaweedfs/weed/storage" + "github.com/seaweedfs/seaweedfs/weed/storage/needle" + "github.com/seaweedfs/seaweedfs/weed/storage/super_block" + "github.com/seaweedfs/seaweedfs/weed/storage/types" +) + +func TestGetVolumeLayoutOfAbsentKey(t *testing.T) { + rp, _ := super_block.NewReplicaPlacementFromString("000") + c := NewCollection("c", 32*1024, false) + + vl, found := c.GetVolumeLayout(rp, needle.EMPTY_TTL, types.HardDriveType) + if found || vl != nil { + t.Fatalf("absent layout: got (%v, %v), want (nil, false)", vl, found) + } +} + +// Volume servers dropping the last replica of several volumes that share one +// layout all see the layout go empty and all delete it. The losers used to +// crash the master on a nil type assertion. +func TestConcurrentLastReplicaRemoval(t *testing.T) { + const nodeCount = 8 + ttl, _ := needle.ReadTTL("5m") + for round := 0; round < 500; round++ { + topo := NewTopology("weedfs", sequence.NewMemorySequencer(), 32*1024, 5, false) + rack := topo.GetOrCreateDataCenter("dc1").GetOrCreateRack("rack1") + + // A second layout keeps the collection alive, so every remover reaches + // the layout deletion instead of stopping at a vanished collection. + keeper := rack.GetOrCreateDataNode("127.0.0.1", 9000, 0, "", "", map[string]uint32{"": 100}) + topo.SyncDataNodeRegistration([]*master_pb.VolumeInformationMessage{{ + Id: 999, Collection: "c", Version: uint32(needle.GetCurrentVersion()), Ttl: ttl.ToUint32(), + }}, keeper) + + var wg sync.WaitGroup + start := make(chan struct{}) + for i := 0; i < nodeCount; i++ { + dn := rack.GetOrCreateDataNode("127.0.0.1", 8080+i, 0, "", "", map[string]uint32{"": 100}) + m := &master_pb.VolumeInformationMessage{ + Id: uint32(i + 1), Collection: "c", Version: uint32(needle.GetCurrentVersion()), + } + topo.SyncDataNodeRegistration([]*master_pb.VolumeInformationMessage{m}, dn) + vi, err := storage.NewVolumeInfo(m) + if err != nil { + t.Fatalf("NewVolumeInfo: %v", err) + } + wg.Add(1) + go func() { + defer wg.Done() + <-start + topo.UnRegisterVolumeLayout(vi, dn) + }() + } + close(start) + wg.Wait() + + c, found := topo.FindCollection("c") + if !found { + t.Fatalf("round %d: collection dropped while a layout still had volumes", round) + } + if layouts := c.GetAllVolumeLayouts(); len(layouts) != 1 { + t.Fatalf("round %d: got %d layouts, want only the one still holding a volume", round, len(layouts)) + } + } +} diff --git a/weed/topology/topology.go b/weed/topology/topology.go index 276d7a092..5155eaab0 100644 --- a/weed/topology/topology.go +++ b/weed/topology/topology.go @@ -533,12 +533,11 @@ func (t *Topology) DeleteCollection(collectionName string) { // the node's held and servable digests apart forever, and the master asks // for the full volume list on every heartbeat from then on. // Unpublish first so a racing registration re-resolves into a fresh collection. - collection, found := t.FindCollection(collectionName) - t.collectionMap.Delete(collectionName) + collection, found := t.collectionMap.Delete(collectionName) if !found { return } - for _, vl := range collection.GetAllVolumeLayouts() { + for _, vl := range collection.(*Collection).GetAllVolumeLayouts() { vl.releaseLookupOwnership() } } diff --git a/weed/util/concurrent_read_map.go b/weed/util/concurrent_read_map.go index 28b6ae0f1..546c2551a 100644 --- a/weed/util/concurrent_read_map.go +++ b/weed/util/concurrent_read_map.go @@ -53,8 +53,12 @@ func (m *ConcurrentReadMap) Items() (itemsCopy []interface{}) { return itemsCopy } -func (m *ConcurrentReadMap) Delete(key string) { +// Delete removes the key and returns what it held, so a caller that has to +// wind the entry down does not race another deleter for it. +func (m *ConcurrentReadMap) Delete(key string) (interface{}, bool) { m.Lock() + value, ok := m.items[key] delete(m.items, key) m.Unlock() + return value, ok }