[Store] Give LOCAL_DISK segments an unmount half, and let the offload RPC honour connect timeouts (#3316)
* [Store] Give LOCAL_DISK segments an unmount half, and let the offload RPC honour connect timeouts
A LOCAL_DISK segment can only leave the master by client expiry. Every other
segment kind has a client-callable unmount RPC -- memory has UnmountSegment and
GracefulUnmountSegment, NVMe-oF has UnmountNoFSegment -- while
UnmountLocalDiskSegment existed solely as an internal master function called
from the client-expiry branch of ClientMonitorFunc. Mount, by contrast, is
plumbed through every layer.
The consequence is that a store which is shutting down stays advertised as the
owner of every key it offloaded for up to one client_ttl after it stops pinging.
A read that picks up that stale replica reaches a peer that can no longer serve.
Client::~Client() cannot help: it stops the ping thread and then unmounts the
memory segments in mounted_segments_, and a LOCAL_DISK segment is never put in
that map.
That read then blocks for 91 seconds. ClientRequester hardcodes
connect_retry_count = 3 and reconnect_wait_time = 1000 ms and leaves the connect
timeout at coro_rpc's built-in 30 s, and unlike MasterClient it reads no
environment override -- 3 x 30 s + 1 s, measured to the millisecond
(91,002 ms) on a two-node RDMA rig.
Two changes, covering two different paths:
1. Expose UnmountLocalDiskSegment through the layers its own mount half already
goes through (rpc_service, master_client, client_service), so a departing
store can deregister while it can still serve. FileStorage gains
DrainLocalDiskSegment(grace_period_ms), reachable from Python and over the
store's REST API as POST /api/unmount_local_disk for a shutdown hook. The
grace period exists because a disk replica, unlike a memory replica served by
the NIC, is read and pushed by the store process, so it must outlive the
deregistration by long enough for in-flight reads.
Two races make this more than a plumbing change. The drain latches offloading
off before it deregisters, and does both under offloading_mutex_ -- the lock
Heartbeat holds across its master RPCs -- because FileStorage::Heartbeat
re-mounts the segment whenever the master answers SEGMENT_NOT_FOUND, which
would otherwise undo the deregistration within one heartbeat interval; a tick
parked on that lock re-checks the latch when it acquires it. On the master
side the sweep targets exactly the departing owner
(ClearLocalDiskHandlesOwnedBy) instead of the complement of a liveness
snapshot, so a peer that mounts and registers concurrently cannot be
misclassified as stale, and the deregistration runs under the exclusive
snapshot_mutex_ while NotifyOffloadSuccess and AddReplica refuse a disk
replica whose client no longer has a LOCAL_DISK registration -- so nothing
admitted against the old registration can land after the sweep.
Object metadata is treated exactly as on expiry, including erasing a key
whose last replica was that disk. A store that comes back re-adopts its files
through MountLocalDiskSegment plus NotifyOffloadSuccess, which recreates the
metadata, so this costs a restart nothing -- 1152 keys came back in 4 ms on
the same rig.
2. Route both RPC client pools through one helper that applies
MC_RPC_TIMEOUT_MS and MC_RPC_CONNECT_TIMEOUT_MS, so the offload path can be
bounded at a few seconds. This covers the paths where nobody gets to unmount
anything -- a crash, an OOM kill, a lost node. Defaults are unchanged when
the variables are unset. ClientRequester::invoke_rpc now also maps
coro_rpc::errc::timed_out to ErrorCode::RPC_TIMEOUT, as MasterClient already
does, so a fired timeout is distinguishable from a generic RPC failure.
Related: #2953 (client liveness state machine RFC, which aims to separate
segment lifecycle from liveness), #2039 (a dead peer propagating to all
replicas), #2077 (the same asymmetry worked around on the remount side).
* [Store] Validate /api/unmount_local_disk's request body before draining
Review follow-up for #3316 (zxpdemonio): the handler folded any JSON parse
failure into an empty request and read grace_period_seconds with no type or
range check, so a malformed preStop payload -- a truncated
`{"grace_period_seconds": 30}`, or a typo'd value in milliseconds -- reached
the real unmount with a silently substituted default (or an absurd wait) and
still returned 200.
Now the body is validated before anything runs: malformed JSON and a
non-object body get 400, and grace_period_seconds must be a non-bool
integer, non-negative, and at most 3600s (a request for a longer wait is far
more likely to be a units mistake than an intentional one) -- also 400,
store untouched. Docs note the bound. Added handler tests for each rejection
path plus the default and positive-value paths, asserting the fake store's
unmount_local_disk_segment is never called on the invalid-input paths. J
Juhyun-Kim-Memphis committed
7c2270361caaeeac62671a3f44b667fe9472673e
Parent: 777cc77
Committed by GitHub <noreply@github.com>
on 8/21/2026, 4:10:37 AM