Skip to content

[fix] Reject foreign ZMQ peers at the storage proxy - #168

Merged
ji-huazhong merged 1 commit into
Ascend:mainfrom
huniu20:fix/storage-reject-foreign-zmq-peers
Sep 7, 2026
Merged

[fix] Reject foreign ZMQ peers at the storage proxy#168
ji-huazhong merged 1 commit into
Ascend:mainfrom
huniu20:fix/storage-reject-foreign-zmq-peers

Conversation

@huniu20

@huniu20 huniu20 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

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.deserialize sat outside the worker's try, so one stray payload
terminated 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

  1. Filter at the proxy. zmq.proxy() offers no interception point, so the
    proxy now forwards by hand and keeps only the two identity prefixes real peers
    use (storage managers and the metrics collector).
  2. Survive bad frames anyway. The filter cannot cover everything: an allowed
    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 error
    reply instead of hanging on recv.
  3. One source for the prefixes. They now live in zmq_utils and the
    identities 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 TLS
    ClientHello 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 identity
    sending undecodable frames gets PUT_GET_ERROR and the worker keeps serving.

Each was confirmed to fail when its corresponding fix is reverted.

@ascend-robot

Copy link
Copy Markdown

CLA Signature Pass

huniu20, 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
huniu20 force-pushed the fix/storage-reject-foreign-zmq-peers branch from 2577966 to 972bf72 Compare September 7, 2026 10:14
@ascend-robot

Copy link
Copy Markdown

CLA Signature Pass

huniu20, thanks for your pull request. All authors of the commits have signed the CLA. 👍

@ji-huazhong
ji-huazhong merged commit 3ece216 into Ascend:main Sep 7, 2026
8 checks passed
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants