lei.chen/myxce

Fork 1

Rongsong Shen 033f34c07f add kiro-sdd documents

2026-05-13 10:58:51 +00:00

12 KiB

Raw Blame History

Gap Analysis: resval-id-decoupling

Generated: 2026-05-12T07:25:00-07:00 Feature: resval-id-decoupling

Executive Summary

Scope: Three call sites in the controller use collect_resval_with_idx() to produce index-keyed resource values sent to two consumers (scheduler and API server). Fixing all three brings every cross-subsystem resval path into alignment with the already-correct Data_data_resvals dataset path.
Existing Infrastructure: xce_name_val_t (NameValue in capnp) already exists in types.h and has a complete capnp codec (encode_NameValue_list / decode_NameValue_list) in the generated code — it was defined for this purpose but never wired in at the resval-transfer boundary.
Wire format impact: Conf4Sched.resvals (field @6) and Worker.resvals (field @5) in controller.capnp currently declare List(ValueWithIdx). Changing these to List(NameValue) is a breaking wire change — no intermediate backward-compatibility window unless two field slots are used.
Build chain: The capnp schema in src/schema/controller.capnp drives generation of src/build/lib/controller.capnp.[ch] via meson + capnpc-c. A schema edit automatically regenerates the C codecs on the next build.
Key insight: makeResource_() in resobj.c:137 always overwrites resobj->idx = rc (from key_add) regardless of the idx transmitted in the Resource wire struct — so the controller's idx is lost at the scheduler's resmgr and the assumption that they match is never enforced.

Existing Components Inventory

Types and Structs

Symbol	Location	Role	Status
`xce_val_with_idx_t`	`src/include/xce/types.h:38`	`{int idx, xce_val_t* val}` — current cross-subsystem carrier	Replace at boundaries
`xce_name_val_t`	`src/include/xce/types.h:43`	`{char* name, xce_val_t* val}` — name-keyed carrier	Ready to use
`xce_resval_t`	`src/include/xce/types.h:48`	`{char* name, char* host, xce_val_t* val}` — used by Path C	Already correct
`xce_conf4sched_t.resvals`	`src/include/typemap.h:50`	`xce_val_with_idx_t**` — shared resvals in initial config	Change to `xce_name_val_t`**
`xce_worker_t.resvals_to_sched`	`src/include/xce/types.h:217`	`xce_val_with_idx_t**` — per-worker resvals	Change to `xce_name_val_t`**

Controller — Call Sites (Sender Side)

File	Line	Function	Purpose	Change needed
`src/controller/dataex.c`	62	`collect_resval_with_idx()`	Iterates `zarray_t`, emits `{idx, val}`	Replace/augment with name-keyed variant
`src/controller/dataex.c`	363	`collect_shared_resvals()`	Calls `collect_resval_with_idx(&c->sharedinst)`	Change to name-keyed collection
`src/controller/dataex.c`	141–163	`collect_resvals_workers()`	Calls `collect_resval_with_idx(&w->resvals)` per worker	Change to name-keyed collection
`src/controller/sched.c`	118–120	`xcesched_push_configs()`	Puts shared resvals into `conf4sched->resvals`	Change field type; update free
`src/controller/sched.c`	442–444	`xcesched_push_workerlist()`	Puts worker resvals into `w->host.resvals_to_sched`	Change field type; update free
`src/controller/apiserver.c`	367–369	`apiserver_worker_change()`	Same pattern as sched for API server path	Change field type; update free

Free/cleanup Sites

File	Line	Call	Must be updated
`src/controller/sched.c`	145	`free_resval_with_idx_list(&conf4sched->resvals, ...)`	→ `free_NameValue_list` or equivalent
`src/controller/sched.c`	226, 463	`free_resval_with_idx_list(&w->resvals_to_sched, ...)`	Same
`src/controller/apiserver.c`	51, 397–399	`free_resval_with_idx_list(...)`	Same
`src/controller/dataex.c`	404	`free_resval_with_idx_list(...)`	Same
`src/controller/reply.c`	66	`free_resval_with_idx_list(...)`	Same
`src/controller/opget.c`	128	`free_resval_with_idx_list(...)`	Same
`src/include/utils.h`	130	`free_resval_with_idx_list` declaration	Update or keep alongside new

Scheduler — Consumer Side

File	Line	Code	Change needed
`src/scheduler/src/scheduler.c`	574–586	`sched_init_sharedInst()` — uses `idx` directly from `config->resvals[i]->idx`	Change to name-lookup in `sched->resmgr`
`src/scheduler/src/schedobj.c`	274–280	`make_sched_worker()` — uses `idx` directly from `w->resvals_to_sched[i]->idx`	Change to name-lookup

Capnp Schema and Generated Code

Asset	Location	Current	Change
`Conf4Sched.resvals`	`src/schema/controller.capnp:230`	`List(ValueWithIdx)`	→ `List(NameValue)`
`Worker.resvals`	`src/schema/controller.capnp:129`	`List(ValueWithIdx)`	→ `List(NameValue)`
`controller.capnp.c/h`	`src/build/lib/`	Auto-generated — regenerated by `ninja -C build`	Automatically updated after schema edit
`encode_NameValue_list`	`src/build/lib/controller.capnp.c:3402`	Already exists, unused at these boundaries	Wire in to replace `encode_ValueWithIdx_list`
`decode_NameValue_list`	`src/build/lib/controller.capnp.c:3426`	Already exists, unused at these boundaries	Wire in to replace `decode_ValueWithIdx_list`
`free_NameValue_list`	`src/build/lib/controller.capnp.c:3458`	Already exists	Replace `free_ValueWithIdx_list` at these call sites

API Server (Go frontend) — Impact

The Go API server (src/frontend/apiserver/backend/clib.go) includes controller.capnp.h and works with xce_worker_t* via CGo. It accesses resvals_to_sched in apiserver_worker_change() (controller side) which populates xce_worker_t.resvals_to_sched. The Go frontend's cResvalToDistribution in convert.go:273 already processes xce_resval_t (name-keyed), not xce_val_with_idx_t. The API server impact is limited to updating any code that reads resvals_to_sched field — if it accesses it at all on the Go side (currently no direct access found).

Root Cause Deep Dive

The zarray-based stores (c->sharedinst, w->host.resvals) use r->idx as the slot key. r->idx is assigned by key_add() in makeResource_ (resobj.c:137) — it is the slot returned by the internal container, which depends on insertion order. The controller and scheduler each call makeResource() independently (controller at cluster init, scheduler at sched_init_resources()), with no guarantee of identical ordering.

Crucially, the Resource capnp struct does transmit r->idx on the wire (encode_Resource at build/lib/controller.capnp.c:3889), but decode_Resource writes it into a temporary xce_resource_t, which is then passed to makeResource(), which immediately overwrites the decoded idx with the local key_add return value (resobj->idx = rc). The transmitted idx is therefore discarded at the scheduler.

Implementation Options

Option A — Replace `ValueWithIdx` with `NameValue` at both capnp fields (Recommended)

What changes:

controller.capnp: change Conf4Sched.resvals @6 and Worker.resvals @5 from List(ValueWithIdx) to List(NameValue)
Rebuild capnp — generated encode_Conf4Sched/decode_Conf4Sched and worker codec automatically use NameValue functions
typemap.h: xce_conf4sched_t.resvals → xce_name_val_t**
types.h: xce_worker_t.resvals_to_sched → xce_name_val_t**
Controller: new collect_resval_by_name(zarray_t* vals, var resmgr, xlist_t* l) that walks resmgr entries, reads the value by r->idx from the array, and emits {name, val} records
Scheduler sched_init_sharedInst(): iterate name-val list, key_find(sched->resmgr, name) → r->idx → zarray_put
Scheduler make_sched_worker(): same pattern
Update all free/cleanup call sites to use free_NameValue_list

Pros: Cleanest — removes the misleading idx field entirely; NameValue codec already exists; aligns with Path C's design; no backward compatibility concerns given no public release Cons: None significant — wire-format break is a non-issue as there is no released version requiring compatibility

Affected files (estimate): 8 C files, 1 capnp schema, 0 Go files

Option B — Add `name` field to `xce_val_with_idx_t` / `ValueWithIdx` (Additive wire change) [REJECTED]

Ruled out: backward compatibility is not a requirement for this project at its current stage. Option B's only advantage over Option A is a softer wire transition — without that constraint, it is strictly worse: it keeps a semantically dead idx field, requires more code changes, and leaves misleading structure in the API.

Option C — Use Controller's idx as canonical; stop overwriting in `makeResource_` [REJECTED]

Ruled out: does not fix the design — just moves the source of truth without eliminating the hidden identity assumption. A resource deleted and re-added in the controller gets a new idx, and the scheduler has no way to know its cached idx is stale.

Recommendation

Option A is the selected approach. Backward compatibility is not a constraint (no public release), so the wire-format change to List(NameValue) is straightforward. The NameValue codec already exists in the generated capnp code and is fully functional — this is primarily a wiring exercise. The semantic change aligns the initial-config and worker-data paths with the already-correct Data_data_resvals update path (Path C).

Files to Change (Option A)

File	Change Type
`src/schema/controller.capnp`	Schema edit: 2 field type changes
`src/include/xce/types.h`	Struct field type change: `resvals_to_sched`
`src/include/typemap.h`	Struct field type change: `resvals`
`src/controller/dataex.c`	New `collect_resval_by_name()` function; update `collect_shared_resvals()` and `collect_resvals_workers()`; remove `collect_resval_with_idx` and `collect_resval_with_idx_`
`src/controller/sched.c`	Update 3 call sites (populate and free)
`src/controller/apiserver.c`	Update 2 call sites (populate and free)
`src/controller/reply.c`	Update free call
`src/controller/opget.c`	Update free call
`src/scheduler/src/scheduler.c`	Update `sched_init_sharedInst()`
`src/scheduler/src/schedobj.c`	Update `make_sched_worker()`
`src/include/utils.h`	Remove `free_resval_with_idx_list` declaration; add `free_name_val_list` declaration
`src/lib/utils.c`	Remove `free_resval_with_idx_list`; add `free_name_val_list(xce_name_val_t***, int)`
`src/controller/include/cluster.h`	Remove `collect_resval_with_idx` declaration

Not changed: src/scheduler/src/handler.c (Path C untouched), src/frontend/apiserver/ (Go API server does not directly access resvals_to_sched in Go code), capnp generated files (auto-regenerated by ninja -C build).

Decisions (Closed Questions)

New collector function signature: collect_resval_by_name is controller-internal logic — add it to dataex.c alongside collect_resval_with_idx. It needs both the zarray_t (values indexed by r->idx) and the resmgr (name→idx mapping) as parameters.
Handling unknown resources in scheduler: Warn and skip. When sched_init_sharedInst or make_sched_worker encounters a name not in sched->resmgr, emit a WARN log and continue. This is safe because the resource list always arrives in the same config push before resvals are applied (see decision 3).
Ordering guarantee: Confirmed — current behavior, must be preserved. sched_init_resources() is called before sched_init_sharedInst() in sched_init_config() (scheduler.c:603-609). The design must not reorder these calls.
Removal of collect_resval_with_idx: Remove entirely. No cross-subsystem callers will remain after this change, and intra-subsystem callers (e.g., worker.c reading from sharedinst for API get-resval) access the zarray directly by r->idx without going through this function. Delete collect_resval_with_idx, collect_resval_with_idx_, and remove the declaration from src/controller/include/cluster.h.
free_name_val_list utility: Add to utils.c/utils.h. A new free_name_val_list(xce_name_val_t***, int) function mirroring free_resval_with_idx_list is needed. The capnp-generated free_NameValue_list is an option but couples cleanup to the serialization layer — a standalone utility in utils.c is cleaner and consistent with the existing pattern.

12 KiB Raw Blame History Unescape Escape