feat(controller): clusterconfig-schema-split task 2.1 — cluster_open_listener two-tier port fallback
Replaces the cluster_config->port fallback (dead after task 1.2) with: --peers candidate match (unchanged, including single self-referencing entries for a custom single-node port) -> hardcoded XCE_CONTROLLER_DEFAULT_PORT (9990) when --peers was never passed at all. The --peers-given-but-local- name-absent case still returns -EINVAL and refuses to start, unchanged. Also folds in task 1.3's discovery (HashZArrayConfig/ResmgrConfig capnp structs left orphaned by tasks 1.1/1.2, breaking codegen) into requirements.md (new Req 2.5)/design.md/research.md/tasks.md, and fixes a design.md citation error (validate_peer_count -> xce_parse_peers) found during task 2.1's review.
This commit is contained in:
parent
0205ac3cd2
commit
1229f9a945
5 changed files with 53 additions and 13 deletions
|
|
@ -99,7 +99,7 @@ graph TB
|
|||
## File Structure Plan
|
||||
|
||||
### Modified Files
|
||||
- `src/schema/controller.capnp` — remove `port` (@1), `candidates` (@2), `vcpus` (@7), `jobmgr` (@13), `workermgr` (@14), `qmgr` (@15), `resmgr` (@16), `xstrings` (@17) from `ClusterConfig`; remove the `MasterCandidate` struct entirely. Cap'n Proto requires gapless ordinals, so the 13 retained fields are renumbered to a contiguous `0..12` range as a direct consequence — see `research.md`'s ordinal-gap finding (Req 1.1, 1.2, 1.6, 2.1, 3.1).
|
||||
- `src/schema/controller.capnp` — remove `port` (@1), `candidates` (@2), `vcpus` (@7), `jobmgr` (@13), `workermgr` (@14), `qmgr` (@15), `resmgr` (@16), `xstrings` (@17) from `ClusterConfig`; remove the `MasterCandidate` struct entirely; remove the now-orphaned `HashZArrayConfig`/`ResmgrConfig` structs (used only by the removed fields — discovered during `/kiro-impl` task 2.1, their `$C.mapname` bindings broke the capnp-generated code once task 1.2 removed the C typedefs they referenced). Cap'n Proto requires gapless ordinals, so the 13 retained fields are renumbered to a contiguous `0..12` range as a direct consequence — see `research.md`'s ordinal-gap finding (Req 1.1, 1.2, 1.6, 2.1, 2.5, 3.1).
|
||||
- `ha/data/cluster.bin` (binary fixture, not source) — regenerated in place against the renumbered schema, preserving the retained fields' semantic values from the pre-spec blob (Req 3.2, 5.3).
|
||||
- `src/include/clusterconfig.h` — remove the matching `cluster_config_t` members: `port`, `ncandidates`, `candidates`/`master_candidate_t**`, `vcpus`, `jobmgr`, `workermgr`, `qmgr`, `resmgr`, `xstrings` (Req 1.5, 2.4).
|
||||
- `src/controller/cluster.c` — (a) remove the `candidates`/`ncandidates` KV-mirror loop in `cluster_build_config` (Req 1.4); (b) change `cluster_open_listener`'s fallback branch from reading `cluster_config->port` to a hardcoded default constant, reached only when `--peers` was never passed at all — the `--peers` candidate-match branch itself is unchanged (Req 1.3, 1.7); `init_controller_tunables()` itself is unchanged (Req 2.2).
|
||||
|
|
@ -150,7 +150,7 @@ flowchart TD
|
|||
|
||||
**Key decisions** (corrected from the initial draft during `/kiro-validate-design`; the `--port` CLI flag was further revised away in favor of the mechanism below during `/kiro-impl` kickoff — see `research.md`'s "Design Revision" section):
|
||||
- **Fallback trigger is port value, not just match presence.** A matched candidate whose port field is `0` (a malformed `--peers` entry — already WARNed about at `main.c:289-291`) falls through to the hardcoded-default tier exactly like today's `if (my_port == 0)` check, rather than binding to literal port 0. This preserves Requirement 1.3's "exactly as it does today" for the already-tolerated malformed-entry case.
|
||||
- **Error path is preserved, but only for one specific case.** `candidates_count == 0` can only happen when `--peers` was never passed at all (`validate_peer_count` already rejects even counts including 0 for a passed-but-empty `--peers`) — that's genuine single-node mode, and gets the hardcoded-default fallback per Requirement 1.7. An operator who wants a non-default port in this mode passes a single self-referencing `--peers <name>=<host>:<port>` entry instead — an already-existing, already-supported invocation (this resolves via the unchanged `NameFound`/`PortValid` branches above, not a new mechanism). But when `--peers` WAS passed and the local name simply isn't in it, that's a likely operator misconfiguration (own name typo'd or omitted) — this spec keeps today's `-EINVAL`/refuse-to-start behavior for that case rather than silently defaulting, per the project's fail-loud convention (`src/controller/CLAUDE.md` process-lifecycle invariant). Decided interactively during design review rather than defaulting to "simplest single code path."
|
||||
- **Error path is preserved, but only for one specific case.** `candidates_count == 0` can only happen when `--peers` was never passed at all — a passed-but-empty or malformed `--peers` string is rejected earlier by `xce_parse_peers` itself (`src/lib/cluster.c:58` returns `-1` unless `candidates_count > 0`), which `main.c` treats as a startup error before `cluster_open_listener` is ever reached (corrected citation, 2026-07-20: this guarantee comes from `xce_parse_peers`'s contract, not `validate_peer_count`, which only rejects even non-zero counts) — that's genuine single-node mode, and gets the hardcoded-default fallback per Requirement 1.7. An operator who wants a non-default port in this mode passes a single self-referencing `--peers <name>=<host>:<port>` entry instead — an already-existing, already-supported invocation (this resolves via the unchanged `NameFound`/`PortValid` branches above, not a new mechanism). But when `--peers` WAS passed and the local name simply isn't in it, that's a likely operator misconfiguration (own name typo'd or omitted) — this spec keeps today's `-EINVAL`/refuse-to-start behavior for that case rather than silently defaulting, per the project's fail-loud convention (`src/controller/CLAUDE.md` process-lifecycle invariant). Decided interactively during design review rather than defaulting to "simplest single code path."
|
||||
|
||||
## Requirements Traceability
|
||||
|
||||
|
|
@ -164,6 +164,7 @@ flowchart TD
|
|||
| 1.6 | Remove `MasterCandidate` struct | Schema Component | `controller.capnp` |
|
||||
| 1.7 | Replacement port-sourcing mechanism (existing `--peers` self-reference + hardcoded default) | Listener/Port-Resolution Component | `cluster.c`, `myconf.c`, `config_repl.c` |
|
||||
| 2.1 | Remove 6 perf-tuning fields from schema | Schema Component | `controller.capnp` |
|
||||
| 2.5 | Remove orphaned `HashZArrayConfig`/`ResmgrConfig` structs | Schema Component | `controller.capnp` |
|
||||
| 2.2 | Hardcoded tunables unchanged | Tunables Component | `cluster.c` (no change) |
|
||||
| 2.3 | Controller stops attempting to read removed tunable fields | Tunables Component | `cluster.c` (implicit — field no longer exists) |
|
||||
| 2.4 | `cluster_config_t` drops tunable members | C Struct Component | `clusterconfig.h` |
|
||||
|
|
|
|||
|
|
@ -61,6 +61,7 @@ This closes an assumption flagged in `.kiro/specs/admin-config-tool/requirements
|
|||
2. While these fields are absent from the schema, the controller shall continue to use its existing hardcoded tunable values (`c->tunables.*` defaults in `cluster.c`) with no behavior change from before this spec.
|
||||
3. When the controller starts, the system shall not attempt to read `vcpus`, `jobmgr`, `workermgr`, `qmgr`, `resmgr`, or `xstrings` from `cluster_config_t` (the fields no longer exist).
|
||||
4. The `cluster_config_t` C struct (`src/include/clusterconfig.h`) shall not contain `vcpus`, `jobmgr`, `workermgr`, `qmgr`, `resmgr`, or `xstrings` members, matching the capnp struct.
|
||||
5. The `HashZArrayConfig` and `ResmgrConfig` capnp structs shall be removed from `controller.capnp` (discovered during `/kiro-impl` task 2.1, 2026-07-20: verified via grep that these structs are referenced only by the removed `jobmgr`/`workermgr`/`qmgr`/`xstrings` fields and the removed `resmgr` field respectively, nowhere else in the schema — their removal is a direct consequence of Criterion 2.1's field removal, the same class of cleanup as Requirement 1.6's `MasterCandidate` removal. Left in place, their `$C.mapname(...)` bindings reference C typedefs that Requirement 2.4's struct-member removal deletes, breaking the capnp-generated bindings' compile).
|
||||
|
||||
### Requirement 3: Preserve Cluster-Shared Fields Unchanged
|
||||
|
||||
|
|
|
|||
|
|
@ -80,3 +80,11 @@ Neither `requirements.md` nor `design.md` had anticipated this; both were writte
|
|||
- **Decision (user, interactive)**: add a task to regenerate `ha/data/cluster.bin` — decode its current retained-field values under the pre-spec schema, re-encode under the new schema with a small throwaway tool (written and discarded, not shipped), overwrite the fixture in place. New Requirement 5.3; old 5.3 (3-node soak) → 5.4, old 5.4 (single-node) → 5.5. `tasks.md` gained a new task 5.1 (regenerate fixture) ahead of the renumbered 3-node-soak task.
|
||||
- **Rationale**: given the renumbering choice, the fixture WILL be silently wrong if left untouched — this is not optional cleanup, it's a repo-wide landmine for every HA spec's live verification, self-inflicted by this spec's own schema edit. Regenerating it once, in-scope, is far cheaper than every future spec independently rediscovering "why is deploy-ha producing garbage config."
|
||||
- **Scope boundary held**: the throwaway encoding tool is explicitly NOT a permanent addition — building a real `cluster.bin` authoring tool is `admin-config-tool`'s job (`xceadmin config build`), not this spec's. This task only needs to reproduce ONE known blob's values under the new layout, not build general tooling.
|
||||
|
||||
## Design Revision 3: Orphaned HashZArrayConfig/ResmgrConfig Structs (`/kiro-impl` task 2.1, 2026-07-20)
|
||||
|
||||
The task 1.1 implementer deliberately left `HashZArrayConfig` and `ResmgrConfig` (the capnp struct types used by the now-removed `jobmgr`/`workermgr`/`qmgr`/`xstrings`/`resmgr` fields) in `controller.capnp`, correctly noting no requirement or task text asked for their removal — they compile fine as unreferenced-but-valid struct definitions on their own. Task 1.2 then removed their corresponding hand-written C typedefs (`hash_zarray_config_t`, `resmgr_config_t`) from `clusterconfig.h`, since nothing in `cluster_config_t` referenced them anymore. The task 2.1 implementer's partial syntax check surfaced the resulting break: capnp's C codegen still emits `HashZArrayConfig`/`ResmgrConfig` encode/decode prototypes referencing `hash_zarray_config_t`/`resmgr_config_t` via their `$C.mapname(...)` bindings — types that no longer exist anywhere in the tree.
|
||||
|
||||
- **Root cause**: this is the same class of gap Requirement 1.6 already closed for `MasterCandidate` (used only by the removed `candidates` field) — just missed for the tunables side during the original requirements pass, since `MasterCandidate` was called out explicitly by name while the tunables' supporting struct types were not.
|
||||
- **Decision**: extend the same treatment — remove `HashZArrayConfig`/`ResmgrConfig` from `controller.capnp`. Verified via grep (same method as the original `MasterCandidate` verification) that neither struct is referenced anywhere else in the schema. New Requirement 2.5 added (mirrors 1.6's phrasing); design.md's File Structure Plan and Requirements Traceability updated; `tasks.md` gained task 1.3 (Schema Component, same boundary as 1.1) to close this before task 4's full build.
|
||||
- **Why this wasn't caught in the requirements-phase discovery pass**: the original `cluster-config-split-analysis.md` and this spec's requirements-phase grep swept for the FIELDS being removed, not for supporting/nested struct TYPES those fields' capnp types depended on. A useful pattern for future field-removal specs: after identifying fields to remove, also check whether any of their types are struct types used nowhere else in the schema.
|
||||
|
|
|
|||
|
|
@ -18,8 +18,17 @@
|
|||
- _Depends: 1.1_
|
||||
- _Boundary: C Struct Component_
|
||||
|
||||
- [ ] 1.3 Remove the orphaned `HashZArrayConfig`/`ResmgrConfig` capnp structs
|
||||
- Discovered during task 2.1's partial compile check: these structs (used only by the fields task 1.1 already removed) were deliberately left in `controller.capnp`, but task 1.2 removed their corresponding hand-written C typedefs (`hash_zarray_config_t`, `resmgr_config_t`) — the capnp-generated bindings still reference those now-nonexistent types via `$C.mapname(...)`, breaking compilation
|
||||
- Remove the `HashZArrayConfig` and `ResmgrConfig` struct definitions entirely from `controller.capnp` (grep-verify first that neither is referenced anywhere else in the schema, same method used for `MasterCandidate` in task 1.1)
|
||||
- Regenerate the capnp C bindings
|
||||
- Observable: capnp compiler succeeds; the generated bindings no longer emit any `HashZArrayConfig`/`ResmgrConfig`/`hash_zarray_config_t`/`resmgr_config_t` reference anywhere
|
||||
- _Requirements: 2.5_
|
||||
- _Depends: 1.1, 1.2_
|
||||
- _Boundary: Schema Component_
|
||||
|
||||
- [ ] 2. Core: cluster.c Port Resolution, Debug-Dump & Port-Defaulting Cleanup
|
||||
- [ ] 2.1 Update `cluster_open_listener`'s port-resolution fallback
|
||||
- [x] 2.1 Update `cluster_open_listener`'s port-resolution fallback
|
||||
- Change the fallback branch from reading `cluster_config->port` to a new hardcoded `XCE_CONTROLLER_DEFAULT_PORT` constant (9990), reached only when `--peers` was never passed at all (`peers_cfg.candidates_count == 0`)
|
||||
- Do NOT add a new CLI flag or `myconf_t` field — the `--peers` candidate-match branch (including its existing single self-referencing `--peers <name>=<host>:<port>` usage for single-node mode) is left completely unchanged; this is the only mechanism an operator uses to get a non-default single-node port
|
||||
- Preserve the exact precedence and branch structure: a `--peers` match with nonzero port binds to it unchanged; a matched candidate with port == 0 falls through to the hardcoded-default tier exactly as today's tolerated case; `--peers` passed but local name absent still returns `-EINVAL` and refuses to start, unchanged
|
||||
|
|
@ -58,7 +67,7 @@
|
|||
- Run the full existing suite and compare against the pre-spec baseline
|
||||
- Observable: build completes with zero compile errors; `controller_unit_test` passes with no new failures attributable to the removed fields
|
||||
- _Requirements: 5.1, 5.2_
|
||||
- _Depends: 1.1, 1.2, 2.1, 2.2, 2.3, 3_
|
||||
- _Depends: 1.1, 1.2, 1.3, 2.1, 2.2, 2.3, 3_
|
||||
|
||||
- [ ] 5. Validation: Live HA Verification
|
||||
- [ ] 5.1 Regenerate the `ha/data/cluster.bin` fixture against the renumbered schema
|
||||
|
|
|
|||
|
|
@ -1213,6 +1213,14 @@ int cluster_init_phase1(cluster_t* c)
|
|||
return 0;
|
||||
}
|
||||
|
||||
/* Hardcoded fallback listen port, used only when --peers was never passed
|
||||
* at all (true single-node mode, peers_cfg.candidates_count == 0). An
|
||||
* operator who wants a non-default single-node port passes a single
|
||||
* self-referencing --peers <name>=<host>:<port> entry instead, which
|
||||
* resolves via the candidate-match branch below, not this constant.
|
||||
* Matches the pre-schema-split ClusterConfig.port default value. */
|
||||
#define XCE_CONTROLLER_DEFAULT_PORT 9990
|
||||
|
||||
int cluster_open_listener(cluster_t* c)
|
||||
{
|
||||
int rc;
|
||||
|
|
@ -1224,9 +1232,21 @@ int cluster_open_listener(cluster_t* c)
|
|||
if (c->ep.fd >= 0)
|
||||
return 0;
|
||||
|
||||
/* Determine listen port: prefer per-candidate port from --peers list,
|
||||
* fall back to cluster_config->port if config is already loaded. */
|
||||
/* Determine listen port:
|
||||
* 1. Search --peers candidates for the local name.
|
||||
* - Matched with a nonzero port: bind to it (unchanged happy
|
||||
* path; also how a single-node operator gets a custom port,
|
||||
* via a single self-referencing --peers entry).
|
||||
* - Matched with port == 0 (malformed --peers entry): falls
|
||||
* through to the hardcoded default below, same as today's
|
||||
* tolerated case.
|
||||
* - Not matched at all, but --peers WAS passed (candidates_count
|
||||
* > 0): likely operator misconfiguration (own name typo'd or
|
||||
* omitted) -- refuse to start, unchanged from today.
|
||||
* 2. --peers never passed at all (candidates_count == 0): true
|
||||
* single-node mode, use the hardcoded default. Always succeeds. */
|
||||
uint32_t my_port = 0;
|
||||
bool matched = false;
|
||||
{
|
||||
const xce_cluster_config_t* pcfg = &c->myconf->peers_cfg;
|
||||
for (int j = 0; j < pcfg->candidates_count; j++) {
|
||||
|
|
@ -1235,19 +1255,20 @@ int cluster_open_listener(cluster_t* c)
|
|||
: pcfg->candidates[j].hostname;
|
||||
if (strncmp(key, c->myconf->name, XCE_MAXNAME_LEN - 1) == 0) {
|
||||
my_port = pcfg->candidates[j].port;
|
||||
matched = true;
|
||||
break;
|
||||
}
|
||||
}
|
||||
}
|
||||
if (my_port == 0) {
|
||||
if (c->myconf->cluster_config != NULL) {
|
||||
my_port = (uint32_t)c->myconf->cluster_config->port;
|
||||
} else {
|
||||
ERROR("cluster_open_listener: no port in --peers and cluster_config "
|
||||
"not yet loaded; specify port in --peers list");
|
||||
|
||||
if (!matched && pcfg->candidates_count > 0) {
|
||||
ERROR("cluster_open_listener: local name '%s' not found in "
|
||||
"--peers list; refusing to start", c->myconf->name);
|
||||
return -EINVAL;
|
||||
}
|
||||
}
|
||||
if (my_port == 0) {
|
||||
my_port = XCE_CONTROLLER_DEFAULT_PORT;
|
||||
}
|
||||
|
||||
rc = endpoint_init(&(c->ep), AF_INET, SOCK_STREAM, (int)my_port, 5);
|
||||
if (rc < 0)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue