physrep: skip rtcpu'd / decommissioned hosts when establishing reverse connections - #6091
Open
markhannum wants to merge 2 commits into
Open
physrep: skip rtcpu'd / decommissioned hosts when establishing reverse connections#6091markhannum wants to merge 2 commits into
markhannum wants to merge 2 commits into
Conversation
roborivers
approved these changes
Jul 28, 2026
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Success ✓.
Regression testing: Success ✓.
The first 10 failing tests are:
queuedb_rollover_noroll1_generated **quarantined**
consumer_non_atomic_default_consumer_generated **quarantined**
sc_downgrade [timeout] **quarantined**
The reverse-connection machinery never consulted rtcpu, unlike the normal (non-reverse) source-selection path which rejects rtcpu'd nodes in physrep_allowed_source(). As a result, a source/intermediary would spawn a reverse-connection worker per target host read from the physrep sources table and spin forever trying to connect to a host that rtcpu reports as down or decommissioned (e.g. a stale sources-table entry left behind after a decomm), producing an endless stream of "Failed to connect" errors. Harden both sides of the reverse connection: - Sender (reverse_conn.c): the worker now skips a connect attempt when the target host is not up per rtcpu, and resumes automatically once it is. - Receiver (reversesql.c): an inbound 'reversesql' request from a source that rtcpu reports as down/decommissioned is now rejected. A host is considered "not up" when machine_is_up() returns anything other than 1, which covers both rtcpu'd-down (0) and unknown/decommissioned (-1) hosts. The shared predicate physrep_revconn_host_is_up() is also exposed via a new 'revconn_host_is_up <host>' message command for testing. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Hannum <mhannum@bloomberg.net>
Add physrep_revconn_rtcpu to phys_rep_tiered.test, modeled on the existing physrep_rtcpu_source case. Using the 'fakedr' trap to force machine_is_up() to report a host as down, it asserts that the new 'revconn_host_is_up' predicate (physrep_revconn_host_is_up) returns "up" for a healthy host, "down" once the host is marked rtcpu'd, and "up" again after the mark is cleared -- i.e. the reverse-connection sender and receiver will skip a decommissioned/rtcpu'd host. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Mark Hannum <mhannum@bloomberg.net>
markhannum
force-pushed
the
drqs_185361198_revconn_rtcpu
branch
from
July 28, 2026 18:03
678cb88 to
b867609
Compare
roborivers
suggested changes
Jul 28, 2026
roborivers
left a comment
There was a problem hiding this comment.
Cbuild submission: Error ⚠.
Regression testing: Success ✓.
The first 10 failing tests are:
ssl_san
sc_parallel_logicalsc_generated
consumer_non_atomic_default_consumer_generated **quarantined**
ssl_set_cmd
ssl_prefer
ssl_dbname
sc_downgrade [timeout] **quarantined**
reco-ddlk-sql [timeout] **quarantined**
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.
Summary
The physrep reverse-connection machinery never consulted rtcpu, unlike the normal (non-reverse) source-selection path which already rejects rtcpu'd nodes in
physrep_allowed_source(). As a result, a source/intermediary would spawn a reverse-connection worker per target host read from the physrep sources table and spin forever trying to connect to a host that rtcpu reports as down or decommissioned — for example a stale sources-table entry left behind after a machine was decommed — producing an endless stream of "Failed to connect" errors. This change brings the reverse-connection path in line with the rtcpu discipline the normal path already enforces, on both the sending and receiving sides.Background
For cross-cloud physrep (PPCLD -> PDCLD), the replicant often cannot dial out to its source, so the source dials in to the replicant and hands it a socket (a "reverse connection"), which the replicant then uses to pull logs. There are two sides: the sender of reverse connections is the source/intermediary (
db/reverse_conn.c), which runs one worker thread per target host and periodically callssend_reversesql_request(); the receiver is the replicant accepting the inboundreversesqlappsock (plugins/reversesql/reversesql.c). Neither side checked rtcpu before this change.Changes
Sender (
db/reverse_conn.c): the reverse-connection worker now skips a connect attempt when the target host is not up per rtcpu, and resumes automatically once the host comes back.Receiver (
plugins/reversesql/reversesql.c): an inboundreversesqlrequest from a source that rtcpu reports as down/decommissioned is now rejected.A host is considered "not up" when
machine_is_up()returns anything other than 1, which covers both rtcpu'd-down (0) and unknown/decommissioned (-1) hosts. Note this is intentionally stricter thanphysrep_allowed_source(), which uses!nodeup_rtn(...)and would treat an unknown (-1) host as up; the stricter check is what actually catches a fully decommed machine.The shared predicate
physrep_revconn_host_is_up()is exposed via a newrevconn_host_is_up <host>message command for testing.Testing
Adds
physrep_revconn_rtcputotests/phys_rep_tiered.test, modeled on the existingphysrep_rtcpu_sourcecase. Using thefakedrtrap to forcemachine_is_up()to report a host as down, it asserts thatrevconn_host_is_upreturns "up" for a healthy host, "down" once the host is marked rtcpu'd, and "up" again after the mark is cleared. Connections use--admin, since the node under test can itself be the one marked rtcpu'd (drtest) and would otherwise refuse normal client traffic.Notes
This is framed as hardening rather than the root-cause fix. In the originating incident the SQL engine pool was saturated with inbound
comdb2_transaction_logsqueries and the timestamps did not correlate with the reverse-connect failures, and the underlying trigger — a decommissioned machine left in thecomdb2_physrep_sourcestable — has been acknowledged and cleaned up separately by the DBA group. This change is defense-in-depth so a stale or decommissioned source entry can no longer cause an endless reverse-connect storm.