[zmqclient]: Add send-path blip absorber, back-pressure counters, and inner retry cap - #1233
Conversation
|
/azp run |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
This PR enhances swss::ZmqClient’s send-path observability by adding per-instance atomic counters for ZMQ back-pressure/retry behavior and introducing an optional configuration API to cap the internal retry ladder, with accompanying unit tests to validate counter behavior under forced retry conditions.
Changes:
- Added
setSendRetryConfig(maxRetries, maxBackoffMs)to optionally bound the internal retry/backoff ladder. - Added four per-instance atomic counters with inline getters to expose EAGAIN/retry/backoff/ladder-exhaustion signals.
- Added unit tests to force a retry/ladder-exhaustion scenario and validate counter increments and initialization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
common/zmqclient.h |
Adds retry-cap API, send-path counter getters, and new member state for caps/counters. |
common/zmqclient.cpp |
Implements retry-cap setter and increments/records counters on retry/backoff and ladder exhaustion. |
tests/zmq_state_ut.cpp |
Adds unit tests to validate counter behavior and default initialization. |
c351568 to
6b8d4e4
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
tests/zmq_state_ut.cpp:467
- The comment says this unique ipc:// endpoint avoids a "TCP-port collision", but this test uses an IPC path (no TCP port). This is misleading when reading the test intent.
// any timing dependence. A unique per-process ipc endpoint avoids any fixed
// TCP-port collision across parallel/repeated test runs.
common/zmqclient.h:73
- The PR description claims four counters/getters (including getSendRetryTotal() and getSendLadderExhaustedTotal()), but the public API added here exposes three getters (getSendEagainTotal/getSendBlipAbsorbedTotal/getSendBackoffMaxMs) and does not track total retries or ladder exhaustion. This mismatch makes the new observability surface unclear for consumers and leaves the "ladder cap" behavior without an exhaustion counter.
// Send-path back-pressure counters. When the socket is full, sendMsg() only
// logs today; these process-local, per-instance counters expose the same
// signal so a producer can publish it as a leading indicator of downstream
// congestion. ZmqClient stays DB-agnostic — it only counts; the owner reads
// via these getters and decides where/how to publish.
// eagain_total : total EAGAIN (socket-full) occurrences across all sends.
// blip_absorbed_total: sends that hit >=1 EAGAIN but still succeeded within
// the inner retries — a transient blip the absorber
// swallowed so the caller never had to re-queue.
// backoff_max_ms : deepest per-attempt backoff ever waited — a coarse
// congestion-depth gauge.
uint64_t getSendEagainTotal() const { return m_sendEagainTotal.load(std::memory_order_relaxed); }
uint64_t getSendBlipAbsorbedTotal() const { return m_sendBlipAbsorbedTotal.load(std::memory_order_relaxed); }
uint64_t getSendBackoffMaxMs() const { return m_sendBackoffMaxMs.load(std::memory_order_relaxed); }
6b8d4e4 to
e3ed1ac
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
common/zmqclient.cpp:286
sendMsg()currently sleeps after every failed attempt, including the final attempt wheni == max_retriesand no further retry will occur. With a smallmaxRetriescap, this adds an extra (potentially largest) backoff delay before throwing, reducing the effectiveness of the "return control quickly" configuration.
usleep(retry_delay * 1000);
}
// Inner retries exhausted. The caller owns the outer retry (re-queue and
// retry later); surface the failure by throwing.
common/zmqclient.h:107
- This comment claims the change is "additive for consumers" because
ZmqClientis "only ever held by reference/pointer", but in-tree code instantiates it by value (e.g.,tests/zmq_state_ut.cpp:469). Appending data members changessizeof(ZmqClient)and its layout, so this is not ABI-additive for prebuilt consumers. Please adjust the comment to avoid giving a false ABI guarantee.
// Send-path back-pressure counters (see accessor doc above). Trailing
// members: ZmqClient is only ever held by reference/pointer, so appending
// these keeps the change additive for consumers.
e3ed1ac to
ce01084
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (4)
common/zmqclient.h:77
- The comment says trailing members keep this change ABI-additive, but adding non-static data members changes sizeof(ZmqClient) and is not binary-ABI compatible with prebuilt consumers. Please correct the comment (and consider an ABI/SONAME strategy if strict binary compatibility is required).
// Send-path back-pressure counters (see getters above). Trailing members
// keep the change ABI-additive.
std::atomic<uint64_t> m_sendEagainTotal{0};
std::atomic<uint64_t> m_sendBlipAbsorbedTotal{0};
std::atomic<uint64_t> m_sendBackoffMaxMs{0};
common/zmqclient.h:39
- setSendRetryConfig() is documented as "set-once, before first send", but the implementation supports changing it between sends (and the new unit test relies on doing so). Update the header comment to match the actual behavior to avoid misleading API consumers.
// Optionally shorten the inner send-retry loop (set-once, before first send).
// maxRetries < 0 keeps the default ladder; otherwise clamped to MQ_MAX_RETRY.
// maxBackoffMs < 0 keeps the exponential backoff; >= 0 caps each retry sleep (ms).
void setSendRetryConfig(int maxRetries, int maxBackoffMs = -1);
common/zmqclient.cpp:190
- This comment refers to the retry caps as "set-once", but they are read from atomics and can be changed between sendMsg() calls (as exercised by the unit test). Consider rewording to avoid implying a stricter usage contract than the code enforces.
bool saw_eagain = false;
// Snapshot the set-once caps so the loop reads stable values.
const int max_retries = m_sendMaxRetries.load(std::memory_order_relaxed);
const int max_backoff_ms = m_sendMaxBackoffMs.load(std::memory_order_relaxed);
for (int i = 0; i <= max_retries; ++i)
tests/zmq_state_ut.cpp:512
- Test comment refers to "additive" accessors linking, but this PR also changes ZmqClient's data member layout. Consider rewording to avoid reinforcing the inaccurate ABI-additive claim.
TEST(ZmqClientSendPathCounters, defaultCountersStartZeroAndAccessorsLink)
{
// Counters read zero before any send and the additive accessors link.
std::string endpoint = "ipc:///tmp/zmqclient_ut_default_" + std::to_string(getpid());
ZmqClient client(endpoint, 0);
EXPECT_EQ(client.getSendEagainTotal(), 0u);
EXPECT_EQ(client.getSendBlipAbsorbedTotal(), 0u);
EXPECT_EQ(client.getSendBackoffMaxMs(), 0u);
ce01084 to
e3801c8
Compare
|
/azp run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
common/zmqclient.cpp:259
- On the final attempt (i == max_retries), the code still logs "will retry" and updates getSendBackoffMaxMs(), even though no sleep/retry will occur (the loop exits and throws). This makes the log misleading and makes getSendBackoffMaxMs() reflect a scheduled backoff rather than a backoff that was actually waited.
// Track the deepest backoff waited (congestion-depth gauge).
uint64_t observed = m_sendBackoffMaxMs.load(std::memory_order_relaxed);
while (static_cast<uint64_t>(retry_delay) > observed &&
!m_sendBackoffMaxMs.compare_exchange_weak(observed,
static_cast<uint64_t>(retry_delay),
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
common/zmqclient.cpp:286
- sendMsg() currently calls usleep() after the final failed attempt (i == max_retries). That adds an extra delay right before throwing even though no retry will follow, which contradicts the PR description’s “no sleep after the final attempt” / “regains control immediately on exhaustion” behavior. Consider sleeping only when another retry will occur.
// unconditional final sleep keeps the default/uncapped ladder's total
// wait exactly as before (~41 s at MQ_MAX_RETRY); a capped caller only
// adds its (small) max_backoff_ms on the terminal attempt before the
// loop exits and throws.
usleep(retry_delay * 1000);
9f778fe to
6ce9d70
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
common/zmqclient.cpp:281
- sendMsg() currently calls usleep() unconditionally after every failed attempt, including the final attempt when the inner retry budget is exhausted. That contradicts the PR description (“No sleep is issued after the final attempt”) and also makes a capped retry configuration slower to return control (it still waits one extra backoff after the last attempt before throwing). Consider skipping the sleep when no further retry will occur, while preserving the legacy final-sleep only for the default (uncapped) configuration to avoid behavior changes for existing callers.
// Sleep after every failed attempt, incl. the last, to preserve the
// default ladder's total wait. A capped caller only adds max_backoff_ms.
usleep(retry_delay * 1000);
6ce9d70 to
65e956a
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
common/zmqclient.h:74
- The member comment says the new retry-cap defaults "preserve behavior", but sendMsg() now conditionally skips the final backoff sleep, so the overall default retry timing is no longer identical to the historical behavior. Consider updating this comment to describe the defaults precisely (MQ_MAX_RETRY + uncapped exponential backoff) rather than claiming behavioral equivalence.
// Inner send-retry caps (see setSendRetryConfig); defaults preserve behavior.
common/zmqclient.cpp:283
- This block unconditionally removes the final backoff sleep (and the preceding comment states this is intended). That shortens the maximum time sendMsg() will block even when the caller does not configure any caps, which contradicts the PR description's claim that default behavior is preserved for existing producers. If preserving historical default timing is required, keep the final sleep when using default settings and only skip it when the caller has configured a cap.
// No sleep after the final attempt — the loop is about to throw, so an
// uncapped caller stops ~one ladder step sooner and a capped caller
// regains control immediately on exhaustion (the outer loop owns retry).
if (i < max_retries)
…uge/log On inner-retry exhaustion the loop throws without waiting, so no usleep is issued after the final attempt (uncapped exhaustion ~41s -> ~20s; a capped caller regains control immediately, its outer loop owning retry). The backoff-depth gauge and the 'will retry' WARN are recorded only when a retry actually follows (i < max_retries), so the gauge reflects backoff waited. Signed-off-by: Deepak Singhal <deepsinghal@microsoft.com>
65e956a to
1e087f9
Compare
|
/azp run |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
common/zmqclient.cpp:235
- The EINTR/EFSM retry path currently sets
retry_delay = 0, which persists into later iterations. If a later iteration hits EAGAIN, the exponential backoff collapses to 0ms (doubling 0 stays 0), potentially causing a tight retry loop under back-pressure after an interrupt/state blip. Consider retrying immediately on EINTR/EFSM without permanently zeroing the ladder state (e.g., reset to the base delay andcontinue).
if (zmq_err == EINTR
|| zmq_err== EFSM)
{
Why I did it
Transport-layer half of the fix for sonic-buildimage #28369 (fpmsyncd ZMQ route drop under burst). The end-to-end design is described in the HLD sonic-net/SONiC #2481 — Producer-side ZMQ route delivery resiliency and heap release; this PR implements the swss-common transport-layer portion (send-path observability + a boundable inner retry loop) that the producer redesign builds on.
ZmqClient::sendMsg()drives the ZMQ socket in non-blocking mode and, on a full socket (EAGAIN) or transient unready state (EINTR/EFSM), retries with an exponential back-off ladder. Two gaps make that behavior hard to build a resilient producer on:MQ_MAX_RETRY), so a single full-socket message can block the send path for tens of seconds — which defeats a producer that owns an outer re-queue/coalescing loop and wants control back quickly.How I did it
The new API and counters are additive — no existing call site changes and every getter defaults to zero. One deliberate timing change applies to the retry loop itself (documented in the Bounded back-off bullet): the sleep after the final, exhausting attempt is now skipped for all callers.
setSendRetryConfig(int maxRetries, int maxBackoffMs = -1)— optional bound on the inner retry loop.maxRetries < 0keeps the defaultMQ_MAX_RETRYladder; otherwise it is clamped toMQ_MAX_RETRY(this can only shorten the ladder, never extend it) and the loop makes at mostmin(maxRetries, MQ_MAX_RETRY) + 1attempts.maxBackoffMs < 0keeps the default exponential back-off;>= 0clamps each retry sleep to that ceiling. A producer with its own outer loop can configure a short "blip absorber" so control returns fast instead of blocking on one message.getSendEagainTotal()— totalEAGAIN(socket-full) occurrences; the leading indicator.getSendBlipAbsorbedTotal()— sends that hit ≥1EAGAINbut still succeeded within the inner retries (a transient blip the absorber swallowed so the caller never re-queued).getSendBackoffMaxMs()— deepest per-attempt back-off ever waited; a coarse congestion-depth gauge.int, so the multiply cannot overflow andusleep()always receives a sane duration. The clamped value is what gets logged, recorded inbackoff_max_ms, and slept. No sleep is issued after the final, exhausting attempt — the loop is about to throw, so waiting served no purpose. This shortens the default/uncapped ladder's total wait from ~41 s to ~20 s (the deepest back-off actually waited is 10240 ms rather than 20480 ms); the outcome is unchanged (the call still throws on exhaustion, leaving the retry decision to the caller). The back-off-depth gauge and the retry log are updated only when a retry actually follows, sobackoff_max_msreflects back-off waited, not scheduled.ZmqClientstays DB-agnostic — it only counts and exposes getters; the owner decides where/how to publish. The counters and atomics are appended as new members and no existing call site or default behavior changes; all swss-common consumers are rebuilt from source, so no binary-ABI guarantee is implied. The consuming fpmsyncd change is a follow-up sonic-swss PR.How to verify it
New
ZmqClientSendPathCountersunit tests intests/zmq_state_ut.cpp:blipAbsorberExhaustionCountsEagainAndClampsBackoff— fills a peerless PUSH socket's local send buffer toSNDHWMto force deterministicEAGAIN, then assertseagain_totaladvances by exactlymaxRetries + 1, no blip is absorbed on a send that never succeeds, and each back-off is clamped to the caller's ceiling. A smallsetSendRetryConfigcap keeps it at a few ms instead of the default multi-second ladder.defaultCountersStartZeroAndAccessorsLink— guards that the accessors read zero before any send (the appended accessors compile, link, and initialize correctly).Full swss-common ZMQ unit-test suite: 28/28 pass, 0 regressions.
Which release branch to backport (provide reason below if selected)
Description for the changelog
zmqclient: add optional inner send-retry cap (
setSendRetryConfig) and process-local send-path back-pressure counters (getSendEagainTotal/getSendBlipAbsorbedTotal/getSendBackoffMaxMs); clamp the retry back-off against integer overflow. Defaults preserve existing behavior.Link to config_db schema for YANG module changes