[fix] Reject foreign ZMQ peers at the storage proxy - #168
Merged
ji-huazhong merged 1 commit intoSep 7, 2026
Conversation
CLA Signature Passhuniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
A storage unit binds its put/get ROUTER to a TCP port, so a stray dialer (a scanner's TLS ClientHello) reaches the worker, which dies decoding it and leaves the unit queueing requests nobody answers. zmq.proxy() has no interception point, so forward by hand and keep only the identity prefixes real peers use. Decoding also moves inside the worker's try, since an allowed peer can still send frames that fail to decode. Both prefixes live in zmq_utils and build the identities, so a rename cannot silently make every request unroutable. MockStorageClient set no identity, which the filter drops; give it a storage-manager identity. Signed-off-by: huniu20 <huniumail@gmail.com>
huniu20
force-pushed
the
fix/storage-reject-foreign-zmq-peers
branch
from
September 7, 2026 10:14
2577966 to
972bf72
Compare
CLA Signature Passhuniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍 |
ji-huazhong
approved these changes
Sep 7, 2026
adoda
pushed a commit
to adoda/TransferQueue
that referenced
this pull request
Sep 9, 2026
A storage unit stops answering and every client routed to it fails when its own recv timeout expires. Raising that timeout (400s to 1800s in our deployment) changed nothing. Evidence from one such failure: the unit's node was alive and serving other traffic throughout, a TCP connect to its put_get_socket succeeded, and the unit's own counters showed it fully healthy (1025 GET_DATA served, 9.6ms p99, 1.33GB RSS). It had served exactly one GET_DATA fewer than its cohort. So the unit never saw the request that timed out; it was lost between the two ends, not queued behind slow work. Ascend#168 protected the worker thread from dying, which is a different cause of the same symptom; here the thread was intact. That loss is invisible by construction. ZMQ connect is asynchronous and SNDHWM is 0, so a DEALER accepts send() into an unbounded local queue for a peer it has not reached yet. The message sits there and the caller only learns anything when its own RCVTIMEO expires, which is why no timeout value can distinguish a lost request from a slow one. Lowering SNDHWM would not help either, it only trades silent queuing for silent dropping. Retry the request on a new socket and TCP connection, which is the part that matters, up to TQ_SIMPLE_STORAGE_MAX_ATTEMPTS (default 3). Only a missing answer is retried: zmq.error.Again now raises StorageUnitTimeout, while an error the unit actually reported still surfaces on the first attempt. Replaying an attempt is safe: put is keyed by global index and overwrites, get is read-only. Make the residual failure self-diagnosing, so a next occurrence does not need another round of manual probing. After the last attempt, ask the unit for its own counters over a fresh socket with a short timeout. That probe is served by the same worker thread as put and get, so an answer proves the unit is serving and the request was lost in flight, while silence means the unit itself stopped. The failure log now carries that verdict plus tcp reachability, the unit's op counts and RSS, and the shape of the request that failed. Log volume is unchanged in the steady state. A recovered request logs one line and skips the diagnosis entirely; storage units log a request only above TQ_STORAGE_SLOW_REQUEST_SECONDS (5s) or TQ_STORAGE_LARGE_PAYLOAD_MB (256MB), both far above the single-digit-millisecond norm, so tripping either one is itself the finding. The put_data failure log no longer dumps every routed unit id, which on a large job was thousands of them per line, matching what get_data already does. Tests cover recovery on retry, the bounded attempt count, that reported errors are not retried, and each diagnosis verdict. Signed-off-by: jathonzhang <jathonzhang@tencent.com>
OutstanderWang
added a commit
to OutstanderWang/TransferQueue
that referenced
this pull request
Sep 10, 2026
Rebasing onto main replayed this branch over the identity filter added in Ascend#168, and the pool's changes to the same region dropped the prefixes it filters on. Restore them: the storage proxy rejects any identity lacking one, so without these constants the filter has nothing to match and the module does not import. The pooled sockets already satisfy the filter -- the storage manager pool passes storage_manager_id and the metrics pool now derives its owner id from METRICS_COLLECTOR_IDENTITY_PREFIX rather than repeating the literal, so a change to the prefix reaches both ends. Signed-off-by: OutstanderWang <wangweiyanster@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A storage unit binds its put/get ROUTER to a TCP port. Anything that dials that
port is forwarded straight to the worker, and the worker then dies decoding it:
ZMQMessage.deserializesat outside the worker'stry, so one stray payloadterminated the request loop. The actor process, its bound socket and the proxy
thread all stayed up, so requests kept being accepted with nobody left to answer
them and every client blocked until its own timeout.
Fix
zmq.proxy()offers no interception point, so theproxy now forwards by hand and keeps only the two identity prefixes real peers
use (storage managers and the metrics collector).
peer can still send frames that fail to decode. Decoding moves inside the
try, mirroring the controller's request loop, and the peer gets an errorreply instead of hanging on
recv.zmq_utilsand theidentities are built from them, so a rename cannot silently make every request
unroutable.
The controller already tolerates undecodable messages, so it needs no change.
Tests
tests/test_simple_storage_unit.py: 17 passed.test_foreign_payload_dropped_and_unit_stays_usable— feeds a real TLSClientHello and asserts it draws no reply at all (dropped before the worker,
unlike a decode failure), then that the unit still serves.
test_undecodable_request_answered_and_worker_survives— an allowed identitysending undecodable frames gets
PUT_GET_ERRORand the worker keeps serving.Each was confirmed to fail when its corresponding fix is reverted.