diff --git a/weed/wdclient/vid_map.go b/weed/wdclient/vid_map.go index 1cb917abe..fdae7dfca 100644 --- a/weed/wdclient/vid_map.go +++ b/weed/wdclient/vid_map.go @@ -8,7 +8,6 @@ import ( "strconv" "strings" "sync" - "sync/atomic" "github.com/seaweedfs/seaweedfs/weed/pb" @@ -32,10 +31,19 @@ func (l Location) ServerAddress() pb.ServerAddress { return pb.NewServerAddressWithGrpcPort(l.Url, l.GrpcPort) } +// locationsEntry is what a volume id maps to: the locations themselves plus the +// generation they were learned in. An entry is immutable once stored; every +// update installs a new one, so locations handed to a reader are never +// rewritten underneath it. +type locationsEntry struct { + locations []Location + generation uint64 +} + type vidMap struct { sync.RWMutex - vid2Locations map[uint32][]Location - ecVid2Locations map[uint32][]Location + vid2Locations map[uint32]*locationsEntry + ecVid2Locations map[uint32]*locationsEntry // serverRefCount tracks how many vid locations (regular + EC) currently // reference each volume server address. Maintaining it incrementally lets // hasVolumeServer answer in O(1) instead of walking every volume entry. @@ -43,15 +51,25 @@ type vidMap struct { // pass either "host:port" or "host:port.grpc" find the same entry. serverRefCount map[string]int DataCenter string - cache atomic.Pointer[vidMap] + // generation counts resets. Each entry remembers the generation it was + // learned in, so history expires per volume rather than by keeping + // snapshot copies of the whole map. + generation uint64 + // retainGenerations is how many resets an entry survives without being + // refreshed before reset drops it. + retainGenerations uint64 } -func newVidMap(dataCenter string) *vidMap { +func newVidMap(dataCenter string, retainGenerations int) *vidMap { + if retainGenerations <= 0 { + retainGenerations = DefaultVidMapCacheSize + } return &vidMap{ - vid2Locations: make(map[uint32][]Location), - ecVid2Locations: make(map[uint32][]Location), - serverRefCount: make(map[string]int), - DataCenter: dataCenter, + vid2Locations: make(map[uint32]*locationsEntry), + ecVid2Locations: make(map[uint32]*locationsEntry), + serverRefCount: make(map[string]int), + DataCenter: dataCenter, + retainGenerations: uint64(retainGenerations), } } @@ -128,31 +146,42 @@ func (vc *vidMap) GetVidLocations(vid string) (locations []Location, err error) } func (vc *vidMap) GetLocations(vid uint32) (locations []Location, found bool) { - // glog.V(4).Infof("~ lookup volume id %d: %+v ec:%+v", vid, vc.vid2Locations, vc.ecVid2Locations) - // Read the cache link before the live map: resetVidMap trims the chain, so a - // link loaded after the local miss may already be severed, turning a lookup - // that could have been served by the cache into a spurious miss. - cachedMap := vc.cache.Load() + vc.RLock() + defer vc.RUnlock() - locations, found = vc.getLocations(vid) - if found { - // If volume is explicitly tracked (found=true), return its locations even if empty. - // An empty array means "volume has no locations" (e.g., during pod restart), - // which is different from "volume never existed" (found=false). - // Don't fall back to stale cache for explicitly empty volumes. - if len(locations) > 0 { - return locations, found + regular, hasRegular := lookupEntry(vc.vid2Locations, vid) + ec, hasEc := lookupEntry(vc.ecVid2Locations, vid) + + switch { + case hasRegular && hasEc: + // Whichever was learned last wins: once a volume is EC encoded, the + // regular copies a previous generation knew must stop answering for + // it, and a decoded volume must stop answering with its shards. A tie + // means one generation reported both, where the regular copies serve. + if ec.generation > regular.generation { + return ec.locations, true } - // Volume exists but has no locations - return empty, don't check cache + return regular.locations, true + case hasRegular: + return regular.locations, true + case hasEc: + return ec.locations, true + } + + // Nothing older to fall back to: a volume's history lives in its own entry, + // so a volume whose locations are all gone (a pod restarting, say) is a + // miss rather than a reason to serve what it used to have. + return nil, false +} + +// lookupEntry returns vid's entry when it still holds locations. Callers must +// hold the lock. +func lookupEntry(vid2Locations map[uint32]*locationsEntry, vid uint32) (*locationsEntry, bool) { + entry, found := vid2Locations[vid] + if !found || len(entry.locations) == 0 { return nil, false } - - // Volume not found in current map - check cache for unknown volumes - if cachedMap != nil { - return cachedMap.GetLocations(vid) - } - - return nil, false + return entry, true } func (vc *vidMap) GetLocationsClone(vid uint32) (locations []Location, found bool) { @@ -168,41 +197,17 @@ func (vc *vidMap) GetLocationsClone(vid uint32) (locations []Location, found boo return nil, false } -func (vc *vidMap) getLocations(vid uint32) (locations []Location, found bool) { - vc.RLock() - defer vc.RUnlock() - - locations, found = vc.vid2Locations[vid] - if found && len(locations) > 0 { - return - } - locations, found = vc.ecVid2Locations[vid] - return -} - // hasVolumeServer reports whether any tracked volume (regular or EC) is hosted -// on addr. It walks the cache chain so recently expired maps are still -// considered. Used to gate admission of operations targeting a volume server. -// The lookup is O(1) thanks to serverRefCount; we still consult the cache -// chain to keep covering volume servers that just rolled out of the live map. +// on addr, including volumes still held from earlier generations. Used to gate +// admission of operations targeting a volume server. func (vc *vidMap) hasVolumeServer(addr pb.ServerAddress) bool { key := addr.ToHttpAddress() if key == "" { return false } - // Same ordering requirement as GetLocations: grab the cache link before the - // local lookup so a concurrent reset cannot sever it underneath us. - cachedMap := vc.cache.Load() vc.RLock() - count := vc.serverRefCount[key] - vc.RUnlock() - if count > 0 { - return true - } - if cachedMap != nil { - return cachedMap.hasVolumeServer(addr) - } - return false + defer vc.RUnlock() + return vc.serverRefCount[key] > 0 } func (vc *vidMap) addLocation(vid uint32, location Location) { @@ -211,22 +216,7 @@ func (vc *vidMap) addLocation(vid uint32, location Location) { glog.V(4).Infof("+ volume id %d: %+v", vid, location) - locations, found := vc.vid2Locations[vid] - if !found { - vc.vid2Locations[vid] = []Location{location} - vc.incrementServerRef(locationServerKey(location)) - return - } - - for _, loc := range locations { - if loc.Url == location.Url { - return - } - } - - vc.vid2Locations[vid] = append(locations, location) - vc.incrementServerRef(locationServerKey(location)) - + vc.addLocationToMap(vc.vid2Locations, vid, location) } func (vc *vidMap) addEcLocation(vid uint32, location Location) { @@ -235,88 +225,133 @@ func (vc *vidMap) addEcLocation(vid uint32, location Location) { glog.V(4).Infof("+ ec volume id %d: %+v", vid, location) - locations, found := vc.ecVid2Locations[vid] - if !found { - vc.ecVid2Locations[vid] = []Location{location} + vc.addLocationToMap(vc.ecVid2Locations, vid, location) +} + +// addLocationToMap records location for vid. The first write of a generation +// replaces what an earlier one held instead of merging with it: after a reset +// the new master is the authority, so a volume that moved must not keep +// answering with the server it moved off. Callers must hold the write lock. +func (vc *vidMap) addLocationToMap(vid2Locations map[uint32]*locationsEntry, vid uint32, location Location) { + entry, found := vid2Locations[vid] + if !found || entry.generation != vc.generation { + if found { + vc.releaseEntry(entry) + } + vid2Locations[vid] = &locationsEntry{ + locations: []Location{location}, + generation: vc.generation, + } vc.incrementServerRef(locationServerKey(location)) return } - for _, loc := range locations { + for _, loc := range entry.locations { if loc.Url == location.Url { return } } - vc.ecVid2Locations[vid] = append(locations, location) + locations := make([]Location, 0, len(entry.locations)+1) + locations = append(locations, entry.locations...) + locations = append(locations, location) + vid2Locations[vid] = &locationsEntry{locations: locations, generation: entry.generation} vc.incrementServerRef(locationServerKey(location)) - } func (vc *vidMap) deleteLocation(vid uint32, location Location) { - if cachedMap := vc.cache.Load(); cachedMap != nil { - cachedMap.deleteLocation(vid, location) - } - vc.Lock() defer vc.Unlock() glog.V(4).Infof("- volume id %d: %+v", vid, location) - locations, found := vc.vid2Locations[vid] - if !found { - return - } - - for i, loc := range locations { - if loc.Url == location.Url { - vc.vid2Locations[vid] = append(locations[0:i], locations[i+1:]...) - vc.decrementServerRef(locationServerKey(loc)) - break - } - } + vc.deleteLocationFromMap(vc.vid2Locations, vid, location) } func (vc *vidMap) deleteEcLocation(vid uint32, location Location) { - if cachedMap := vc.cache.Load(); cachedMap != nil { - cachedMap.deleteEcLocation(vid, location) - } - vc.Lock() defer vc.Unlock() glog.V(4).Infof("- ec volume id %d: %+v", vid, location) - locations, found := vc.ecVid2Locations[vid] + vc.deleteLocationFromMap(vc.ecVid2Locations, vid, location) +} + +// deleteLocationFromMap drops one location from vid's entry, and the entry +// itself once its last location is gone. The generation is untouched: a delete +// only speaks about the location it names, it does not make the rest of the +// entry any fresher. Callers must hold the write lock. +func (vc *vidMap) deleteLocationFromMap(vid2Locations map[uint32]*locationsEntry, vid uint32, location Location) { + entry, found := vid2Locations[vid] if !found { return } - for i, loc := range locations { - if loc.Url == location.Url { - vc.ecVid2Locations[vid] = append(locations[0:i], locations[i+1:]...) - vc.decrementServerRef(locationServerKey(loc)) - break + for i, loc := range entry.locations { + if loc.Url != location.Url { + continue } + vc.decrementServerRef(locationServerKey(loc)) + if len(entry.locations) == 1 { + delete(vid2Locations, vid) + return + } + remaining := make([]Location, 0, len(entry.locations)-1) + remaining = append(remaining, entry.locations[:i]...) + remaining = append(remaining, entry.locations[i+1:]...) + vid2Locations[vid] = &locationsEntry{locations: remaining, generation: entry.generation} + return } } func (vc *vidMap) deleteVid(vid uint32) { - if cachedMap := vc.cache.Load(); cachedMap != nil { - cachedMap.deleteVid(vid) - } - vc.Lock() defer vc.Unlock() - for _, loc := range vc.vid2Locations[vid] { + if entry, found := vc.vid2Locations[vid]; found { + vc.releaseEntry(entry) + delete(vc.vid2Locations, vid) + } + if entry, found := vc.ecVid2Locations[vid]; found { + vc.releaseEntry(entry) + delete(vc.ecVid2Locations, vid) + } +} + +// reset starts a new generation, as when the master changes and everything it +// told us has to be relearned. Entries stay readable while they are relearned +// and are dropped once they fall out of the retained window. +func (vc *vidMap) reset() { + vc.Lock() + defer vc.Unlock() + + vc.generation++ + if vc.generation <= vc.retainGenerations { + return + } + oldest := vc.generation - vc.retainGenerations + vc.expire(vc.vid2Locations, oldest) + vc.expire(vc.ecVid2Locations, oldest) +} + +// expire drops entries last refreshed before oldest. Callers must hold the +// write lock. +func (vc *vidMap) expire(vid2Locations map[uint32]*locationsEntry, oldest uint64) { + for vid, entry := range vid2Locations { + if entry.generation >= oldest { + continue + } + vc.releaseEntry(entry) + delete(vid2Locations, vid) + } +} + +// releaseEntry drops the server references an entry holds. Callers must hold +// the write lock. +func (vc *vidMap) releaseEntry(entry *locationsEntry) { + for _, loc := range entry.locations { vc.decrementServerRef(locationServerKey(loc)) } - for _, loc := range vc.ecVid2Locations[vid] { - vc.decrementServerRef(locationServerKey(loc)) - } - delete(vc.vid2Locations, vid) - delete(vc.ecVid2Locations, vid) } // incrementServerRef increases the refcount for key. Empty keys are skipped diff --git a/weed/wdclient/vid_map_generation_test.go b/weed/wdclient/vid_map_generation_test.go new file mode 100644 index 000000000..008e1f9b5 --- /dev/null +++ b/weed/wdclient/vid_map_generation_test.go @@ -0,0 +1,330 @@ +package wdclient + +import ( + "runtime" + "sync" + "testing" + + "github.com/seaweedfs/seaweedfs/weed/pb" +) + +func urlsOf(locations []Location) []string { + urls := make([]string, 0, len(locations)) + for _, loc := range locations { + urls = append(urls, loc.Url) + } + return urls +} + +// A volume that moved while we were talking to the previous master must answer +// with where it is now, not with both servers: the first write of a generation +// replaces the entry instead of merging into it. +func TestAddLocationReplacesEarlierGeneration(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vid := uint32(3) + movedFrom := Location{Url: "10.0.0.1:8080"} + movedTo := Location{Url: "10.0.0.2:8080"} + + vm.addLocation(vid, movedFrom) + vm.reset() + vm.addLocation(vid, movedTo) + + locs, found := vm.GetLocations(vid) + if !found || len(locs) != 1 || locs[0].Url != movedTo.Url { + t.Fatalf("expected only %s after the move, got %v", movedTo.Url, urlsOf(locs)) + } + if vm.hasVolumeServer(pb.ServerAddress(movedFrom.Url)) { + t.Errorf("server %s should no longer be referenced after the volume moved", movedFrom.Url) + } + if !vm.hasVolumeServer(pb.ServerAddress(movedTo.Url)) { + t.Errorf("server %s should be referenced after the volume moved", movedTo.Url) + } +} + +// Replicas reported within one generation accumulate, and a repeated report of +// the same server is not counted twice. +func TestAddLocationMergesWithinGeneration(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vid := uint32(4) + first := Location{Url: "10.0.0.1:8080"} + second := Location{Url: "10.0.0.2:8080"} + + vm.addLocation(vid, first) + vm.addLocation(vid, second) + vm.addLocation(vid, first) + + locs, found := vm.GetLocations(vid) + if !found || len(locs) != 2 { + t.Fatalf("expected both replicas, got %v", urlsOf(locs)) + } + + // One delete must be enough to evict the server: the duplicate report of + // `first` must not have taken a second reference. + vm.deleteLocation(vid, first) + if vm.hasVolumeServer(pb.ServerAddress(first.Url)) { + t.Errorf("server %s should be evicted after a single delete", first.Url) + } +} + +// An entry survives exactly retainGenerations resets without being relearned. +func TestResetRetainsUntilWindowExpires(t *testing.T) { + const retain = 2 + vm := newVidMap("", retain) + vid := uint32(5) + location := Location{Url: "10.0.0.1:8080"} + vm.addLocation(vid, location) + + for i := 0; i < retain; i++ { + vm.reset() + if _, found := vm.GetLocations(vid); !found { + t.Fatalf("location should still be retained after %d reset(s)", i+1) + } + } + + vm.reset() + if _, found := vm.GetLocations(vid); found { + t.Errorf("location should be dropped after %d resets", retain+1) + } + if vm.hasVolumeServer(pb.ServerAddress(location.Url)) { + t.Errorf("expiry should release the server reference for %s", location.Url) + } + if len(vm.serverRefCount) != 0 { + t.Errorf("expiry left %d dangling server refcounts", len(vm.serverRefCount)) + } +} + +// Relearning an entry restarts its retention window. +func TestResetWindowRestartsOnRelearn(t *testing.T) { + const retain = 2 + vm := newVidMap("", retain) + vid := uint32(6) + location := Location{Url: "10.0.0.1:8080"} + + for i := 0; i < retain+3; i++ { + vm.addLocation(vid, location) + vm.reset() + } + + if _, found := vm.GetLocations(vid); !found { + t.Error("a location relearned every generation must never expire") + } +} + +// A delete names one location; it must not make the rest of a stale entry look +// freshly learned and so outlive its retention window. +func TestDeleteLocationDoesNotRefreshGeneration(t *testing.T) { + const retain = 2 + vm := newVidMap("", retain) + vid := uint32(7) + first := Location{Url: "10.0.0.1:8080"} + second := Location{Url: "10.0.0.2:8080"} + + vm.addLocation(vid, first) + vm.addLocation(vid, second) + for i := 0; i < retain; i++ { + vm.reset() + } + vm.deleteLocation(vid, first) + + locs, found := vm.GetLocations(vid) + if !found || len(locs) != 1 || locs[0].Url != second.Url { + t.Fatalf("expected %s to remain, got found=%v %v", second.Url, found, urlsOf(locs)) + } + + vm.reset() + if _, found := vm.GetLocations(vid); found { + t.Error("the surviving location was learned in the expired generation and should be dropped") + } +} + +// Encoding a volume must stop the regular copies a previous generation knew +// from answering for it, and decoding it must stop its shards from answering. +func TestNewestGenerationWinsAcrossEcTransition(t *testing.T) { + regular := Location{Url: "10.0.0.1:8080"} + ecShard := Location{Url: "10.0.0.2:8080"} + + t.Run("encoded", func(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vm.addLocation(1, regular) + vm.reset() + vm.addEcLocation(1, ecShard) + + locs, found := vm.GetLocations(1) + if !found || len(locs) != 1 || locs[0].Url != ecShard.Url { + t.Fatalf("expected the freshly learned EC shard, got found=%v %v", found, urlsOf(locs)) + } + }) + + t.Run("decoded", func(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vm.addEcLocation(1, ecShard) + vm.reset() + vm.addLocation(1, regular) + + locs, found := vm.GetLocations(1) + if !found || len(locs) != 1 || locs[0].Url != regular.Url { + t.Fatalf("expected the freshly learned regular copy, got found=%v %v", found, urlsOf(locs)) + } + }) + + t.Run("same generation prefers regular", func(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vm.addEcLocation(1, ecShard) + vm.addLocation(1, regular) + + locs, found := vm.GetLocations(1) + if !found || len(locs) != 1 || locs[0].Url != regular.Url { + t.Fatalf("expected the regular copy, got found=%v %v", found, urlsOf(locs)) + } + }) +} + +// Losing the last location drops the entry: a client that never resets should +// not accumulate one empty entry per volume it has ever seen deleted. +func TestDeleteLastLocationDropsEntry(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vid := uint32(12) + location := Location{Url: "10.0.0.1:8080"} + + vm.addLocation(vid, location) + vm.deleteLocation(vid, location) + + if _, found := vm.GetLocations(vid); found { + t.Error("a volume with no locations left must not resolve") + } + if len(vm.vid2Locations) != 0 { + t.Errorf("expected the emptied entry to be dropped, got %v", vm.vid2Locations) + } + if len(vm.serverRefCount) != 0 { + t.Errorf("server refcounts leaked: %v", vm.serverRefCount) + } +} + +// EC locations follow the same rules as regular ones, and back a volume whose +// regular locations are gone. +func TestEcLocationsFollowGenerationRules(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vid := uint32(8) + regular := Location{Url: "10.0.0.1:8080"} + ecShard := Location{Url: "10.0.0.2:8080"} + movedEcShard := Location{Url: "10.0.0.3:8080"} + + vm.addLocation(vid, regular) + vm.addEcLocation(vid, ecShard) + + locs, found := vm.GetLocations(vid) + if !found || len(locs) != 1 || locs[0].Url != regular.Url { + t.Fatalf("regular locations should win while they exist, got %v", urlsOf(locs)) + } + + // Regular copy goes away: the EC shards still serve the volume. + vm.deleteLocation(vid, regular) + locs, found = vm.GetLocations(vid) + if !found || len(locs) != 1 || locs[0].Url != ecShard.Url { + t.Fatalf("expected EC shard location, got found=%v %v", found, urlsOf(locs)) + } + + // EC shards move under a new master: no merging with the old report. + vm.reset() + vm.addEcLocation(vid, movedEcShard) + locs, found = vm.GetLocations(vid) + if !found || len(locs) != 1 || locs[0].Url != movedEcShard.Url { + t.Fatalf("expected only the moved EC shard, got found=%v %v", found, urlsOf(locs)) + } +} + +// Locations handed to a caller are never rewritten underneath it. +func TestGetLocationsResultIsStable(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + vid := uint32(9) + first := Location{Url: "10.0.0.1:8080"} + second := Location{Url: "10.0.0.2:8080"} + + vm.addLocation(vid, first) + vm.addLocation(vid, second) + + locs, found := vm.GetLocations(vid) + if !found || len(locs) != 2 { + t.Fatalf("expected both replicas, got %v", urlsOf(locs)) + } + snapshot := append([]Location(nil), locs...) + + vm.deleteLocation(vid, first) + vm.addLocation(vid, Location{Url: "10.0.0.3:8080"}) + + for i := range snapshot { + if locs[i] != snapshot[i] { + t.Errorf("location %d changed under the caller: %v became %v", i, snapshot[i], locs[i]) + } + } +} + +func TestNewVidMapDefaultsRetention(t *testing.T) { + for _, retain := range []int{0, -1} { + if got := newVidMap("", retain).retainGenerations; got != DefaultVidMapCacheSize { + t.Errorf("newVidMap(%d) retention = %d, want %d", retain, got, DefaultVidMapCacheSize) + } + } +} + +// Readers must never see a spurious miss for a volume that stays live, and the +// bookkeeping must survive concurrent writers. Run with -race. +func TestConcurrentResetAndUpdates(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) + live := Location{Url: "10.0.0.1:8080"} + churn := Location{Url: "10.0.0.2:8080"} + const liveVid, churnVid = 1, 2 + + vm.addLocation(liveVid, live) + + var wg sync.WaitGroup + stop := make(chan struct{}) + + for i := 0; i < 8; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for { + select { + case <-stop: + return + default: + if _, found := vm.GetLocations(liveVid); !found { + t.Error("a volume that is relearned every generation must always resolve") + return + } + vm.hasVolumeServer(pb.ServerAddress(live.Url)) + vm.GetLocationsClone(churnVid) + runtime.Gosched() + } + } + }() + } + + for i := 0; i < 4; i++ { + wg.Add(1) + go func() { + defer wg.Done() + for j := 0; j < 200; j++ { + vm.addEcLocation(churnVid, churn) + vm.deleteEcLocation(churnVid, churn) + vm.deleteVid(churnVid) + } + }() + } + + for i := 0; i < 300; i++ { + vm.reset() + vm.addLocation(liveVid, live) + } + close(stop) + wg.Wait() + + if _, found := vm.GetLocations(liveVid); !found { + t.Fatal("live volume lost after the churn") + } + vm.deleteVid(liveVid) + if len(vm.serverRefCount) != 0 { + t.Errorf("server refcounts leaked: %v", vm.serverRefCount) + } +} diff --git a/weed/wdclient/vidmap_client.go b/weed/wdclient/vidmap_client.go index c66beeb98..f2fc9add9 100644 --- a/weed/wdclient/vidmap_client.go +++ b/weed/wdclient/vidmap_client.go @@ -8,7 +8,6 @@ import ( "sort" "strconv" "strings" - "sync" "golang.org/x/sync/singleflight" @@ -27,28 +26,23 @@ type VolumeLocationProvider interface { // vidMapClient provides volume location caching with pluggable lookup // It wraps the battle-tested vidMap with customizable volume lookup strategies type vidMapClient struct { - vidMap *vidMap - vidMapLock sync.RWMutex - vidMapCacheSize int - provider VolumeLocationProvider - vidLookupGroup singleflight.Group + vidMap *vidMap + provider VolumeLocationProvider + vidLookupGroup singleflight.Group } const ( - // DefaultVidMapCacheSize is the default number of historical vidMap snapshots to keep - // This provides cache history when volumes move between servers + // DefaultVidMapCacheSize is the default number of resets a volume location + // survives without being relearned. This provides cache history when + // volumes move between servers. DefaultVidMapCacheSize = 5 ) // newVidMapClient creates a new client with the given provider and data center func newVidMapClient(provider VolumeLocationProvider, dataCenter string, cacheSize int) *vidMapClient { - if cacheSize <= 0 { - cacheSize = DefaultVidMapCacheSize - } return &vidMapClient{ - vidMap: newVidMap(dataCenter), - vidMapCacheSize: cacheSize, - provider: provider, + vidMap: newVidMap(dataCenter, cacheSize), + provider: provider, } } @@ -59,12 +53,9 @@ func (vc *vidMapClient) GetLookupFileIdFunction() LookupFileIdFunctionType { // LookupFileIdWithFallback looks up a file ID, checking cache first, then using provider func (vc *vidMapClient) LookupFileIdWithFallback(ctx context.Context, fileId string) (fullUrls []string, err error) { - // Try cache first - hold read lock during entire vidMap access to prevent swap during operation - vc.vidMapLock.RLock() - vm := vc.vidMap - dataCenter := vm.DataCenter - fullUrls, err = vm.LookupFileId(ctx, fileId) - vc.vidMapLock.RUnlock() + // Try cache first + dataCenter := vc.vidMap.DataCenter + fullUrls, err = vc.vidMap.LookupFileId(ctx, fileId) // Cache hit - return immediately if err == nil && len(fullUrls) > 0 { @@ -146,9 +137,6 @@ func (vc *vidMapClient) LookupVolumeIdsWithFallback(ctx context.Context, volumeI // Check cache first and parse volume IDs once vidStringToUint := make(map[string]uint32, len(volumeIds)) - // Get stable pointer to vidMap with minimal lock hold time - vm := vc.getStableVidMap() - for _, vidString := range volumeIds { vid, err := strconv.ParseUint(vidString, 10, 32) if err != nil { @@ -156,7 +144,7 @@ func (vc *vidMapClient) LookupVolumeIdsWithFallback(ctx context.Context, volumeI } vidStringToUint[vidString] = uint32(vid) - locations, found := vm.GetLocations(uint32(vid)) + locations, found := vc.vidMap.GetLocations(uint32(vid)) if found && len(locations) > 0 { result[vidString] = locations } else { @@ -178,12 +166,9 @@ func (vc *vidMapClient) LookupVolumeIdsWithFallback(ctx context.Context, volumeI stillNeedLookup := make([]string, 0, len(needsLookup)) batchResult := make(map[string][]Location) - // Get stable pointer with minimal lock hold time - vm := vc.getStableVidMap() - for _, vidString := range needsLookup { vid := vidStringToUint[vidString] // Use pre-parsed value - if locations, found := vm.GetLocations(vid); found && len(locations) > 0 { + if locations, found := vc.vidMap.GetLocations(vid); found && len(locations) > 0 { batchResult[vidString] = locations } else { stillNeedLookup = append(stillNeedLookup, vidString) @@ -244,117 +229,74 @@ func (vc *vidMapClient) LookupVolumeIdsWithFallback(ctx context.Context, volumeI return result, errors.Join(lookupErrors...) } -// getStableVidMap gets a stable pointer to the vidMap, releasing the lock immediately. -// WARNING: Use with caution. The returned vidMap pointer is stable (won't be garbage collected -// due to cache chain), but the vidMapClient.vidMap field may be swapped by resetVidMap(). -// For operations that must use the current vidMap atomically, use withCurrentVidMap() instead. -func (vc *vidMapClient) getStableVidMap() *vidMap { - vc.vidMapLock.RLock() - vm := vc.vidMap - vc.vidMapLock.RUnlock() - return vm -} - -// withCurrentVidMap executes a function with the current vidMap under a read lock. -// This guarantees the vidMap instance cannot be swapped during the function execution. -// Use this when you need atomic access to the current vidMap for multiple operations. -func (vc *vidMapClient) withCurrentVidMap(f func(vm *vidMap)) { - vc.vidMapLock.RLock() - defer vc.vidMapLock.RUnlock() - f(vc.vidMap) -} - // Public methods for external access +// +// The vidMap itself is never replaced, so these all read the one map under its +// own lock. Resets bump its generation instead of swapping in a fresh instance. // GetLocations safely retrieves volume locations func (vc *vidMapClient) GetLocations(vid uint32) (locations []Location, found bool) { - return vc.getStableVidMap().GetLocations(vid) + return vc.vidMap.GetLocations(vid) } // GetLocationsClone safely retrieves a clone of volume locations func (vc *vidMapClient) GetLocationsClone(vid uint32) (locations []Location, found bool) { - return vc.getStableVidMap().GetLocationsClone(vid) + return vc.vidMap.GetLocationsClone(vid) } // GetVidLocations safely retrieves volume locations by string ID func (vc *vidMapClient) GetVidLocations(vid string) (locations []Location, err error) { - return vc.getStableVidMap().GetVidLocations(vid) + return vc.vidMap.GetVidLocations(vid) } // LookupFileId safely looks up URLs for a file ID func (vc *vidMapClient) LookupFileId(ctx context.Context, fileId string) (fullUrls []string, err error) { - return vc.getStableVidMap().LookupFileId(ctx, fileId) + return vc.vidMap.LookupFileId(ctx, fileId) } // LookupVolumeServerUrl safely looks up volume server URLs func (vc *vidMapClient) LookupVolumeServerUrl(vid string) (serverUrls []string, err error) { - return vc.getStableVidMap().LookupVolumeServerUrl(vid) + return vc.vidMap.LookupVolumeServerUrl(vid) } // HasVolumeServer reports whether addr is currently a known volume server // (hosts at least one volume or EC shard) in the cached vid map. Used by // admission paths that must only contact peers learned from the master. func (vc *vidMapClient) HasVolumeServer(addr pb.ServerAddress) bool { - return vc.getStableVidMap().hasVolumeServer(addr) + return vc.vidMap.hasVolumeServer(addr) } // GetDataCenter safely retrieves the data center func (vc *vidMapClient) GetDataCenter() string { - return vc.getStableVidMap().DataCenter + return vc.vidMap.DataCenter } // Thread-safe helpers for vidMap operations // addLocation adds a volume location func (vc *vidMapClient) addLocation(vid uint32, location Location) { - vc.withCurrentVidMap(func(vm *vidMap) { - vm.addLocation(vid, location) - }) + vc.vidMap.addLocation(vid, location) } // deleteLocation removes a volume location func (vc *vidMapClient) deleteLocation(vid uint32, location Location) { - vc.withCurrentVidMap(func(vm *vidMap) { - vm.deleteLocation(vid, location) - }) + vc.vidMap.deleteLocation(vid, location) } // addEcLocation adds an EC volume location func (vc *vidMapClient) addEcLocation(vid uint32, location Location) { - vc.withCurrentVidMap(func(vm *vidMap) { - vm.addEcLocation(vid, location) - }) + vc.vidMap.addEcLocation(vid, location) } // deleteEcLocation removes an EC volume location func (vc *vidMapClient) deleteEcLocation(vid uint32, location Location) { - vc.withCurrentVidMap(func(vm *vidMap) { - vm.deleteEcLocation(vid, location) - }) + vc.vidMap.deleteEcLocation(vid, location) } -// resetVidMap resets the volume ID map +// resetVidMap starts a new generation, as when the master changes: what the +// previous one told us stays readable until it is relearned or expires. func (vc *vidMapClient) resetVidMap() { - vc.vidMapLock.Lock() - defer vc.vidMapLock.Unlock() - - // Preserve the existing vidMap in the cache chain - tail := vc.vidMap - - nvm := newVidMap(tail.DataCenter) - nvm.cache.Store(tail) - vc.vidMap = nvm - - // Trim cache chain to vidMapCacheSize - node := tail - for i := 0; i < vc.vidMapCacheSize-1; i++ { - if node.cache.Load() == nil { - return - } - node = node.cache.Load() - } - // node is guaranteed to be non-nil after the loop - node.cache.Store(nil) + vc.vidMap.reset() } // InvalidateCache removes all cached locations for a volume ID @@ -365,7 +307,5 @@ func (vc *vidMapClient) InvalidateCache(fileId string) { if err != nil { return } - vc.withCurrentVidMap(func(vm *vidMap) { - vm.deleteVid(uint32(vid)) - }) + vc.vidMap.deleteVid(uint32(vid)) } diff --git a/weed/wdclient/vidmap_invalidation_test.go b/weed/wdclient/vidmap_invalidation_test.go index 0d25de975..3fc876652 100644 --- a/weed/wdclient/vidmap_invalidation_test.go +++ b/weed/wdclient/vidmap_invalidation_test.go @@ -8,22 +8,14 @@ import ( func TestInvalidateCacheValidFileId(t *testing.T) { // Create a simple vidMapClient (can use nil provider for this test) vc := &vidMapClient{ - vidMap: newVidMap(""), - vidMapCacheSize: 5, + vidMap: newVidMap("", DefaultVidMapCacheSize), } // Add some locations to the cache vid := uint32(456) - vc.vidMap.Lock() - vc.vidMap.vid2Locations[vid] = []Location{{Url: "http://server1:8080"}} - vc.vidMap.Unlock() + vc.addLocation(vid, Location{Url: "http://server1:8080"}) - // Verify location exists - vc.vidMap.RLock() - _, found := vc.vidMap.vid2Locations[vid] - vc.vidMap.RUnlock() - - if !found { + if _, found := vc.GetLocations(vid); !found { t.Fatal("Location should exist before invalidation") } @@ -32,11 +24,7 @@ func TestInvalidateCacheValidFileId(t *testing.T) { vc.InvalidateCache(fileId) // Verify the locations were removed - vc.vidMap.RLock() - _, foundAfter := vc.vidMap.vid2Locations[vid] - vc.vidMap.RUnlock() - - if foundAfter { + if _, found := vc.GetLocations(vid); found { t.Errorf("Expected locations for vid %d to be removed after InvalidateCache", vid) } } @@ -57,222 +45,128 @@ func TestInvalidateCacheInvalidFileId(t *testing.T) { for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { vc := &vidMapClient{ - vidMap: newVidMap(""), - vidMapCacheSize: 5, + vidMap: newVidMap("", DefaultVidMapCacheSize), } // Add a location to ensure the cache isn't empty - vc.vidMap.Lock() - vc.vidMap.vid2Locations[1] = []Location{{Url: "http://server:8080"}} - vc.vidMap.Unlock() + vc.addLocation(1, Location{Url: "http://server:8080"}) // This should not panic or cause errors vc.InvalidateCache(tc.fileId) // Verify the existing location is still there (not affected) - vc.vidMap.RLock() - _, found := vc.vidMap.vid2Locations[1] - vc.vidMap.RUnlock() - - if !found { + if _, found := vc.GetLocations(1); !found { t.Errorf("InvalidateCache with invalid fileId '%s' should not affect other entries", tc.fileId) } }) } } -// TestInvalidateCacheWithHistory tests that invalidation propagates through cache history +// TestInvalidateCacheWithHistory tests that invalidation also drops locations +// that were learned before the last reset and are still being retained. func TestInvalidateCacheWithHistory(t *testing.T) { vid := uint32(789) - // Create first vidMap with the volume - vm1 := newVidMap("") - vm1.Lock() - vm1.vid2Locations[vid] = []Location{{Url: "http://server1:8080"}} - vm1.Unlock() - - // Create second vidMap with the cached first one - vm2 := newVidMap("") - vm2.cache.Store(vm1) // vm1 becomes the cache/history - vm2.Lock() - vm2.vid2Locations[vid] = []Location{{Url: "http://server2:8080"}} - vm2.Unlock() - - // Create vidMapClient with vm2 as current vc := &vidMapClient{ - vidMap: vm2, - vidMapCacheSize: 5, + vidMap: newVidMap("", DefaultVidMapCacheSize), } - // Verify both have the vid before invalidation - vm2.RLock() - _, foundInCurrent := vm2.vid2Locations[vid] - vm2.RUnlock() + // Learned from an earlier master, then kept across a reset + vc.addLocation(vid, Location{Url: "http://server1:8080"}) + vc.resetVidMap() - vm1.RLock() - _, foundInCache := vm1.vid2Locations[vid] - vm1.RUnlock() - - if !foundInCurrent || !foundInCache { - t.Fatal("Both maps should have the vid before invalidation") + if _, found := vc.GetLocations(vid); !found { + t.Fatal("Retained location should still be readable after a reset") } - // Invalidate the cache - fileId := "789,xyz123" - vc.InvalidateCache(fileId) + vc.InvalidateCache("789,xyz123") - // Check that current map doesn't have the vid - vm2.RLock() - _, foundInCurrentAfter := vm2.vid2Locations[vid] - vm2.RUnlock() - - if foundInCurrentAfter { - t.Error("Expected vid to be removed from current vidMap after InvalidateCache") - } - - // Check that cache doesn't have the vid either (recursive deletion) - vm1.RLock() - _, foundInCacheAfter := vm1.vid2Locations[vid] - vm1.RUnlock() - - if foundInCacheAfter { - t.Error("Expected vid to be removed from cached vidMap as well (recursive deletion)") + if _, found := vc.GetLocations(vid); found { + t.Error("Expected retained location to be dropped by InvalidateCache") } } -// TestDeleteVidRecursion tests the deleteVid method removes from history chain -func TestDeleteVidRecursion(t *testing.T) { +// TestDeleteVidDropsRetainedGenerations tests that deleteVid removes a volume +// no matter which generation it was last refreshed in. +func TestDeleteVidDropsRetainedGenerations(t *testing.T) { vid := uint32(999) - // Create a chain: vm3 -> vm2 -> vm1 - vm1 := newVidMap("") - vm1.Lock() - vm1.vid2Locations[vid] = []Location{{Url: "http://server1:8080"}} - vm1.Unlock() + vm := newVidMap("", DefaultVidMapCacheSize) + vm.addLocation(vid, Location{Url: "http://server1:8080"}) + vm.addEcLocation(vid, Location{Url: "http://server2:8080"}) + vm.reset() + vm.reset() - vm2 := newVidMap("") - vm2.cache.Store(vm1) - vm2.Lock() - vm2.vid2Locations[vid] = []Location{{Url: "http://server2:8080"}} - vm2.Unlock() - - vm3 := newVidMap("") - vm3.cache.Store(vm2) - vm3.Lock() - vm3.vid2Locations[vid] = []Location{{Url: "http://server3:8080"}} - vm3.Unlock() - - // Verify all have the vid - vm3.RLock() - _, found3 := vm3.vid2Locations[vid] - vm3.RUnlock() - - vm2.RLock() - _, found2 := vm2.vid2Locations[vid] - vm2.RUnlock() - - vm1.RLock() - _, found1 := vm1.vid2Locations[vid] - vm1.RUnlock() - - if !found1 || !found2 || !found3 { - t.Fatal("All maps should have the vid before deletion") + if _, found := vm.GetLocations(vid); !found { + t.Fatal("Volume should still be readable before deletion") } - // Delete from vm3 (should cascade) - vm3.deleteVid(vid) + vm.deleteVid(vid) - // Verify it's gone from all - vm3.RLock() - _, found3After := vm3.vid2Locations[vid] - vm3.RUnlock() - - vm2.RLock() - _, found2After := vm2.vid2Locations[vid] - vm2.RUnlock() - - vm1.RLock() - _, found1After := vm1.vid2Locations[vid] - vm1.RUnlock() - - if found3After { - t.Error("Expected vid to be removed from vm3") + if _, found := vm.GetLocations(vid); found { + t.Error("Expected volume to be gone after deleteVid") } - if found2After { - t.Error("Expected vid to be removed from vm2 (cascaded)") - } - if found1After { - t.Error("Expected vid to be removed from vm1 (cascaded)") + if vm.hasVolumeServer("http://server1:8080") || vm.hasVolumeServer("http://server2:8080") { + t.Error("Expected deleteVid to release the server references it held") } } -// TestGetLocationsEmptyArrayNoFallback tests that empty location arrays don't fall back to cache -// This tests the fix for the bug where volume pods restart and vidMap has empty array [], -// but GetLocations would fall back to stale cached locations from before restart. +// TestGetLocationsEmptyArrayNoFallback tests that a volume known to have no +// locations reports a miss instead of serving what an earlier generation held. +// Covers the bug where a volume pod restarts, the vid map holds an empty array, +// and lookups fall back to the stale locations from before the restart. func TestGetLocationsEmptyArrayNoFallback(t *testing.T) { - // Setup: Create vidMap with cache - currentMap := newVidMap("") + vm := newVidMap("", DefaultVidMapCacheSize) vid := uint32(10) oldLocation := Location{Url: "10.131.1.28:8081"} newLocation := Location{Url: "10.131.1.65:8081"} - // Scenario: Volume initially has old location - currentMap.addLocation(vid, oldLocation) - locs, found := currentMap.GetLocations(vid) + // Volume initially has its old location, which is then retained across a reset + vm.addLocation(vid, oldLocation) + vm.reset() + + locs, found := vm.GetLocations(vid) if !found || len(locs) != 1 || locs[0].Url != oldLocation.Url { t.Fatalf("Expected to find old location, got found=%v locs=%v", found, locs) } - // Create cache chain with old location - cachedMap := newVidMap("") - cachedMap.addLocation(vid, oldLocation) - currentMap.cache.Store(cachedMap) + // Volume server restarts and the old location is deleted + vm.deleteLocation(vid, oldLocation) - // Simulate: Volume server restarts, old location is deleted - currentMap.deleteLocation(vid, oldLocation) - - // BUG: At this point vid2Locations[vid] = [] (empty array, key exists) - // OLD BEHAVIOR: GetLocations would see found=true, len=0 and fall back to cache - // returning stale oldLocation - // NEW BEHAVIOR: GetLocations should return nil, false (no locations available) - - locs, found = currentMap.GetLocations(vid) + locs, found = vm.GetLocations(vid) if found { t.Errorf("Expected found=false for empty location array, got found=true with locs=%v", locs) } if locs != nil { - t.Errorf("Expected nil locations for empty array, got %v (should not fall back to stale cache!)", locs) + t.Errorf("Expected nil locations for empty array, got %v (should not serve stale history!)", locs) } - // Verify: When new location is added, it should be returned (not stale cache) - currentMap.addLocation(vid, newLocation) - locs, found = currentMap.GetLocations(vid) + // When the new location is added, it should be returned (not the stale one) + vm.addLocation(vid, newLocation) + locs, found = vm.GetLocations(vid) if !found || len(locs) != 1 { t.Fatalf("Expected to find new location, got found=%v locs=%v", found, locs) } if locs[0].Url != newLocation.Url { - t.Errorf("Expected new location %s, got %s (got stale cache!)", newLocation.Url, locs[0].Url) + t.Errorf("Expected new location %s, got %s (got stale history!)", newLocation.Url, locs[0].Url) } } -// TestGetLocationsUnknownVolumeUsesCache tests that truly unknown volumes still use cache -func TestGetLocationsUnknownVolumeUsesCache(t *testing.T) { - // Setup: Current map doesn't know about volume, but cache does - currentMap := newVidMap("") - cachedMap := newVidMap("") +// TestGetLocationsRetainedAcrossReset tests that a volume the new master has +// not mentioned yet is still served from what the previous one told us. +func TestGetLocationsRetainedAcrossReset(t *testing.T) { + vm := newVidMap("", DefaultVidMapCacheSize) vid := uint32(99) - cachedLocation := Location{Url: "cache-server:8081"} + retained := Location{Url: "cache-server:8081"} - cachedMap.addLocation(vid, cachedLocation) - currentMap.cache.Store(cachedMap) + vm.addLocation(vid, retained) + vm.reset() - // Volume 99 is completely unknown to currentMap (not in vid2Locations) - // This should fall back to cache - locs, found := currentMap.GetLocations(vid) + locs, found := vm.GetLocations(vid) if !found || len(locs) != 1 { - t.Fatalf("Expected to find cached location for unknown volume, got found=%v locs=%v", found, locs) + t.Fatalf("Expected to find retained location after reset, got found=%v locs=%v", found, locs) } - if locs[0].Url != cachedLocation.Url { - t.Errorf("Expected cached location %s, got %s", cachedLocation.Url, locs[0].Url) + if locs[0].Url != retained.Url { + t.Errorf("Expected retained location %s, got %s", retained.Url, locs[0].Url) } }