154 lines
12 KiB
Markdown
154 lines
12 KiB
Markdown
# 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:**
|
||
1. `controller.capnp`: change `Conf4Sched.resvals @6` and `Worker.resvals @5` from `List(ValueWithIdx)` to `List(NameValue)`
|
||
2. Rebuild capnp — generated `encode_Conf4Sched`/`decode_Conf4Sched` and worker codec automatically use `NameValue` functions
|
||
3. `typemap.h`: `xce_conf4sched_t.resvals` → `xce_name_val_t**`
|
||
4. `types.h`: `xce_worker_t.resvals_to_sched` → `xce_name_val_t**`
|
||
5. 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
|
||
6. Scheduler `sched_init_sharedInst()`: iterate name-val list, `key_find(sched->resmgr, name)` → `r->idx` → `zarray_put`
|
||
7. Scheduler `make_sched_worker()`: same pattern
|
||
8. 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)
|
||
|
||
1. **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.
|
||
|
||
2. **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).
|
||
|
||
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.
|
||
|
||
4. **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`.
|
||
|
||
5. **`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.
|