fix(master): do not re-enter warmup when a fresh cluster grows its first volume (#9092)
* fix(master): do not re-enter warmup when a fresh cluster grows its first volume Follow-up investigation on #8777. After fixing the writable-chunk cap deadlock, a second issue surfaced on the same fio reproducer: the weed mount would log thousands of upload data X: filerGrpcAddress assign volume: assign volume failure count:1 path:"/test2/X": assign volume: rpc error: code = Canceled desc = grpc: the client connection is closing cascading from the filer's handler. The mount's own cached gRPC connection to the filer was never invalidated — the "client connection is closing" text is the FILER's cached connection to the MASTER going away, and the message is forwarded verbatim in the filer's AssignVolumeResponse.Error. Root cause: Topology.IsWarmingUp checks the *live* GetMaxVolumeId. On a fresh cluster the master is initialized with SetLastLeaderChangeTime (master_server.go:239 "Seed the warmup timestamp so IsWarmingUp() is active even if the leader change event hasn't fired yet"), but the MaxVolumeId==0 guard is supposed to short-circuit IsWarmingUp for bootstraps so there is no wait. That guard breaks the moment the first volume is grown inside the warmup window: MaxVolumeId flips from 0 to 1, the lastLeaderChangeTime is still within 3*pulse (15 s default), and IsWarmingUp retroactively returns true for the next several seconds. Every AssignVolume in that window returns codes.Unavailable, which trips the filer's shouldInvalidateConnection guard and tears down its cached master connection, which in turn surfaces as "client connection is closing" to every concurrent in-flight call from the mount's file-close flush storm. fio reports EIO on close and the user sees a thousand scary error lines in the mount log for an otherwise correct run. Fix: snapshot `hadVolumesAtLeaderChange` inside SetLastLeaderChangeTime and read that snapshot in IsWarmingUp instead of the live MaxVolumeId. A fresh cluster snapshots "no volumes at leader change" → IsWarmingUp stays false through the entire warmup window regardless of how fast the first grow lands. A real leader transition on a populated cluster still snapshots "has volumes" → IsWarmingUp behaves exactly as before until the 3*pulse window closes. The lock ordering in SetLastLeaderChangeTime reads MaxVolumeId before taking the lastLeaderChangeTimeLock so the two calls cannot interleave weirdly; IsWarmingUp reads both fields under a single RLock acquisition. Verified with the same containerized reproducer used for the deadlock fix: 4 jobs × 250 nrfiles × 40 MiB × 4k randwrite direct. - baseline (master): fio rc=1 (EIO on close), 2809 mount error lines matching "filerGrpcAddress assign volume ... client connection is closing", 788 filer lines matching "warming up", 331 "Removing cached gRPC connection to ...19333 due to error: master is warming up". - patched: fio rc=0 in 1.9 s at 79.2 MiB/s, 0 mount errors, 0 filer "warming up" lines, 0 cached-master-conn invalidations. go test ./weed/topology/... passes. * fix(topology): make NodeImpl.maxVolumeId atomic CodeRabbit flagged on #9092 that my new IsWarmingUp snapshot (hadVolumesAtLeaderChange) reads through GetMaxVolumeId(), which until now returned an unprotected int field on NodeImpl. UpAdjustMaxVolumeId is called from the volume server heartbeat path in parallel with GetMaxVolumeId reads on the assign path, and neither side had any synchronization — a long-standing data race the race detector would flag if the warmup test suite stressed both sides. Switch maxVolumeId to atomic.Uint32 (needle.VolumeId is uint32) and implement UpAdjustMaxVolumeId as a CAS loop so the check-then-set stays linearizable: two heartbeats racing to promote the field will land the higher value deterministically and propagate to the parent exactly once. GetMaxVolumeId is a single atomic load. Callers of both helpers are unchanged; the struct field comment documents why the atomic is necessary. go test -race ./weed/topology/... passes. * refactor(topology): use WarmupDuration helper in IsWarmingUp Gemini review on #9092 flagged that IsWarmingUp re-derives the warmup duration from pulse and WarmupPulseMultiplier instead of using the existing WarmupDuration() helper, which RemainingWarmupDuration already uses. Fold the duration calculation through the helper and short-circuit on lastChange.IsZero() before the time.Since call. No behavior change.
This commit is contained in:
parent
bdebd63f7c
commit
dfecd664f9
@ -145,12 +145,16 @@ type Node interface {
|
||||
}
|
||||
|
||||
type NodeImpl struct {
|
||||
diskUsages *DiskUsages
|
||||
id NodeId
|
||||
parent Node
|
||||
diskUsages *DiskUsages
|
||||
id NodeId
|
||||
parent Node
|
||||
sync.RWMutex // lock children
|
||||
children map[NodeId]Node
|
||||
maxVolumeId needle.VolumeId
|
||||
children map[NodeId]Node
|
||||
// maxVolumeId uses atomic ops so UpAdjustMaxVolumeId (called from the
|
||||
// volume server heartbeat path) and GetMaxVolumeId (called from the
|
||||
// master's assign / warmup checks) can run concurrently without a
|
||||
// data race on NodeImpl.
|
||||
maxVolumeId atomic.Uint32
|
||||
|
||||
//for rack, data center, topology
|
||||
nodeType string
|
||||
@ -391,16 +395,23 @@ func (n *NodeImpl) UpAdjustDiskUsageDelta(diskType types.DiskType, diskUsage *Di
|
||||
n.parent.UpAdjustDiskUsageDelta(diskType, diskUsage)
|
||||
}
|
||||
}
|
||||
func (n *NodeImpl) UpAdjustMaxVolumeId(vid needle.VolumeId) { //can be negative
|
||||
if n.maxVolumeId < vid {
|
||||
n.maxVolumeId = vid
|
||||
if n.parent != nil {
|
||||
n.parent.UpAdjustMaxVolumeId(vid)
|
||||
func (n *NodeImpl) UpAdjustMaxVolumeId(vid needle.VolumeId) {
|
||||
target := uint32(vid)
|
||||
for {
|
||||
current := n.maxVolumeId.Load()
|
||||
if current >= target {
|
||||
return
|
||||
}
|
||||
if n.maxVolumeId.CompareAndSwap(current, target) {
|
||||
break
|
||||
}
|
||||
}
|
||||
if n.parent != nil {
|
||||
n.parent.UpAdjustMaxVolumeId(vid)
|
||||
}
|
||||
}
|
||||
func (n *NodeImpl) GetMaxVolumeId() needle.VolumeId {
|
||||
return n.maxVolumeId
|
||||
return needle.VolumeId(n.maxVolumeId.Load())
|
||||
}
|
||||
|
||||
func (n *NodeImpl) LinkChildNode(node Node) {
|
||||
|
||||
@ -71,6 +71,7 @@ type Topology struct {
|
||||
topologyIdLock sync.RWMutex
|
||||
|
||||
lastLeaderChangeTime time.Time
|
||||
hadVolumesAtLeaderChange bool
|
||||
lastLeaderChangeTimeLock sync.RWMutex
|
||||
}
|
||||
|
||||
@ -121,10 +122,17 @@ func (t *Topology) IsChildLocked() (bool, error) {
|
||||
}
|
||||
|
||||
// SetLastLeaderChangeTime records the time of the most recent leader transition.
|
||||
// It also snapshots whether the topology already had known volumes at that
|
||||
// moment. IsWarmingUp uses the snapshot instead of the live MaxVolumeId so a
|
||||
// fresh cluster that happens to grow its first volume inside the warmup window
|
||||
// does not retroactively flip into "warming up" state — there is no prior
|
||||
// topology to wait for on a bootstrap.
|
||||
func (t *Topology) SetLastLeaderChangeTime(ts time.Time) {
|
||||
hadVolumes := t.GetMaxVolumeId() > 0
|
||||
t.lastLeaderChangeTimeLock.Lock()
|
||||
defer t.lastLeaderChangeTimeLock.Unlock()
|
||||
t.lastLeaderChangeTime = ts
|
||||
t.hadVolumesAtLeaderChange = hadVolumes
|
||||
}
|
||||
|
||||
// GetLastLeaderChangeTime returns the time of the most recent leader transition.
|
||||
@ -137,15 +145,21 @@ func (t *Topology) GetLastLeaderChangeTime() time.Time {
|
||||
// IsWarmingUp returns true if the master recently became leader and may not yet
|
||||
// have a complete topology. After a leader change or restart, volume servers need
|
||||
// up to WarmupPulseMultiplier heartbeat intervals to reconnect and report their volumes.
|
||||
// Returns false on a fresh cluster start (MaxVolumeId == 0) since there are no
|
||||
// existing volumes to wait for.
|
||||
// Returns false on a fresh cluster start — i.e. when no volumes existed at the
|
||||
// time of the leader change — since there is no prior topology state to wait for.
|
||||
// Checking the *live* MaxVolumeId here would make a bootstrapping cluster flip
|
||||
// into warming-up the moment its first volume is grown, which manifested as a
|
||||
// 15-second window of spurious Unavailable errors on AssignVolume for workloads
|
||||
// that start writing immediately (see #8777).
|
||||
func (t *Topology) IsWarmingUp() bool {
|
||||
if t.GetMaxVolumeId() == 0 {
|
||||
t.lastLeaderChangeTimeLock.RLock()
|
||||
lastChange := t.lastLeaderChangeTime
|
||||
hadVolumes := t.hadVolumesAtLeaderChange
|
||||
t.lastLeaderChangeTimeLock.RUnlock()
|
||||
if !hadVolumes || lastChange.IsZero() {
|
||||
return false
|
||||
}
|
||||
warmupDuration := time.Duration(t.pulse*WarmupPulseMultiplier) * time.Second
|
||||
lastChange := t.GetLastLeaderChangeTime()
|
||||
return !lastChange.IsZero() && time.Since(lastChange) < warmupDuration
|
||||
return time.Since(lastChange) < t.WarmupDuration()
|
||||
}
|
||||
|
||||
// WarmupDuration returns the configured warmup duration based on pulse interval.
|
||||
|
||||
Loading…
Reference in New Issue
Block a user