From 1229f9a9455c437fc4e9b37699666a32ea14c16a Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Tue, 21 Jul 2026 03:36:01 +0000 Subject: [PATCH] =?UTF-8?q?feat(controller):=20clusterconfig-schema-split?= =?UTF-8?q?=20task=202.1=20=E2=80=94=20cluster=5Fopen=5Flistener=20two-tie?= =?UTF-8?q?r=20port=20fallback?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../clusterconfig-schema-split/design.md | 5 ++- .../requirements.md | 1 + .../clusterconfig-schema-split/research.md | 8 ++++ .../specs/clusterconfig-schema-split/tasks.md | 13 ++++++- src/controller/cluster.c | 39 ++++++++++++++----- 5 files changed, 53 insertions(+), 13 deletions(-) diff --git a/.kiro/specs/clusterconfig-schema-split/design.md b/.kiro/specs/clusterconfig-schema-split/design.md index c395542f..b8ae1752 100644 --- a/.kiro/specs/clusterconfig-schema-split/design.md +++ b/.kiro/specs/clusterconfig-schema-split/design.md @@ -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 =:` 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 =:` 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` | diff --git a/.kiro/specs/clusterconfig-schema-split/requirements.md b/.kiro/specs/clusterconfig-schema-split/requirements.md index cb24c4f0..45a6c86f 100644 --- a/.kiro/specs/clusterconfig-schema-split/requirements.md +++ b/.kiro/specs/clusterconfig-schema-split/requirements.md @@ -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 diff --git a/.kiro/specs/clusterconfig-schema-split/research.md b/.kiro/specs/clusterconfig-schema-split/research.md index 5d126419..c1ce5a4a 100644 --- a/.kiro/specs/clusterconfig-schema-split/research.md +++ b/.kiro/specs/clusterconfig-schema-split/research.md @@ -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. diff --git a/.kiro/specs/clusterconfig-schema-split/tasks.md b/.kiro/specs/clusterconfig-schema-split/tasks.md index 1b651a15..7651776a 100644 --- a/.kiro/specs/clusterconfig-schema-split/tasks.md +++ b/.kiro/specs/clusterconfig-schema-split/tasks.md @@ -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 =:` 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 diff --git a/src/controller/cluster.c b/src/controller/cluster.c index e952ac6b..a01526c2 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -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 =: 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)