chore: baseline snapshot — prior-session work + ha-consistency-invariants spec family
Pre-existing uncommitted work: ha-repl-hardening payload data plane (7.x), heartbeat liveness fix (Req 1.3), connection-reuse ASAN fixes, doc cleanup. New: ha-consistency-invariants umbrella (requirements/design/research) and its four implementation sub-specs (specs generated via batch, cross-spec review applied), steering roadmap + product.md Raft+CP decision.
This commit is contained in:
parent
084244941b
commit
06135954ff
88 changed files with 25611 additions and 5285 deletions
|
|
@ -5,10 +5,9 @@ C daemon managing cluster-level job and queue operations. Acts as the central co
|
|||
|
||||
## Build
|
||||
```bash
|
||||
# From workspace root
|
||||
nix develop
|
||||
meson build && ninja -C build
|
||||
/xce-build controller # debug + ASAN + clean build + runs tests
|
||||
```
|
||||
For full build system details, see `.kiro/steering/build-deployment.md`.
|
||||
|
||||
## Architecture
|
||||
Core source files handle distinct domains:
|
||||
|
|
@ -25,7 +24,9 @@ Links against `libxce` from `src/lib/`.
|
|||
- Component-specific headers in `include/`
|
||||
- Shared types defined in `src/include/xce/` (libxce)
|
||||
|
||||
## HA Protocol Verification
|
||||
## HA Protocol
|
||||
|
||||
See `.kiro/steering/ha-async-io.md` for the full HA protocol reference: async I/O model (iomgr/vtaskmgr/delaymgr), connection ownership, Raft election, heartbeat model, two-tier replication consistency, dispatch gate, and TLA+ variable mapping. The TLA+ safety invariants and protocol verification checklist below are the authoritative controller-specific verification gates — keep them here.
|
||||
|
||||
### Raft TLA+ Formal Specification
|
||||
|
||||
|
|
@ -91,94 +92,9 @@ When modifying HA protocol code (election, replication, heartbeat), verify:
|
|||
|
||||
## HA Design Constraints
|
||||
|
||||
### Inbound vs. Outbound Connection Ownership (CRITICAL)
|
||||
|
||||
**Strict rule: inbound connections are for voting only. All persistent control channels are outbound from the primary.**
|
||||
|
||||
| Connection type | Direction | Purpose | Who closes |
|
||||
|---|---|---|---|
|
||||
| Vote RPC | Standby → Primary (outbound from standby) | `OPCode_requestVote` + reply only | Remote closes after reply |
|
||||
| Standby control channel | Primary → Standby (outbound from primary) | ctrlhello, config push, WAL replication, heartbeat | `vote_ep_err_handler` / `standby_ep_err_handler_primary` on disconnect |
|
||||
|
||||
Rules:
|
||||
- **Inbound vote connections** (accepted on the listener) are used **exclusively** for the vote exchange. After the reply is sent the primary does NOT reuse the connection for config/replication. The inbound ep is left open until the remote closes it.
|
||||
- **Primary never uses an inbound connection to push state.** All state push (ctrlhello, config, WAL, heartbeat nop) goes over **outbound connections** stored in `ha->standby_endpoints`.
|
||||
- During election: candidate opens outgoing vote connections (one per peer). After winning, `raft_cb_become_leader` promotes surviving vote connections to `standby_endpoints`.
|
||||
- Late joiners: the primary's heartbeat task (`primary_heartbeat_fn`) scans the peers list and **opens a fresh outbound connection** to any peer absent from `standby_endpoints`. On connect, `on_standby_connected` sends ctrlhello and registers the ep.
|
||||
- `CTRL_STANDBY` must not call `ha_find_and_connect_to_primary` or schedule `fetch_config_from_primary` after an election result — the primary will connect to it.
|
||||
|
||||
### Asynchronous, Non-Blocking I/O (CRITICAL)
|
||||
All inter-controller communication uses iomgr (epoll-based) and vtaskmgr:
|
||||
- `iomgr` dispatches inbound packets to vtask handlers — never block inside a vtask.
|
||||
- `vtaskmgr` runs handlers on a thread pool; each handler must return quickly.
|
||||
- Outgoing sends use `xce_request(ep, &req)` which enqueues to the endpoint's send buffer — non-blocking.
|
||||
- Blocking I/O (e.g., `fetch_config_from_primary`, `ha_find_and_connect_to_primary`) is only permitted from `delaymgr` worker threads, **never** from iomgr/vtask context.
|
||||
|
||||
### Heartbeat Model
|
||||
|
||||
XCE uses a bidirectional heartbeat model aligned with Raft's AppendEntries:
|
||||
- The **primary** sends periodic heartbeats (`OPCode_nop`) to all standbys via `ha_start_primary_heartbeat` (delaymgr task, interval=3s). During `CTRL_PRIMARY_COMMITTING`, this also serves as the replication driver — the primary evaluates standby state from heartbeat replies and pushes config/WAL/snapshot as needed.
|
||||
- The **standby** replies to each heartbeat with its current `(applied_lsn, config_tag)` in `RequestReply.stateReport`. This gives the primary continuous visibility into standby convergence.
|
||||
- The **standby** also arms a liveness watchdog (`ha_arm_heartbeat_timeout_vtask`) when entering `CTRL_STANDBY`. If no message arrives from the primary within `heartbeat_timeout_ms` (10s), it triggers re-election via `ha_on_primary_ctrl_fd_rdhup`.
|
||||
- `raft.c` (the election library) is election-only. Heartbeat and replication logic belongs in the HA layer (`ha.c`, `config_repl.c`).
|
||||
|
||||
### Connection Ownership
|
||||
- Promoted vote connections (`ha->standby_endpoints`): owned by `vote_ep_err_handler`; freed when the standby disconnects.
|
||||
- Late-joiner inbound connections: owned by the cluster endpoint array (`c->endpoints`); the `ep_private_t` role identifies `HostRole_controller` for cleanup in `handle_endpoint_error`.
|
||||
- `ha->primary_ctrl_fd`: set by `handle_ctrlhello` on the standby when the first ctrlhello arrives; used by `ha_on_primary_ctrl_fd_rdhup` to trigger re-election on disconnect.
|
||||
|
||||
## HA Cluster Deployment
|
||||
|
||||
Use the `/xce-deploy-ha` skill to deploy a local 3-node HA cluster for testing:
|
||||
|
||||
```
|
||||
/xce-deploy-ha localhost:9991 localhost:9992 localhost:9993
|
||||
```
|
||||
|
||||
**Prerequisites:**
|
||||
- Build binary: `/xce-build controller`
|
||||
- TLS certs and `cluster.bin` at `ha/data/` (already in repo)
|
||||
|
||||
**Quick deploy (from repo root):**
|
||||
```bash
|
||||
# Deploy + start all 3 nodes with raft trace logging
|
||||
bash src/tools/deploy-ha.sh \
|
||||
src/xce.build/controller/xce_controller \
|
||||
ha/data \
|
||||
/tmp/xce-deploy \
|
||||
"localhost:9991,localhost:9992,localhost:9993"
|
||||
|
||||
VERBOSE=-vvv /tmp/xce-deploy/scripts/start.sh ctrl-1 &
|
||||
sleep 0.5
|
||||
VERBOSE=-vvv /tmp/xce-deploy/scripts/start.sh ctrl-2 &
|
||||
sleep 0.5
|
||||
VERBOSE=-vvv /tmp/xce-deploy/scripts/start.sh ctrl-3 &
|
||||
```
|
||||
|
||||
**Verify health (after ~15s):**
|
||||
```bash
|
||||
# Primary elected and config pushed
|
||||
grep -i "CTRL_PRIMARY\|Config committed" /tmp/xce-deploy/logs/ctrl-1.log | grep -v DEBUG3
|
||||
# Config files present on standbys
|
||||
find /tmp/xce-deploy/controller/ctrl-2/conf /tmp/xce-deploy/controller/ctrl-3/conf -type f
|
||||
```
|
||||
|
||||
**Clean restart** (keeps ctrl-1 config + all certs, wipes data/standby-conf/logs):
|
||||
```bash
|
||||
kill -9 $(pgrep xce_controller) 2>/dev/null || true
|
||||
bash /tmp/xce-deploy/scripts/cleanup.sh
|
||||
# Then restart nodes (no redeploy needed)
|
||||
VERBOSE=-vvv /tmp/xce-deploy/scripts/start.sh ctrl-1 &
|
||||
sleep 0.5; VERBOSE=-vvv /tmp/xce-deploy/scripts/start.sh ctrl-2 &
|
||||
sleep 0.5; VERBOSE=-vvv /tmp/xce-deploy/scripts/start.sh ctrl-3 &
|
||||
```
|
||||
|
||||
See `.claude/skills/xce-deploy-ha/SKILL.md` for full troubleshooting reference.
|
||||
Covered in `.kiro/steering/ha-async-io.md` (connection ownership, async I/O, heartbeat, two-tier replication, dispatch gate) and `.kiro/steering/build-deployment.md` (deploy commands, cluster startup/restart/verification). The TLA+ safety invariants above remain the controller-specific verification reference.
|
||||
|
||||
## Key Files
|
||||
- `src/controller/` — Controller source files
|
||||
- `include/` — Controller-specific headers
|
||||
- `src/tools/deploy-ha.sh` — HA cluster deploy script
|
||||
|
||||
## See Also
|
||||
- Shared library: `src/lib/CLAUDE.md`
|
||||
|
|
|
|||
|
|
@ -694,6 +694,17 @@ int handle_repl_config_push(cluster_t *c, endpoint_t *ep, xce_request_t *req)
|
|||
return -EINVAL;
|
||||
}
|
||||
|
||||
/* Req 1.3 (ha-heartbeat-liveness): any msg from primary resets standby
|
||||
* liveness and steps down from CTRL_ELECTING (live primary discovered). */
|
||||
{
|
||||
ctrl_role_t role = (ctrl_role_t)atomic_load_explicit(
|
||||
&c->ha.role, memory_order_acquire);
|
||||
if (role == CTRL_STANDBY)
|
||||
ha_on_heartbeat_received(&c->ha);
|
||||
else if (role == CTRL_ELECTING)
|
||||
ha_discover_primary(&c->ha, c);
|
||||
}
|
||||
|
||||
repl_config_push_req_t *rcpr = req->body.replconfigpushrequestmsg;
|
||||
|
||||
if (!rcpr->raw_bytes || rcpr->raw_bytes_len == 0 || !rcpr->raw_sha256) {
|
||||
|
|
@ -1404,6 +1415,9 @@ int (*xce_wal_stream_request_hook)(endpoint_t *ep, xce_request_t *req) = NULL;
|
|||
#endif
|
||||
|
||||
/* send_wal_batch: enqueue a single OPCode_walCatchupEntry batch message.
|
||||
* Entry payloads (raw_payload/raw_payload_len) travel as capnp Data via
|
||||
* xce_wal_batch_req_raw (task 7.3) — the generated encode path cannot
|
||||
* carry raw bytes (xce_data_t limitation, same as replConfigPush).
|
||||
* Does not free any memory — caller manages entry lifetime.
|
||||
* Returns 0 on success. */
|
||||
static int send_wal_batch(endpoint_t *ep,
|
||||
|
|
@ -1412,24 +1426,24 @@ static int send_wal_batch(endpoint_t *ep,
|
|||
if (n_entries <= 0)
|
||||
return 0;
|
||||
|
||||
/* Build the batch container entry */
|
||||
wal_catchup_entry_t batch;
|
||||
memset(&batch, 0, sizeof(batch));
|
||||
batch.n_entries = n_entries;
|
||||
batch.entries = entries;
|
||||
|
||||
xce_request_t req;
|
||||
memset(&req, 0, sizeof(req));
|
||||
req.opcode = OPCode_walCatchupEntry;
|
||||
req.body_which = Request_body_nomsg;
|
||||
req.body.walcatchupentrymsg = &batch;
|
||||
|
||||
#ifdef XCE_TESTING
|
||||
if (xce_wal_stream_request_hook != NULL)
|
||||
if (xce_wal_stream_request_hook != NULL) {
|
||||
/* Build the batch container the hook expects */
|
||||
wal_catchup_entry_t batch;
|
||||
memset(&batch, 0, sizeof(batch));
|
||||
batch.n_entries = n_entries;
|
||||
batch.entries = entries;
|
||||
|
||||
xce_request_t req;
|
||||
memset(&req, 0, sizeof(req));
|
||||
req.opcode = OPCode_walCatchupEntry;
|
||||
req.body_which = Request_body_walcatchupentrymsg;
|
||||
req.body.walcatchupentrymsg = &batch;
|
||||
return xce_wal_stream_request_hook(ep, &req);
|
||||
}
|
||||
#endif
|
||||
if (ep)
|
||||
xce_request(ep, &req);
|
||||
xce_wal_batch_req_raw(ep, entries, n_entries);
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
@ -1505,7 +1519,7 @@ int ha_stream_wal_to_standby(endpoint_t *ep, const char *wal_path,
|
|||
if (batch_n > 0) {
|
||||
send_wal_batch(ep, batch_entries, batch_n);
|
||||
for (int bi = 0; bi < batch_n; bi++) {
|
||||
free(batch_entries[bi]->payload);
|
||||
free(batch_entries[bi]->raw_payload);
|
||||
free(batch_entries[bi]);
|
||||
}
|
||||
batch_n = 0;
|
||||
|
|
@ -1525,10 +1539,12 @@ int ha_stream_wal_to_standby(endpoint_t *ep, const char *wal_path,
|
|||
if (pbuf) {
|
||||
ssize_t pr = pread(fd, pbuf, hdr.payload_len,
|
||||
(off_t)(off + sizeof(hdr)));
|
||||
if (pr == (ssize_t)hdr.payload_len)
|
||||
entry->payload = (xce_data_t *)pbuf;
|
||||
else
|
||||
if (pr == (ssize_t)hdr.payload_len) {
|
||||
entry->raw_payload = pbuf;
|
||||
entry->raw_payload_len = hdr.payload_len;
|
||||
} else {
|
||||
free(pbuf);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1541,7 +1557,7 @@ int ha_stream_wal_to_standby(endpoint_t *ep, const char *wal_path,
|
|||
batch_bytes >= WAL_STREAM_BATCH_BYTES) {
|
||||
send_wal_batch(ep, batch_entries, batch_n);
|
||||
for (int bi = 0; bi < batch_n; bi++) {
|
||||
free(batch_entries[bi]->payload);
|
||||
free(batch_entries[bi]->raw_payload);
|
||||
free(batch_entries[bi]);
|
||||
}
|
||||
batch_n = 0;
|
||||
|
|
@ -1557,7 +1573,7 @@ int ha_stream_wal_to_standby(endpoint_t *ep, const char *wal_path,
|
|||
if (batch_n > 0) {
|
||||
send_wal_batch(ep, batch_entries, batch_n);
|
||||
for (int bi = 0; bi < batch_n; bi++) {
|
||||
free(batch_entries[bi]->payload);
|
||||
free(batch_entries[bi]->raw_payload);
|
||||
free(batch_entries[bi]);
|
||||
}
|
||||
}
|
||||
|
|
@ -1610,17 +1626,14 @@ int ha_send_snapshot_to_standby(endpoint_t *ep, const char *snap_dir,
|
|||
uint64_t offset = 0;
|
||||
uint32_t chunk_index = 0;
|
||||
|
||||
/* Chunk the snapshot file and enqueue each chunk via xce_request().
|
||||
* Payload encoding uses NULL for now — the user-defined Data type in capnp
|
||||
* (xce_data_t tagged union) requires a proper table dispatch for non-null
|
||||
* payloads. This is a known limitation shared with wal_catchup_entry_t
|
||||
* payload; both will be resolved when the capnp schema is updated to use
|
||||
* the built-in capnp Data (List(UInt8)) type. */
|
||||
/* Chunk the snapshot file and enqueue each chunk with its file bytes.
|
||||
* Payload travels as capnp Data via xce_snapshot_chunk_req_raw (task 7.2)
|
||||
* — the generated encode path cannot carry raw bytes (xce_data_t
|
||||
* limitation, same as replConfigPush). */
|
||||
while (offset < total_size) {
|
||||
size_t chunk_len = (total_size - offset > SNAPSHOT_CHUNK_SIZE)
|
||||
? SNAPSHOT_CHUNK_SIZE
|
||||
: (size_t)(total_size - offset);
|
||||
/* Consume the file bytes to advance offset — data not used yet */
|
||||
if (fread(buf, 1, chunk_len, f) != chunk_len) {
|
||||
rc = -EIO;
|
||||
break;
|
||||
|
|
@ -1630,26 +1643,26 @@ int ha_send_snapshot_to_standby(endpoint_t *ep, const char *snap_dir,
|
|||
|
||||
snapshot_transfer_t chunk;
|
||||
memset(&chunk, 0, sizeof(chunk));
|
||||
chunk.snapshot_lsn = checkpoint_lsn;
|
||||
chunk.chunk_index = chunk_index;
|
||||
chunk.chunk_offset = offset;
|
||||
chunk.total_size = total_size;
|
||||
chunk.is_last = is_last;
|
||||
chunk.payload = NULL; /* encoding deferred */
|
||||
|
||||
xce_request_t req;
|
||||
memset(&req, 0, sizeof(req));
|
||||
req.opcode = OPCode_snapshotTransfer;
|
||||
req.body_which = Request_body_nomsg;
|
||||
req.body.snapshottransfermsg = &chunk;
|
||||
chunk.snapshot_lsn = checkpoint_lsn;
|
||||
chunk.chunk_index = chunk_index;
|
||||
chunk.chunk_offset = offset;
|
||||
chunk.total_size = total_size;
|
||||
chunk.is_last = is_last;
|
||||
chunk.raw_payload = buf; /* borrowed for hook/encode only */
|
||||
chunk.raw_payload_len = chunk_len;
|
||||
|
||||
#ifdef XCE_TESTING
|
||||
if (xce_snapshot_chunk_hook)
|
||||
if (xce_snapshot_chunk_hook) {
|
||||
xce_request_t req;
|
||||
memset(&req, 0, sizeof(req));
|
||||
req.opcode = OPCode_snapshotTransfer;
|
||||
req.body_which = Request_body_snapshottransfermsg;
|
||||
req.body.snapshottransfermsg = &chunk;
|
||||
xce_snapshot_chunk_hook(ep, &req);
|
||||
else
|
||||
} else
|
||||
#endif
|
||||
if (ep)
|
||||
xce_request(ep, &req);
|
||||
xce_snapshot_chunk_req_raw(ep, &chunk, buf, chunk_len);
|
||||
|
||||
offset += chunk_len;
|
||||
chunk_index++;
|
||||
|
|
|
|||
|
|
@ -540,7 +540,10 @@ static void raft_cb_become_leader(void *ctx, uint64_t term)
|
|||
ectx->votes[i].peer->hostname,
|
||||
(uint32_t)ectx->votes[i].peer->port);
|
||||
if (rc >= 0)
|
||||
TRACE("HA: repl registered standby ep[%d] (slot %d)", i, rc);
|
||||
INFO("HA: repl registered standby ep[%d] (slot %d, %s:%u)",
|
||||
i, rc,
|
||||
ectx->votes[i].peer->hostname,
|
||||
ectx->votes[i].peer->port);
|
||||
else
|
||||
WARN("HA: repl_add_standby ep[%d] failed: %d", i, rc);
|
||||
}
|
||||
|
|
@ -1489,6 +1492,38 @@ int ha_dispatch_repl_action_for_standby(ha_state_t *ha,
|
|||
* For CTRL_INIT / CTRL_ELECTING the allowlist is tighter: only
|
||||
* OP_requestVote and OP_probeConfigTag pass through.
|
||||
*/
|
||||
/* ha_has_write_quorum — check whether the cluster has enough connected
|
||||
* standbys to achieve majority for state-changing operations.
|
||||
*
|
||||
* Quorum = ceil(n_peers / 2) + 1 (self-vote included).
|
||||
* Example: n_peers=3 → quorum=2 → need ≥1 connected standby.
|
||||
*
|
||||
* Called from ha_dispatch_gate for CTRL_PRIMARY — if the cluster loses
|
||||
* majority (standby failures), write ops are rejected with -XCE_ERR_NOT_READY
|
||||
* to preserve the majority-consistency invariant.
|
||||
*
|
||||
* Read-only queries (joblist, reslist, hostlist, etc.) remain available
|
||||
* regardless of majority status — they don't mutate state. */
|
||||
static bool ha_has_write_quorum(ha_state_t *ha)
|
||||
{
|
||||
if (!ha)
|
||||
return false;
|
||||
|
||||
/* Single-node cluster: no standbys needed — self is the majority. */
|
||||
if (ha->n_peers <= 1)
|
||||
return true;
|
||||
|
||||
/* Count connected standbys. standby_endpoints is populated by
|
||||
* raft_cb_become_leader (promoted vote eps) and on_standby_connected
|
||||
* (late joiners); removed by error handlers. */
|
||||
int connected = ha->standby_endpoints
|
||||
? ha->standby_endpoints->count : 0;
|
||||
|
||||
/* Need at least n_peers/2 connected standbys to achieve quorum.
|
||||
* Integer division: n_peers/2 for n_peers=3 → 1 standby minimum. */
|
||||
return connected >= (int)(ha->n_peers / 2);
|
||||
}
|
||||
|
||||
int ha_dispatch_gate(ha_state_t *ha, int opcode)
|
||||
{
|
||||
ctrl_role_t role =
|
||||
|
|
@ -1496,6 +1531,31 @@ int ha_dispatch_gate(ha_state_t *ha, int opcode)
|
|||
|
||||
switch (role) {
|
||||
case CTRL_PRIMARY:
|
||||
/* State-changing operations require majority so committed entries
|
||||
* are durable across failover. Read-only queries and replication
|
||||
* ops are always allowed. */
|
||||
if (!ha_has_write_quorum(ha)) {
|
||||
switch (opcode) {
|
||||
/* Replication ops — always allowed (they build majority) */
|
||||
case OPCode_heartbeat:
|
||||
case OPCode_replConfig:
|
||||
case OPCode_replConfigPush:
|
||||
case OPCode_upgradeConfigAck:
|
||||
case OPCode_hello:
|
||||
case OPCode_walAck:
|
||||
case OPCode_replEntry:
|
||||
case OPCode_replAck:
|
||||
case OPCode_replCatchUpDone:
|
||||
case OPCode_walCatchupEntry:
|
||||
case OPCode_snapshotTransfer:
|
||||
/* Read-only queries */
|
||||
case OPCode_get:
|
||||
case OPCode_probeConfigTag:
|
||||
return 0;
|
||||
default:
|
||||
return -XCE_ERR_NOT_READY;
|
||||
}
|
||||
}
|
||||
return 0;
|
||||
|
||||
case CTRL_INIT:
|
||||
|
|
@ -2623,12 +2683,18 @@ static int repl_ack_decode_and_dispatch(endpoint_t *ep, vote_ep_ctx_t *vctx)
|
|||
int peer_idx = vctx->peer_idx;
|
||||
uint64_t committed_lsn = 0;
|
||||
uint64_t committed_tag = 0;
|
||||
uint64_t last_written = 0; /* latest WAL LSN — may be > committed */
|
||||
#ifndef XCE_TESTING
|
||||
if (c) {
|
||||
committed_lsn = atomic_load_explicit(&c->persist.committed_lsn,
|
||||
memory_order_acquire);
|
||||
committed_tag = atomic_load_explicit(&ha->committed_config_tag,
|
||||
memory_order_acquire);
|
||||
/* current_lsn is the next LSN to write; last written = current - 1.
|
||||
* Read without lock — monotonic, approximate value is sufficient
|
||||
* for the replication decision. */
|
||||
last_written = (c->persist.current_lsn > 0)
|
||||
? c->persist.current_lsn - 1 : 0;
|
||||
}
|
||||
#else
|
||||
/* In test mode, c may be NULL; caller sets ha fields directly */
|
||||
|
|
@ -2658,8 +2724,15 @@ static int repl_ack_decode_and_dispatch(endpoint_t *ep, vote_ep_ctx_t *vctx)
|
|||
oldest_wal_lsn = ha_compute_oldest_wal_lsn(c->persist.wal_path);
|
||||
}
|
||||
#endif
|
||||
/* Replication target: use max(committed_lsn, last_written) so
|
||||
* standbys can catch up even when committed_lsn is stuck at 0
|
||||
* (no replicator-registered standbys to advance it). */
|
||||
uint64_t target_lsn = committed_lsn;
|
||||
if (last_written > target_lsn)
|
||||
target_lsn = last_written;
|
||||
|
||||
ha_dispatch_repl_action_for_standby(ha, c, ep, peer_idx,
|
||||
committed_tag, committed_lsn,
|
||||
committed_tag, target_lsn,
|
||||
oldest_wal_lsn);
|
||||
} else if (rep_s.data_which == RequestReply_data_walackreplymsg) {
|
||||
/* Task 4.1: WAL ACK reply from standby — extract lsn + success,
|
||||
|
|
@ -3750,8 +3823,8 @@ static void on_standby_connected(endpoint_t *ep)
|
|||
int rc = repl_add_standby(&c->persist.repl, ep,
|
||||
peer->hostname, (uint32_t)peer->port);
|
||||
if (rc >= 0)
|
||||
TRACE("HA: repl registered late-joiner standby peer[%d] (slot %d)",
|
||||
sctx->peer_idx, rc);
|
||||
INFO("HA: repl registered late-joiner standby peer[%d] (slot %d, %s:%u)",
|
||||
sctx->peer_idx, rc, peer->hostname, (uint32_t)peer->port);
|
||||
else
|
||||
WARN("HA: repl_add_standby late-joiner peer[%d] failed: %d",
|
||||
sctx->peer_idx, rc);
|
||||
|
|
|
|||
|
|
@ -525,16 +525,12 @@ int handle_jobsubmit(cluster_t* c, endpoint_t* ep, xce_request_t* request)
|
|||
return -errno;
|
||||
}
|
||||
|
||||
reply.status = 0;
|
||||
reply.data_which = RequestReply_data_jobid;
|
||||
reply.data.jobid = jobid;
|
||||
|
||||
xce_reply(ep, c->myhost, &reply);
|
||||
|
||||
/* Build job list for the AFTER_EVENT hook BEFORE replying to client.
|
||||
* The AFTER_EVENT hook writes WAL and waits for majority replication —
|
||||
* the client must not be acknowledged until the job is durable. */
|
||||
xlist_init(&jlist, submit->array_size);
|
||||
|
||||
for (i = 0; i < submit->array_size; i++) {
|
||||
/* Run hooks to tell scheduler */
|
||||
lock(c->jobmgr, RTTI_LOCK_READ);
|
||||
rc = key_find(c->jobmgr, (void*)(jobid + i), (void**)&j);
|
||||
if (j != NULL) {
|
||||
|
|
@ -554,13 +550,33 @@ int handle_jobsubmit(cluster_t* c, endpoint_t* ep, xce_request_t* request)
|
|||
}
|
||||
}
|
||||
|
||||
xce_run_hooks(&(c->hooksys), XCE_CONTROLLER_HOOK_JOB, HOOK_AFTER_EVENT,
|
||||
HOOK_DETAIL_JOB_NEW, c, &jlist);
|
||||
int hook_rc = xce_run_hooks(&(c->hooksys), XCE_CONTROLLER_HOOK_JOB,
|
||||
HOOK_AFTER_EVENT,
|
||||
HOOK_DETAIL_JOB_NEW, c, &jlist);
|
||||
|
||||
xlist_iter(&jlist, (xlist_iter_fn_t)xlist_deref_job_item_, NULL);
|
||||
|
||||
xlist_fini(&jlist);
|
||||
|
||||
/* If persistence failed (replication timeout, I/O error), reject the
|
||||
* submission so the client knows the job was not made durable. */
|
||||
if (hook_rc != 0) {
|
||||
WARN("handle_jobsubmit: persistence hook failed (rc=%d) — rejecting job",
|
||||
hook_rc);
|
||||
reply.status = hook_rc;
|
||||
reply.data_which = RequestReply_data_nodata;
|
||||
|
||||
xce_reply(ep, c->myhost, &reply);
|
||||
return hook_rc;
|
||||
}
|
||||
|
||||
/* Reply only AFTER WAL write + majority replication succeeded. */
|
||||
reply.status = 0;
|
||||
reply.data_which = RequestReply_data_jobid;
|
||||
reply.data.jobid = jobid;
|
||||
|
||||
xce_reply(ep, c->myhost, &reply);
|
||||
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -1038,7 +1038,14 @@ int persist_wal_sync_append(persist_t* p, uint32_t type,
|
|||
}
|
||||
if (sync_rc_out)
|
||||
*sync_rc_out = sync_err;
|
||||
/* sync_err < 0 = degraded mode (local-only durability); continue operating. */
|
||||
/* sync_err < 0 — replication failed.
|
||||
* -ENODEV: no standbys registered (single-node or degraded).
|
||||
* -ETIMEDOUT: standbys were registered but didn't ACK in time.
|
||||
* Only -ENODEV is acceptable for forward progress; timeout means
|
||||
* the cluster had standbys but couldn't reach quorum — the write
|
||||
* must be rejected to preserve the majority-consistency invariant. */
|
||||
if (sync_err == -ETIMEDOUT)
|
||||
return -ETIMEDOUT;
|
||||
return 0;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -219,6 +219,14 @@ int repl_broadcast(replicator_t *r, const wal_entry_hdr_t *hdr,
|
|||
|
||||
mylock_lock(&r->lock);
|
||||
|
||||
if (r->nstandbys == 0) {
|
||||
WARN("repl_broadcast: no standbys registered — WAL entry lsn=%lu type=%u"
|
||||
" not replicated",
|
||||
(unsigned long)hdr->lsn, hdr->type);
|
||||
mylock_unlock(&r->lock);
|
||||
return 0;
|
||||
}
|
||||
|
||||
for (int i = 0; i < MAX_STANDBYS; i++) {
|
||||
standby_conn_t *s = &r->standbys[i];
|
||||
|
||||
|
|
|
|||
|
|
@ -357,6 +357,8 @@ static void test_become_follower_closes_vote_endpoints(void)
|
|||
ha_phase2_hook = NULL;
|
||||
atomic_store_explicit(&ha.elect_ctx, NULL, memory_order_release);
|
||||
free_elect_ctx(ctx);
|
||||
free(ep0);
|
||||
free(ep1);
|
||||
ha_fini(&ha);
|
||||
printf("test_become_follower_closes_vote_endpoints: PASS\n");
|
||||
}
|
||||
|
|
@ -494,6 +496,8 @@ static void test_follower_passive_no_outbound_connect(void)
|
|||
ha_phase2_hook = NULL;
|
||||
atomic_store_explicit(&ha.elect_ctx, NULL, memory_order_release);
|
||||
free_elect_ctx(ctx);
|
||||
free(ep0);
|
||||
free(ep1);
|
||||
ha_fini(&ha);
|
||||
printf("test_follower_passive_no_outbound_connect: PASS\n");
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,108 +1,7 @@
|
|||
# XCE Controller: Future Work Items
|
||||
|
||||
> **Status**: Tracked from config-replication-design.md review
|
||||
> **Last Updated**: 2026-05-12
|
||||
|
||||
---
|
||||
|
||||
## 1. Admin Config Tool (prerequisite for Phase 2)
|
||||
|
||||
`OP_upgradeConfig` requires an admin tool to drive it. The tool must:
|
||||
|
||||
- Encode a `ClusterConfig` YAML/JSON file to Cap'n Proto binary
|
||||
- Compute SHA-256 over the binary bytes
|
||||
- Connect to the primary controller's endpoint (SSL, same CA as controllers)
|
||||
- Send `OP_upgradeConfig` with the binary blob and SHA-256
|
||||
- Display the reply: new version tag, acked count, and any error message
|
||||
- Poll `OP_probe` after restart to confirm the primary is back online with the new config
|
||||
|
||||
Suggested interface: `xce-config push --controller <host:port> --config cluster.yaml`
|
||||
|
||||
---
|
||||
|
||||
## 2. Quorum-ACK Gate Before Restart (§14.4 extension)
|
||||
|
||||
Current `OP_upgradeConfig` restarts the primary after best-effort push to standbys.
|
||||
A quorum-ACK gate would refuse to restart unless a minimum number of standbys
|
||||
acknowledge the push, preventing silent rollback when all standbys are offline.
|
||||
|
||||
Requires: `OP_upgradeConfig` reply to include `--require-standby-ack=N` flag.
|
||||
|
||||
---
|
||||
|
||||
## 3. OP_heartbeat Peers Cross-Validation (§11.2)
|
||||
|
||||
Include a hash of the local `--peers` string in the heartbeat request. If two
|
||||
candidates disagree on the peers list, log ERROR. Detects misconfigured nodes
|
||||
at election time rather than silently producing dual-primary split-brain.
|
||||
|
||||
---
|
||||
|
||||
## 4. Lost and Found Drain Command (§6.5 extension)
|
||||
|
||||
`xcecli lost-and-found drain` — reassign or cancel sidelined jobs, reset
|
||||
`lost_and_found_job_count` to 0. Automates operator cleanup after config upgrade.
|
||||
|
||||
---
|
||||
|
||||
## 5. OP_upgradeConfig Standby Redirect (§10.12 verification)
|
||||
|
||||
Test that a standby correctly returns an error reply with the primary's address
|
||||
when an admin tool sends `OP_upgradeConfig` to a standby, and the admin tool
|
||||
retries against the primary.
|
||||
|
||||
---
|
||||
|
||||
## 6. validate_host() Implementation Plan
|
||||
|
||||
`validate_host()` is currently a stub that returns success for all peers. Before
|
||||
`CONF_ENABLE_UPGRADE_CONFIG` can be set to 1, it MUST be enhanced with a real
|
||||
authorisation check. Minimum viable implementation:
|
||||
|
||||
- Verify the peer's X.509 certificate Common Name (CN) against a configured
|
||||
allowlist of authorised admin hosts.
|
||||
- Add `--admin-cn` CLI flag or `admin_cn_list` config field to specify allowed CNs.
|
||||
- Reject `OP_upgradeConfig` requests from peers whose CN is not in the allowlist
|
||||
with a clear error message.
|
||||
- Log all rejected attempts at WARN level with peer CN and IP.
|
||||
|
||||
Future extensions (not required for gate removal): IP allowlist, role-based
|
||||
access, mutual-TLS with admin-specific CA.
|
||||
|
||||
---
|
||||
|
||||
## 7. OP_replConfig Access Control Hardening
|
||||
|
||||
`OP_replConfig` currently serves the full cluster config blob to any peer that
|
||||
completes an SSL handshake with a CA-signed cert (trusted-LAN model, §5.3).
|
||||
Enhancements to consider:
|
||||
|
||||
- Verify the requesting peer is a known controller (listed in `--peers`) before
|
||||
serving the config blob.
|
||||
- Rate-limit `OP_replConfig` requests per peer to prevent repeated disk I/O on
|
||||
the primary.
|
||||
- Add a `minimum_role` check (e.g., only serve to `HostRole_controller` peers,
|
||||
not arbitrary SSL clients).
|
||||
|
||||
---
|
||||
|
||||
## 8. Lost-and-Found Internal Keyword Validation
|
||||
|
||||
The name `"lost_and_found"` is an internal controller keyword for synthetic
|
||||
queue and host entries created during WAL recovery (§6.5). The controller
|
||||
rejects this name at both `load_primary_config()` and `OP_upgradeConfig` entry
|
||||
points.
|
||||
|
||||
**Config tool responsibility**: The admin config tool (Future Work §1) MUST
|
||||
validate that user-authored config files do not use the reserved names
|
||||
`"lost_and_found"` for any queue or host. The tool should emit a clear error
|
||||
and refuse to encode the config blob. This provides defence-in-depth: the
|
||||
controller enforces at runtime, and the tool prevents at authoring time.
|
||||
|
||||
**Migration path for pre-existing clusters**: If a cluster already has a
|
||||
user-defined queue or host named `"lost_and_found"` before upgrading to this
|
||||
controller version, the operator must rename it in the cluster config YAML
|
||||
before the upgrade. The admin config tool will validate this; if bypassing
|
||||
the tool and hand-crafting a Cap'n Proto blob, the controller's
|
||||
`load_primary_config()` will FATAL with a message identifying the reserved
|
||||
name. See `config-replication-design.md` §6.5 and §7.1.
|
||||
> **Moved (2026-06-10)**: All items merged into the single consolidated future-work document at
|
||||
> [`.kiro/steering/roadmap.md`](../../.kiro/steering/roadmap.md).
|
||||
>
|
||||
> Section mapping: §1+§8 → admin-config-tool (backlog); §2 → superseded by ha-consistency-invariants Req 5;
|
||||
> §3, §4, §5, §6, §7 → Hardening / Direct-Implementation Items.
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load diff
|
|
@ -1,225 +0,0 @@
|
|||
# API Server Backend Gap Analysis
|
||||
|
||||
Source: `src/controller/testsuite/test.c` (C test tool) vs `src/frontend/apiserver/` (Go API server)
|
||||
|
||||
## Overview
|
||||
|
||||
The C test tool (`test.c`) directly calls `xcecli_*` C library functions to interact
|
||||
with the controller. The Go API server currently operates in a **push-based, read-only-from-store**
|
||||
model: the controller pushes data (jobs, workers, queues, resources) via Cap'n Proto, and
|
||||
the API server reads from its in-memory stores. Write/control operations are forwarded
|
||||
via the CGO backend layer, but only a subset of the C API is wired up.
|
||||
|
||||
---
|
||||
|
||||
## Currently Implemented (Backend Forwards to C Controller)
|
||||
|
||||
| C Test Command | C API Call | Go Backend Function | REST Endpoint |
|
||||
|----------------|------------------------|-------------------------|------------------------------------|
|
||||
| `jobsub` | `xcecli_jobsubmit()` | `backend.SubmitJob()` | `POST /xce/v1/jobs/create` |
|
||||
| `jobkill/jobckpt` | `xcecli_jobctrl()` | `backend.ControlJob()` | `POST /xce/v1/jobs/:jobids/control` |
|
||||
| `qctrl` | `xcecli_queuectrl()` | `backend.ControlQueue()`| `POST /xce/v1/queues/control` |
|
||||
| *(partial)* | `xcecli_hostctrl()` (N/A) | `backend.ControlHost()` **stub only** | `POST /xce/v1/hosts/control` |
|
||||
|
||||
Push notifications (read path, already working via `handlePush`):
|
||||
|
||||
- Job updates via `OnJobUpdated` -> `store.Jobs`
|
||||
- Worker/host updates via `OnHostUpdated` -> `store.Hosts`
|
||||
- Queue updates via `OnQueueUpdated` -> `store.Queues`
|
||||
- Resource definitions via `OnResourceUpdated` -> `store.Resources`
|
||||
- Resource locations via `OnResourceLocationUpdated` -> `store.ResourceLocations`
|
||||
|
||||
---
|
||||
|
||||
## NOT Implemented -- Backend Request Gaps
|
||||
|
||||
### 1. Resource Value Query (`cmd_resval` -> `xcecli_resval()`)
|
||||
|
||||
- **What it does**: Queries current numeric/string values of a named resource on a
|
||||
specific host (or cluster-wide). Returns typed values (float, string, bool).
|
||||
- **REST impact**: No `GET /xce/v1/resources/values` endpoint exists. The resource
|
||||
controller only serves definitions and distributions, not live values.
|
||||
- **Priority**: **High** -- Essential for monitoring dashboards to show real-time
|
||||
`mem`, `r1m`, `slots` etc.
|
||||
|
||||
[feedback] api server will not pull resources from controller, it just waits controller to push
|
||||
updated resource information, so api server just handle resource information update will be OK.
|
||||
|
||||
### 2. Resource Value Update (`cmd_resupdate` -> `xcecli_update_resvals()`)
|
||||
|
||||
- **What it does**: Worker pushes updated resource values to the controller.
|
||||
- **REST impact**: No equivalent. Currently workers call the C API directly. If the
|
||||
API server should support an admin endpoint to update resource values (e.g., for
|
||||
testing or override), this is missing.
|
||||
- **Priority**: **Low-Medium** -- Workers typically call this directly; rarely needed
|
||||
via REST. Could be useful for admin overrides.
|
||||
|
||||
[feedback] This item will not be applied to api server, resource update function need to be
|
||||
used to update resources information of worker node(such as cpu/memory/..., and api
|
||||
server will not talke workload, so it do not need this)
|
||||
|
||||
### 3. Job Dispatch (`cmd_jobrun` -> `xcecli_jobdispatch()`)
|
||||
|
||||
- **What it does**: Dispatches a job to specific hosts with allocation info
|
||||
(host:ntasks pairs). This is how the scheduler tells the controller to start
|
||||
running a job on specific workers.
|
||||
- **REST impact**: Not exposed. The `POST /xce/v1/jobs/create` handles submission,
|
||||
but **dispatch** (assignment to hosts) is a separate operation currently done by
|
||||
the scheduler directly.
|
||||
- **Priority**: **Low** -- Dispatch is an internal scheduler->controller operation.
|
||||
Not typically user-facing.
|
||||
|
||||
[feedback] treat this as low priority, IBM LSF has a brun command which issued by user to
|
||||
force a job to run in given host, we may need this. also cmd_jobrun() has
|
||||
been used to test execution flow/protocol between controller and work daemon
|
||||
|
||||
### 4. Job Info / Single Job Detail (`cmd_jobinfo` -> `xcecli_jobinfo()`)
|
||||
|
||||
- **What it does**: Queries detailed info for a single job by ID, including pend
|
||||
reasons.
|
||||
- **REST impact**: The `GET /xce/v1/jobs/:jobids` endpoint reads from the in-memory
|
||||
store, which is populated by push notifications. This works for already-known jobs
|
||||
but **cannot query a job that hasn't been pushed yet**. The C `xcecli_jobinfo()` is
|
||||
a synchronous request-response to the controller -- fundamentally different from
|
||||
the store-read pattern.
|
||||
- **Priority**: **Medium** -- Store-based reads are sufficient for most cases. The
|
||||
sync query path would only matter if pushes are delayed or the store is stale.
|
||||
|
||||
[feedback] no need this, as job information will be pushed to api server
|
||||
|
||||
### 5. Cluster Configuration (`cmd_getconf` -> `xcecli_clusterconf()`)
|
||||
|
||||
- **What it does**: Synchronously fetches the full cluster config (resources, queues,
|
||||
workers, host groups, user groups, global configs).
|
||||
- **REST impact**: Currently the API server relies entirely on push notifications to
|
||||
populate stores. There's no way to do a one-shot **full sync** with the controller.
|
||||
On startup, it joins via `xcecli_join(HostRole_apiserver)` but doesn't explicitly
|
||||
request the current state.
|
||||
- **Priority**: **High** -- Without an initial sync, if the API server restarts
|
||||
mid-session, it loses all state until the next push. The store is stale until a
|
||||
new push arrives.
|
||||
|
||||
[feedback] api server need handle pushed cluster configuration
|
||||
|
||||
### 6. Host Control (`backend.ControlHost()` -> `xcecli_hostctrl()`)
|
||||
|
||||
- **What it does**: The C library doesn't expose `xcecli_hostctrl()` yet (the Go
|
||||
stub returns an error).
|
||||
- **REST impact**: `POST /xce/v1/hosts/control` exists but will always fail:
|
||||
`"host control not yet supported via C library"`.
|
||||
- **Priority**: **High** -- Users cannot open/close hosts via the API.
|
||||
|
||||
[feedback] we need this
|
||||
|
||||
### 7. Set Pending Reason (`cmd_setpend` -> `xcecli_set_pend_reason()`)
|
||||
|
||||
- **What it does**: Scheduler sets pend reasons on a job (host-based or
|
||||
resource-based).
|
||||
- **REST impact**: No equivalent endpoint. Job pend reasons are currently populated
|
||||
only via push data.
|
||||
- **Priority**: **Low** -- This is a scheduler->controller internal operation.
|
||||
|
||||
[feedback] set pending reason is for scheduler to set why job will be pend, this is
|
||||
used for test scheduler to controller job pending api
|
||||
|
||||
### 8. Run Tasks (`cmd_runtasks` -> `xcecli_runtask()`)
|
||||
|
||||
- **What it does**: Launches individual tasks on specific hosts (not a full job, but
|
||||
sub-task execution).
|
||||
- **REST impact**: No endpoint exists.
|
||||
- **Priority**: **Low-Medium** -- Used for advanced task launch scenarios. Could be
|
||||
needed for MPI job management.
|
||||
|
||||
[feedback] this function is used by worker to run non rank 0 tasks(this is for MPI jobs) and api server
|
||||
need not handle this
|
||||
|
||||
### 9. Shutdown (`cmd_shutdown` -> `xcecli_shutdown()`)
|
||||
|
||||
- **What it does**: Sends shutdown command to the cluster controller.
|
||||
- **REST impact**: No endpoint. Currently cluster management operations (shutdown,
|
||||
restart) are not available via REST.
|
||||
- **Priority**: **Medium** -- Operations teams need remote cluster control capability.
|
||||
|
||||
[feedback] we may need to admin tool, which talk to controller directly for this, as normal
|
||||
user do not need this at all
|
||||
|
||||
### 10. Resource Requirement Select (`FilterHostByResReqSelect`)
|
||||
|
||||
- **What it does**: The hosts `POST /select` endpoint exists but returns **empty
|
||||
results** -- it's a stub: `processHostData([]utils.OrderMap{}, "")`.
|
||||
- **REST impact**: `POST /xce/v1/hosts/select` is defined but non-functional.
|
||||
- **Priority**: **Medium** -- This is the XCE equivalent of `lsload -R` / host
|
||||
matching by resource requirements. Important for job submission workflows to
|
||||
find eligible hosts.
|
||||
|
||||
[feedback] we may need this
|
||||
|
||||
---
|
||||
|
||||
## Enhancement Proposal (Prioritized)
|
||||
|
||||
| Priority | Enhancement | Effort | Description |
|
||||
|----------|---------------------------------------------|------------|-------------|
|
||||
| **P0** | Initial state sync via `xcecli_clusterconf()` | Medium | On startup (after `RegisterToBackend`), call `xcecli_clusterconf()` to do a full state sync into stores. Prevents stale stores after API server restart. |
|
||||
| **P0** | Implement `ControlHost()` in C library + Go wrapper | Medium | Expose `xcecli_hostctrl()` in `client.h`, then replace the Go stub in `clib.go` with a real CGO call. |
|
||||
| **P1** | Resource value query endpoint | Medium | Add `GET /xce/v1/resources/values?host=X&name=Y` -> `backend.QueryResourceValues()` -> `xcecli_resval()`. This is a synchronous request-response pattern (different from push). |
|
||||
| **P1** | Implement `FilterHostByResReqSelect` | Medium-High| Wire `POST /xce/v1/hosts/select` to evaluate resource requirement expressions against current host state from the store. |
|
||||
| **P2** | Cluster shutdown endpoint | Low | Add `POST /xce/v1/clusters/control` -> `backend.ShutdownCluster()` -> `xcecli_shutdown()`. Should be admin-only with RBAC. |
|
||||
| **P2** | Resource value update endpoint | Low | Add `POST /xce/v1/resources/values/update` -> `backend.UpdateResourceValues()` -> `xcecli_update_resvals()`. Admin/testing use. |
|
||||
| **P3** | Task run endpoint | Low-Medium | Add `POST /xce/v1/tasks/run` -> `backend.RunTask()` -> `xcecli_runtask()`. For advanced MPI task management. |
|
||||
| **P3** | Set pending reason endpoint | Low | Add `POST /xce/v1/jobs/:jobid/pending-reason` -> `backend.SetPendReason()` -> `xcecli_set_pend_reason()`. Internal/scheduler-facing only. |
|
||||
|
||||
---
|
||||
|
||||
## Architecture Note: Synchronous vs Push-Based
|
||||
|
||||
The biggest design decision lurking here: several of these C APIs (`xcecli_resval()`,
|
||||
`xcecli_clusterconf()`, `xcecli_jobinfo()`) are **synchronous request-response** calls
|
||||
-- the client sends a request and blocks until the controller replies. This is
|
||||
fundamentally different from the current **push-based** pattern where the controller
|
||||
asynchronously pushes updates.
|
||||
|
||||
To implement these, we'd need to either:
|
||||
|
||||
1. **Direct CGO call** (simpler but blocks the Go goroutine -- acceptable if calls
|
||||
are fast and we set timeouts)
|
||||
2. **Request-response via push channel** (more complex -- send request, register a
|
||||
wait, receive push reply)
|
||||
|
||||
Recommendation: option 1 for initial implementation (it's what `SubmitJob` and
|
||||
`ControlJob` already do), with timeouts and goroutine isolation to prevent a slow
|
||||
controller from blocking the API server.
|
||||
|
||||
---
|
||||
|
||||
## Additional C API Calls Not Used Anywhere (Reference)
|
||||
|
||||
These exist in `client.h` but are not called by `test.c` either:
|
||||
|
||||
| C API Call | Purpose |
|
||||
|--------------------------|---------|
|
||||
| `xcecli_hello()` | Health check / greeting |
|
||||
| `xcecli_nop()` | No-op heartbeat |
|
||||
| `xcecli_heartbeat()` | Periodic heartbeat |
|
||||
| `xcecli_notify_jobstart()`| Worker notifies job started (with PID info) |
|
||||
| `xcecli_notify_jobfail()` | Worker notifies job failed |
|
||||
| `xcecli_notify_jobstop()` | Worker notifies job stopped |
|
||||
| `xcecli_notify_jobresume()`| Worker notifies job resumed |
|
||||
| `xcecli_notify_jobpostdone()`| Worker notifies job post-exec done |
|
||||
| `xcecli_update_jusage()` | Worker updates job resource usage |
|
||||
| `xcecli_notify()` | Generic notification |
|
||||
| `xcecli_notify_taskstart()`| Worker notifies task started |
|
||||
| `xcecli_notify_taskfail()`| Worker notifies task failed |
|
||||
| `xcecli_notify_taskfinish()`| Worker notifies task finished |
|
||||
| `xcecli_clear_reason()` | Clear pend reasons for jobs |
|
||||
| `xcecli_block_tasks()` | Block task processing |
|
||||
| `xcecli_unblock_tasks()` | Unblock task processing |
|
||||
| `xcecli_get_my_endpoint()`| Get local endpoint info |
|
||||
| `xcecli_get_myinfo()` | Get local peer info |
|
||||
|
||||
[feedback] I think we just need heartbeat to tell controller that I'm alive, and other
|
||||
functions for worker and scheduler
|
||||
|
||||
These are primarily worker->controller notifications. They would be needed if
|
||||
the API server were to act as a proxy for worker daemons, which is not the
|
||||
current design.
|
||||
|
|
@ -1,459 +0,0 @@
|
|||
# Integration Test Plan — P0 + P1 Features
|
||||
|
||||
## Purpose
|
||||
|
||||
Verify end-to-end correctness of every feature delivered by tasks P0-1 through P1-3,
|
||||
exercising the full stack: handler → store → handler merge → store readback.
|
||||
These tests **do not** require a live C controller or CGO — they use the pure-Go stores
|
||||
and `DefaultHandler` directly, which is what unit tests already do. The integration layer
|
||||
adds: **multi-step sequences**, **cross-feature interactions**, and **edge-case coverage**
|
||||
that unit tests leave out.
|
||||
|
||||
---
|
||||
|
||||
## Test Environment
|
||||
|
||||
- All tests run with `CGO_ENABLED=0` (pure Go)
|
||||
- Each test calls `store.InitStores()` to reset all global state
|
||||
- `utils.Log = zap.NewNop()` to suppress log output
|
||||
- **§1–§5, §7, §8**: call handler/store methods directly — no HTTP server required
|
||||
- **§6 (RBAC)**: requires a test HTTP server via `httptest.NewRecorder` with a full Gin router; JWT tokens must be real HS256-signed tokens using the configured signing secret (typically `"xce-api-key"` in test config); `ctx.Set("user", "admin")` does **not** satisfy `GetUsernameFromToken()` which reads and parses the `Authorization` header
|
||||
- **JWT IP binding:** `isValidToken` checks `claims.IP == ctx.ClientIP()`; in `httptest`, `ClientIP()` reads from `req.RemoteAddr` — set `req.RemoteAddr = "127.0.0.1:0"` on every test request and embed `"ip": "127.0.0.1"` in the JWT claims; a mismatch returns 401 for the wrong reason, silently invalidating the test
|
||||
- **Auth config:** `isValidToken` dereferences `utils.AuthenticationYamlConfig`, a package-level pointer that is `nil` until `LoadConfig()` is called — before any JWT test set `utils.AuthenticationYamlConfig = &utils.Authentication{Token: utils.TokenConfig{SecretKey: "xce-api-key"}}` directly; do not call `LoadConfig()` (reads a file that won't exist in CI)
|
||||
|
||||
---
|
||||
|
||||
## 1. P0-1: Cluster Config Push
|
||||
|
||||
### 1.1 Config push populates all stores in a single call
|
||||
|
||||
**Steps:**
|
||||
1. Call `dispatchConfigPush(handler, queues, resources, hostConfigs, cluster)` with non-nil data for all four.
|
||||
2. Verify `store.Queues`, `store.Resources`, `store.Hosts`, `store.Clusters` all contain the pushed data.
|
||||
|
||||
**Asserts:** A single config push updates all four store types in sequence (queues → resources → hosts → cluster); the four updates are NOT atomic — a concurrent reader may see a partial state.
|
||||
|
||||
### 1.2 Second config push merges (does not replace)
|
||||
|
||||
**Steps:**
|
||||
1. Seed `store.Hosts` with worker-sourced data (HostStatus, CPUFactor, NumCPUs, etc.)
|
||||
2. Call `h.OnHostConfigUpdated()` with config-sourced fields (HostModel, HostType, MaxSlot)
|
||||
3. Call `h.OnHostConfigUpdated()` a second time with `HostModel: "modelB"` (distinct from step 2's "modelA")
|
||||
4. Verify worker-sourced fields from step 1 are still preserved
|
||||
5. Verify `HostModel == "modelB"` (config-sourced field reflects the second push, not the first)
|
||||
|
||||
**Asserts:** Partial merge is idempotent and correctly preserves the other source's fields across multiple pushes.
|
||||
|
||||
### 1.3 Cluster update with zero HeartbeatInterval preserves existing value
|
||||
|
||||
**Steps:**
|
||||
1. `store.Clusters.Put("c1", ClusterInfo{HeartbeatInterval: 45})`
|
||||
2. Call `h.OnClusterUpdated(&ClusterInfo{ClusterName: "c1", HeartbeatInterval: 0, Vcpus: 8})`
|
||||
3. Verify `HeartbeatInterval` is still 45 (zero is ignored per `if incoming.HeartbeatInterval > 0` guard)
|
||||
|
||||
**Asserts:** Cluster push with `HeartbeatInterval=0` does not zero out the stored interval.
|
||||
|
||||
### 1.4 Config push with nil cluster argument is a no-op
|
||||
|
||||
**Steps:**
|
||||
1. Call `dispatchConfigPush(handler, nil, nil, nil, nil)`
|
||||
2. Verify `store.Clusters.Count() == 0`
|
||||
|
||||
**Covers:** `dispatchConfigPush` skips the cluster update when the cluster argument is nil. (The empty-name guard lives in the CGO layer `cClusterConfToClusterInfo` and is not reachable in `CGO_ENABLED=0` tests; a `ClusterInfo{ClusterName:""}` passed directly would be stored under an empty key.)
|
||||
|
||||
### 1.5 Config push with nil queues/resources/hosts is safe
|
||||
|
||||
**Steps:**
|
||||
1. Call `dispatchConfigPush(handler, nil, nil, nil, nil)`
|
||||
2. Verify no panics, no store entries.
|
||||
|
||||
**Covers:** Nil-safety of the dispatch loop.
|
||||
|
||||
---
|
||||
|
||||
## 2. P0-2: Host Control via Backend
|
||||
|
||||
### 2.1 Valid control action (open/close) routes through to backend
|
||||
|
||||
**Steps:**
|
||||
1. `store.Hosts.Put("node01", HostInfo{HostName: "node01"})`
|
||||
2. Call `preCheckHostControlRequest(&HostControlRequest{ControlAction: "open", Hostname: []string{"node01"}})`
|
||||
3. Assert `allFlag == false`, `err == nil`
|
||||
|
||||
**Asserts:** Valid actions pass pre-check.
|
||||
|
||||
### 2.2 Invalid action is rejected
|
||||
|
||||
**Steps:**
|
||||
1. Call `preCheckHostControlRequest(&HostControlRequest{ControlAction: "reboot", Hostname: []string{"node01"}})`
|
||||
2. Assert error is returned.
|
||||
|
||||
**Asserts:** Only "open" and "close" are accepted.
|
||||
|
||||
### 2.3 Comment length limit (512 chars)
|
||||
|
||||
**Steps:**
|
||||
1. Call `preCheckHostControlRequest` with `Comment` of 513 chars
|
||||
2. Assert error
|
||||
3. Call with 512 chars — assert no error
|
||||
|
||||
**Already covered in unit tests.** Integration adds: verify the error message is human-readable.
|
||||
|
||||
### 2.4 Hostname validation checks store existence
|
||||
|
||||
**Steps:**
|
||||
1. `store.Hosts.Put("node01", HostInfo{HostName: "node01"})`
|
||||
2. Call `validateHost(ctx, action, []string{"node01", "nonexistent"})`
|
||||
3. Assert `node01` in success list, `nonexistent` in failed map
|
||||
|
||||
**Note:** `validateHost` calls `GetUsernameFromToken()` internally, requiring the same JWT setup as §6 — set `utils.AuthenticationYamlConfig`, set `req.RemoteAddr = "127.0.0.1:0"`, embed `"ip": "127.0.0.1"` in the JWT claims, and pass the token via `Authorization` header. `ctx.Set("user", "admin")` is **not** sufficient. See §6 Test Environment notes for the full setup checklist. Unlike §2.1–2.3, this test is grouped with the §6 JWT-required tests in `controllers/integration_test.go`.
|
||||
|
||||
### 2.5 RBAC: host:control requires cluster_admin role
|
||||
|
||||
**Steps:**
|
||||
1. Load RBAC config with `cluster_admin` having `host:control`
|
||||
2. Use `httptest.NewRecorder` + Gin router with a spy handler that sets a response body
|
||||
3. Call with a user that is NOT cluster_admin — assert 403 response
|
||||
4. Call with cluster_admin user — assert 2xx and spy handler body present
|
||||
|
||||
**Note:** `RequirePermission` short-circuits for `cluster_admin` via early `return` (without calling `ctx.Next()`). Asserting `ctx.IsAborted() == false` gives a **false pass** — the middleware doesn't abort, but the handler is also never called. Use an HTTP recorder to verify the handler response actually appeared.
|
||||
|
||||
**Covers:** Route middleware integration.
|
||||
|
||||
### 2.6 RBAC: host:view accessible to basic_user
|
||||
|
||||
**Steps:**
|
||||
1. Load RBAC config with `basic_user` having `host:view`
|
||||
2. Call `RequirePermission("host:view")` with basic_user
|
||||
3. Assert request proceeds
|
||||
|
||||
### 2.7 Concurrent control requests do not race
|
||||
|
||||
**Already covered:** `TestPreCheckHostControlRequest_NoRace` (50 goroutines).
|
||||
Integration adds: test that concurrent `ControlHost` calls to the same host don't corrupt store state.
|
||||
|
||||
### 2.8 Batch response construction
|
||||
|
||||
**Steps:**
|
||||
1. Build success and failed slices directly (do NOT call `ControlHost` — it calls the CGO stub which returns `errNoCGO` under `CGO_ENABLED=0`):
|
||||
- `succeededItems := []map[string]interface{}{{"node": "node01"}, {"node": "node02"}}`
|
||||
- `failedItems := []map[string]interface{}{{"node": "bad", "reason": "not found"}}`
|
||||
2. Call `BuildBatchOperationResponse("close", "host", succeededItems, failedItems)`
|
||||
3. Verify returned status is `"partially_accepted"`, success list has 2 entries, failed map has 1
|
||||
|
||||
**Covers:** Batch response construction logic; the CGO backend call is out of scope for pure-Go integration tests.
|
||||
|
||||
---
|
||||
|
||||
## 3. P1-1: Resource Value Push
|
||||
|
||||
### 3.1 All 7 value types convert correctly (pure-Go path)
|
||||
|
||||
**Steps:**
|
||||
1. For each value type (i32, i64, u32, u64, fval, sval, bval):
|
||||
- Create `rawResvalData` with that type
|
||||
- Call `rawResvalToDistribution(r)`
|
||||
- Assert `ValueType`, `NumericValue`/`StringValue`/`BooleanValue` match expected
|
||||
|
||||
**Already covered in unit tests.** Integration adds: round-trip through `OnResourceLocationUpdated`:
|
||||
2. Call `h.OnResourceLocationUpdated(rd)` for each type
|
||||
3. Read back from `store.ResourceLocations` using composite key `name:host`
|
||||
4. Assert stored values match
|
||||
|
||||
### 3.2 Nil val produces nil return (no store update)
|
||||
|
||||
**Steps:**
|
||||
1. Create `rawResvalData{valNil: true, ...}`
|
||||
2. Assert `rawResvalToDistribution` returns nil
|
||||
3. Assert `store.ResourceLocations.Count() == 0`
|
||||
|
||||
### 3.3 Unknown discriminant produces nil return with warn log
|
||||
|
||||
**Steps:**
|
||||
1. Create `rawResvalData{valWhich: 99, ...}`
|
||||
2. Assert `rawResvalToDistribution` returns nil
|
||||
3. Assert `store.ResourceLocations.Count() == 0`
|
||||
|
||||
### 3.4 Multiple resource values in a single push
|
||||
|
||||
**Steps:**
|
||||
1. Call `h.OnResourceLocationUpdated(rd1)` then `h.OnResourceLocationUpdated(rd2)` with different `name:host` keys
|
||||
2. Assert `store.ResourceLocations.Count() == 2`
|
||||
3. Assert each key returns the correct value
|
||||
|
||||
### 3.5 Resource value update overwrites previous value
|
||||
|
||||
**Steps:**
|
||||
1. `h.OnResourceLocationUpdated(rd)` with `NumericValue: 100.0`
|
||||
2. `h.OnResourceLocationUpdated(rd)` with same `name:host` but `NumericValue: 200.0`
|
||||
3. Assert stored value is 200.0 (last-write-wins)
|
||||
|
||||
**Covers:** Store `Put` is a full replacement, not merge.
|
||||
|
||||
### 3.6 Composite key format is `res_name:location`
|
||||
|
||||
**Steps:**
|
||||
1. Push a resource with `ResourceName: "mem"` and `Location: "node01"`
|
||||
2. Assert `store.ResourceLocations.Has("mem:node01") == true`
|
||||
3. Assert `store.ResourceLocations.Has("mem") == false`
|
||||
|
||||
---
|
||||
|
||||
## 4. P1-3: Heartbeat Keep-Alive
|
||||
|
||||
> **Isolation requirement:** `heartbeatIntervalSecs` is a package-level `var int32` in the `backend` package shared across all tests in that package. Tests in this section MUST NOT call `t.Parallel()`. Run the full heartbeat test group sequentially to avoid cross-test interference via `SetHeartbeatInterval`.
|
||||
|
||||
### 4.1 Heartbeat interval updates on cluster config push
|
||||
|
||||
**Steps:**
|
||||
1. `atomic.StoreInt32(&heartbeatIntervalSecs, 30)` (default)
|
||||
2. Call `h.OnClusterUpdated(&ClusterInfo{ClusterName: "c1", HeartbeatInterval: 45})`
|
||||
3. Assert `atomic.LoadInt32(&heartbeatIntervalSecs) == 45`
|
||||
|
||||
**Covers:** The P1-3 integration point — cluster config push triggers heartbeat interval change.
|
||||
|
||||
### 4.2 Cluster push with HeartbeatInterval=0 preserves current interval
|
||||
|
||||
**Steps:**
|
||||
1. `atomic.StoreInt32(&heartbeatIntervalSecs, 45)`
|
||||
2. Call `h.OnClusterUpdated(&ClusterInfo{ClusterName: "c1", HeartbeatInterval: 0})`
|
||||
3. Assert `atomic.LoadInt32(&heartbeatIntervalSecs) == 45` (unchanged)
|
||||
|
||||
**Covers:** The `> 0` guard in `OnClusterUpdated` and `SetHeartbeatInterval`.
|
||||
|
||||
---
|
||||
|
||||
## 5. Cross-Feature Integration
|
||||
|
||||
### 5.1 Config push updates heartbeat interval AND stores cluster data
|
||||
|
||||
**Steps:**
|
||||
1. Default `heartbeatIntervalSecs = 30`
|
||||
2. Call `dispatchConfigPush(h, nil, nil, nil, &ClusterInfo{ClusterName: "c1", HeartbeatInterval: 60, Vcpus: 32})`
|
||||
3. Assert `store.Clusters.Get("c1")` has `HeartbeatInterval=60, Vcpus=32`
|
||||
4. Assert `atomic.LoadInt32(&heartbeatIntervalSecs) == 60`
|
||||
|
||||
**Covers:** P0-1 + P1-3 interaction in a single dispatch.
|
||||
|
||||
### 5.2 Config push + host control on same host
|
||||
|
||||
**Steps:**
|
||||
1. `dispatchConfigPush(h, nil, nil, []HostInfo{{HostName: "node01", HostModel: "x86_64"}}, nil)`
|
||||
2. Assert `store.Hosts.Get("node01")` has `HostModel="x86_64"`
|
||||
3. Assert `store.Hosts.Has("node01") == true` — verifies config-push data is visible in the host store
|
||||
4. Call `preCheckHostControlRequest(&HostControlRequest{ControlAction: "close", Hostname: []string{"node01"}})` — assert `allFlag == false`, `err == nil` (validates action format; store lookup is handled by `validateHost` which requires JWT — see §2.4 note)
|
||||
|
||||
**Covers:** Data created by config push is written to the host store and is visible to subsequent operations.
|
||||
|
||||
### 5.3 Resource push after host config push — same host referenced
|
||||
|
||||
**Steps:**
|
||||
1. `h.OnHostConfigUpdated(&HostInfo{HostName: "node01", HostModel: "x86_64"})`
|
||||
2. `h.OnResourceLocationUpdated(&ResourceDistributionInfo{ResourceName: "mem", Location: "node01", NumericValue: 65536.0, ValueType: NUMERIC, IsDynamic: true})`
|
||||
3. Assert `store.Hosts.Get("node01")` has `HostModel="x86_64"` (preserved)
|
||||
4. Assert `store.ResourceLocations.Get("mem:node01")` has `NumericValue=65536.0`
|
||||
5. Assert `store.Hosts.Get("node01").HostModel` still "x86_64" (host config was not overwritten)
|
||||
|
||||
**Covers:** Two independent push paths don't interfere with each other's store data.
|
||||
|
||||
### 5.4 Worker push after config push preserves config fields
|
||||
|
||||
**Steps:**
|
||||
1. `h.OnHostConfigUpdated(&HostInfo{HostName: "node01", HostModel: "x86_64", MaxSlot: 8})`
|
||||
2. `h.OnHostUpdated(&HostInfo{HostName: "node01", HostStatus: 3, CPUFactor: 2.5, NumCPUs: 16})`
|
||||
3. Assert `got.HostModel == "x86_64" && got.MaxSlot == 8` (config preserved)
|
||||
4. Assert `got.HostStatus == 3 && got.CPUFactor == 2.5` (worker updated)
|
||||
|
||||
**Covers:** Two-push merge correctness — already tested in unit tests but verified here at integration scope.
|
||||
|
||||
### 5.5 Multiple sequential config pushes (idempotent)
|
||||
|
||||
**Steps:**
|
||||
1. Push config with `ClusterInfo{Vcpus: 16, HeartbeatInterval: 30}`
|
||||
2. Push config with `ClusterInfo{Vcpus: 32, HeartbeatInterval: 60}`
|
||||
3. Assert store has `Vcpus=32, HeartbeatInterval=60` (last-write-wins for config fields)
|
||||
4. Assert `heartbeatIntervalSecs == 60`
|
||||
|
||||
**Covers:** Rapid config push updates don't leave stale data.
|
||||
|
||||
---
|
||||
|
||||
## 6. RBAC Integration (End-to-End)
|
||||
|
||||
> **RBAC isolation:** Call `resetRBACState()` (not `store.InitStores()`) at the start of each RBAC test. `resetRBACState()` zeros `rbacConfig`, preventing `AddPermissionsForRole` from appending duplicate permissions across test reloads. `store.InitStores()` does not reset RBAC state.
|
||||
|
||||
### 6.1 Full middleware chain: JWT → RBAC → handler
|
||||
|
||||
**Steps:**
|
||||
1. Set up Gin router with real middleware (`RequirePermission("host:control")`)
|
||||
2. Create a valid JWT token for a cluster_admin user
|
||||
3. POST `/xce/v1/hosts/control` with `action=close, host_name=["node01"]`
|
||||
4. Assert 202 Accepted (or mocked backend response)
|
||||
|
||||
5. Create a valid JWT token for a basic_user
|
||||
6. POST `/xce/v1/hosts/control` — assert 403 Forbidden
|
||||
|
||||
7. POST `/xce/v1/hosts/select` with basic_user — assert passes (host:view)
|
||||
|
||||
**Note:** This requires spinning up a test HTTP server via `httptest.NewRecorder`. Do not mock at the middleware level — the purpose of §6 is to exercise the real middleware chain end-to-end.
|
||||
|
||||
### 6.2 RBAC: cluster_admin bypasses permission check
|
||||
|
||||
**Steps:**
|
||||
1. Set up Gin router with `RequirePermission("host:control")`
|
||||
2. Sign a JWT for "admin1" with role "cluster_admin" and issue `POST /xce/v1/hosts/control`
|
||||
3. Assert response status is **not 403** — verifies the middleware did not block the request
|
||||
|
||||
**Note:** `RequirePermission` returns early for `cluster_admin` without calling `ctx.Next()`, so downstream handlers are **never reached**. Do not assert a spy handler body or `ctx.IsAborted() == false` — both give false results. To verify full handler execution for a privileged user, write a separate test with a non-cluster_admin user that has explicit `host:control` permission (see §6.3).
|
||||
|
||||
### 6.3 RBAC: basic_user cannot control hosts but can view
|
||||
|
||||
**Steps:**
|
||||
1. Assign user "user1" role "basic_user"
|
||||
2. Call `RequirePermission("host:control")` — assert 403
|
||||
3. Call `RequirePermission("host:view")` — assert passes
|
||||
|
||||
### 6.4 JWT algorithm confusion is rejected
|
||||
|
||||
**Steps:**
|
||||
1. Configure test server with HS256 signing secret `"xce-api-key"`
|
||||
2. Craft a token header claiming `"alg": "none"` or a mismatched algorithm (RS256)
|
||||
3. POST to `/xce/v1/hosts/control` with that token (use valid IP binding so IP mismatch is not the cause of rejection)
|
||||
4. Assert 401 Unauthorized
|
||||
5. Assert response body contains a token/signature error string (e.g., `"token"`, `"invalid"`, or `"signature"`) — not `"ip"` or `"expired"` — to confirm the rejection is due to algorithm/signature failure, not an unrelated check
|
||||
|
||||
**Covers:** Prevents algorithm confusion attacks where an attacker downgrades signing to `none`.
|
||||
|
||||
### 6.5 Expired JWT is rejected
|
||||
|
||||
**Steps:**
|
||||
1. Sign a JWT with `exp = time.Now().Unix() - 3600` (expired 1 hour ago)
|
||||
2. POST to any protected endpoint
|
||||
3. Assert 401 Unauthorized
|
||||
|
||||
**Covers:** Token replay after expiry — ensures expired tokens cannot be reused.
|
||||
|
||||
### 6.6 Known gap: hostgroup domain restriction bypass
|
||||
|
||||
> **Documents a known limitation, not a working feature.** `validateHost` at `hosts.go:505-507` contains a `// TODO` that silently skips hostgroup domain enforcement. This test makes the gap explicit so it is visible in code review and prevents accidental regression if the TODO is removed.
|
||||
|
||||
**Steps:**
|
||||
1. Provision a user with `host:control` scoped to hostgroup "gpu-nodes"
|
||||
2. Issue `POST /xce/v1/hosts/control` targeting a host NOT in "gpu-nodes"
|
||||
3. **Assert request is accepted** (not 403) — documents that hostgroup scoping is not yet enforced
|
||||
|
||||
**Note:** When P1-2b implements hostgroup enforcement, flip this assertion to assert 403 and remove the `// TODO` marker.
|
||||
|
||||
### 6.7 Post-logout token reuse
|
||||
|
||||
> **Design gap:** JWTs are stateless and the current server does not maintain a JTI blacklist. A token valid at logout time remains valid until expiry. This test documents the current behavior as an explicit design decision.
|
||||
|
||||
**Steps:**
|
||||
1. Sign a valid JWT with a 1-hour expiry
|
||||
2. Call `POST /xce/v1/users/logout` (or equivalent) — record any logout response
|
||||
3. Immediately reuse the same token on `GET /xce/v1/hosts` — assert **200 OK** (not 401)
|
||||
|
||||
**Note:** If a JTI blacklist is added in future, flip this assertion to expect 401 and add the blacklist provisioning step.
|
||||
|
||||
---
|
||||
|
||||
## 7. Store Thread Safety
|
||||
|
||||
> **CI requirement:** All §7 tests must run with `-race`: `go test -race ./store/...`. A test that passes without `-race` provides no guarantee of absence of data races — the Go race detector instruments memory accesses at runtime and is the authoritative check.
|
||||
|
||||
### 7.1 Concurrent config pushes don't corrupt data
|
||||
|
||||
**Steps:**
|
||||
1. Start 10 goroutines, each calling `h.OnHostConfigUpdated(&HostInfo{HostName: "node01", ...})` with slightly different HostModel values
|
||||
2. Wait for all goroutines to complete
|
||||
3. Assert `store.Hosts.Get("node01")` returns a valid HostInfo (no zero-value corruption)
|
||||
4. Assert `HostName` field is "node01"
|
||||
|
||||
**Race detection note:** A simple value-validity assertion cannot prove absence of a data race — concurrent writes all produce plausible values. Run this test with `go test -race ./store/...` to detect RMW races via the Go race detector. The assertion above verifies structural integrity; `-race` detects the underlying race condition.
|
||||
|
||||
### 7.2 Concurrent host control + config push
|
||||
|
||||
**Steps:**
|
||||
1. Goroutine A: call `OnHostConfigUpdated` for "node01" in a loop
|
||||
2. Goroutine B: call `validateHost` checking "node01" existence in a loop
|
||||
3. Assert no panics and no store corruption after 100 iterations
|
||||
|
||||
### 7.3 Concurrent resource value pushes for same key
|
||||
|
||||
**Steps:**
|
||||
1. Start 10 goroutines, each calling `OnResourceLocationUpdated` for `"mem:node01"` with different NumericValue
|
||||
2. Assert final stored value is one of the 10 values (no corruption)
|
||||
3. Assert key exists exactly once
|
||||
|
||||
---
|
||||
|
||||
## 8. Input Safety
|
||||
|
||||
### 8.1 Hostname length limits
|
||||
|
||||
> **Skip until P1-2b is implemented.** No hostname length validation exists in `preCheckHostControlRequest` yet — this test will pass vacuously. Add `//go:build p1_2b` build tag or comment out pending that task.
|
||||
|
||||
**Steps:**
|
||||
1. Call `preCheckHostControlRequest` with hostname exceeding the P1-2b limit (>1024 chars)
|
||||
2. Assert `err != nil` with message indicating length exceeded
|
||||
|
||||
### 8.2 Empty hostname list
|
||||
|
||||
**Steps:**
|
||||
1. Call `preCheckHostControlRequest` with `Hostname: []`
|
||||
2. Assert `err != nil` — inspect the actual error message from `preCheckHostControlRequest` and assert it contains the expected string (e.g., "no hostnames provided" or similar)
|
||||
|
||||
### 8.3 Select expression length (current limit: 4094)
|
||||
|
||||
**Steps:**
|
||||
1. Call handler with `Select` string > 4094 chars
|
||||
2. Assert error
|
||||
3. Call with 4094 chars — assert accepted
|
||||
|
||||
### 8.4 Special characters in hostnames
|
||||
|
||||
**Steps:**
|
||||
1. Call with hostname containing `<script>alert(1)</script>`
|
||||
2. Assert the hostname is stored as a string without interpretation (XSS safety at API layer)
|
||||
|
||||
---
|
||||
|
||||
## Test File Organization
|
||||
|
||||
| File | Tests |
|
||||
|------|-------|
|
||||
| `backend/integration_test.go` | §1 (config push), §3 (resource push), §4 (heartbeat), §5 (cross-feature) |
|
||||
| `controllers/integration_test.go` | §2.1–2.3, §2.5–2.8 (host control — no JWT required), §2.4 (host validation — **requires full JWT setup; see §6 prerequisites**), §6 (RBAC middleware), §8 (input safety) |
|
||||
| `store/concurrency_test.go` | §7 (thread safety) |
|
||||
|
||||
All test files use `//go:build !cgo` or `CGO_ENABLED=0` compatible patterns.
|
||||
|
||||
**Package declarations:**
|
||||
- `backend/integration_test.go` must declare `package backend` (not `package backend_test`) to access unexported symbols: `heartbeatIntervalSecs`, `rawResvalToDistribution`, `dispatchConfigPush`
|
||||
- `controllers/integration_test.go` may use `package controllers_test` for black-box tests, but any test accessing unexported helpers must use `package controllers`
|
||||
|
||||
**JWT signing secret in tests:** Integration tests in `controllers/integration_test.go` that create real JWTs must use a test-only signing secret (e.g., `"xce-api-key"` from the test config file). Never use the production signing secret. Document the expected test secret in a comment at the top of the test file so it is clearly identifiable during security audits.
|
||||
|
||||
---
|
||||
|
||||
## Priority Classification
|
||||
|
||||
| Priority | Tests | Rationale |
|
||||
|----------|-------|-----------|
|
||||
| **P0 — Must have** | 1.1, 1.2, 1.3, 2.1, 2.2, 2.5, 3.1, 3.4, 4.1, 5.1 | Core functionality and merge correctness |
|
||||
| **P1 — Should have** | 1.4, 1.5, 2.4, 2.6, 2.8, 3.2, 3.3, 3.5, 3.6, 4.2, 5.2, 5.3, 6.1, 6.2, 6.3, 6.4, 6.5, 6.6, 6.7 | Edge cases, RBAC, JWT security, and known gap documentation |
|
||||
| **P2 — Nice to have** | 2.3, 2.7, 5.4, 5.5, 7.1, 7.2, 7.3, 8.1–8.4 | Robustness and stress |
|
||||
|
||||
---
|
||||
|
||||
## Existing Test Coverage
|
||||
|
||||
The following unit tests already cover much of the ground above and should NOT be duplicated:
|
||||
|
||||
| Existing Test File | What It Covers | Integration Gap |
|
||||
|--------------------|----------------|-----------------|
|
||||
| `backend/handler_test.go` | Single-push merge for hosts, clusters, dispatch | Multi-push sequences, cross-feature |
|
||||
| `backend/convert_test.go` | All 7 value types, nil val, unknown discriminant | Store round-trip after conversion |
|
||||
| `backend/heartbeat_test.go` | SetHeartbeatInterval, runHeartbeat call/cancel | Interval change triggered by OnClusterUpdated |
|
||||
| `controllers/host_control_test.go` | preCheck validation, concurrent no-race | Full handler flow with store interaction |
|
||||
| `controllers/rbac_check_test.go` | host:control/host:view permissions | Full middleware chain with JWT |
|
||||
|
|
@ -1,325 +0,0 @@
|
|||
---
|
||||
reviewers: [adversarial, feasibility, cgo-correctness, security, architecture]
|
||||
reviewed_at: 2026-04-16T07:00:00Z
|
||||
document_reviewed: apiserver-backend-enhancement-plan.md
|
||||
prior_review_at: 2026-04-15T04:00:00Z
|
||||
---
|
||||
|
||||
# Final Cross-Perspective Review — API Server Backend Enhancement Plan
|
||||
|
||||
> **Prior review** (adversarial + feasibility, 2026-04-15) is summarised at the bottom.
|
||||
> This document covers only **new findings** from the final three-reviewer pass.
|
||||
> All issues flagged in the prior review are confirmed fixed in the current plan.
|
||||
|
||||
---
|
||||
|
||||
## Round 2 Verification (2026-04-16)
|
||||
|
||||
### Claims Verified Against Codebase
|
||||
|
||||
**RBAC `cluster_admin` role**: ✅ PASS. `rbac.yaml:88` assigns `action: host:control` to `cluster_admin` role with `domain: type: all`. `routers.go:112` already wires `RequirePermission("host:control")` for `POST /xce/v1/hosts/control`. No `host_admin` role exists. The plan's update to `cluster_admin` is correct.
|
||||
|
||||
**`go_push_handler` is single-threaded**: ✅ PASS. `clib.go:32` calls `xcecli_set_handler(ctx, handler, NULL)` — this registers ONE handler function. `clib.go:150` `go_push_handler` runs on the C library's callback thread. The C library dispatches events sequentially on this thread. There is no concurrent push scenario for a single cluster.
|
||||
|
||||
### Remaining HIGH Issues Still in the Plan
|
||||
|
||||
| # | Issue | Status |
|
||||
|---|-------|--------|
|
||||
| CGO-1 | `unsafe.Slice(conf.hconfigs, n)` — missing `conf.hconfigs == nil` guard | **FIXED** — plan now guards `if n == 0 \|\| conf.hconfigs == nil { return nil }` |
|
||||
| CGO-2 | `unsafe.Slice(rvl.vals, n)` — missing `rvl.vals == nil` guard | **FIXED** — plan now guards `if n == 0 \|\| rvl.vals == nil { return 0 }` |
|
||||
| SEC-1 | `recover()` claim in plan said it catches C crashes | **FIXED** — plan now states `recover()` does NOT catch C SIGSEGV; C-level hardening required |
|
||||
| SEC-2 | `allFlag` package-level data race in `hosts.go:22` | **FIXED** — plan now specifies converting `allFlag` to local return value |
|
||||
| ARCH-1 | `cResourceToModel` naming collision — duplicate function name | **FIXED** — P0-1 new function renamed to `cResdefToModel(r *C.xce_resdef_t) *models.ResourceInfo` |
|
||||
| ARCH-4 | No RED test for `OnHostUpdated` behavioral change | **FIXED** — plan now requires `backend/handler_test.go` before implementation |
|
||||
| ARCH-5 | No unit tests for `rawHostConfigToModel`/`rawClusterToModel` | **FIXED** — plan now requires table-driven tests in `convert_test.go` |
|
||||
| ARCH-6 | `HostStateStr(0)` = `"OK"` for hosts with no worker data | **FIXED** — plan now specifies `HostStateStr(0)` → `"UNKNOWN"` + `LastConfigSeenAt`/`LastWorkerSeenAt` timestamps |
|
||||
|
||||
### Fresh Findings
|
||||
|
||||
**SEC-7 — `C.GoString(nil)` undefined behavior on NULL C strings (MEDIUM)**
|
||||
- `cClusterConfToClusterInfo` calls `C.GoString(conf.clustername)`. If `conf->clustername` is NULL, `C.GoString(nil)` is undefined behavior per CGO rules.
|
||||
- The pattern in existing `convert.go` (e.g., `cResourceToModel` line 98) uses `goString(res.name)` — a helper that should guard against NULL.
|
||||
- **Verdict**: The plan should use `C.GoString` only after a nil guard, or use the existing `goString` helper which presumably handles nil.
|
||||
- **Severity**: MEDIUM (Go's `C.GoString` typically panics on nil, but this depends on C library always providing a non-NULL string for `clustername`)
|
||||
|
||||
**ARCH-12 — Action string-to-integer mapping not documented (LOW)**
|
||||
- The Go HTTP handler receives JSON `{"action": "open"}` but C `HOST_OPEN`/`HOST_CLOSE` are integer enum constants. The plan's P0-2 does not document where this string-to-integer mapping happens.
|
||||
- The existing `ValidateControlAction` at `controllers/hosts.go:557` validates the string. The actual mapping to C constants must happen in `ControlHost()`.
|
||||
- **Verdict**: Include a comment or table in `ControlHost()` documenting the mapping: `"open"` → `HOST_OPEN` (C constant), `"close"` → `HOST_CLOSE`.
|
||||
|
||||
**ARCH-13 — `parse_select` errmsg ownership not specified (LOW)**
|
||||
- P1-2 does `C.free(unsafe.Pointer(errmsg))` when errmsg != nil. This is correct only if `parse_select` allocates errmsg with `malloc` (or equivalent). The plan should explicitly state the memory ownership contract for the errmsg output parameter.
|
||||
- **Verdict**: Add a one-line contract note: "parse_select allocates errmsg via malloc on error; caller owns and must free it."
|
||||
|
||||
---
|
||||
|
||||
## CGO / C Interop Correctness Review
|
||||
|
||||
### CGO-1 — `unsafe.Slice(conf.hconfigs, n)` panics when pointer is nil (HIGH)
|
||||
|
||||
**Location:** P0-1, `cClusterConfHostConfigs()` in `backend/convert.go`
|
||||
|
||||
**Bug:** The guard is `if n == 0 { return nil }` but there is no check for `conf.hconfigs == nil`. The Go specification states that `unsafe.Slice(ptr, n)` panics if `ptr == nil` and `n > 0`. A controller or partial-initialization path that sets `nhconfigs = N` and leaves `hconfigs = NULL` will crash the apiserver. This pattern is a systemic gap: the same nil guard is missing in the existing `cClusterConfQueues` and `cClusterConfResources` functions — the new function replicates the existing defect.
|
||||
|
||||
**Fix:**
|
||||
```go
|
||||
if n == 0 || conf.hconfigs == nil {
|
||||
return nil
|
||||
}
|
||||
```
|
||||
|
||||
### CGO-2 — `unsafe.Slice(rvl.vals, n)` panics when pointer is nil (HIGH)
|
||||
|
||||
**Location:** P1-1, `handleNotifyData()` in `backend/clib.go`
|
||||
|
||||
**Bug:** The guard `if rvl == nil { return 0 }` does not check `rvl.vals`. `xce_resval_list_t` in `typemap.h` has `int count` and `xce_resval_t** vals` — a null `vals` with non-zero `count` is a valid C state on error paths or partial serialization. `unsafe.Slice(rvl.vals, n)` with `n > 0` and `rvl.vals == nil` panics.
|
||||
|
||||
**Fix:**
|
||||
```go
|
||||
if rvl == nil || rvl.vals == nil || n == 0 {
|
||||
return 0
|
||||
}
|
||||
```
|
||||
|
||||
### CGO-3 — `StartHeartbeat` goroutine blocked in CGO during shutdown (MEDIUM)
|
||||
|
||||
**Location:** P1-3, `StartHeartbeat()` in `backend/clib.go`
|
||||
|
||||
**Issue:** `C.xcecli_heartbeat()` is a blocking CGO call. When `ctx.Done()` fires, the goroutine can only exit after the current C call returns. If the controller is unreachable and the C call has a long (or no) timeout, the process shutdown is delayed by up to one heartbeat cycle. This is not a goroutine leak but will delay `defer cancel()` in `main.go`. Add a comment documenting this known limitation. No API change needed for initial implementation.
|
||||
|
||||
### CGO-4 — Items confirmed safe (informational)
|
||||
|
||||
- `req.body_which` field access: `xce_request_t.body_which` is a plain `int` at struct level, not inside the union. CGO-accessible. ✓
|
||||
- `nd.event` field: `xce_notify_data_t.event` is a plain top-level `int`. CGO-accessible. ✓
|
||||
- `(*[0]byte)(C.xce_select_get_attr)`: correct standard CGO idiom for `//export` function pointer. ✓
|
||||
- `C.uintptr_t(h)` where `h` is `cgo.Handle`: integer-to-integer cast, not a pointer conversion. Avoids `go vet` warning. ✓
|
||||
|
||||
---
|
||||
|
||||
## Security Review
|
||||
|
||||
### SEC-1 — `recover()` does NOT catch C SIGSEGV — plan's stated protection is false (HIGH)
|
||||
|
||||
**Location:** P1-2, "Input safety" section — "Wrap the EvalSelectForHost call in a `recover()` block"
|
||||
|
||||
**Bug:** Go's `recover()` catches Go panics only. A null pointer dereference or out-of-bounds access inside `eval_select` or `parse_select` (C code) raises SIGSEGV via the OS, which terminates the process immediately — Go defers and recover() are bypassed. The plan's claim that `recover()` protects the server from C-side crashes is incorrect and must not be used as justification for skipping C-level validation.
|
||||
|
||||
**Fix:** Remove or correct the recover() claim. The actual protection must come from:
|
||||
1. Hardening `parse_select`/`eval_select` with internal bounds checking and null guards in C.
|
||||
2. Fuzz testing the PEG parser with a corpus before shipping.
|
||||
3. Optionally running the parse/eval in a subprocess with a timeout (more complex, not required for initial implementation, but the only true isolation).
|
||||
|
||||
Add a comment in the Go handler: `// Note: recover() does not catch C SIGSEGV. C-level null dereference in parse_select/eval_select will crash the process. Rely on C-level validation, not Go recovery.`
|
||||
|
||||
### SEC-2 — `allFlag` package-level global is a data race on concurrent requests (HIGH)
|
||||
|
||||
**Location:** `controllers/hosts.go:22`, `controllers/queues.go` (pre-existing, exposed by this plan)
|
||||
|
||||
**Bug:** `var allFlag bool` is a package-level shared variable. `preCheckHostControlRequest` sets it, `ControlHost` reads it — with no synchronization between goroutines. Two concurrent `POST /hosts/control` requests can interleave: Request A sets `allFlag = false` (targeted), Request B sets `allFlag = true` (all hosts), then Request A reads `true` and silently operates on the entire cluster instead of a single host. This is a logic race that can trigger unintended cluster-wide operations.
|
||||
|
||||
The plan's P0-2 test coverage (Verification step 2) will make this race more observable. The architecture reviewer confirmed it is present in the existing code; it must be fixed before P0-2 lands since the plan adds the real implementation path.
|
||||
|
||||
**Fix:** Eliminate the package-level variable. Return the `allFlag` value from `preCheckHostControlRequest` as a local `bool` and pass it to `ControlHost`. The same fix applies to `queues.go`.
|
||||
|
||||
### SEC-3 — In-process push data has no Go-side range check on `nrows` (MEDIUM)
|
||||
|
||||
**Location:** `backend/clib.go`, `handleDataset()` — pre-existing, not introduced by this plan
|
||||
|
||||
**Issue:** `unsafe.Slice(C.dataset_jobs(d), n)` constructs a Go slice over C memory where `n = int(d.nrows)`. If the C library delivers a dataset with a crafted or corrupted `nrows` exceeding the actual allocation, the slice construction succeeds but iteration reads out-of-bounds C memory. This can corrupt the in-memory store. The plan does not address this because it is pre-existing, but the new P1-1 resource push handler follows the same pattern and has the same risk.
|
||||
|
||||
**Fix for P1-1:** Add a sanity bound on `rvl.count` before the `unsafe.Slice` call:
|
||||
```go
|
||||
const maxResVals = 65536 // sanity cap
|
||||
if n > maxResVals {
|
||||
logger.Warn("resval count exceeds sanity limit, truncating", zap.Int("count", n))
|
||||
n = maxResVals
|
||||
}
|
||||
```
|
||||
|
||||
### SEC-4 — PEG parser DoS: 1024-byte limit alone is insufficient (MEDIUM)
|
||||
|
||||
**Location:** P1-2, "Input safety" — expression length limit
|
||||
|
||||
**Issue:** PEG parsers without memoization (packrat parsing) have O(2^n) backtracking on adversarial inputs like `((((((((a`. A 1024-byte limit does not prevent CPU exhaustion — it sets the budget. 1024 bytes of adversarial expression can still pin a CPU core for seconds in a naive PEG implementation.
|
||||
|
||||
**Mitigation required:** Verify that `xce_select.peg` generates a **packrat** (memoized) parser — packcc supports this with the `--packrat` flag or equivalent option. Document this requirement explicitly in `lib/meson.build`'s `custom_target` invocation. Without packrat, the DoS risk is real. `/hosts/select` rate limiting (already inherited from RateLimit middleware) reduces but does not eliminate the risk.
|
||||
|
||||
### SEC-5 — Error message leaks internal C error codes (LOW)
|
||||
|
||||
**Location:** P0-2, `ControlHost()` — `fmt.Errorf("xcecli_hostctrl failed: %d", int(rc))`
|
||||
|
||||
**Issue:** Negative C return codes propagate to the HTTP response body. In HPC contexts these are typically library-specific codes (not OS errno values) and are not meaningfully sensitive. However, the current stub error at `clib.go:311` already leaks the hostname and action integer in the error string — this must be cleaned up when the stub is replaced.
|
||||
|
||||
**Fix:** Map C error codes to generic messages at the API boundary: `"host control failed"` (with the code logged server-side at WARN level, not returned to the client).
|
||||
|
||||
### SEC-6 — Confirmed safe (informational)
|
||||
|
||||
- `host:view` permission for all authenticated users: intentional HPC design. Users need host capacity info to craft job resource requests. ✓
|
||||
- Hostname as `C.CString()`: the store lookup in `validateHost` acts as an allowlist — only controller-pushed hostnames reach C. ✓
|
||||
|
||||
---
|
||||
|
||||
## Architecture & Design Consistency Review
|
||||
|
||||
### ARCH-1 — `cResourceToModel` naming collision: plan adds a duplicate with a different signature (HIGH)
|
||||
|
||||
**Location:** P0-1, "convert.go" section; P1-1, "cResourceToModel note"
|
||||
|
||||
**Bug:** `convert.go` already contains `cResourceToModel(res *C.xce_resource_t) *models.ResourceDefinitionInfo` (line 95-104). The plan's P0-1 describes adding `cResourceToModel(r *C.xce_resdef_t) *models.ResourceInfo` — a different input type (`xce_resdef_t` vs `xce_resource_t`) and a different output type (`ResourceInfo` vs `ResourceDefinitionInfo`). Two functions with the same name in the same package is a compile error.
|
||||
|
||||
The plan's P1-1 note "cResourceToModel already exists — do NOT add a duplicate" is internally contradictory: it refers to the **existing** function (xce_resource_t → ResourceDefinitionInfo), not the new P0-1 function described in the same document.
|
||||
|
||||
**Fix:** Rename the new function in P0-1 to `cResdefToModel(r *C.xce_resdef_t) *models.ResourceInfo`. Update all call sites. P1-1 can continue to use the existing `cResourceToModel` (the old one) without conflict.
|
||||
|
||||
### ARCH-2 — `host_admin` RBAC role referenced in plan does not exist (HIGH)
|
||||
|
||||
**Location:** P0-2, "Go Side / router/routers.go"
|
||||
|
||||
**Bug:** The plan states "Permitted roles: admin users with the `host_admin` role." The actual `rbac.yaml` has three roles: `basic_user`, `cluster_admin`, `queue_admin`. There is no `host_admin` role. The `host:control` permission is already assigned to `cluster_admin`. Both routes at `routers.go:111-112` already exist with `RequirePermission("host:control")` and `RequirePermission("host:view")`.
|
||||
|
||||
**Fix:** Remove all references to `host_admin` from the plan. Clarify: `host:control` → `cluster_admin` (already configured). Verification step 7 should read "returns 403 for `basic_user` and `queue_admin`, succeeds for `cluster_admin`."
|
||||
|
||||
### ARCH-3 — P0-2 stubs reference P1-3 symbols: CGO build fails between phases (HIGH)
|
||||
|
||||
**Location:** P0-2, "backend/stub.go"
|
||||
|
||||
**Issue:** The plan adds `StartHeartbeat(ctx context.Context) {}` and `SetHeartbeatInterval(secs int) {}` stubs to `stub.go` in P0-2, but the corresponding CGO implementations in `clib.go` are not added until P1-3. The `!cgo` build will compile correctly with the stubs. The CGO build (used for integration testing) will fail with "undefined: backend.StartHeartbeat" if P0-2 is tested before P1-3.
|
||||
|
||||
**Fix:** Add a note: "The CGO build will fail to compile after P0-2 until P1-3 adds `StartHeartbeat` and `SetHeartbeatInterval` to `clib.go`. Implement P0-2 stub additions and P1-3 `clib.go` additions in the same commit, or accept a temporary CGO build break between phases."
|
||||
|
||||
### ARCH-4 — `OnHostUpdated` behavioral change has no RED test written first (HIGH)
|
||||
|
||||
**Location:** P0-1, "OnHostUpdated fix"
|
||||
|
||||
**Issue:** The plan changes `handler.go:27` from full-overwrite to partial merge — a behavioral change to existing code. There are zero tests for `DefaultHandler` in the entire test suite. CLAUDE.md mandates TDD: "Write a failing test first." A wrong field classification in the partial merge (e.g., accidentally preserving a worker field that should be updated) will silently corrupt host state with no test catching it.
|
||||
|
||||
**Fix:** Before implementing the `OnHostUpdated` change, add `backend/handler_test.go` with:
|
||||
- A test that proves the current full-overwrite behavior (RED for the new code)
|
||||
- Tests for `OnHostConfigUpdated` partial merge: config fields update, worker fields preserved
|
||||
- Tests for `OnHostUpdated` partial merge: worker fields update, config fields preserved
|
||||
- Tests for `OnClusterUpdated` partial merge
|
||||
|
||||
Use `store.InitStores()` for store reset between test cases.
|
||||
|
||||
### ARCH-5 — Pure conversion functions have no unit tests specified (HIGH)
|
||||
|
||||
**Location:** P0-1, "backend/convert_pure.go"
|
||||
|
||||
**Issue:** `rawHostConfigToModel` and `rawClusterToModel` are placed in `convert_pure.go` (no CGO, fully testable) but the plan's Verification section specifies only post-hoc integration checks (start server, hit endpoints). The `convert_test.go` file already tests `rawJobToModel` and `rawWorkerToModel` with table-driven tests — the same pattern must be applied here per TDD rules.
|
||||
|
||||
**Fix:** Add to the P0-1 implementation instructions: "Before implementing `rawHostConfigToModel` and `rawClusterToModel`, add table-driven tests in `backend/convert_test.go` covering: nil/empty string fields, zero integer fields (MaxSlot=0, HeartbeatInterval=0), normal values."
|
||||
|
||||
### ARCH-6 — First-startup host state appears "OK" with zero worker fields (HIGH)
|
||||
|
||||
**Location:** P0-1, "Partial merge semantics"
|
||||
|
||||
**Issue:** After P0-1, the config push fires and creates host entries with `HostModel`, `HostType`, `MaxSlot` set but all worker-sourced fields (`HostStatus`, `MaxMem`, `NumRun`, etc.) at zero. `HostStateStr` in `models/hosts.go:20` maps `state=0` to `"OK"`. A `GET /xce/v1/hosts` call in the window before any worker has checked in will return hosts showing status `"OK"` with 0 CPUs and 0 memory. Operators and monitoring dashboards will see misleading data.
|
||||
|
||||
**Fix (minimal, non-breaking):** Change `HostStateStr(0)` from `"OK"` to `"UNKNOWN"` or `"PENDING"`. This is a one-line change in `models/hosts.go` that accurately reflects the not-yet-reported state. Alternatively, add a `WorkerDataReceived bool` field to `HostInfo` set by `OnHostUpdated`.
|
||||
|
||||
### ARCH-7 — `meson.build` custom_target block is not specified (MEDIUM)
|
||||
|
||||
**Location:** P1-2, "Build Integration"
|
||||
|
||||
**Issue:** The plan says "add `xce_select.peg` to that explicit list." The actual `meson.build` pattern requires a full `custom_target` block mirroring the existing `xce_resreq_parser` block — the packcc tool, input/output filenames, command invocation — followed by adding `xce_select_parser[0]` to `libxce_src`. Simply adding the filename to `libxce_src` will fail because meson does not auto-run packcc on `.peg` files.
|
||||
|
||||
**Fix:** The plan must include the complete `custom_target` block for `xce_select.peg`.
|
||||
|
||||
### ARCH-8 — Heartbeat test uses `time.Sleep` despite plan prohibiting it (MEDIUM)
|
||||
|
||||
**Location:** Verification section, item 5
|
||||
|
||||
**Issue:** The plan states "The test must not depend on real time" then immediately provides a test using `time.Sleep(3500ms)`. This is a 3.5-second wall-clock delay per test run, and the pass/fail boundary (`n < 2 || n > 5`) will be flaky on loaded CI systems.
|
||||
|
||||
**Fix:** Expose the ticker as injectable: `StartHeartbeat(ctx, tickCh <-chan time.Time)` where production code passes `time.NewTicker(...).C` and tests pass a manually-driven `chan time.Time`. The test sends exactly N ticks and asserts the counter equals N — zero wall-clock time required.
|
||||
|
||||
### ARCH-9 — No code-level TODO for O(H) parse-per-call bottleneck (MEDIUM)
|
||||
|
||||
**Location:** P1-2, "Performance" note
|
||||
|
||||
**Issue:** The plan acknowledges that `EvalSelectForHost` calls `parse_select` once per host (O(H) parse calls per request) and says "do not over-engineer." The optimization path is described only in the design document — future developers reading the code will not see it.
|
||||
|
||||
**Fix:** Add this comment in `EvalSelectForHost` in `clib.go`:
|
||||
```go
|
||||
// TODO(perf): parse_select is called once per host. For large clusters, pre-parse
|
||||
// the expression once and pass the *C.xce_select_t to a batch evaluator.
|
||||
// See EvalSelectForHostParsed design note in apiserver-backend-enhancement-plan.md.
|
||||
```
|
||||
|
||||
### ARCH-10 — Stale decommissioned hosts have no API signal (MEDIUM)
|
||||
|
||||
**Location:** P0-1, "Partial merge semantics" — host deletion paragraph
|
||||
|
||||
**Issue:** Hosts removed from the cluster configuration remain in `store.Hosts` indefinitely. There is no `LastSeenAt` field, no stale flag, no way for operators to distinguish a decommissioned host from a healthy-but-unreachable one. The plan defers TTL, but adds no groundwork for it.
|
||||
|
||||
**Fix:** Add `LastConfigSeenAt time.Time` and `LastWorkerSeenAt time.Time` fields to `HostInfo`. Set them in `OnHostConfigUpdated` and `OnHostUpdated` respectively. These fields cost nothing in correctness and provide the data source for a future TTL implementation without requiring a model change later.
|
||||
|
||||
---
|
||||
|
||||
## Consensus Summary
|
||||
|
||||
### Issues Requiring Plan Correction Before Implementation
|
||||
|
||||
| # | Finding | Severity | Phase |
|
||||
|---|---------|----------|-------|
|
||||
| CGO-1 | `unsafe.Slice` nil pointer — `cClusterConfHostConfigs` | HIGH | P0-1 |
|
||||
| CGO-2 | `unsafe.Slice` nil pointer — `handleNotifyData` | HIGH | P1-1 |
|
||||
| SEC-1 | `recover()` does not catch C SIGSEGV — stated protection is false | HIGH | P1-2 |
|
||||
| SEC-2 | `allFlag` package-level data race — concurrent requests corrupt state | HIGH | P0-2 |
|
||||
| ARCH-1 | `cResourceToModel` naming collision — compile error | HIGH | P0-1 |
|
||||
| ARCH-2 | `host_admin` role does not exist in `rbac.yaml` | HIGH | P0-2 |
|
||||
| ARCH-3 | P0-2 stubs cause CGO link failure before P1-3 | HIGH | P0-2 |
|
||||
| ARCH-4 | `OnHostUpdated` change has no RED test (TDD violation) | HIGH | P0-1 |
|
||||
| ARCH-5 | `rawHostConfigToModel`/`rawClusterToModel` have no unit tests specified | HIGH | P0-1 |
|
||||
| ARCH-6 | Zero worker fields at first startup show as `"OK"` status | HIGH | P0-1 |
|
||||
|
||||
### Issues Requiring Plan Clarification
|
||||
|
||||
| # | Finding | Severity | Phase |
|
||||
|---|---------|----------|-------|
|
||||
| SEC-3 | No Go-side range check on `nrows` before `unsafe.Slice` in push handler | MEDIUM | P1-1 |
|
||||
| SEC-4 | PEG parser packrat requirement not specified (DoS risk) | MEDIUM | P1-2 |
|
||||
| ARCH-7 | `meson.build` custom_target block missing from plan | MEDIUM | P1-2 |
|
||||
| ARCH-8 | Heartbeat test uses `time.Sleep` despite plan prohibiting it | MEDIUM | P1-3 |
|
||||
| ARCH-9 | No code-level TODO for O(H) parse-per-call bottleneck | MEDIUM | P1-2 |
|
||||
| ARCH-10 | Stale decommissioned hosts have no API signal | MEDIUM | P0-1 |
|
||||
| CGO-3 | `StartHeartbeat` goroutine cannot be interrupted during CGO call | MEDIUM | P1-3 |
|
||||
| SEC-5 | C error codes leak to HTTP response body | LOW | P0-2 |
|
||||
|
||||
### Confirmed Strengths (all reviewers agreed)
|
||||
|
||||
- Partial merge semantics are correctly designed: field ownership lists are explicit and the read-then-write pattern is sound for the single-threaded push handler.
|
||||
- `OPCode_notify` placed at the top-level switch (not inside `handlePush`) is architecturally correct.
|
||||
- `cgo.Handle` usage for the select callback context is the right CGO approach.
|
||||
- `RegisterToBackend` being synchronous ensures heartbeat starts with the correct interval.
|
||||
- PEG grammar placed in `libxce` (not Go layer) is correct for scheduler reuse.
|
||||
- The two-step extraction (`xce_request_body` then `xce_notify_data_ptr`) is now correct in the plan.
|
||||
|
||||
### Items Confirmed Not Issues
|
||||
|
||||
- `req.body_which` and `nd.event` are plain struct fields, CGO-accessible. ✓
|
||||
- `(*[0]byte)(C.xce_select_get_attr)` is the correct CGO function pointer idiom. ✓
|
||||
- `C.uintptr_t(h)` for cgo.Handle is an integer cast (not pointer), go-vet clean. ✓
|
||||
- `host:view` for all authenticated users is intentional HPC design. ✓
|
||||
- `xcecli_hostctrl` blocking CGO: consistent with existing queuectrl pattern, local socket. ✓
|
||||
|
||||
---
|
||||
|
||||
## Prior Review Summary (2026-04-15)
|
||||
|
||||
The prior adversarial + feasibility review identified the following issues, all of which are **confirmed fixed** in the current plan:
|
||||
|
||||
| Issue | Fix Confirmed |
|
||||
|-------|---------------|
|
||||
| `handleNotifyData()` used wrong function to extract body | Fixed: plan now uses `xce_request_body(req)` first |
|
||||
| `xce_val_t.which` referenced as `.type` | Fixed: plan uses `val.which` throughout |
|
||||
| `handleUpdate()` union field accessed directly | Fixed: plan explicitly states OPCode_update is NOT used by apiserver |
|
||||
| P1-2 CGO map-as-context was undefined behavior | Fixed: plan uses `cgo.Handle` |
|
||||
| `stub.go` stubs omitted for `!cgo` builds | Fixed: comprehensive stub section added |
|
||||
| `helper.c` missing from file list | Fixed: included with `Request_body_hostctrlmsg` case |
|
||||
| `free_select()` / `kill_resreq()` linkage — memory leak | Fixed: plan shows both `free_select` + `FREEUP(resreq->select)` |
|
||||
| Heartbeat interval not reconfigurable after start | Fixed: `SetHeartbeatInterval` + `ticker.Reset` per tick |
|
||||
|
||||
---
|
||||
|
||||
*To incorporate this feedback: update `apiserver-backend-enhancement-plan.md` addressing all HIGH-severity items before implementation begins. MEDIUM items should be resolved before the first PR is opened.*
|
||||
|
|
@ -1,252 +0,0 @@
|
|||
# Config Replication — Gap Analysis: Design vs Implementation
|
||||
|
||||
> **Date**: 2026-05-20
|
||||
> **Design doc**: `config-replication-design.md`
|
||||
> **Implementation status**: `config-replication-impl-status.md`
|
||||
> **Branch**: `persistent_ha`
|
||||
> **Scope**: Validates what is implemented vs what the design specifies.
|
||||
|
||||
---
|
||||
|
||||
## Executive Summary
|
||||
|
||||
The infrastructure is largely in place: schema, struct fields, storage functions, election probing,
|
||||
and the Phase 1 opcode dispatch all exist. However **the core wire protocol is not functional**:
|
||||
both the standby fetch (`do_one_config_fetch`) and the primary's reply payload
|
||||
(`handle_config_request`) are explicitly stubbed pending "Task 11" — Cap'n Proto `Data`-typed
|
||||
field extraction shims. Additionally, a path inconsistency bug affects `conf_save_config()`, and
|
||||
the push timeout design is not implemented.
|
||||
|
||||
**Phase 1 (pull-only replication)**: Infrastructure exists; wire protocol is a no-op stub.
|
||||
**Phase 2 (remote push)**: Gate compiles but returns `-ENOSYS`; functional logic exists in `do_upgrade_config_with_bytes()` but is not wired.
|
||||
|
||||
---
|
||||
|
||||
## Gap 1 — CRITICAL: `do_one_config_fetch()` is a no-op stub
|
||||
|
||||
**Design (§5.5):** The standby connects to the primary over a standalone SSL socket (before
|
||||
Phase 2 / iomgr), sends `OP_replConfig` with its saved version tag, receives `ReplConfigReply`
|
||||
with the full config bytes + SHA-256, verifies integrity, writes to `conf/`, and updates
|
||||
`conf/current`.
|
||||
|
||||
**Code (`config_repl.c:674–683`):**
|
||||
```c
|
||||
static int do_one_config_fetch(cluster_t *c, const char *host, uint32_t port,
|
||||
uint64_t our_tag)
|
||||
{
|
||||
(void)c; (void)host; (void)port; (void)our_tag;
|
||||
/* Task 11: wire full capnp encode/decode over standalone SSL socket */
|
||||
return -ENOSYS;
|
||||
}
|
||||
```
|
||||
|
||||
**Impact:** All 20 retry attempts in `fetch_config_from_primary()` fail immediately with
|
||||
`-ENOSYS` (~47 seconds of log spam), then fall back to the locally saved config. A brand-new
|
||||
standby with no saved config will FATAL with "no saved config" error.
|
||||
|
||||
**Root cause:** Task 11 (Cap'n Proto `Data`-type field extraction shims for the standalone SSL path) is not yet done.
|
||||
|
||||
---
|
||||
|
||||
## Gap 2 — CRITICAL: `handle_config_request()` sends NULL bytes and SHA-256
|
||||
|
||||
**Design (§5.4):** When the standby's tag differs from the primary's, the primary sends back
|
||||
`ReplConfigReply { up_to_date=false, versionTag=..., sha256=<32 bytes>, bytes=<raw blob> }`.
|
||||
|
||||
**Code (`config_repl.c:650–655`):**
|
||||
```c
|
||||
/* sha256 and bytes are xce_data_t* — raw-byte encoding is wired in Task 11.
|
||||
* For now set to NULL so encode_Data_ptr emits capn_null pointers. */
|
||||
rmsg->sha256 = NULL;
|
||||
rmsg->bytes = NULL;
|
||||
```
|
||||
|
||||
**Impact:** Even if the standby's SSL connection were wired, the primary would reply with an
|
||||
incomplete message. The standby would receive a tag but no config payload.
|
||||
|
||||
---
|
||||
|
||||
## Gap 3 — HIGH: `handle_upgrade_config()` always returns `-ENOSYS`
|
||||
|
||||
**Design (§6.4):** When `CONF_ENABLE_UPGRADE_CONFIG=1`, `handle_upgrade_config()` validates
|
||||
SHA-256, decodes Cap'n Proto, increments generation, saves to disk, fans out to standbys, then
|
||||
signals restart. The full logic exists in `do_upgrade_config_with_bytes()`.
|
||||
|
||||
**Code (`config_repl.c:1031–1048`):**
|
||||
```c
|
||||
#if CONF_ENABLE_UPGRADE_CONFIG
|
||||
int handle_upgrade_config(cluster_t *c, endpoint_t *ep, xce_request_t *req)
|
||||
{
|
||||
/* Task 11 dependency: ... until Task 11 lands this handler rejects all
|
||||
* upgrade requests with -ENOSYS even when CONF_ENABLE_UPGRADE_CONFIG=1. */
|
||||
send_upgrade_error(ep, req, c,
|
||||
"OP_upgradeConfig byte extraction requires Task 11 ...");
|
||||
return -ENOSYS;
|
||||
}
|
||||
```
|
||||
|
||||
**Impact:** Phase 2 is completely non-functional even when the compile-time gate is enabled.
|
||||
The `do_upgrade_config_with_bytes()` helper containing the real logic exists but is not called.
|
||||
|
||||
---
|
||||
|
||||
## Gap 4 — MEDIUM (latent bug): `conf_save_config()` uses wrong directory path
|
||||
|
||||
**Design (§3.1):** Config files live at `${workdir}/${name}/conf/cluster.bin.<sha>`.
|
||||
|
||||
**Code mismatch:**
|
||||
|
||||
| Function | Effective path | Correct? |
|
||||
|---|---|---|
|
||||
| `load_primary_config()` | builds `conf_dir = workdir/name`, reads `workdir/name/conf/current` | ✓ |
|
||||
| `fetch_config_from_primary()` | builds `conf_dir = workdir/name`, calls helpers correctly | ✓ |
|
||||
| `conf_save_config()` | `make_conf_dir(myconf->workdir, ...)` → `workdir/conf` (no `name`) | ✗ |
|
||||
| `conf_trim_old_versions()` (called from save) | same `myconf->workdir` → `workdir/conf` | ✗ |
|
||||
|
||||
**`conf_save_config()` (line 235):**
|
||||
```c
|
||||
make_conf_dir(myconf->workdir, conf_dir, sizeof(conf_dir));
|
||||
// → conf_dir = "/var/xce/conf" (missing "/ctrl01")
|
||||
```
|
||||
|
||||
**`load_primary_config()` (line 75–76):**
|
||||
```c
|
||||
snprintf(conf_dir, sizeof(conf_dir), "%s/%s", myconf->workdir, myconf->name);
|
||||
// → conf_dir = "/var/xce/ctrl01" (correct)
|
||||
```
|
||||
|
||||
**Status:** Currently latent because `handle_upgrade_config()` returns `-ENOSYS` before calling
|
||||
`do_upgrade_config_with_bytes()` → `conf_save_config()`. Will trigger as a real bug when Task 11
|
||||
wires the upgrade path.
|
||||
|
||||
**Fix:** Change `conf_save_config()` to accept an `effective_dir` parameter (or construct
|
||||
`workdir/name` internally from `myconf->workdir` + `myconf->name`), consistent with how
|
||||
`load_primary_config()` and `fetch_config_from_primary()` build the path.
|
||||
|
||||
---
|
||||
|
||||
## Gap 5 — MEDIUM: No aggregate timeout in `push_config_to_standbys()`
|
||||
|
||||
**Design (§6.3):** "The primary waits at most `CONF_PUSH_TIMEOUT_MS` (default 500 ms) **total**
|
||||
for all ACKs. This bounds the pre-restart delay to a single timeout period regardless of standby
|
||||
count."
|
||||
|
||||
**Code (`config_repl.c:840–858`):**
|
||||
```c
|
||||
for (int i = 0; i < n; i++) {
|
||||
if (!ctxs[i].started) continue;
|
||||
pthread_join(threads[i], NULL); // ← blocks indefinitely per thread
|
||||
if (ctxs[i].acked) acked++;
|
||||
}
|
||||
```
|
||||
|
||||
`CONF_PUSH_TIMEOUT_MS` is defined in the header (line 26) but is **never used** in this function.
|
||||
The design calls for `wait_all_with_timeout(push_threads, n_standbys, CONF_PUSH_TIMEOUT_MS)` which
|
||||
is not implemented.
|
||||
|
||||
**Impact:** If any standby hangs during push, the primary could block indefinitely before
|
||||
restarting. The `CONF_PUSH_TIMEOUT_MS` safety bound is missing.
|
||||
|
||||
---
|
||||
|
||||
## Gap 6 — MEDIUM: `start_heartbeat_listener()` is a stub
|
||||
|
||||
**Design (§9.0):** A dedicated heartbeat listener thread starts immediately after
|
||||
`am_i_primary()` determines the node is primary. It allows lower-priority candidates to detect
|
||||
the primary during Phase 2 initialization (before iomgr is up).
|
||||
|
||||
**Code (`ha.c:356–365`):**
|
||||
```c
|
||||
void start_heartbeat_listener(cluster_t *c)
|
||||
{
|
||||
(void)c;
|
||||
/* Heartbeat listener stub — full implementation in cluster.c Task 7 */
|
||||
}
|
||||
void stop_heartbeat_listener(cluster_t *c) { (void)c; }
|
||||
```
|
||||
|
||||
**Impact:** `impl-status.md` notes: "simultaneous-boot symmetry relies on priority step only."
|
||||
This means the §9.0 guarantee — that a lower-priority self-declared primary can receive a
|
||||
heartbeat from the higher-priority true primary during Phase 2 init — is not enforced.
|
||||
In practice, the priority-based step delay (500ms gap) makes simultaneous-boot split-brain
|
||||
unlikely, but not impossible.
|
||||
|
||||
---
|
||||
|
||||
## Gap 7 — LOW: WAL replay routing helpers (`resolve_queue_or_lost_and_found()` etc.) not implemented
|
||||
|
||||
**Design (§6.5):** Three helpers for routing orphaned WAL records:
|
||||
- `resolve_queue_or_lost_and_found(cluster_t *c, uint32_t queue_id)`
|
||||
- `resolve_host_or_lost_and_found()`
|
||||
- `clear_resource_and_pend()`
|
||||
|
||||
These are declared in §12 (Files Changed → `config_repl.c`) and are supposed to be called from
|
||||
`apply_wal_entry()` in `persist.c`.
|
||||
|
||||
**Code:** Not found in `config_repl.c` or `persist.c`. `create_lost_and_found()` exists and
|
||||
creates the synthetic queue/host, but the WAL apply path does not use them — unknown queue/host
|
||||
IDs during `persist_recover()` are not routed to `lost_and_found`.
|
||||
|
||||
**Impact:** Config upgrade (removing a queue/host) + WAL replay would hit an unknown-ID path
|
||||
that is not yet gracefully handled. Currently not a production risk since `OP_upgradeConfig` is
|
||||
gated, but a prerequisite for safe queue/host removal.
|
||||
|
||||
---
|
||||
|
||||
## Gap 8 — LOW: Certificate expiry warning (§11.1) not implemented
|
||||
|
||||
**Design (§11.1):** At startup (after `create_ssl_context`), check `X509_get0_notAfter()` and
|
||||
WARN if the certificate expires within `CERT_WARN_DAYS` (30 days).
|
||||
|
||||
**Code:** `CERT_WARN_DAYS 30` is defined in `config_repl.h:30` but no code reads it or calls
|
||||
`X509_get0_notAfter()`.
|
||||
|
||||
---
|
||||
|
||||
## Gap 9 — LOW: `sd_notify()` not implemented
|
||||
|
||||
**Design (§11.1):** The systemd unit file specifies `Type=notify`, which requires the controller
|
||||
to call `sd_notify(0, "READY=1")` after Phase 2 completes.
|
||||
|
||||
**Code:** No `sd_notify()` call found in any controller source file.
|
||||
|
||||
---
|
||||
|
||||
## Summary Table
|
||||
|
||||
| # | Gap | Severity | Currently Reachable? | Blocks |
|
||||
|---|---|---|---|---|
|
||||
| 1 | `do_one_config_fetch()` stub | **CRITICAL** | Yes (standby startup) | Standby config fetch |
|
||||
| 2 | `handle_config_request()` null payload | **CRITICAL** | Yes (any replConfig request) | Primary config reply |
|
||||
| 3 | `handle_upgrade_config()` -ENOSYS | **HIGH** | Yes (when gate=1) | Phase 2 entirely |
|
||||
| 4 | `conf_save_config()` wrong path | **MEDIUM** | No (latent; blocked by Gap 3) | Config upgrade restart cycle |
|
||||
| 5 | No push timeout | **MEDIUM** | No (blocked by Gap 3) | Upgrade restart latency |
|
||||
| 6 | Heartbeat listener stub | **MEDIUM** | Yes | Simultaneous-boot robustness |
|
||||
| 7 | WAL routing helpers missing | **LOW** | No (blocked by Gate=0) | Safe config upgrade |
|
||||
| 8 | Cert expiry warning missing | **LOW** | No (operational only) | Observability |
|
||||
| 9 | sd_notify() missing | **LOW** | No (operational only) | Systemd integration |
|
||||
|
||||
---
|
||||
|
||||
## Root Cause: Task 11 Dependency
|
||||
|
||||
Gaps 1, 2, and 3 share a common root cause: **"Task 11"** — Cap'n Proto `Data`-typed field
|
||||
extraction shims in the typemap layer. The design's `ReplConfigReply.sha256` and
|
||||
`ReplConfigReply.bytes` (and the corresponding request fields for `OP_upgradeConfig`) are typed as
|
||||
`xce_data_t*` (a discriminated cluster-data union) in the current typemap. Extracting raw bytes
|
||||
from them requires shims that don't yet exist.
|
||||
|
||||
Completing Task 11 would unblock:
|
||||
- `handle_config_request()` → send actual bytes
|
||||
- `do_one_config_fetch()` → receive and decode config
|
||||
- `handle_upgrade_config()` → extract input bytes from request
|
||||
|
||||
---
|
||||
|
||||
## Recommended Fix Order
|
||||
|
||||
1. **Task 11 (typemap Data shims)** — unblocks Gaps 1, 2, 3 simultaneously
|
||||
2. **Gap 4 fix** (`conf_save_config()` path) — fix before Task 11 ships to avoid silent bug
|
||||
3. **Gap 5 fix** (push timeout) — add `wait_all_with_timeout()` helper alongside Task 11
|
||||
4. **Gap 6** (heartbeat listener) — implement to complete §9.0 simultaneous-boot guarantee
|
||||
5. **Gap 7** (WAL routing helpers) — required before enabling `CONF_ENABLE_UPGRADE_CONFIG=1`
|
||||
|
|
@ -1,71 +1,52 @@
|
|||
# P1-2: Host Resource Requirement Select — Investigation
|
||||
|
||||
## Status
|
||||
- [>] Under investigation (moved from apiserver-backend-tasks.md)
|
||||
- **C layer (P1-2a): COMPLETE** — `eval_select_simple` wired in `xce_select.c`
|
||||
(PEG grammar → AST → CSE, memoized eval). Scheduler integration (Phase 6) done.
|
||||
See `src/docs/resreq-ast-cse-design.md` for architecture reference.
|
||||
- **Go bridge (P1-2b): PENDING** — `EvalSelectForHost` remains a stub returning
|
||||
`errNoCGO`. CGO boundary, resource value lookup from in-memory store, and
|
||||
`FilterHostByResReqSelect` are still to implement.
|
||||
|
||||
---
|
||||
|
||||
## Background
|
||||
|
||||
The `select` field in an LSF-style resource requirement string (e.g. `select[mem>4096 && type==X86_64]`)
|
||||
filters candidate hosts before slot allocation. Currently `EvalSelectForHost` is a no-CGO stub that
|
||||
always returns `errNoCGO`.
|
||||
The `select` field in an LSF-style resource requirement string (e.g.
|
||||
`select[mem>4096 && type==X86_64]`) filters candidate hosts before slot
|
||||
allocation. Currently `EvalSelectForHost` is a no-CGO stub that always returns
|
||||
`errNoCGO`.
|
||||
|
||||
The full feature requires two layers:
|
||||
|
||||
1. **C parser + evaluator** — parse the boolean select expression into an AST and evaluate it against
|
||||
a host's resource values.
|
||||
2. **Go bridge** — call the C evaluator via CGO and wire it into `FilterHostByResReqSelect` in the
|
||||
controllers layer.
|
||||
1. **C parser + evaluator** — parse the boolean select expression into an
|
||||
AST and evaluate it against a host's resource values.
|
||||
2. **Go bridge** — call the C evaluator via CGO and wire it into
|
||||
`FilterHostByResReqSelect` in the controllers layer.
|
||||
|
||||
---
|
||||
|
||||
## Sub-tasks
|
||||
|
||||
### #11 P1-2a: C select expression parser (PEG + eval)
|
||||
### #11 P1-2a: C select expression parser (PEG + eval) ✓ COMPLETE
|
||||
|
||||
**Files implemented:**
|
||||
- `include/resreq.h` — expression-tree types, enums, `extern` declarations.
|
||||
- `lib/xce_select.peg` — PEG grammar for select boolean expressions.
|
||||
- `lib/xce_select.c` — `parse_select()`, `eval_select_simple()`, CSE tree.
|
||||
- `lib/meson.build` — `xce_select.peg` in build.
|
||||
|
||||
### #12 P1-2b: Go EvalSelectForHost + FilterHostByResReqSelect (remaining)
|
||||
|
||||
**Files to touch:**
|
||||
- `include/resreq.h` — Replace empty `xce_resreq_select_` stub with expression-tree types, enums,
|
||||
and `extern` declarations for `parse_select`, `eval_select`, `free_select`.
|
||||
- `lib/xce_select.peg` — New PEG grammar for select boolean expressions (right-recursive, handles
|
||||
`&&`, `||`, `!`, comparison operators, numeric literals, string literals, resource names).
|
||||
- `lib/resreq.c` — Implement `parse_select()`, `eval_select()`, `free_select()`; update
|
||||
`kill_primary_resreq()` to call `free_select()`.
|
||||
- `lib/meson.build` — Add `xce_select.peg` to explicit source list so the PEG tool generates C.
|
||||
- `lib/testsuite/` — TDD tests for parse/eval before implementing.
|
||||
|
||||
**Open questions:**
|
||||
- Which PEG library is already available in the Nix/Meson build? (`peg`, `lemon`, `re2c`?)
|
||||
- Exact grammar for comparison operators — LSF allows `mem > 4096`, `type == X86_64`, `swp >= 0`.
|
||||
Need to confirm all supported operators and value types.
|
||||
- Does `eval_select` receive a flat key-value map of resource values, or a pointer to the full
|
||||
`xce_host_t` struct?
|
||||
- Error reporting strategy — return a bool+error-string, or a tri-state (true/false/parse-error)?
|
||||
|
||||
### #12 P1-2b: Go EvalSelectForHost + FilterHostByResReqSelect (blocked by #11)
|
||||
|
||||
**Files to touch:**
|
||||
- `backend/clib.go` — Implement `EvalSelectForHost(hostname, selectExpr string) error` using
|
||||
`cgo.Handle` for callback context; add `#include "resreq.h"`, extern C declarations,
|
||||
`xce_select_destroy` helper.
|
||||
- `backend/clib.go` — Implement `EvalSelectForHost(hostname, selectExpr string) error`
|
||||
using `cgo.Handle` for callback context; add `#include "resreq.h"`, extern C
|
||||
declarations, `xce_select_destroy` helper.
|
||||
- `controllers/hosts.go` — Implement `FilterHostByResReqSelect(hosts []models.HostInfo, expr string)`,
|
||||
update `preCheckHostRequest()` limit from 4094 → 1024 bytes.
|
||||
|
||||
**Open questions:**
|
||||
- Does `EvalSelectForHost` need to look up live resource values from the in-memory store, or are
|
||||
values passed in directly by the C layer?
|
||||
- `cgo.Handle` usage pattern — confirm whether a per-call handle or a session-level handle is needed.
|
||||
- Does `EvalSelectForHost` look up live resource values from the in-memory
|
||||
store, or are values passed directly from C?
|
||||
- `cgo.Handle` usage pattern — per-call or session-level?
|
||||
- Input validation: max expression length (1024 bytes), allowed characters.
|
||||
|
||||
---
|
||||
|
||||
## Investigation Checklist
|
||||
|
||||
- [ ] Survey existing PEG tooling in the Meson/Nix build environment.
|
||||
- [ ] Read `include/resreq.h` current state to understand `xce_resreq_select_` stub.
|
||||
- [ ] Trace how `kill_primary_resreq()` works today and where `free_select()` must be called.
|
||||
- [ ] Confirm resource value structure passed to `eval_select`.
|
||||
- [ ] Confirm LSF select grammar spec (operators, value types, nesting depth).
|
||||
- [ ] Prototype PEG grammar for a subset (numeric comparisons only) and get it compiling.
|
||||
- [ ] Decide error propagation strategy across the CGO boundary.
|
||||
- [ ] Write TDD tests for C parser before any implementation.
|
||||
|
|
|
|||
|
|
@ -1,21 +0,0 @@
|
|||
# Phase 6 Pre-Audit: Single-Threaded Invariant and Stub Locations
|
||||
|
||||
## Audit Results
|
||||
|
||||
- **sched_meet_host_selection stub confirmed** at `src/scheduler/src/resource.c:127-131`
|
||||
- Declaration: `src/scheduler/include/scheduler.h:111`
|
||||
- Call site: `src/scheduler/src/resource.c:368` (within sched_try_alloc_on_host_)
|
||||
- Stub body: returns 1 unconditionally
|
||||
|
||||
- **Allocator-thread-only invariant confirmed**: Single `while(1)` loop in `src/scheduler/src/main.c:119`
|
||||
- `sched_begin_session()` at line 121
|
||||
- `sched_go()` at line 122
|
||||
- `sched_end_session()` at line 161
|
||||
- All three calls executed sequentially in the same thread context
|
||||
|
||||
- **eval_select_simple signature confirmed**: `int eval_select_simple(const xce_select_node_t* root, const host_eval_ctx_t* host)`
|
||||
- Declaration: `src/include/resreq.h:110-111`
|
||||
- Definition: `src/lib/xce_select.c:558`
|
||||
- Returns: 1 (match), 0 (no match), negative (error)
|
||||
|
||||
No unexpected references to `sched_meet_host_selection`, `eval_select`, `alloc_thread`, or `sched_session` found in scheduler source.
|
||||
File diff suppressed because it is too large
Load diff
|
|
@ -1,233 +0,0 @@
|
|||
# Resreq AST + CSE Implementation Plan
|
||||
|
||||
> Generated from: `resreq-ast-cse-design.md`
|
||||
> Date: 2026-04-28
|
||||
> Branch: merge_api_server
|
||||
|
||||
---
|
||||
|
||||
## Summary Priority Order
|
||||
|
||||
```
|
||||
P0 (blocking): makeString() TOCTOU fix, section.peg boundary bug ✓ DONE
|
||||
Phase 1: TDD test skeletons (parallel with Phase 3) ✓ DONE
|
||||
Phase 2: Header definitions + ABI audit ✓ DONE
|
||||
Phase 3: PEG grammar (parallel with Phase 1) ✓ DONE 143 PASS / 3 FAIL (stubs)
|
||||
Phase 4a: Core interning + sel_optimize_level ✓ DONE 145 PASS / 1 FAIL (Phase 4b stub)
|
||||
Phase 4b: Type validation (gated on P0-1) ✓ DONE 203 PASS (2026-04-29)
|
||||
Phase 4c: AND/OR associativity flattening ✓ DONE 157 PASS (2026-04-29)
|
||||
Phase 4d: Const-fold transform ✓ DONE (2026-04-29)
|
||||
Phase 4e: Memoized eval ✓ DONE 73 PASS (2026-04-29)
|
||||
Phase 4f: GC + recompaction ✓ DONE 73 PASS (2026-04-29)
|
||||
Phase 5: Build wiring ✓ DONE
|
||||
Phase 6: Scheduler integration ✓ DONE 10 PASS (2026-04-30)
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## P0 — Critical Prerequisites (blocking gate)
|
||||
|
||||
Must complete before any Phase 4 work begins.
|
||||
|
||||
| # | Task | Section | Rationale |
|
||||
|---|------|---------|-----------|
|
||||
| P0-1 | ~~**Fix `makeString()` TOCTOU in `src/lib/xstring.c`**~~ **DONE** — extended lock region to cover `makeInstance` + `key_add`, eliminating the TOCTOU window. `key_add` can no longer return `-EEXIST` on a miss path. | §5.4 | Silent NULL return corrupts the interned AST in all §7 numeric-to-string coercions; blocks Phase 4b entirely |
|
||||
| P0-2 | ~~**Fix `section.peg` clause-boundary grammar**~~ **DONE** — Root cause: the original grammar used multiple ordered-choice alternatives in `select_statement` / `usage_statement` / `span_statement`, but PEG's `e*` always succeeds (matches empty), so alternatives 2+ were dead code (confirmed by packcc generating "defined but not used" for all `pcc_action_select_statement_N` functions). Fix: collapsed each statement rule into a single alternative with a compound negative lookahead covering all subsequent clause keywords. `select_statement` now stops at any of `rusage[`, `span[`, `same[`, `affinity[`, or `]`; `usage_statement` stops at `span[`, `same[`, `affinity[`, or `]`; `span_statement` stops at `same[`, `affinity[`, or `]`. Both implicit (`type==any span[ptile=1]`) and explicit (`select[type==any] span[ptile=1]`) select forms verified by TDD: `test_parse_section3` (implicit), `test_parse_section_explicit_select_with_span`, `test_parse_section_explicit_select_rusage_span`, `test_parse_section_span_not_in_select` — all PASS, no packcc dead-code warnings. | §3.1 | Silent span/rusage content bleed into selectString in all implicit-select + multi-clause inputs |
|
||||
|
||||
---
|
||||
|
||||
## Phase 1 — TDD Tests (write failing skeletons first) ✓ DONE
|
||||
|
||||
**Priority: HIGH — must come before implementation (TDD contract)**
|
||||
|
||||
Develop concurrently with Phase 3 (grammar is the test oracle).
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 1-1 | ~~Write `lib/testsuite/test_select.c` skeletons: parse valid expressions, unknown resource names (error), type mismatches (error)~~ **DONE** | |
|
||||
| 1-2 | ~~Write tests: `SELECT_MAX_INPUT_LEN` (8192) overflow, `SELECT_MAX_DEPTH` (64) nesting overflow~~ **DONE** | |
|
||||
| 1-3 | ~~Write CSE tests: `A&&B` and `B&&A` produce same pointer; two `makeSelectNode` calls with same args return same pointer~~ **DONE** — FAIL until Phase 4a | |
|
||||
| 1-4 | ~~Write eval tests: numeric `>/<`/`==`, string `==`, bool standalone, `!`/`&&`/`||`, short-circuit~~ **DONE** | |
|
||||
| 1-5 | ~~Write GC tests: unlink job root → `gc_sweep_select_nodes` frees unreachable node; subsequent `makeSelectNode` re-interns fresh node~~ **DONE** — FAIL until Phase 4f | |
|
||||
| 1-6 | ~~Write GC predicate contract tests (`sched_session_active`, `sched_eval_in_progress`, `staging_merge_complete`) using `fake_scheduler_t` scaffold in `test_select.c`~~ **DONE** — FAIL until Phase 4f | Phase 6 wires real scheduler — signatures stable |
|
||||
| 1-7 | ~~Create shared resmgr fixture: `src/lib/testsuite/test_resmgr_fixture.h` + `.c` with `make_test_resmgr()`/`free_test_resmgr()`~~ **DONE** | |
|
||||
|
||||
---
|
||||
|
||||
## Phase 2 — Header Definitions ✓ DONE
|
||||
|
||||
**Priority: HIGH — unblocks all implementation work**
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 2-1 | ~~Add `res_type_t`, `sel_node_type_t`, `xce_select_node_t` typedef + struct to `include/resreq.h`~~ **DONE** | |
|
||||
| 2-2 | ~~Fill `struct xce_resreq_select_` with `xce_select_node_t* root`~~ **DONE** | |
|
||||
| 2-3 | ~~Declare all public APIs: `makeSelectNode`, `eval_select`, `eval_select_simple`, `select_eval_ctx_*`, `sel_node_table_*`, `gc_sweep_select_nodes`, `recompact_node_ids`~~ **DONE** | `gc_mark_tree` stays `static` in `.c` |
|
||||
| 2-4 | ~~Add `var resmgr` field to `resreq_context_t` (outer PEG auxil)~~ **DONE** | |
|
||||
| 2-5 | ~~Add `valtype_to_res_type` helper; omit `res_type_to_valtype` (no current consumer — YAGNI)~~ **DONE** | |
|
||||
| 2-6 | ~~Add `xce_str_match` as `static inline` wrapping `strcmp`~~ **DONE** | |
|
||||
| 2-7 | ~~**CGO audit**: `grep -rn 'parse_resreq\|C\.parse_resreq' src/frontend/`~~ **DONE** — no CGO callers found | |
|
||||
| 2-8 | ~~Update `parse_resreq_primary` and `parse_resreq` signatures to accept `var resmgr`~~ **DONE** — call sites updated: `schedobj.c:171`, `test_resreq.c:244/301/330` | |
|
||||
|
||||
---
|
||||
|
||||
## Phase 3 — PEG Grammar (`lib/xce_select.peg`) ✓ DONE
|
||||
|
||||
**Priority: HIGH — develop concurrently with Phase 1**
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 3-1 | ~~Implement `select_expr` grammar (§3.1): `\|\|`, `&&`, `!`, comparisons, resname, literals, `any`~~ **DONE** | |
|
||||
| 3-2 | ~~Implement `select_context_t` auxil with `resmgr`, `depth`, `errmsg[256]`, `has_error`~~ **DONE** | |
|
||||
| 3-3 | ~~Implement `resname` action: `key_find`, `res_idx` validity guard, `valtype_to_res_type` mapping~~ **DONE** | |
|
||||
| 3-4 | ~~**Clause-keyword conflict check** in `resname` action: check against `{select, rusage, span, same, affinity}`~~ **DONE** | |
|
||||
| 3-5 | ~~Implement `SELECT_MAX_INPUT_LEN` (8192) and `SELECT_MAX_DEPTH` (64) guards~~ **DONE** | |
|
||||
| 3-6 | ~~LSF-compatible disambiguation: bare word vs known resource name → `SEL_RESNAME` vs `SEL_STR`; `any` excluded by grammar; `true`/`false` produce migration diagnostic~~ **DONE** | |
|
||||
| 3-R1 | ~~**[Review fix — HIGH]** Switch `SEL_STR` from `strdup`/`free` to `makeString`/`dereference` in `xce_select.peg` and `xce_select.c`~~ **DONE** — xstring-interned strings now refcounted; `kill_select_node` calls `dereference()` | |
|
||||
| 3-R2 | ~~**[Review fix — MEDIUM]** Add `_Static_assert(sizeof(uintptr_t) >= sizeof(double))` before `makeSelectNode` in `xce_select.c`~~ **DONE** — guards the `SEL_NUM` bit-cast at compile time | |
|
||||
| 3-R3 | ~~**[Review fix — MEDIUM]** Add `!("any" ![a-zA-Z_0-9])` negative lookahead to `resname_node` and `resname_leaf` in `xce_select.peg`~~ **DONE** — prevents `any` keyword from being tried as a resource name | |
|
||||
|
||||
---
|
||||
|
||||
## Phase 4a — Core Node Allocation and Interning ✓ DONE
|
||||
|
||||
**Priority: HIGH — foundational; unblocks 4b–4f**
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 4a-1 | ~~Implement `sel_optimize_level` (`static _Atomic int`, sentinel -1) with `sel_optimize_set()` write-once CAS and `sel_get_optimize_level()` accessor~~ **DONE** | |
|
||||
| 4a-2 | ~~Implement `sel_make_key()` — zero padding bytes via struct literal init; SEL_RESNAME zeroes `op2` for key purposes (res_type stored in node, not in key)~~ **DONE** | |
|
||||
| 4a-3 | ~~Implement `sel_key_hash` over the 3 key fields~~ **DONE** | |
|
||||
| 4a-4 | ~~Implement `sel_node_table_init` / `sel_node_table_fini` — `makeInstance(&kvtab_type, ¶ms)` with **`datakiller=NULL`**, `zarray_init` for linear nodes array~~ **DONE** | |
|
||||
| 4a-5 | ~~Implement `makeSelectNode` — CSE-disabled fast path; CSE-enabled path with commutativity sort, key lookup, TOCTOU double-check under lock, node_id assignment via `atomic_fetch_add`, zarray rollback via `atomic_fetch_sub`~~ **DONE** | |
|
||||
| 4a-6 | ~~Implement `sel_node_to_key()` for GC key reconstruction (must apply same commutativity sort as `makeSelectNode`)~~ **DONE** | |
|
||||
| 4a-7 | ~~Add `sel_optimize_test_reset()` test-only helper (`#ifdef UNIT_TEST`): `atomic_store_explicit` reset to -1 plus `sel_node_table_fini`~~ **DONE** | |
|
||||
| 4a-R1 | ~~**[Review fix — HIGH]** Move `node_id` assignment inside lock after `key_add` succeeds so TOCTOU losers need no counter rollback~~ **DONE** | Counter divergence under concurrent load |
|
||||
| 4a-R2 | ~~**[Review fix — HIGH]** Add `dereference((char*)op1)` on first-lock cache hit for SEL_STR — caller transfers xstring ownership~~ **DONE** | Ref leak on every cache hit for string nodes |
|
||||
| 4a-R3 | ~~**[Review fix — HIGH]** Add `SEL_MAX_INTERNED_NODES` cap check in both lock sections (first miss + TOCTOU re-check)~~ **DONE** | Unbounded table growth without cap |
|
||||
| 4a-R4 | ~~**[Review fix — HIGH]** Fix `sel_node_table_fini` to iterate and free all interned nodes before `destroy(table)` — NULL datakiller does not free them~~ **DONE** | All nodes leaked at teardown; LSAN clean now |
|
||||
| 4a-R5 | ~~**[Review fix — MEDIUM]** Set `node->hashcode = sel_key_hash_fn(&key)` in CSE path — was left 0, breaking Phase 4e memoization~~ **DONE** | |
|
||||
| 4a-R6 | ~~**[Review fix — MEDIUM]** Add `dereference((char*)op1)` on `calloc` failure in level==0 path~~ **DONE** | xstring ref leak on OOM |
|
||||
| 4a-R7 | ~~**[Review fix — MEDIUM]** Add `-EINVAL` guard on `key_find` — corrupt table must not silently proceed~~ **DONE** | |
|
||||
| 4a-R8 | ~~**[Review fix — MEDIUM]** Add `-EEXIST` recovery in `key_add` failure path — re-find and return the racing winner~~ **DONE** | Rare concurrent insert race |
|
||||
| 4a-R9 | ~~**[Review fix — LOW]** Change `_Static_assert(sizeof(uintptr_t) >= ...)` to `==` per spec requirement~~ **DONE** | |
|
||||
|
||||
---
|
||||
|
||||
## Phase 4b — Parse-Time Type Validation ✓ DONE (2026-04-29)
|
||||
|
||||
**Priority: HIGH — gate: P0-1 `makeString()` fix must be merged first**
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 4b-1 | ~~Wire `parse_select()` into `parse_resreq_primary` **before** `kill_resreq_data`~~ **DONE** | Use-after-free if called after `kill_resreq_data` frees `data.selectString` |
|
||||
| 4b-2 | ~~Implement EQ/NE type validation: reject BOOL×STR; coerce SEL_NUM to SEL_STR when comparing with string resource (`snprintf "%.17g"`), NULL-check `makeString` result, symmetric coercion~~ **DONE** | Free orphaned SEL_NUM node in unoptimized mode before overwriting pointer |
|
||||
| 4b-3 | ~~Implement `>/<`/`>=`/`<=` type validation: reject non-numeric operands~~ **DONE** | `log_fatal` (not `assert`) for belt-and-suspenders at eval time too |
|
||||
| 4b-4 | ~~Implement `res_idx` validity guard and unknown-resource error; add migration diagnostic for `true`/`false` identifiers (parse as SEL_RESNAME with no match)~~ **DONE** | |
|
||||
| 4b-5 | ~~Update `kill_primary_resreq` for conditional teardown: CSE-enabled → null root only; CSE-disabled → `free_select_tree()` then `FREEUP`~~ **DONE** | NULL store before `FREEUP` to prevent GC mark dangling-pointer race |
|
||||
| 4b-6 | ~~**LC_NUMERIC enforcement**: wrap PEG parser call in `parse_select` with `newlocale(LC_NUMERIC_MASK, "C")` / `uselocale` / `freelocale`; `log_fatal` on failure in scheduler, `log_error`+return in API server~~ **DONE** | `_XOPEN_SOURCE 700` required before system headers |
|
||||
| 4b-7 | ~~**strtod locale coverage check**~~ **DONE — SAFE**: packcc emits single `strtod` in `pcc_action_value_node_1` (line 1593 of generated file), inside the `uselocale` scope — no locale vulnerability | |
|
||||
| 4b-R1 | ~~**[Review fix — HIGH]** Add `lock(auxil->resmgr, RTTI_LOCK_READ)` / `unlock(auxil->resmgr)` around `key_find` in `resolve_resname()` (`xce_select.peg:38`) — missing read lock caused data race in multi-threaded scheduler~~ **DONE** | |
|
||||
| 4b-R2 | **[Review check — F2]** STRING×NUM coercion error paths reviewed: `makeSelectNode` releases the xstring on OOM failure (level-0 path calls `dereference(op1)` before returning NULL); outer `makeSelectNode(op_type)` failure correctly calls `SEL_DISCARD(l)` + `SEL_DISCARD(r)` — **no leak, false positive** | |
|
||||
|
||||
---
|
||||
|
||||
## Phase 4c — Commutativity + Associativity Flattening ✓ DONE (2026-04-29)
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 4c-1 | ~~Verify commutativity sort in `makeSelectNode` step 1 covers AND/OR/EQ/NE; non-commutative GT/LT/GE/LE/NOT unchanged~~ **DONE** — `test_cse_gt_not_commutative` confirms GT is not commutative; AND/OR/EQ/NE sort verified. | Already integrated in §5.4 design |
|
||||
| 4c-2 | ~~Implement post-parse **flatten pass** for `&&`/`||` chains: `do { changed = flatten_pass(root); } while (changed)`~~ **DONE** — `sel_flatten_pass` added to `xce_select.c`; do-while loop in `parse_select()` in `resreq.c`; 7 tests: noop, single-level, deep 3+, OR, mixed-ops, CSE mode. 157 PASS. | Repeat-until-stable needed for depth-3+ explicit parenthesization |
|
||||
| 4c-3 | ~~In unoptimized mode: `flatten_pass` must `free()` each orphaned node — not its children (re-parented children must not be freed)~~ **DONE** — level==0 path frees orphaned intermediate node (`free(new_right); free(node)`) after re-parenting A/B/C; SEL_NOT branch frees `node` only. | Children freed would cause double-free when `free_select_tree` walks from root |
|
||||
|
||||
**Senior review fixes applied (2026-04-29):**
|
||||
- **[MEDIUM — OOM safety]** In AND/OR branch: update `node->u.binop.left/right` in-place (level==0) immediately after recursive calls, before any `makeSelectNode`. This ensures `return node` after OOM returns a valid tree with live (not dangling) children.
|
||||
- **[LOW — dead code]** Removed dead `if (node->type == SEL_STR ...)` / `if (new_right->type == SEL_STR ...)` guards in AND/OR and SEL_NOT branches — those nodes are never SEL_STR in those arms.
|
||||
|
||||
---
|
||||
|
||||
## Phase 4d — Const-Fold Transform ✓ DONE (2026-04-29)
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 4d-1 | ~~`resname == any` → `SEL_TRUE`; `resname != any` → `SEL_FALSE` — check **both** child slots for SEL_ANY pointer (commutativity sort is ASLR-nondeterministic across runs)~~ **DONE** | |
|
||||
| 4d-2 | ~~Identity laws: `SEL_TRUE && X` → `X`; `SEL_FALSE \|\| X` → `X`~~ **DONE** | |
|
||||
| 4d-3 | ~~Annihilation laws: `SEL_FALSE && X` → `SEL_FALSE`; `SEL_TRUE \|\| X` → `SEL_TRUE`~~ **DONE** | |
|
||||
| 4d-4 | ~~Complement laws: `!SEL_TRUE` → `SEL_FALSE`; `!SEL_FALSE` → `SEL_TRUE`~~ **DONE** | Single-pass — const-fold shrinks trees, does not create new flattening opportunities |
|
||||
|
||||
---
|
||||
|
||||
## Phase 4e — Evaluation with Memoization ✓ DONE (2026-04-29)
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 4e-1 | ~~Implement `eval_get_bool`/`eval_set_bool` 2-bit bitmap accessors with `node_id >= bool_bits_cap` guards using `log_fatal`~~ **DONE** | `_Static_assert(EVAL_NONE == 0)` for memset-reset correctness |
|
||||
| 4e-2 | ~~Implement `eval_select_inner` — recursive eval with **explicit** error propagation (`if (lv < 0) return -1`), NOT C `&&`/`||` (C's `-1` is truthy, would swallow errors)~~ **DONE** | SEL_ANY reaching eval → `log_fatal("SEL_ANY reached eval — const-fold not applied")` |
|
||||
| 4e-3 | ~~Leaf cache writes in `eval_select_inner`: SEL_RESNAME writes to `ctx->num_vals[node_id]` or `ctx->str_vals[node_id]`; comparison ops call `eval_select(child, ctx)` then read cached arrays~~ **DONE** | **Critical**: `get_leaf_value()` must NOT be called directly — bypasses cache writes, silently reads 0.0/NULL |
|
||||
| 4e-4 | ~~Implement memoized `eval_select` wrapper — bitmap cache check before calling `eval_select_inner`; cache EVAL_ERROR too (node-structural errors repeat per host)~~ **DONE** | |
|
||||
| 4e-5 | ~~Implement `eval_select_simple` for unoptimized path — plain recursive tree-walk, no `select_eval_ctx_t`, thread-safe (no shared state)~~ **DONE** | |
|
||||
| 4e-6 | ~~Add `eval_select` guard: `log_fatal` if `sel_get_optimize_level() <= 0`~~ **DONE** | |
|
||||
| 4e-7 | ~~Implement `select_eval_ctx_init` / `select_eval_ctx_reset` / `select_eval_ctx_fini`~~ **DONE** | `node_count` snapshot at init; `reset` uses snapshot, never re-reads `sel_node_next_id` |
|
||||
| 4e-8 | ~~Pre-session capacity check: `memset` extended bool_bits bytes after realloc (realloc does not zero extension); partial-OOM contract — all three reallocs (bool_bits, num_vals, str_vals) must succeed atomically or `log_fatal`~~ **DONE** | Mismatched capacities allow out-of-bounds write to num_vals with no inline bounds check |
|
||||
|
||||
---
|
||||
|
||||
## Phase 4f — Mark-and-Sweep GC + Recompaction ✓ DONE (2026-04-29)
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 4f-1 | ~~Implement `gc_mark_tree` — recursive DFS, early return on `node == NULL \|\| node->gc_mark`; handles leaf / SEL_NOT / binary node types~~ **DONE** — `__attribute__((unused))` for Phase 6 wire-in | Recursion bounded by `SELECT_MAX_DEPTH` (64); any increase requires stack analysis |
|
||||
| 4f-2 | ~~Implement `gc_sweep_select_nodes` — Phase 6 guard placed **before** mark-clear (prevents two-call sweep-of-reachable-nodes bug); mark-clear iterates `caps`; sweep iterates `caps`, `zarray_del` before `free`~~ **DONE** — live_jobs traversal deferred to Phase 6 (sched/types.h not in libxce scope) | GC does NOT acquire `sel_node_table.lock` (idle-phase invariant) |
|
||||
| 4f-3 | Insert **`atomic_thread_fence(memory_order_seq_cst)`** after all `primary->select->root = NULL` unlinks and before `gc_sweep_select_nodes` call — **Phase 6 call-site task** | Without fence: CPU/compiler may reorder NULL store after GC mark pass |
|
||||
| 4f-4 | Runtime GC idle invariant guards at GC call site (`sched_session_active`, `sched_eval_in_progress`, `staging_merge_complete`) — `log_fatal` not `assert` — **Phase 6 call-site task** → see 6-3/6-4 | `assert` compiled out by NDEBUG; use-after-free would result |
|
||||
| 4f-5 | ~~Implement `recompact_node_ids` — iterate `nodes` zarray over `caps` slots (not `count`, gaps exist after `zarray_del`), assign sequential `node_id`, reset `sel_node_next_id` via `atomic_store_explicit(..., memory_order_release)`~~ **DONE** | `node_id` and zarray slot index are **decoupled** — do not conflate |
|
||||
| 4f-R1 | ~~**[Review fix — HIGH]** Move Phase 6 early-return guard to before mark-clear loop — prior ordering allowed two consecutive calls (njobs>0 then njobs==0) to sweep still-reachable nodes~~ **DONE** | Two-call scenario: first call clears all marks and returns; second call sweeps everything |
|
||||
| 4f-R2 | ~~**[Review fix — HIGH]** Swap `free(node)` / `zarray_del` order — `zarray_del` NULLs slot first, then `free`; eliminates latent use-after-free if killer is ever installed on nodes zarray~~ **DONE** | |
|
||||
| 4f-R3 | ~~**[Bug fix]** `sel_node_table_fini` iterated `nodes.count` instead of `nodes.caps` — after `zarray_del` creates gaps, new nodes slot into positions beyond `count` boundary and leaked~~ **DONE** | Manifested as 32-byte ASAN leak after GC sweep + reintern in same lifetime |
|
||||
|
||||
---
|
||||
|
||||
## Phase 5 — Build Wiring
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 5-1 | ~~Add `custom_target` for `xce_select.peg` in `lib/meson.build` matching `section_parser`/`rusage_parser` pattern~~ **DONE** | Verify `-o 'lib/xce_select'` path format: `grep -A8 'custom_target' src/lib/meson.build \| grep '\-o'` |
|
||||
| 5-2 | ~~Add `xce_select_parser[0]` (generated `.c`) to `libxce_src`~~ **DONE** | |
|
||||
| 5-3 | ~~Add `test_resmgr_fixture.c` to testsuite `executable()` sources in `lib/meson.build`~~ **DONE** | |
|
||||
|
||||
---
|
||||
|
||||
## Phase 6 — Scheduler Integration
|
||||
|
||||
**Status: ✓ DONE — 2026-04-30 — 10/10 scheduler tests pass**
|
||||
|
||||
| # | Task | Notes |
|
||||
|---|------|-------|
|
||||
| 6-1 | ~~**Pre-Phase-6 audit**: confirm allocator-thread-only invariant~~ **DONE** `bb4a95c` | Single-threaded confirmed; stub at `resource.c:127` |
|
||||
| 6-2 | ~~**Blocking gate**: all `sched_meet_host_selection` call sites accounted for~~ **DONE** `bb4a95c` | No missed call sites |
|
||||
| 6-3 | ~~Add `_Atomic int session_active`, `eval_in_progress`, `staging_merge_complete` to `xce_scheduler_t`~~ **DONE** `823a66e` | Use `_Atomic int` not `volatile int` — ARM atomicity guarantee |
|
||||
| 6-4 | ~~Implement GC predicate wrappers referencing real `xce_scheduler_t` fields~~ **DONE** `acda9bb` | Signatures unchanged; only wrapper body changed |
|
||||
| 6-5 | ~~Implement `sched_meet_host_selection` via `eval_select_simple`~~ **DONE** `40261b7` | Full implementation replacing stub that always returned 1 |
|
||||
| 6-6 | ~~`sched_meet_host_selection` returns int (0/1); preason on fail~~ **DONE** `40261b7` | int return kept; preason wired |
|
||||
| 6-7 | ~~Implement `build_host_eval_ctx`~~ **DONE** `907d8d8` | Flattens `worker->resvals` zarray to `numvals`/`strvals` using `Value_u_fvalue/svalue/bvalue` |
|
||||
| 6-8 | ~~Materialize `live_jobs` flat array from `sched->jobs` kvmgr~~ **DONE** `5267d36` | Growth-loop heap alloc; OOM-safe (double-guarded: `calloc` fail + `realloc` null-sentinel `742d79e`, `fb5127a`) |
|
||||
| 6-9 | ~~Wire `sel_optimize_set(1)` at scheduler startup~~ **DONE** `acda9bb` | Called in `sched_init`; no `sel_optimize_set(0)` needed (API server has separate process) |
|
||||
| 6-10 | ~~Set `sel_alloc_thread_id`; add thread-ID checks in `eval_select` and `gc_sweep_select_nodes`~~ **DONE** `b722e5c` `1eb4eb1` | `_Atomic int` registered flag + `pthread_equal` guard; registered before `while(1)` loop |
|
||||
| 6-11 | ~~Write integration/parity tests against known-good resreq/host pairs~~ **DONE** `5fcf85c` | Numeric (`SEL_GE`) and string (`SEL_EQ`) pass/fail cases |
|
||||
| 6-12 | ~~`atomic_thread_fence(memory_order_seq_cst)` before `gc_sweep_select_nodes`~~ **DONE** `5267d36` | After all `gc_mark_select_tree` calls in `sched_end_session` |
|
||||
| R1 | ~~**[Review fix — CRITICAL]** `staging_merge_complete` bootstrap deadlock — GC never ran~~ **DONE** `d438742` | `sched_begin_session` never set it to 1; guard in `sched_end_session` always fired |
|
||||
| R2 | ~~**[Review fix — HIGH]** `recompact_node_ids()` missing from GC protocol~~ **DONE** `d438742` | Added after `gc_sweep_select_nodes` in `sched_end_session` |
|
||||
|
||||
---
|
||||
|
||||
## Key Implementation Traps
|
||||
|
||||
1. **`free_select_tree` vs GC mode confusion** — calling `free_select_tree` in CSE-enabled mode or forgetting it in disabled mode both produce memory corruption. The `sel_get_optimize_level()` guard inside `free_select_tree` catches this with `log_fatal`.
|
||||
|
||||
2. **`get_leaf_value()` bypass bug** — calling `get_leaf_value()` directly in comparison branches bypasses the `num_vals`/`str_vals` cache write. Comparisons silently read 0.0/NULL (calloc default) instead of actual host values. Every comparison operator must call `eval_select(child, ctx)` first, then read the typed cache array.
|
||||
|
||||
3. **TOCTOU in `makeSelectNode`** — the double-check under lock (step 5) prevents node leaks when two threads race to intern the same node. The re-find must happen inside the lock or the winner could be freed before the loser reads it.
|
||||
|
||||
4. **`atomic_thread_fence` before GC** — root-unlink writes (`primary->select->root = NULL`) must be visible to the GC mark pass. Without a seq_cst fence, CPU/compiler reordering causes GC to retain a node that should be swept (or vice versa).
|
||||
|
||||
5. **Partial-OOM in `select_eval_ctx_t` resize** — if one of the three reallocs (bool_bits, num_vals, str_vals) succeeds and another fails, the capacities diverge and `eval_select_inner` can write out-of-bounds to `num_vals` (no inline bounds guard at write site). Treat all three as atomic — `log_fatal` on partial failure.
|
||||
|
|
@ -1,32 +1,58 @@
|
|||
Help of XCE
|
||||
===========
|
||||
|
||||
This is the entrypoint of xce tool. Currently, we have following sub-commands available:
|
||||
This is the entrypoint of xce tool. Currently, we have following sub-commands
|
||||
available:
|
||||
|
||||
- controller
|
||||
- worker
|
||||
- scheduler
|
||||
- apiserver
|
||||
- shell
|
||||
- **controller** — start xce controller daemon
|
||||
- **worker** — start xcecd worker daemon
|
||||
- **scheduler** — start xce scheduler daemon
|
||||
- **apiserver** — start xce REST API server
|
||||
- **shell** — interactive bash shell for troubleshooting
|
||||
- **xcecli** — CLI tool for cluster interaction (job submit, query, admin)
|
||||
|
||||
If you run xce without any parameter or with parameter which is not in above, then it will show this help screen
|
||||
If you run xce without any parameter or with a parameter not listed above, it
|
||||
will show this help screen.
|
||||
|
||||
## controller
|
||||
|
||||
This command with start xce controller with given parameters after controller sub-command
|
||||
Start the xce controller daemon with the given parameters after the
|
||||
`controller` sub-command.
|
||||
|
||||
Example:
|
||||
```
|
||||
xce controller --config /etc/xce/controller.yaml
|
||||
```
|
||||
|
||||
## scheduler
|
||||
|
||||
This command with start xce scheduler with given parameters after scheduler sub-command
|
||||
Start the xce scheduler daemon with the given parameters after the
|
||||
`scheduler` sub-command.
|
||||
|
||||
## worker
|
||||
|
||||
This command will start xcecd with given parameters after worker sub-command
|
||||
Start xcecd with the given parameters after the `worker` sub-command.
|
||||
|
||||
## apiserver
|
||||
|
||||
This command will start xce apiserver with given parameters after apiserver sub-command
|
||||
Start the xce REST API server with the given parameters after the
|
||||
`apiserver` sub-command. The API server provides HTTP endpoints for job
|
||||
submission, queue management, and cluster queries, backed by JWT
|
||||
authentication and RBAC authorization.
|
||||
|
||||
## xcecli
|
||||
|
||||
The xce CLI (`src/frontend/xcecli/`) provides command-line access to the
|
||||
cluster. Common operations:
|
||||
|
||||
- `xcecli job submit <jobspec>` — submit a job
|
||||
- `xcecli job list` — list jobs
|
||||
- `xcecli queue status` — show queue state
|
||||
|
||||
Built as a standalone Go binary; communicates with the API server over
|
||||
its REST interface.
|
||||
|
||||
## shell
|
||||
|
||||
An interactive bash shell to start, this is just for trouble shoot
|
||||
An interactive bash shell used for troubleshooting and inspection. Not
|
||||
intended for normal cluster operation.
|
||||
|
|
|
|||
|
|
@ -35,37 +35,14 @@ config/ YAML configs and JSON schemas
|
|||
|
||||
## Conventions
|
||||
|
||||
### Controllers
|
||||
- Struct: `type {Resource}Controller struct{}`
|
||||
- Handler signature: `func (c {Resource}Controller) {Action}{Resource}(ctx *gin.Context)`
|
||||
- Always return via helpers — never write response directly:
|
||||
```go
|
||||
ReturnSuccess(ctx, common.SUCCESS_CODE, common.SUCCESS_MSG, data)
|
||||
ReturnError(ctx, common.ERROR_CODE, err.Error(), nil)
|
||||
```
|
||||
See `.kiro/steering/api-standards.md` (handler pattern, middleware chain, error codes, response format), `.kiro/steering/error-handling.md` (ReturnError early-return, zap logging), `.kiro/steering/security.md` (RBAC, JWT, auth backends), and `.kiro/steering/testing.md` (TDD, table-driven tests, state isolation).
|
||||
|
||||
### Error Handling
|
||||
- Early return on every error — no deferred error accumulation
|
||||
- Log with zap fields before returning: `logger.Error("msg", zap.Error(err))`
|
||||
- Use `preCheck{Resource}Request()` helpers for input validation before business logic
|
||||
|
||||
### RBAC
|
||||
- Protect routes with `controllers.RequirePermission("action:domain")` middleware
|
||||
- Permission strings follow `action:scope` pattern (e.g., `job:view:self`, `queue:submit:all`)
|
||||
|
||||
### Logging
|
||||
- Import as: `logger "apiserver/utils"`
|
||||
- Always include context: `zap.String("key", val)`, `zap.Error(err)`
|
||||
|
||||
### Models
|
||||
- JSON tags use snake_case: `json:"field_name"`
|
||||
- Required fields: `binding:"required"`
|
||||
- YAML tags for config structs: `yaml:"field_name,omitempty"`
|
||||
|
||||
### Tests
|
||||
- Use `t.TempDir()` for file fixtures
|
||||
- Reset global state with a helper (e.g., `resetRBACState()` pattern in rbac_check_test.go)
|
||||
- Table-driven tests for filter/validation logic
|
||||
### Quick reference (from steering)
|
||||
- Handler: `preCheck*() → service call → ReturnSuccess/ReturnError; return`
|
||||
- Middleware chain: WhitelistURL → WhitelistIP → RateLimit → JWT → CORS → RBAC
|
||||
- RBAC: `RequirePermission("action:scope")` — permissions in `config/rbac.yaml`
|
||||
- Logging: `logger.Error("msg", zap.Error(err))` — never `fmt.Println`
|
||||
- Tests: co-located `_test.go`, `t.TempDir()`, global state reset via `t.Cleanup`
|
||||
|
||||
## Key Files
|
||||
- `controllers/response.go` — ReturnSuccess / ReturnError helpers
|
||||
|
|
|
|||
|
|
@ -112,6 +112,47 @@ extern int xce_replconfig_reply_ssl_decode(SSL *ssl,
|
|||
*/
|
||||
extern void xce_free_request(xce_request_t *req);
|
||||
|
||||
/*
|
||||
* xce_wal_batch_encode_raw / xce_wal_batch_req_raw: encode/send an
|
||||
* OPCode_walCatchupEntry batch carrying raw WAL payload bytes.
|
||||
*
|
||||
* Bypasses the generated encode path (payload :Data cannot be carried
|
||||
* through xce_data_t — same limitation as replConfigPush). Each
|
||||
* entries[i]->raw_payload/raw_payload_len is written as a capnp Data
|
||||
* (List(UInt8)); the legacy xce_data_t payload field is ignored.
|
||||
* The receiver's xce_decode() extracts the bytes back into
|
||||
* raw_payload/raw_payload_len on each decoded sub-entry.
|
||||
*
|
||||
* encode_raw fills pkt (caller frees via free_packet on error paths);
|
||||
* req_raw allocates the packet and enqueues it on ep (non-blocking).
|
||||
*/
|
||||
extern int xce_wal_batch_encode_raw(wal_catchup_entry_t **entries,
|
||||
int n_entries,
|
||||
xce_peerinfo_t *from,
|
||||
uint64_t seq,
|
||||
io_packet_t *pkt);
|
||||
extern int xce_wal_batch_req_raw(endpoint_t *ep,
|
||||
wal_catchup_entry_t **entries,
|
||||
int n_entries);
|
||||
|
||||
/*
|
||||
* xce_snapshot_chunk_encode_raw / xce_snapshot_chunk_req_raw: encode/send
|
||||
* an OPCode_snapshotTransfer chunk carrying raw snapshot file bytes.
|
||||
* Same raw-Data convention as xce_wal_batch_req_raw; chunk envelope
|
||||
* fields (snapshot_lsn, chunk_index, chunk_offset, total_size, is_last)
|
||||
* are taken from *chunk, payload bytes from payload/payload_len.
|
||||
*/
|
||||
extern int xce_snapshot_chunk_encode_raw(const snapshot_transfer_t *chunk,
|
||||
const uint8_t *payload,
|
||||
size_t payload_len,
|
||||
xce_peerinfo_t *from,
|
||||
uint64_t seq,
|
||||
io_packet_t *pkt);
|
||||
extern int xce_snapshot_chunk_req_raw(endpoint_t *ep,
|
||||
const snapshot_transfer_t *chunk,
|
||||
const uint8_t *payload,
|
||||
size_t payload_len);
|
||||
|
||||
/*
|
||||
* xce_upgrade_config_req_extract: extract sha256 + bytes from a decoded
|
||||
* OP_upgradeConfig request. xce_decode() stores malloc'd copies in
|
||||
|
|
|
|||
|
|
@ -1,5 +1,4 @@
|
|||
#include <errno.h>
|
||||
#include <signal.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
|
|
|
|||
|
|
@ -173,6 +173,7 @@ libxce_test_src = ['testsuite/test.c',
|
|||
'testsuite/test_libxce_retry.c',
|
||||
'testsuite/test_netio_on_connected.c',
|
||||
'testsuite/test_raft.c',
|
||||
'testsuite/test_wal_wire.c',
|
||||
'raft/raft_sim.c',
|
||||
]
|
||||
|
||||
|
|
|
|||
|
|
@ -4,6 +4,7 @@
|
|||
#include <netdb.h>
|
||||
#include <openssl/err.h>
|
||||
#include <openssl/ssl.h>
|
||||
#include <signal.h>
|
||||
#include <stdio.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
|
|
@ -949,6 +950,11 @@ SSL_CTX* create_ssl_context(char* cert_file, char* key_file)
|
|||
|
||||
int iomgr_init(iomgr_t* iomgr)
|
||||
{
|
||||
/* Ignore SIGPIPE — writing to a closed peer socket returns EPIPE
|
||||
* instead of killing the daemon. All daemons call iomgr_init before
|
||||
* any socket I/O, so this protects controller, worker, and scheduler. */
|
||||
signal(SIGPIPE, SIG_IGN);
|
||||
|
||||
mylock_init(&(iomgr->lock));
|
||||
iomgr->epoll_fd = epoll_create1(0);
|
||||
if (iomgr->epoll_fd < 0) {
|
||||
|
|
|
|||
|
|
@ -56,6 +56,35 @@ int xce_encode(xce_request_t* req, io_packet_t* pkt)
|
|||
return 1;
|
||||
}
|
||||
|
||||
/* extract_data_bytes: copy a capnp Data (List(UInt8)) field into a fresh
|
||||
* malloc'd buffer. Returns NULL (*out_len = 0) for absent/empty fields or
|
||||
* allocation failure. Used by xce_decode() to pre-extract raw payload
|
||||
* bytes before capn_free() destroys the segment. */
|
||||
static uint8_t *extract_data_bytes(Data_ptr dp, size_t *out_len)
|
||||
{
|
||||
*out_len = 0;
|
||||
|
||||
capn_list8 l;
|
||||
l.p = dp.p;
|
||||
if (l.p.type == CAPN_NULL)
|
||||
return NULL;
|
||||
|
||||
int len = capn_len(l);
|
||||
if (len <= 0)
|
||||
return NULL;
|
||||
|
||||
uint8_t *buf = (uint8_t *)malloc((size_t)len);
|
||||
if (!buf)
|
||||
return NULL;
|
||||
if (capn_getv8(l, 0, buf, len) != len) {
|
||||
free(buf);
|
||||
return NULL;
|
||||
}
|
||||
|
||||
*out_len = (size_t)len;
|
||||
return buf;
|
||||
}
|
||||
|
||||
int xce_decode(io_packet_t* pkt, xce_request_t* req)
|
||||
{
|
||||
struct capn c;
|
||||
|
|
@ -158,6 +187,47 @@ int xce_decode(io_packet_t* pkt, xce_request_t* req)
|
|||
}
|
||||
}
|
||||
|
||||
/* OPCode_walCatchupEntry: payload :Data per (sub-)entry — pre-extract
|
||||
* raw bytes into raw_payload/raw_payload_len (replConfigPush pattern).
|
||||
* The wire entries list order matches the decoded entries[] order. */
|
||||
if (rq.opcode == OPCode_walCatchupEntry
|
||||
&& req->body_which == Request_body_walcatchupentrymsg
|
||||
&& req->body.walcatchupentrymsg != NULL) {
|
||||
|
||||
wal_catchup_entry_t *batch = req->body.walcatchupentrymsg;
|
||||
WalCatchupEntry_ptr b_ptr = rq.body.walcatchupentrymsg;
|
||||
|
||||
/* Top-level payload (single-entry mode) */
|
||||
batch->raw_payload =
|
||||
extract_data_bytes(WalCatchupEntry_get_payload(b_ptr),
|
||||
&batch->raw_payload_len);
|
||||
|
||||
/* Batch sub-entries */
|
||||
WalCatchupEntry_list wlist = WalCatchupEntry_get_entries(b_ptr);
|
||||
int wn = (wlist.p.type != CAPN_NULL) ? capn_len(wlist) : 0;
|
||||
for (int i = 0; i < wn && i < batch->n_entries; i++) {
|
||||
if (!batch->entries || !batch->entries[i])
|
||||
continue;
|
||||
struct WalCatchupEntry we;
|
||||
get_WalCatchupEntry(&we, wlist, i);
|
||||
batch->entries[i]->raw_payload =
|
||||
extract_data_bytes(we.payload,
|
||||
&batch->entries[i]->raw_payload_len);
|
||||
}
|
||||
}
|
||||
|
||||
/* OPCode_snapshotTransfer: chunk payload :Data — pre-extract raw bytes. */
|
||||
if (rq.opcode == OPCode_snapshotTransfer
|
||||
&& req->body_which == Request_body_snapshottransfermsg
|
||||
&& req->body.snapshottransfermsg != NULL) {
|
||||
|
||||
snapshot_transfer_t *chunk = req->body.snapshottransfermsg;
|
||||
SnapshotTransfer_ptr s_ptr = rq.body.snapshottransfermsg;
|
||||
chunk->raw_payload =
|
||||
extract_data_bytes(SnapshotTransfer_get_payload(s_ptr),
|
||||
&chunk->raw_payload_len);
|
||||
}
|
||||
|
||||
TRACE("%s request op %d seq %ld from %s", __func__, req->opcode, req->seq,
|
||||
REQUEST_FROM(req));
|
||||
capn_free(&c);
|
||||
|
|
@ -210,6 +280,29 @@ void xce_free_request(xce_request_t *req)
|
|||
rcpr->raw_bytes_len = 0;
|
||||
}
|
||||
|
||||
if (req->body_which == Request_body_walcatchupentrymsg
|
||||
&& req->body.walcatchupentrymsg != NULL) {
|
||||
wal_catchup_entry_t *batch = req->body.walcatchupentrymsg;
|
||||
free(batch->raw_payload);
|
||||
batch->raw_payload = NULL;
|
||||
batch->raw_payload_len = 0;
|
||||
for (int i = 0; i < batch->n_entries; i++) {
|
||||
if (!batch->entries || !batch->entries[i])
|
||||
continue;
|
||||
free(batch->entries[i]->raw_payload);
|
||||
batch->entries[i]->raw_payload = NULL;
|
||||
batch->entries[i]->raw_payload_len = 0;
|
||||
}
|
||||
}
|
||||
|
||||
if (req->body_which == Request_body_snapshottransfermsg
|
||||
&& req->body.snapshottransfermsg != NULL) {
|
||||
snapshot_transfer_t *chunk = req->body.snapshottransfermsg;
|
||||
free(chunk->raw_payload);
|
||||
chunk->raw_payload = NULL;
|
||||
chunk->raw_payload_len = 0;
|
||||
}
|
||||
|
||||
free_Request(req);
|
||||
}
|
||||
|
||||
|
|
@ -356,6 +449,197 @@ int xce_replconfigpush_req_raw(endpoint_t *ep, xce_peerinfo_t *from,
|
|||
return 0;
|
||||
}
|
||||
|
||||
/* wal_entry_to_wire: fill a wire WalCatchupEntry from a C entry's header
|
||||
* fields and raw payload bytes. Sub-entry nesting is one level deep —
|
||||
* elements written here never carry their own entries list. */
|
||||
static void wal_entry_to_wire(struct capn_segment *cs,
|
||||
struct WalCatchupEntry *w,
|
||||
const wal_catchup_entry_t *e)
|
||||
{
|
||||
memset(w, 0, sizeof(*w));
|
||||
w->lsn = e->lsn;
|
||||
w->entryType = e->entry_type;
|
||||
if (e->raw_payload && e->raw_payload_len > 0) {
|
||||
capn_list8 pl = capn_new_list8(cs, (int)e->raw_payload_len);
|
||||
capn_setv8(pl, 0, e->raw_payload, (int)e->raw_payload_len);
|
||||
w->payload.p = pl.p;
|
||||
}
|
||||
}
|
||||
|
||||
/*
|
||||
* xce_wal_batch_encode_raw: encode an OPCode_walCatchupEntry batch into pkt.
|
||||
*
|
||||
* Bypasses encode_WalCatchupEntry (whose payload field routes through
|
||||
* xce_data_t — cannot carry raw bytes; same limitation as replConfigPush).
|
||||
* Builds the Request manually: a top-level WalCatchupEntry container
|
||||
* (lsn=0, entryType=0) whose entries list holds one element per batch
|
||||
* entry, each with lsn/entryType and payload written as capnp Data from
|
||||
* raw_payload/raw_payload_len.
|
||||
*
|
||||
* pkt is caller-allocated; on success it holds the serialized message
|
||||
* (caller owns; enqueue or free with free_packet). Returns 0 or -errno.
|
||||
*/
|
||||
int xce_wal_batch_encode_raw(wal_catchup_entry_t **entries, int n_entries,
|
||||
xce_peerinfo_t *from, uint64_t seq,
|
||||
io_packet_t *pkt)
|
||||
{
|
||||
struct capn c;
|
||||
struct capn_segment *cs;
|
||||
struct Request rq;
|
||||
struct WalCatchupEntry top;
|
||||
WalCatchupEntry_ptr tp;
|
||||
Request_ptr rp;
|
||||
int rc;
|
||||
|
||||
if (pkt == NULL || n_entries <= 0 || entries == NULL)
|
||||
return -EINVAL;
|
||||
|
||||
capn_init_malloc(&c);
|
||||
cs = capn_root(&c).seg;
|
||||
|
||||
memset(&rq, 0, sizeof(rq));
|
||||
memset(&top, 0, sizeof(top));
|
||||
|
||||
/* Batch container: header fields zero, entries list carries the batch */
|
||||
WalCatchupEntry_list lst = new_WalCatchupEntry_list(cs, n_entries);
|
||||
for (int i = 0; i < n_entries; i++) {
|
||||
struct WalCatchupEntry w;
|
||||
wal_entry_to_wire(cs, &w, entries[i]);
|
||||
set_WalCatchupEntry(&w, lst, i);
|
||||
}
|
||||
top.entries = lst;
|
||||
|
||||
tp = new_WalCatchupEntry(cs);
|
||||
write_WalCatchupEntry(&top, tp);
|
||||
|
||||
rq.opcode = OPCode_walCatchupEntry;
|
||||
rq.seq = seq;
|
||||
rq.body_which = Request_body_walcatchupentrymsg;
|
||||
rq.body.walcatchupentrymsg = tp;
|
||||
encode_PeerInfo_ptr(cs, &rq.from, from);
|
||||
|
||||
rp = new_Request(cs);
|
||||
write_Request(&rq, rp);
|
||||
capn_setp(capn_root(&c), 0, rp.p);
|
||||
|
||||
pkt->sz = capn_size(&c);
|
||||
rc = alloc_packet(pkt);
|
||||
if (rc < 0) {
|
||||
capn_free(&c);
|
||||
return -errno;
|
||||
}
|
||||
|
||||
capn_write_mem(&c, (uint8_t *)pkt->data, pkt->sz, 0);
|
||||
capn_free(&c);
|
||||
return 0;
|
||||
}
|
||||
|
||||
int xce_wal_batch_req_raw(endpoint_t *ep, wal_catchup_entry_t **entries,
|
||||
int n_entries)
|
||||
{
|
||||
if (ep == NULL)
|
||||
return -EINVAL;
|
||||
|
||||
io_packet_t *pkt = (io_packet_t *)calloc(1, sizeof(*pkt));
|
||||
if (!pkt)
|
||||
return -ENOMEM;
|
||||
|
||||
int rc = xce_wal_batch_encode_raw(entries, n_entries, ep->peer,
|
||||
endpoint_getseq(ep), pkt);
|
||||
if (rc < 0) {
|
||||
free(pkt);
|
||||
return rc;
|
||||
}
|
||||
|
||||
endpoint_enq(ep, &(pkt->link));
|
||||
return 0;
|
||||
}
|
||||
|
||||
/*
|
||||
* xce_snapshot_chunk_encode_raw: encode an OPCode_snapshotTransfer chunk
|
||||
* into pkt. Envelope fields from *chunk; payload bytes written as capnp
|
||||
* Data from payload/payload_len (the legacy xce_data_t payload field is
|
||||
* ignored). pkt is caller-allocated. Returns 0 or -errno.
|
||||
*/
|
||||
int xce_snapshot_chunk_encode_raw(const snapshot_transfer_t *chunk,
|
||||
const uint8_t *payload, size_t payload_len,
|
||||
xce_peerinfo_t *from, uint64_t seq,
|
||||
io_packet_t *pkt)
|
||||
{
|
||||
struct capn c;
|
||||
struct capn_segment *cs;
|
||||
struct Request rq;
|
||||
struct SnapshotTransfer st;
|
||||
SnapshotTransfer_ptr sp;
|
||||
Request_ptr rp;
|
||||
int rc;
|
||||
|
||||
if (pkt == NULL || chunk == NULL)
|
||||
return -EINVAL;
|
||||
|
||||
capn_init_malloc(&c);
|
||||
cs = capn_root(&c).seg;
|
||||
|
||||
memset(&rq, 0, sizeof(rq));
|
||||
memset(&st, 0, sizeof(st));
|
||||
|
||||
st.snapshotLsn = chunk->snapshot_lsn;
|
||||
st.chunkIndex = chunk->chunk_index;
|
||||
st.chunkOffset = chunk->chunk_offset;
|
||||
st.totalSize = chunk->total_size;
|
||||
st.isLast = chunk->is_last ? 1 : 0;
|
||||
if (payload && payload_len > 0) {
|
||||
capn_list8 pl = capn_new_list8(cs, (int)payload_len);
|
||||
capn_setv8(pl, 0, payload, (int)payload_len);
|
||||
st.payload.p = pl.p;
|
||||
}
|
||||
|
||||
sp = new_SnapshotTransfer(cs);
|
||||
write_SnapshotTransfer(&st, sp);
|
||||
|
||||
rq.opcode = OPCode_snapshotTransfer;
|
||||
rq.seq = seq;
|
||||
rq.body_which = Request_body_snapshottransfermsg;
|
||||
rq.body.snapshottransfermsg = sp;
|
||||
encode_PeerInfo_ptr(cs, &rq.from, from);
|
||||
|
||||
rp = new_Request(cs);
|
||||
write_Request(&rq, rp);
|
||||
capn_setp(capn_root(&c), 0, rp.p);
|
||||
|
||||
pkt->sz = capn_size(&c);
|
||||
rc = alloc_packet(pkt);
|
||||
if (rc < 0) {
|
||||
capn_free(&c);
|
||||
return -errno;
|
||||
}
|
||||
|
||||
capn_write_mem(&c, (uint8_t *)pkt->data, pkt->sz, 0);
|
||||
capn_free(&c);
|
||||
return 0;
|
||||
}
|
||||
|
||||
int xce_snapshot_chunk_req_raw(endpoint_t *ep, const snapshot_transfer_t *chunk,
|
||||
const uint8_t *payload, size_t payload_len)
|
||||
{
|
||||
if (ep == NULL)
|
||||
return -EINVAL;
|
||||
|
||||
io_packet_t *pkt = (io_packet_t *)calloc(1, sizeof(*pkt));
|
||||
if (!pkt)
|
||||
return -ENOMEM;
|
||||
|
||||
int rc = xce_snapshot_chunk_encode_raw(chunk, payload, payload_len,
|
||||
ep->peer, endpoint_getseq(ep), pkt);
|
||||
if (rc < 0) {
|
||||
free(pkt);
|
||||
return rc;
|
||||
}
|
||||
|
||||
endpoint_enq(ep, &(pkt->link));
|
||||
return 0;
|
||||
}
|
||||
|
||||
/*
|
||||
* xce_replconfig_reply_raw: primary sends OP_replConfig reply with real
|
||||
* config bytes via an endpoint (uses iomgr queueing).
|
||||
|
|
|
|||
|
|
@ -37,6 +37,7 @@ extern void test_cluster_parse(void);
|
|||
extern void test_libxce_retry(void);
|
||||
extern void test_netio_on_connected(void);
|
||||
extern void test_raft(void);
|
||||
extern void test_wal_wire(void);
|
||||
|
||||
void setUp(void) { return; }
|
||||
|
||||
|
|
@ -143,6 +144,7 @@ int main(int argc, char* argv[])
|
|||
TEST(test_libxce_retry);
|
||||
TEST(test_netio_on_connected);
|
||||
TEST(test_raft);
|
||||
TEST(test_wal_wire);
|
||||
|
||||
return UNITY_END();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -771,10 +771,25 @@ int sched_update(xce_scheduler_t* sched)
|
|||
int i;
|
||||
for (i = 0; i < req->u.resourceupdate.nresources; i++) {
|
||||
xce_resource_t* r = req->u.resourceupdate.resources[i];
|
||||
if (r == NULL) {
|
||||
continue;
|
||||
if (r == NULL) continue;
|
||||
xce_resource_t* existing = makeResource(sched->resmgr, r);
|
||||
if (existing == NULL) {
|
||||
/* EEXIST: update-in-place on existing resource (add-or-update) */
|
||||
xce_resource_t* found = NULL;
|
||||
if (key_find(sched->resmgr, r->name, (void**)&found) == 0
|
||||
&& found != NULL) {
|
||||
/* Copy mutable integer fields; desc/name are identity,
|
||||
* distributions/hbased_defined are deep-structs. */
|
||||
found->valtype = r->valtype;
|
||||
found->ordtype = r->ordtype;
|
||||
found->flags = r->flags;
|
||||
found->interval = r->interval;
|
||||
WARN("%s updated existing resource %s", __func__, r->name);
|
||||
} else {
|
||||
WARN("%s makeResource failed for %s and lookup not found",
|
||||
__func__, r->name);
|
||||
}
|
||||
}
|
||||
makeResource(sched->resmgr, r);
|
||||
}
|
||||
rc = 0;
|
||||
break;
|
||||
|
|
@ -804,14 +819,16 @@ int sched_update(xce_scheduler_t* sched)
|
|||
for (i = 0; i < resvals->count; i++) {
|
||||
xce_resval_t* rv = resvals->vals[i];
|
||||
xce_sched_worker_t* w = NULL;
|
||||
if (rv == NULL) {
|
||||
continue;
|
||||
}
|
||||
xce_resource_t* r = NULL;
|
||||
if (rv == NULL) continue;
|
||||
w = sched_get_worker(sched, rv->host);
|
||||
if (w == NULL) {
|
||||
if (w == NULL) continue;
|
||||
if (key_find(sched->resmgr, rv->name, (void**)&r) < 0 || r == NULL) {
|
||||
WARN("%s unknown resource %s in per-worker resvals update",
|
||||
__func__, rv->name);
|
||||
continue;
|
||||
}
|
||||
zarray_put(&w->resvals, 0, rv->val);
|
||||
zarray_put(&w->resvals, r->idx, rv->val);
|
||||
}
|
||||
rc = 0;
|
||||
break;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue