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 }