fix(controller): standby dispatch-gate rejections now redirect to a known primary (bug-1700, bug-1701)
xceadmin scheduler record status/start/stop against a standby failed with the bare "controller returned status -10014" (E_XCE_STANDBY) with no redirect, forcing the caller to guess the primary manually. bug-1700: myreq_handler's generic ha_dispatch_gate rejection path (cluster.c) now calls fill_redirect_reply() when rc == -XCE_ERR_STANDBY, mirroring the same helper OPCode_join's handler already uses -- -XCE_ERR_NOT_READY (CTRL_INIT/CTRL_ELECTING, no settled primary to redirect to) is deliberately excluded. Landing this alone had zero observable effect live: ha_state_t.primary_host/primary_port were never written by any production path on a standby (only test fixtures set them) -- fill_redirect_reply() always saw an empty/zero redirect target in a real deployment. bug-1701: closes that data gap with a pure local lookup, no new wire field. handle_ctrlhello() (config_repl.c) now resolves the ctrlhello sender's name against this node's own statically-known peers_cfg (parsed once from --peers at boot) and populates primary_host/port at the exact moment a standby first learns who primary is. Live-verified end to end on a fresh 3-node cluster: xceadmin scheduler record status against any standby now prints "redirected: <standby> -> <primary>" and returns the real status, confirmed again after a re-election changed which node was primary. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AgYRU3zDTqNGV5Jk8aiKLg
This commit is contained in:
parent
ac1e312e2f
commit
f886c80930
4 changed files with 305 additions and 0 deletions
|
|
@ -366,6 +366,10 @@ void schedadmin_set_timeout_ms_for_test(long ms)
|
|||
#define SCHEDADMIN_TIMEOUT_MS 5000L
|
||||
#endif
|
||||
|
||||
/* fill_redirect_reply is defined in join.c (no header declaration) --
|
||||
* mirrors config_repl.c's identical local extern for the same helper. */
|
||||
extern void fill_redirect_reply(xce_reply_t *reply, const ha_state_t *ha);
|
||||
|
||||
static int myreq_handler(vtask_t* task)
|
||||
{
|
||||
vtask_data_t* data = (vtask_data_t*)task->data;
|
||||
|
|
@ -408,6 +412,25 @@ static int myreq_handler(vtask_t* task)
|
|||
if (rc != 0) {
|
||||
reply.status = rc;
|
||||
reply.data_which = RequestReply_data_nodata;
|
||||
/* bug-1700: -XCE_ERR_STANDBY (ha_dispatch_gate's CTRL_STANDBY
|
||||
* default-case rejection) is the one gate rejection where this node
|
||||
* may know who the primary is -- populate the redirect hint so
|
||||
* xceadmin's already-generic client-side follow_redirects() can
|
||||
* transparently retry there, exactly like OPCode_join's handler
|
||||
* already does (join.c:277). ha_dispatch_gate's OTHER nonzero
|
||||
* rejection, -XCE_ERR_NOT_READY (CTRL_INIT/CTRL_ELECTING, and a
|
||||
* fenced/quorum-less CTRL_PRIMARY), has no settled primary to
|
||||
* redirect to, so it is deliberately excluded. fill_redirect_reply()
|
||||
* is safe even when no primary is known yet: ha->primary_host is a
|
||||
* fixed buffer embedded in ha_state_t (never NULL), so an
|
||||
* as-yet-unknown primary just yields an empty string / port 0,
|
||||
* which both the capnp encode path (controller.capnp.c's
|
||||
* encode_RequestReply, gated on redirect_host != NULL) and the
|
||||
* client's discovery.py _redirect_target() already treat as
|
||||
* "no redirect" -- verified via code inspection, not assumed. */
|
||||
if (rc == -XCE_ERR_STANDBY) {
|
||||
fill_redirect_reply(&reply, &c->ha);
|
||||
}
|
||||
xce_reply(ep, c->myhost, &reply);
|
||||
xce_free_request(&request);
|
||||
return rc;
|
||||
|
|
|
|||
|
|
@ -2164,6 +2164,9 @@ char *sanitise_cn(const char *src, size_t maxlen)
|
|||
* Steps:
|
||||
* 1. Validate request body
|
||||
* 2. Set primary_ctrl_fd = ep->fd (first ctrlhello from winner, idempotent)
|
||||
* 2b. (bug-1701) Look up the sender's name against our own static
|
||||
* peers_cfg candidate list and copy its host:port into
|
||||
* ha->primary_host/primary_port
|
||||
* 3. Call ha_on_heartbeat_received (resets liveness watchdog)
|
||||
* 4. Arm ha_arm_heartbeat_timeout_vtask
|
||||
*/
|
||||
|
|
@ -2183,6 +2186,57 @@ int handle_ctrlhello(cluster_t *c, endpoint_t *ep, xce_request_t *req)
|
|||
memory_order_acquire) == CTRL_STANDBY &&
|
||||
c->ha.primary_ctrl_fd == -1) {
|
||||
c->ha.primary_ctrl_fd = ep->fd;
|
||||
|
||||
/* bug-1701: ha->primary_host/primary_port (aliased into a redirect
|
||||
* reply by fill_redirect_reply(), join.c:27) were never written by
|
||||
* any production path, so bug-1700's redirect fix was a live no-op.
|
||||
* Fix is a pure LOCAL lookup — no wire-protocol change needed: every
|
||||
* node already knows every candidate's host:port from boot
|
||||
* (c->myconf->peers_cfg, parsed once from --peers). This is the
|
||||
* moment a standby first learns "I am now connected to my primary";
|
||||
* req->from->name carries the sender's name= identity (set for
|
||||
* every incoming request at request.c:379, "req->from = ep->peer;"),
|
||||
* which is the SAME name used in --peers name=host:port. Match it
|
||||
* against our own candidate list (mirrors the lookup-by-name pattern
|
||||
* already used for voted_for_name in ha_init_raft, ha.c:7822-7829)
|
||||
* and copy that candidate's hostname/port. On no match (real config
|
||||
* mismatch) or a NULL req->from/name, leave primary_host/primary_port
|
||||
* at their previous value — a stale-but-previously-correct target is
|
||||
* safer than wiping a working redirect on a transient miss — and log
|
||||
* a WARN so a genuine mismatch is operator-visible.
|
||||
*
|
||||
* No explicit clear is needed on primary loss: ha_on_primary_ctrl_fd_
|
||||
* rdhup (ha.c) resets primary_ctrl_fd back to -1, so the NEXT real
|
||||
* ctrlhello (from the same or a new primary) re-satisfies this same
|
||||
* idempotency guard and re-runs this lookup naturally.
|
||||
*
|
||||
* Locking: ha.h's `lock` field doc ("Protects primary_host/port +
|
||||
* voted_for_term/name") and existing writers of the other field it
|
||||
* covers (voted_for_name, ha.c:2304-2332, 5559-5593) take
|
||||
* c->ha.lock around the write — matched here for the same fields. */
|
||||
if (req->from && req->from->name && c->myconf) {
|
||||
const xce_cluster_config_t *pcfg = &c->myconf->peers_cfg;
|
||||
int matched = 0;
|
||||
for (int i = 0; i < pcfg->candidates_count; i++) {
|
||||
if (strcmp(pcfg->candidates[i].name, req->from->name) == 0) {
|
||||
pthread_mutex_lock(&c->ha.lock);
|
||||
strncpy(c->ha.primary_host, pcfg->candidates[i].hostname,
|
||||
sizeof(c->ha.primary_host) - 1);
|
||||
c->ha.primary_host[sizeof(c->ha.primary_host) - 1] = '\0';
|
||||
c->ha.primary_port = (uint32_t)pcfg->candidates[i].port;
|
||||
pthread_mutex_unlock(&c->ha.lock);
|
||||
matched = 1;
|
||||
break;
|
||||
}
|
||||
}
|
||||
if (!matched) {
|
||||
WARN("handle_ctrlhello: sender '%s' does not match any "
|
||||
"configured peer candidate in --peers -- "
|
||||
"primary_host/primary_port left unchanged (possible "
|
||||
"config mismatch)", req->from->name);
|
||||
}
|
||||
}
|
||||
|
||||
ha_on_heartbeat_received(&c->ha);
|
||||
ha_arm_heartbeat_timeout_vtask(&c->ha, c);
|
||||
INFO("HA: primary control channel established (fd=%d)", ep->fd);
|
||||
|
|
|
|||
|
|
@ -427,6 +427,127 @@ static void test_handle_ctrlhello_sets_primary_ctrl_fd(void)
|
|||
printf("test_handle_ctrlhello_sets_primary_ctrl_fd: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Test 8b (bug-1701): handle_ctrlhello populates primary_host/port from
|
||||
* the sender's name matched against our own static peers_cfg candidate list.
|
||||
* This is the local-lookup fix that unblocks bug-1700's fill_redirect_reply()
|
||||
* mechanism (join.c:27), which is fed by ha->primary_host/primary_port and
|
||||
* was permanently empty before this fix — no wire-protocol change, just a
|
||||
* boot-time-known static lookup keyed by req->from->name. ──────────────── */
|
||||
|
||||
static void test_handle_ctrlhello_populates_primary_host_from_known_peer(void)
|
||||
{
|
||||
cluster_t c;
|
||||
memset(&c, 0, sizeof(c));
|
||||
ha_init(&c.ha, &g_cfg, 0);
|
||||
c.ha.primary_ctrl_fd = -1;
|
||||
atomic_store_explicit(&c.ha.role, (int32_t)CTRL_STANDBY, memory_order_release);
|
||||
|
||||
/* Static peer list (as parsed at boot from --peers name=host:port,...)
|
||||
* containing the candidate that will send us this ctrlhello. */
|
||||
myconf_t conf;
|
||||
memset(&conf, 0, sizeof(conf));
|
||||
conf.peers_cfg.candidates_count = 1;
|
||||
strncpy(conf.peers_cfg.candidates[0].name, "primary1",
|
||||
sizeof(conf.peers_cfg.candidates[0].name) - 1);
|
||||
strncpy(conf.peers_cfg.candidates[0].hostname, "10.0.1.7",
|
||||
sizeof(conf.peers_cfg.candidates[0].hostname) - 1);
|
||||
conf.peers_cfg.candidates[0].port = 9991;
|
||||
c.myconf = &conf;
|
||||
|
||||
endpoint_t ep;
|
||||
memset(&ep, 0, sizeof(ep));
|
||||
ep.fd = 42;
|
||||
|
||||
ctrl_hello_req_t hello;
|
||||
memset(&hello, 0, sizeof(hello));
|
||||
hello.name = "primary1";
|
||||
hello.applied_lsn = 0;
|
||||
hello.config_tag = 0;
|
||||
|
||||
/* req->from is populated in production by request.c:379
|
||||
* ("req->from = ep->peer;") for EVERY incoming request, carrying the
|
||||
* sender's name= identity from --peers name=host:port. */
|
||||
xce_peerinfo_t from;
|
||||
memset(&from, 0, sizeof(from));
|
||||
from.name = "primary1";
|
||||
from.key = NULL;
|
||||
|
||||
xce_request_t req;
|
||||
memset(&req, 0, sizeof(req));
|
||||
req.from = &from;
|
||||
req.body_which = Request_body_ctrlhellomsg;
|
||||
req.body.ctrlhellomsg = &hello;
|
||||
|
||||
handle_ctrlhello(&c, &ep, &req);
|
||||
|
||||
TEST_ASSERT_EQUAL_STRING_MESSAGE("10.0.1.7", c.ha.primary_host,
|
||||
"bug-1701: handle_ctrlhello must copy the matched candidate's "
|
||||
"hostname into ha->primary_host so fill_redirect_reply() has a "
|
||||
"real target");
|
||||
TEST_ASSERT_EQUAL_UINT32_MESSAGE(9991, c.ha.primary_port,
|
||||
"bug-1701: handle_ctrlhello must copy the matched candidate's port "
|
||||
"into ha->primary_port");
|
||||
|
||||
ha_fini(&c.ha);
|
||||
printf("test_handle_ctrlhello_populates_primary_host_from_known_peer: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Test 8c (bug-1701 edge case): unknown sender name must not crash and
|
||||
* must leave primary_host/primary_port unchanged (not cleared to empty) — a
|
||||
* real config mismatch is surfaced via WARN, not a crash or data wipe. ───── */
|
||||
|
||||
static void test_handle_ctrlhello_unknown_peer_leaves_primary_host_unchanged(void)
|
||||
{
|
||||
cluster_t c;
|
||||
memset(&c, 0, sizeof(c));
|
||||
ha_init(&c.ha, &g_cfg, 0);
|
||||
c.ha.primary_ctrl_fd = -1;
|
||||
atomic_store_explicit(&c.ha.role, (int32_t)CTRL_STANDBY, memory_order_release);
|
||||
|
||||
myconf_t conf;
|
||||
memset(&conf, 0, sizeof(conf));
|
||||
conf.peers_cfg.candidates_count = 1;
|
||||
strncpy(conf.peers_cfg.candidates[0].name, "primary1",
|
||||
sizeof(conf.peers_cfg.candidates[0].name) - 1);
|
||||
strncpy(conf.peers_cfg.candidates[0].hostname, "10.0.1.7",
|
||||
sizeof(conf.peers_cfg.candidates[0].hostname) - 1);
|
||||
conf.peers_cfg.candidates[0].port = 9991;
|
||||
c.myconf = &conf;
|
||||
|
||||
endpoint_t ep;
|
||||
memset(&ep, 0, sizeof(ep));
|
||||
ep.fd = 42;
|
||||
|
||||
ctrl_hello_req_t hello;
|
||||
memset(&hello, 0, sizeof(hello));
|
||||
hello.name = "stranger";
|
||||
|
||||
xce_peerinfo_t from;
|
||||
memset(&from, 0, sizeof(from));
|
||||
from.name = "stranger"; /* does not match any configured candidate */
|
||||
|
||||
xce_request_t req;
|
||||
memset(&req, 0, sizeof(req));
|
||||
req.from = &from;
|
||||
req.body_which = Request_body_ctrlhellomsg;
|
||||
req.body.ctrlhellomsg = &hello;
|
||||
|
||||
handle_ctrlhello(&c, &ep, &req); /* must not crash */
|
||||
|
||||
TEST_ASSERT_EQUAL_STRING_MESSAGE("", c.ha.primary_host,
|
||||
"bug-1701: an unmatched sender name must leave primary_host at its "
|
||||
"prior (fresh-node empty) value, never crash");
|
||||
TEST_ASSERT_EQUAL_UINT32_MESSAGE(0, c.ha.primary_port,
|
||||
"bug-1701: an unmatched sender name must leave primary_port at its "
|
||||
"prior (fresh-node zero) value");
|
||||
TEST_ASSERT_EQUAL_INT_MESSAGE(42, c.ha.primary_ctrl_fd,
|
||||
"bug-1701: primary_ctrl_fd bookkeeping must proceed normally even "
|
||||
"when the name lookup misses");
|
||||
|
||||
ha_fini(&c.ha);
|
||||
printf("test_handle_ctrlhello_unknown_peer_leaves_primary_host_unchanged: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Test 9 (Task 7.2): follower path is passive — no outbound connect ──────
|
||||
*
|
||||
* Req 2.1, 2.4: after raft_cb_become_follower the role is CTRL_STANDBY and
|
||||
|
|
@ -511,6 +632,11 @@ void test_ha_connection_reuse(void)
|
|||
/* Req 3.1 — handle_ctrlhello designates primary control channel */
|
||||
RUN_TEST(test_handle_ctrlhello_sets_primary_ctrl_fd);
|
||||
|
||||
/* bug-1701 — handle_ctrlhello populates primary_host/primary_port via a
|
||||
* local peers_cfg lookup keyed by req->from->name */
|
||||
RUN_TEST(test_handle_ctrlhello_populates_primary_host_from_known_peer);
|
||||
RUN_TEST(test_handle_ctrlhello_unknown_peer_leaves_primary_host_unchanged);
|
||||
|
||||
/* Task 7.2 — Req 2.1, 2.4: follower path is fully passive */
|
||||
RUN_TEST(test_follower_passive_no_outbound_connect);
|
||||
|
||||
|
|
|
|||
|
|
@ -235,6 +235,106 @@ static void test_schedadmin_rejects_when_not_primary(void)
|
|||
printf("test_schedadmin_rejects_when_not_primary: PASS\n");
|
||||
}
|
||||
|
||||
/* ── bug-1700: a standby dispatch-gate rejection must redirect to the known
|
||||
* primary ──────────────────────────────────────────────────────────────────
|
||||
*
|
||||
* ha_dispatch_gate() rejects OPCode_schedAdmin on a CTRL_STANDBY node with
|
||||
* -XCE_ERR_STANDBY (ha.c's CTRL_STANDBY case default branch, ~line 4774) --
|
||||
* this node MAY know who the primary is (c->ha.primary_host/primary_port).
|
||||
* Before the fix, myreq_handler's generic ha_dispatch_gate rejection block
|
||||
* (cluster.c ~406-414) never called fill_redirect_reply() (the exact same
|
||||
* one-line helper OPCode_join's handler already uses, join.c:277), so
|
||||
* redirect_host/redirect_port were left at their zero-value defaults on
|
||||
* EVERY dispatch-gate rejection -- xceadmin's client-side, already fully
|
||||
* implemented and generic follow_redirects() had nothing to follow and
|
||||
* surfaced the bare -XCE_ERR_STANDBY status as-is instead of transparently
|
||||
* retrying against the primary. This is the exact scenario the user hit
|
||||
* running `xceadmin scheduler record status` against a standby. */
|
||||
static void test_schedadmin_standby_with_known_primary_redirects(void)
|
||||
{
|
||||
cluster_t* c = sa_make_cluster();
|
||||
atomic_store(&c->ha.role, CTRL_STANDBY);
|
||||
strncpy(c->ha.primary_host, "10.0.1.7", sizeof(c->ha.primary_host) - 1);
|
||||
c->ha.primary_port = 7001;
|
||||
|
||||
/* Poison sentinel mirrors test_schedadmin_rejects_when_not_primary above
|
||||
* -- a gate rejection must never reach the forwarding code. */
|
||||
endpoint_t* poison = (endpoint_t*)(uintptr_t)0x1;
|
||||
c->sched.ep = poison;
|
||||
|
||||
endpoint_t ep;
|
||||
memset(&ep, 0, sizeof(ep));
|
||||
ep.owner = c;
|
||||
|
||||
io_packet_t* pkt = sa_build_request_pkt();
|
||||
int rc = myreq_handler_for_test(&ep, pkt);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rc);
|
||||
TEST_ASSERT_TRUE_MESSAGE(
|
||||
c->sched.ep == poison,
|
||||
"c->sched.ep must be untouched -- a gate rejection never forwards");
|
||||
|
||||
xce_request_t decoded;
|
||||
sa_decode_queued_reply(&ep, &decoded);
|
||||
TEST_ASSERT_EQUAL_INT(Request_body_replymsg, decoded.body_which);
|
||||
xce_reply_t* rr = decoded.body.replymsg;
|
||||
TEST_ASSERT_NOT_NULL(rr);
|
||||
TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rr->status);
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
rr->redirect_host,
|
||||
"bug-1700: a standby dispatch-gate rejection with a known primary "
|
||||
"must carry that primary's address so xceadmin's follow_redirects() "
|
||||
"can transparently retry there");
|
||||
TEST_ASSERT_EQUAL_STRING("10.0.1.7", rr->redirect_host);
|
||||
TEST_ASSERT_EQUAL_UINT16(7001, rr->redirect_port);
|
||||
xce_free_request(&decoded);
|
||||
|
||||
sa_teardown(c);
|
||||
printf("test_schedadmin_standby_with_known_primary_redirects: PASS\n");
|
||||
}
|
||||
|
||||
/* Regression companion: a standby that does NOT yet know any live primary
|
||||
* (fresh ha_state_t straight out of calloc -- primary_host empty,
|
||||
* primary_port 0, e.g. very early boot / mid-election / primary just died
|
||||
* with no successor elected yet) must NOT crash and must produce a reply
|
||||
* the client correctly treats as non-redirecting: discovery.py's
|
||||
* `_redirect_target()` requires BOTH a non-empty host AND a non-zero port
|
||||
* to treat a reply as a redirect, so an empty host / zero port here is
|
||||
* harmless and falls back to today's bare-rejection behavior. Proves the
|
||||
* fix does not regress the genuinely-no-primary-yet case. */
|
||||
static void test_schedadmin_standby_no_known_primary_no_redirect(void)
|
||||
{
|
||||
cluster_t* c = sa_make_cluster();
|
||||
atomic_store(&c->ha.role, CTRL_STANDBY);
|
||||
/* c->ha.primary_host/primary_port left at their calloc'd zero value. */
|
||||
|
||||
endpoint_t ep;
|
||||
memset(&ep, 0, sizeof(ep));
|
||||
ep.owner = c;
|
||||
|
||||
io_packet_t* pkt = sa_build_request_pkt();
|
||||
int rc = myreq_handler_for_test(&ep, pkt);
|
||||
|
||||
TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rc);
|
||||
|
||||
xce_request_t decoded;
|
||||
sa_decode_queued_reply(&ep, &decoded);
|
||||
TEST_ASSERT_EQUAL_INT(Request_body_replymsg, decoded.body_which);
|
||||
xce_reply_t* rr = decoded.body.replymsg;
|
||||
TEST_ASSERT_NOT_NULL(rr);
|
||||
TEST_ASSERT_EQUAL_INT(-XCE_ERR_STANDBY, rr->status);
|
||||
TEST_ASSERT_TRUE_MESSAGE(
|
||||
rr->redirect_host == NULL || rr->redirect_host[0] == '\0',
|
||||
"no known primary: redirect_host must decode as empty/absent, "
|
||||
"never garbage -- the client's _redirect_target() treats an empty "
|
||||
"host as 'no redirect, use this reply as-is'");
|
||||
TEST_ASSERT_EQUAL_UINT16(0, rr->redirect_port);
|
||||
xce_free_request(&decoded);
|
||||
|
||||
sa_teardown(c);
|
||||
printf("test_schedadmin_standby_no_known_primary_no_redirect: PASS\n");
|
||||
}
|
||||
|
||||
/* ── Observable 2: primary with no scheduler connected rejects immediately ──
|
||||
*/
|
||||
static void test_schedadmin_rejects_when_not_connected(void)
|
||||
|
|
@ -775,6 +875,8 @@ void test_sched_admin_dispatch(void)
|
|||
{
|
||||
UnitySetTestFile(__FILE__);
|
||||
RUN_TEST(test_schedadmin_rejects_when_not_primary);
|
||||
RUN_TEST(test_schedadmin_standby_with_known_primary_redirects);
|
||||
RUN_TEST(test_schedadmin_standby_no_known_primary_no_redirect);
|
||||
RUN_TEST(test_schedadmin_rejects_when_not_connected);
|
||||
RUN_TEST(test_schedadmin_fast_rejects_when_admission_exhausted);
|
||||
RUN_TEST(test_schedadmin_round_trip_success);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue