claude skills
This commit is contained in:
parent
94bf5d94b8
commit
9bbff7b016
40 changed files with 4072 additions and 34 deletions
252
src/docs/config-replication-gap-analysis.md
Normal file
252
src/docs/config-replication-gap-analysis.md
Normal file
|
|
@ -0,0 +1,252 @@
|
|||
# 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`
|
||||
Loading…
Add table
Add a link
Reference in a new issue