diff --git a/.kiro/specs/resval-id-decoupling/design.md b/.kiro/specs/resval-id-decoupling/design.md new file mode 100644 index 00000000..56620a14 --- /dev/null +++ b/.kiro/specs/resval-id-decoupling/design.md @@ -0,0 +1,435 @@ +# Design: resval-id-decoupling + +> Feature: resval-id-decoupling +> Created: 2026-05-12 +> Status: Design Generated + +## Architecture Pattern & Boundary Map + +**Pattern:** Targeted replacement of an index-keyed cross-subsystem data carrier with a name-keyed one, following the precedent set by `Data_data_resvals` (Path C). No new architectural layers are introduced. + +**Affected boundary:** Controller → Scheduler (initial config push) and Controller → Scheduler/API-server (per-worker data push). The wire protocol (`controller.capnp`) at these two field positions changes from `List(ValueWithIdx)` to `List(NameValue)`. + +**Scope:** + +``` +src/schema/controller.capnp ← 2 field type edits +src/include/xce/types.h ← 1 struct field type change +src/include/typemap.h ← 1 struct field type change +src/include/utils.h ← swap free function declaration +src/controller/include/cluster.h ← remove collect_resval_with_idx declaration +src/controller/dataex.c ← replace collect_resval_with_idx with collect_resval +src/controller/sched.c ← 3 call-site updates +src/controller/apiserver.c ← 2 call-site updates +src/controller/reply.c ← 1 free call update +src/controller/opget.c ← 1 free call update +src/lib/utils.c ← swap free function implementation +src/scheduler/src/scheduler.c ← sched_init_sharedInst update +src/scheduler/src/schedobj.c ← make_sched_worker update +``` + +**Not in scope:** `src/scheduler/src/handler.c` (Path C untouched), `src/frontend/apiserver/` Go code (no direct access to `resvals_to_sched` in Go layer), capnp generated files (auto-regenerated by `ninja -C build` after schema edit). + +## Technology Stack & Alignment + +| Aspect | Choice | Rationale | +|--------|--------|-----------| +| Language | C99 | Matches existing codebase convention | +| Wire protocol | Cap'n Proto — `List(NameValue)` replacing `List(ValueWithIdx)` | `NameValue`/`xce_name_val_t` already defined in schema and types; codec already generated | +| Build | Meson + capnpc-c (existing) | Schema edit triggers automatic regeneration of `controller.capnp.[ch]` | +| Logging | Existing `WARN`/`ERROR` macros | Consistent with codebase; unknown resource → `WARN` + skip | +| Iterator pattern | `iter_make` / `iter_next` / `iter_destroy` with read lock | Matches `collect_resources()` pattern already in `dataex.c` | +| Backward compatibility | Not required | No public release; breaking wire change is acceptable | + +## Components & Interface Contracts + +--- + +### Component 1: `controller.capnp` — Wire type changes (Req 1, 2, 3) + +Two field type replacements in `src/schema/controller.capnp`. Both field ordinal numbers are preserved (no renumbering); only the element type changes. + +``` +Before: + struct Worker { resvals @5: List(ValueWithIdx) ... } + struct Conf4Sched { resvals @6: List(ValueWithIdx) ... } + +After: + struct Worker { resvals @5: List(NameValue) ... } + struct Conf4Sched { resvals @6: List(NameValue) ... } +``` + +After this edit, `ninja -C build` regenerates `controller.capnp.[ch]`. The generated +`encode_Conf4Sched` and `decode_Conf4Sched` functions will automatically call +`encode_NameValue_list` / `decode_NameValue_list` (already implemented in the generated +code) instead of their `ValueWithIdx` counterparts. + +**Requirement traceability:** 1.1, 2.1, 3.1, 4.2 + +--- + +### Component 2: Type changes in shared headers (Req 1, 2, 3, 4) + +#### `src/include/xce/types.h` — `xce_worker_t.resvals_to_sched` + +``` +Before: + int nresvals_to_sched; + xce_val_with_idx_t** resvals_to_sched; + +After: + int nresvals_to_sched; + xce_name_val_t** resvals_to_sched; +``` + +#### `src/include/typemap.h` — `xce_conf4sched_t.resvals` + +``` +Before: + int nresvals; + xce_val_with_idx_t** resvals; + +After: + int nresvals; + xce_name_val_t** resvals; +``` + +**Requirement traceability:** 1.1, 2.1, 4.2 + +--- + +### Component 3: `dataex.c` — New collector; remove old (Req 1, 2, 5) + +#### Remove: `collect_resval_with_idx_` and `collect_resval_with_idx` + +Both the static inner function and its public wrapper are deleted. The declaration in +`src/controller/include/cluster.h` is also removed. + +#### Add: `collect_resval` + +Iterates `resmgr` (not the zarray directly) so resource names are always available. +For each named resource, looks up its value by `r->idx` in the provided `zarray_t`. + +``` +Signature: + int collect_resval(var resmgr, zarray_t* array, xlist_t* l) + +Parameters: + resmgr — read-locked resource manager; provides name→idx mapping + array — zarray_t holding xce_val_t* values indexed by r->idx (lock held by caller) + l — output xlist_t; receives xce_name_val_t* entries (caller owns memory) + +Behavior: + 1. lock(resmgr, RTTI_LOCK_READ) + 2. iter_make(&it, resmgr) + 3. For each xce_resource_t* r from iter_next: + a. If r == NULL, continue + b. src_val = (xce_val_t*)zarray_get(array, r->idx) [array already caller-locked] + c. If src_val == NULL, continue [resource exists but has no value yet] + d. Allocate xce_name_val_t* nv = calloc(1, sizeof(*nv)) + e. nv->name = strdup(r->name) + f. nv->val = calloc(1, sizeof(xce_val_t)) + g. Copy value: if svalue, strdup; else memcpy + h. xlist_store(l, nv) + 4. iter_destroy(&it) + 5. unlock(resmgr) + 6. Return l->count + +Return value: + >= 0 number of name-val entries collected + -ENOMEM allocation failure (partial list may exist; caller responsible for cleanup) + +Note: array lock must be acquired by the caller before calling this function, +consistent with how collect_resval_with_idx_ was called from collect_resval_with_idx +(which held array->lock). This keeps locking responsibility explicit. +``` + +#### Update: `collect_shared_resvals` + +``` +Before: + void collect_shared_resvals(cluster_t* c, collect_check_fn_t fn, xlist_t* l) { + collect_resval_with_idx(&(c->sharedinst), l); + } + +After: + void collect_shared_resvals(cluster_t* c, collect_check_fn_t fn, xlist_t* l) { + mylock_lock(&(c->sharedinst.lock)); + collect_resval(c->resmgr, &(c->sharedinst), l); + mylock_unlock(&(c->sharedinst.lock)); + } +``` + +`fn` parameter is unused (was unused before); kept for signature compatibility with +other `collect_*` functions. The `cluster_t*` provides access to both `resmgr` and +`sharedinst`. + +#### Update: `collect_resvals_workers` (inlined into `collect_workers`) + +``` +Before (in collect_resvals_workers): + collect_resval_with_idx(&(w->resvals), &lst); + +After: + mylock_lock(&(w->resvals.lock)); + collect_resval(resmgr, &(w->resvals), &lst); + mylock_unlock(&(w->resvals.lock)); +``` + +`collect_resvals_workers` requires access to `resmgr`. Pass it as a new parameter, or +access it from the containing `cluster_t*` context (the function is only called from +`collect_workers` which receives `cluster_t* c`). Preferred: pass `c->resmgr` from +`collect_workers`. + +Updated `collect_resvals_workers` signature: + +``` + static void collect_resvals_workers(var resmgr, xlist_t* l) +``` + +**Requirement traceability:** 1.1, 2.1, 5.1, 5.2 + +--- + +### Component 4: `src/lib/utils.c` and `src/include/utils.h` — Free function swap (Req 5) + +#### Remove: `free_resval_with_idx_list` + +The existing implementation and declaration are deleted. No remaining callers after +the controller-side changes in Component 5. + +#### Add: `free_name_val_list` + +Mirrors the removed function's structure; frees each `xce_name_val_t*` entry. + +``` +Signature: + void free_name_val_list(xce_name_val_t*** vals, int count) + +Behavior: + 1. resvals = *vals + 2. If resvals == NULL or count <= 0, return + 3. For i in [0, count): + a. nv = resvals[i]; if NULL, continue + b. FREEUP(nv->name) + c. If nv->val != NULL: + if nv->val->which == Value_u_svalue: FREEUP(nv->val->u.svalue) + FREEUP(nv->val) + d. FREEUP(nv) + 4. FREEUP(*vals) + +Header declaration (utils.h): + extern void free_name_val_list(xce_name_val_t***, int); +``` + +**Requirement traceability:** 5.1 + +--- + +### Component 5: Controller call sites — `sched.c` and `apiserver.c` (Req 1, 2, 3) + +All call sites that previously called `collect_resval_with_idx` and +`free_resval_with_idx_list` are updated to use the new functions and types. + +#### `sched.c` — `xcesched_push_configs` (shared resvals, Req 1) + +``` +Before: + collect_shared_resvals(c, NULL, &xreslist); + configdata.data.conf4sched->nresvals = xreslist.count; + configdata.data.conf4sched->resvals = (xce_val_with_idx_t**)xreslist.data; + ... + free_resval_with_idx_list(&(configdata.data.conf4sched->resvals), + configdata.data.conf4sched->nresvals); + +After: + collect_shared_resvals(c, NULL, &xreslist); + configdata.data.conf4sched->nresvals = xreslist.count; + configdata.data.conf4sched->resvals = (xce_name_val_t**)xreslist.data; + ... + free_name_val_list(&(configdata.data.conf4sched->resvals), + configdata.data.conf4sched->nresvals); +``` + +#### `sched.c` — `xcesched_push_workerlist` (worker resvals, Req 2) + +``` +Before: + collect_resval_with_idx(&(w->host.resvals), &resvalist); + w->host.nresvals_to_sched = resvalist.count; + w->host.resvals_to_sched = (xce_val_with_idx_t**)resvalist.data; + ... + free_resval_with_idx_list(&(w->host.resvals_to_sched), w->host.nresvals_to_sched); + +After: + mylock_lock(&(w->host.resvals.lock)); + collect_resval(c->resmgr, &(w->host.resvals), &resvalist); + mylock_unlock(&(w->host.resvals.lock)); + w->host.nresvals_to_sched = resvalist.count; + w->host.resvals_to_sched = (xce_name_val_t**)resvalist.data; + ... + free_name_val_list(&(w->host.resvals_to_sched), w->host.nresvals_to_sched); +``` + +#### `apiserver.c` — `apiserver_worker_change` (Req 3) + +Same pattern as the `sched.c` worker case above. Replace collect and free calls; +change cast to `(xce_name_val_t**)`. + +#### `sched.c:226`, `reply.c:66`, `opget.c:128` — cleanup-only sites + +Replace `free_resval_with_idx_list` with `free_name_val_list` at each cleanup site +where `w->resvals_to_sched` is freed without being re-populated. + +**Requirement traceability:** 1.1, 2.1, 3.1, 5.1 + +--- + +### Component 6: Scheduler — `sched_init_sharedInst` (Req 1, 6) + +Receives `xce_conf4sched_t.resvals` which is now `xce_name_val_t**`. +For each entry, look up the resource by name in `sched->resmgr` to get the local idx, +then store into `sched->sharedInst`. + +**Critical ordering constraint:** `sched_init_resources()` must be called before +`sched_init_sharedInst()`. This is already guaranteed by `sched_init_config()` +(`scheduler.c:603-609`). This call order must not change. + +``` +Before: + static int sched_init_sharedInst(xce_scheduler_t* sched) { + for (i = 0; i < sched->config->nresvals; i++) { + int idx = sched->config->resvals[i]->idx; + xce_val_t* val = NULL; + MOVE_PTR(val, sched->config->resvals[i]->val); + zarray_put(&(sched->sharedInst), idx, val); + } + return 0; + } + +After: + static int sched_init_sharedInst(xce_scheduler_t* sched) { + for (i = 0; i < sched->config->nresvals; i++) { + xce_name_val_t* nv = sched->config->resvals[i]; + xce_resource_t* r = NULL; + xce_val_t* val = NULL; + + if (nv == NULL || nv->name == NULL) continue; + + if (key_find(sched->resmgr, (void*)nv->name, (void**)&r) < 0 || r == NULL) { + WARN("%s unknown resource '%s' in resvals, skipping", + __func__, nv->name); + continue; + } + + MOVE_PTR(val, nv->val); + zarray_put(&(sched->sharedInst), r->idx, val); + } + return 0; + } +``` + +**Requirement traceability:** 1.2, 1.3, 6.1, 6.2, 6.3 + +--- + +### Component 7: Scheduler — `make_sched_worker` in `schedobj.c` (Req 2, 6) + +Receives `xce_worker_t.resvals_to_sched` which is now `xce_name_val_t**`. +For each entry, look up by name in `sched->resmgr` to get local `r->idx`. + +``` +Before: + for (i = 0; i < w->nresvals_to_sched; i++) { + int idx = w->resvals_to_sched[i]->idx; + xce_val_t* val = NULL; + MOVE_PTR(val, w->resvals_to_sched[i]->val); + zarray_put(&(worker->resvals), idx, val); + } + +After: + for (i = 0; i < w->nresvals_to_sched; i++) { + xce_name_val_t* nv = w->resvals_to_sched[i]; + xce_resource_t* r = NULL; + xce_val_t* val = NULL; + + if (nv == NULL || nv->name == NULL) continue; + + if (key_find(sched->resmgr, (void*)nv->name, (void**)&r) < 0 || r == NULL) { + WARN("%s unknown resource '%s' in worker resvals, skipping", + __func__, nv->name); + continue; + } + + MOVE_PTR(val, nv->val); + zarray_put(&(worker->resvals), r->idx, val); + } +``` + +`make_sched_worker` already receives `xce_scheduler_t* sched` as its first argument +(`void* arg`), cast at top of function — `sched->resmgr` is available. + +**Requirement traceability:** 2.2, 2.3, 2.4, 6.3 + +--- + +## Data Flow + +### Before (fragile index-keyed path) + +``` +Controller Scheduler +---------- --------- +c->sharedinst[r->idx] = val + │ +collect_resval_with_idx() + emit {idx=i, val} ──────────────► sched_init_sharedInst() + zarray_put(&sharedInst, idx, val) + ↑ + ASSUMES controller idx == scheduler idx + (NO ENFORCEMENT — silent corruption if false) +``` + +### After (name-keyed path, matches Path C) + +``` +Controller Scheduler +---------- --------- +c->sharedinst[r->idx] = val + │ +collect_resval(resmgr, array) + iter resmgr → r->name, r->idx + val = array->data[r->idx] + emit {name=r->name, val} ────────► sched_init_sharedInst() + key_find(sched->resmgr, name) → r + zarray_put(&sharedInst, r->idx, val) + ↑ + Uses scheduler's OWN r->idx + No cross-subsystem index assumption +``` + +## Requirements Traceability + +| Requirement | Component(s) | +|------------|--------------| +| 1.1 — Name-keyed shared resvals in config push | Comp 1, 2, 3, 5 | +| 1.2 — Scheduler resolves by name in resmgr | Comp 6 | +| 1.3 — Warn and skip unknown resource names | Comp 6 | +| 1.4 — Config path produces same sharedInst state as Path C | Comp 6 (same key_find → r->idx logic as Path C) | +| 2.1 — Name-keyed worker resvals | Comp 1, 2, 3, 5 | +| 2.2 — Scheduler resolves worker resvals by name | Comp 7 | +| 2.3 — Warn and skip unknown worker resource names | Comp 7 | +| 2.4 — Dynamic resource catalog changes handled correctly | Comp 7 (no cached idx; lookup on each receive) | +| 3.1 — Name-keyed worker resvals to API server | Comp 1, 2, 5 (`apiserver.c`) | +| 3.2 — API server match by name without numeric index | Comp 2 (type change), Comp 5 | +| 4.1 — Path C (`Data_data_resvals`) unchanged | No changes to `handler.c` or `scheduler.c:770-788` | +| 4.2 — All struct consumers updated consistently | Comp 1, 2, 5, 6, 7 | +| 5.1 — No cross-subsystem slot-indexed resval path remains | Comp 3 (remove old function), Comp 4, 5, 6, 7 | +| 5.2 — `collect_resval_with_idx` removed | Comp 3 | +| 6.1 — Correct values when idx assignments differ | Comp 6, 7 | +| 6.2 — Identical behavior when idx assignments match | Comp 6, 7 | +| 6.3 — Scheduling lookups use scheduler's own indices | Comp 6, 7 | + +## Open Questions + +None — all design decisions resolved in gap analysis and user feedback. diff --git a/.kiro/specs/resval-id-decoupling/requirements.md b/.kiro/specs/resval-id-decoupling/requirements.md new file mode 100644 index 00000000..81dc8abb --- /dev/null +++ b/.kiro/specs/resval-id-decoupling/requirements.md @@ -0,0 +1,95 @@ +# Requirements: resval-id-decoupling + +> Generated: 2026-05-12T06:52:00-07:00 +> Updated: 2026-05-12T07:10:00-07:00 +> Feature: resval-id-decoupling +> Status: Requirements Generated + +## Overview + +The controller passes resource values to the scheduler (initial config push) and to the API server (worker data) using `xce_val_with_idx_t`, a struct carrying a numeric array-slot index. The scheduler and API server then store values at that same index in their own `zarray_t` stores. This works only when both components assign the same slot number to each resource — an assumption that holds when resource registration order is strictly identical but is not enforced by any contract and will silently produce wrong values if it breaks. + +The ongoing update path (`Data_data_resvals` dataset, `xce_resval_t` with `name` field) already resolves this correctly via name-based lookup in the receiver's own resmgr. The goal of this feature is to apply the same name-keyed approach uniformly across all resource-value transfer paths, eliminating the hidden index-identity assumption. + +## Codebase Analysis Summary + +Three distinct paths currently transfer resource values between components: + +**Path A — Shared resvals in initial config push** (`src/controller/sched.c:118-120`) +- `collect_shared_resvals()` → `collect_resval_with_idx(&c->sharedinst, ...)` emits `{idx, val}` pairs where `idx` is the controller's `sharedinst` zarray slot (= controller resource `idx`). +- Scheduler receives via `xce_conf4sched_t.resvals` (`xce_val_with_idx_t**`) and stores directly at the same index: `zarray_put(&sched->sharedInst, idx, val)` (`src/scheduler/src/scheduler.c:583`). +- **Bug**: If `resource->idx` differs between controller and scheduler (different insertion order into each component's independent resmgr), values land in the wrong slot. + +**Path B — Worker resvals to scheduler** (`src/controller/sched.c:442-444`, `src/controller/apiserver.c:367-369`) +- `collect_resval_with_idx(&w->host.resvals, ...)` emits `{idx, val}` from the controller's per-worker resvals zarray (slot = controller resource `idx`). +- Scheduler stores at the same slot: `zarray_put(&worker->resvals, idx, val)` (`src/scheduler/src/schedobj.c:279`). +- Later, `resource.c` indexes `worker->resvals` using the scheduler's own `residx` values — a mismatch if indices differ. + +**Path C — `Data_data_resvals` dataset update** (already correct) +- Uses `xce_resval_t` (name + host + val). Scheduler looks up `r = key_find(sched->resmgr, rv->name)` then uses `r->idx` (its own index): `zarray_put(&sched->sharedInst, r->idx, rv->val)` (`src/scheduler/src/scheduler.c:784`). +- No index-identity assumption. This is the target pattern. + +The fix must align Paths A and B with Path C's name-based resolution approach. The existing `xce_name_val_t` type (`src/include/xce/types.h:43`) already provides the right shape for shared resvals (name + val). The `xce_resval_t` type already provides the shape for per-host/worker resvals (name + host + val). + +## Requirements + +### Requirement 1: Name-keyed shared resource value transfer in initial config + +The scheduler SHALL receive shared resource values from the controller as name-keyed records (resource name + value) rather than as array-slot-indexed records. + +Upon receiving the initial config push, the scheduler SHALL resolve each resource value to its own slot index by looking up the resource name in its own resource manager (`sched->resmgr`), then store the value at the resolved index in `sched->sharedInst`. + +The scheduler SHALL log a warning and skip (not crash or silently corrupt) any resource value for which the resource name is not found in its resource manager. + +**Acceptance criteria:** +- EARS-R1-1: WHEN the controller sends an initial config push (`ConfigData_data_schedconfig`) containing shared resource values, THEN each resource value SHALL be identified by resource name, not by a numeric array slot index. +- EARS-R1-2: WHEN the scheduler processes a shared resource value record whose name matches a resource in `sched->resmgr`, THEN the scheduler SHALL store the value at the index assigned by its own resmgr (`r->idx`). +- EARS-R1-3: WHEN the scheduler processes a shared resource value record whose name does not match any resource in `sched->resmgr`, THEN the scheduler SHALL emit a warning log and skip that record without modifying `sched->sharedInst`. +- EARS-R1-4: WHEN an existing system running Path C updates (`Data_data_resvals`) works correctly, THEN after this change the initial-config path SHALL produce the same `sched->sharedInst` state as Path C would for the same resource values. + +### Requirement 2: Name-keyed per-worker resource value transfer to scheduler + +The scheduler SHALL receive per-worker resource values from the controller as name-keyed records (resource name + value) rather than as array-slot-indexed records. + +Upon receiving worker data, the scheduler SHALL resolve each worker resource value to its own slot index by looking up the resource name in its resource manager, then store the value at the resolved index in the worker's `resvals` zarray. + +The scheduler SHALL log a warning and skip any worker resource value whose resource name is not found. + +**Acceptance criteria:** +- EARS-R2-1: WHEN the controller sends worker data containing per-worker resource values, THEN each resource value SHALL be identified by resource name, not by a numeric array slot index. +- EARS-R2-2: WHEN the scheduler builds a `sched_worker` from received worker data, THEN it SHALL resolve each resource value name to the scheduler's own `r->idx` before calling `zarray_put(&worker->resvals, ...)`. +- EARS-R2-3: WHEN a resource name in a worker's resource values is unknown to the scheduler's resmgr, THEN the scheduler SHALL log a warning and skip that entry without aborting worker construction. +- EARS-R2-4: WHILE the scheduler is running and a resource is added or removed from the resource catalog, THEN subsequently received worker data SHALL correctly resolve resource values using the updated resmgr, with no stale index cached from before the change. + +### Requirement 3: Name-keyed per-worker resource value transfer to API server + +The API server SHALL receive per-worker resource values from the controller as name-keyed records (resource name + value) so that its own resource catalog lookup can resolve values independently of the controller's internal index assignment. + +**Acceptance criteria:** +- EARS-R3-1: WHEN the controller sends worker data to an API server connection, THEN per-worker resource values SHALL be transmitted using resource names as keys. +- EARS-R3-2: WHEN the API server (`src/frontend/apiserver`) converts worker resource values for its models (e.g., `cResvalToDistribution`), THEN it SHALL be able to match values to resources by name without assuming any particular numeric index. + +### Requirement 4: Backward compatibility with the `Data_data_resvals` update path + +The name-keyed changes to the initial config and worker data paths SHALL NOT alter the behavior of the existing `Data_data_resvals` dataset update path (Path C), which is already name-keyed. + +**Acceptance criteria:** +- EARS-R4-1: WHEN the scheduler processes a `Data_data_resvals` dataset message, THEN the behavior SHALL be identical before and after this change. +- EARS-R4-2: WHEN the `xce_conf4sched_t` struct is updated to use name-keyed resval records, THEN all callers that populate or consume this struct SHALL be updated consistently (controller, scheduler, and any serialization/deserialization layer in `libxce`). + +### Requirement 5: Removal of the index-keyed `collect_resval_with_idx` call sites + +The `collect_resval_with_idx` function (or its callers) SHALL be updated or replaced so that no code path transmits resource values indexed solely by a numeric array slot without an accompanying resource name. + +**Acceptance criteria:** +- EARS-R5-1: WHEN this feature is complete, THEN no code path that crosses a subsystem boundary (controller → scheduler, controller → API server) SHALL rely on the receiving subsystem's `zarray_t` slot indices matching the sending subsystem's slot indices. +- EARS-R5-2: WHEN `collect_resval_with_idx` is no longer needed for cross-subsystem transfer, THEN it SHOULD be removed or restricted to intra-subsystem use only, with a comment explaining its scope. + +### Requirement 6: No regression in resource value correctness + +The scheduler's scheduling decisions SHALL produce the same resource value lookups after this change as they did when indices happened to be consistent, and SHALL produce correct results when indices are inconsistent (where the old code would silently corrupt scheduling state). + +**Acceptance criteria:** +- EARS-R6-1: WHEN the controller and scheduler register resources in a different order (causing their resmgr index assignments to differ), THEN `sched->sharedInst` SHALL still contain the correct value for each resource after receiving the initial config push. +- EARS-R6-2: WHEN the controller and scheduler register resources in the same order, THEN the behavior of the scheduler SHALL be identical to the current behavior. +- EARS-R6-3: WHEN worker resource values are applied during a scheduling cycle, THEN `calc_wanted_global_resvals` and `calc_wanted_host_resvals` SHALL retrieve values from `sharedInst` and `worker->resvals` using the scheduler's own resource indices, unaffected by the controller's index ordering. diff --git a/.kiro/specs/resval-id-decoupling/research.md b/.kiro/specs/resval-id-decoupling/research.md new file mode 100644 index 00000000..87085d8b --- /dev/null +++ b/.kiro/specs/resval-id-decoupling/research.md @@ -0,0 +1,154 @@ +# 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. diff --git a/.kiro/specs/resval-id-decoupling/spec.json b/.kiro/specs/resval-id-decoupling/spec.json new file mode 100644 index 00000000..bda8cc75 --- /dev/null +++ b/.kiro/specs/resval-id-decoupling/spec.json @@ -0,0 +1,23 @@ +{ + "feature_name": "resval-id-decoupling", + "description": "investigate a better solution to pass resource val from controller to scheduler/api server, the current solution has a very strong assumption that the idx/id of resource in controller and scheduler should keep the same, but this assumption may be too strong, the key function is collect_resval_with_idx() in src/controller/dataex.c", + "phase": "tasks-generated", + "created_at": "2026-05-12T06:52:00-07:00", + "updated_at": "2026-05-12T07:45:00-07:00", + "language": "en", + "status": "active", + "approvals": { + "requirements": { + "generated": true, + "approved": true + }, + "design": { + "generated": true, + "approved": true + }, + "tasks": { + "generated": true, + "approved": false + } + } +} diff --git a/.kiro/specs/resval-id-decoupling/tasks.md b/.kiro/specs/resval-id-decoupling/tasks.md new file mode 100644 index 00000000..dc55c90f --- /dev/null +++ b/.kiro/specs/resval-id-decoupling/tasks.md @@ -0,0 +1,224 @@ +# Implementation Tasks: resval-id-decoupling + +> Generated: 2026-05-12T07:45:00-07:00 +> Feature: resval-id-decoupling +> Status: Tasks Generated + +## Task Overview + +7 major tasks, 19 sub-tasks. All 6 requirements covered. + +Dependencies: Task 1 must complete first (headers + schema unblock everything). +Tasks 2 and 3 can proceed in parallel after Task 1. +Tasks 4, 5, 6 can proceed in parallel after Tasks 2 and 3. +Task 7 last. + +--- + +## Tasks + +### 1. Update shared headers and capnp schema + +Change the wire type and C struct types that carry resource values across subsystem +boundaries. This is the structural foundation — all downstream tasks depend on these +types compiling correctly. + +- [x] 1.1 In `src/schema/controller.capnp`, change `Worker.resvals @5` and + `Conf4Sched.resvals @6` from `List(ValueWithIdx)` to `List(NameValue)`. Preserve + field ordinal numbers; only the element type changes. + _Requirements: 1, 2, 3, 4_ + +- [x] 1.2 In `src/include/xce/types.h`, change `xce_worker_t.resvals_to_sched` from + `xce_val_with_idx_t**` to `xce_name_val_t**`. + _Requirements: 2, 3_ + +- [x] 1.3 In `src/include/typemap.h`, change `xce_conf4sched_t.resvals` from + `xce_val_with_idx_t**` to `xce_name_val_t**`. + _Requirements: 1, 4_ + +- [x] 1.4 Run `nix develop --command ninja -C src/build` to regenerate + `controller.capnp.[ch]`. Confirm the generated `encode_Conf4Sched` and + `decode_Conf4Sched` now call `encode_NameValue_list` / `decode_NameValue_list` + instead of their `ValueWithIdx` counterparts. Expect compiler errors at this stage + from old call sites — that is expected and resolved in subsequent tasks. + +--- + +### 2. (P) Replace `free_resval_with_idx_list` with `free_name_val_list` + +Swap the heap-cleanup utility function to match the new element type. Old callers in +the controller will be updated in Task 4. + +- [x] 2.1 In `src/lib/utils.c`, add `free_name_val_list(xce_name_val_t*** vals, int count)`: + iterate the array, free each entry's `name`, free `val->u.svalue` if string-typed, + free `val`, free the entry, then null and free the array pointer. Mirror the structure + of the existing `free_resval_with_idx_list`. + +- [x] 2.2 In `src/include/utils.h`, remove the `free_resval_with_idx_list` declaration + and add `extern void free_name_val_list(xce_name_val_t***, int);`. + +- [x] 2.3 In `src/lib/utils.c`, delete the `free_resval_with_idx_list` implementation. + _Requirements: 5_ + +--- + +### 3. (P) Add `collect_resval` and remove `collect_resval_with_idx` + +Replace the index-keyed collector in the controller with a name-keyed one. The key +design inversion: iterate `resmgr` (names) rather than the `zarray_t` (slots), using +`r->idx` to look up values. This guarantees every emitted record carries a valid name. + +- [x] 3.1 In `src/controller/dataex.c`, add `collect_resval(var resmgr, zarray_t* array, xlist_t* l)`: + acquire a read lock on `resmgr`, iterate with `iter_make`/`iter_next`/`iter_destroy`, + for each `xce_resource_t* r` call `zarray_get(array, r->idx)` to get the value (skip + if NULL), allocate an `xce_name_val_t`, copy name (strdup) and value (strdup svalue or + memcpy), store in the list. Release the resmgr lock. Return `l->count`. The array lock + is the caller's responsibility (consistent with old pattern). + _Requirements: 1, 2, 5_ + +- [x] 3.2 In `src/controller/dataex.c`, rewrite `collect_shared_resvals` to lock + `c->sharedinst.lock`, call `collect_resval(c->resmgr, &c->sharedinst, l)`, then + unlock. The unused `fn` parameter is retained for signature consistency. + _Requirements: 1_ + +- [x] 3.3 In `src/controller/dataex.c`, update `collect_resvals_workers` to accept + `var resmgr` as a new first parameter. Replace the `collect_resval_with_idx` call with + `mylock_lock(&w->resvals.lock)` / `collect_resval(resmgr, &w->resvals, &lst)` / + `mylock_unlock`. Update the call site in `collect_workers` to pass `c->resmgr`. + _Requirements: 2_ + +- [x] 3.4 In `src/controller/dataex.c`, delete both `collect_resval_with_idx_` (static) + and `collect_resval_with_idx`. In `src/controller/include/cluster.h`, remove the + `collect_resval_with_idx` declaration. + _Requirements: 5_ + +--- + +### 4. (P) Update controller call sites + +Update all controller sites that populate or clean up `resvals_to_sched` / +`conf4sched->resvals` to use the new type and free function. Tasks 2 and 3 must be +complete before this task. + +- [x] 4.1 In `src/controller/sched.c` — `xcesched_push_configs`: change the cast from + `(xce_val_with_idx_t**)` to `(xce_name_val_t**)` when assigning `xreslist.data` to + `conf4sched->resvals`, and replace `free_resval_with_idx_list` with + `free_name_val_list`. + _Requirements: 1_ + +- [x] 4.2 In `src/controller/sched.c` — `xcesched_push_workerlist` and its worker-loop + cleanup: add `mylock_lock`/`mylock_unlock` around `collect_resval`, change cast to + `(xce_name_val_t**)`, replace all `free_resval_with_idx_list` calls with + `free_name_val_list`. + _Requirements: 2_ + +- [x] 4.3 In `src/controller/sched.c:226`, `src/controller/reply.c:66`, and + `src/controller/opget.c:128`, replace each `free_resval_with_idx_list` call with + `free_name_val_list`. + _Requirements: 5_ + +- [x] 4.4 In `src/controller/apiserver.c` — `apiserver_worker_change`: add + `mylock_lock`/`mylock_unlock` around `collect_resval`, change cast to + `(xce_name_val_t**)`, replace `free_resval_with_idx_list` with `free_name_val_list` + at both cleanup sites (lines 51 and 397–399). + _Requirements: 3_ + +--- + +### 5. (P) Update scheduler — shared resource value initialisation + +Change `sched_init_sharedInst` to resolve resource values by name via the scheduler's +own resmgr instead of using the raw index received from the controller. Task 1 must be +complete before this task. + +- [x] 5.1 In `src/scheduler/src/scheduler.c` — `sched_init_sharedInst`: replace the + `idx`-direct loop with a name-lookup loop. For each `xce_name_val_t* nv` in + `sched->config->resvals`: skip if `nv` or `nv->name` is NULL; call + `key_find(sched->resmgr, nv->name, &r)`; if not found emit `WARN` and continue; then + `MOVE_PTR(val, nv->val)` and `zarray_put(&sched->sharedInst, r->idx, val)`. Do not + change the call order in `sched_init_config` — `sched_init_resources` must remain + before `sched_init_sharedInst`. + _Requirements: 1, 6_ + +--- + +### 6. (P) Update scheduler — per-worker resource value initialisation + +Change `make_sched_worker` to resolve worker resource values by name. Task 1 must be +complete before this task. + +- [x] 6.1 In `src/scheduler/src/schedobj.c` — `make_sched_worker`: replace the + `idx`-direct loop with a name-lookup loop. For each `xce_name_val_t* nv` in + `w->resvals_to_sched`: skip if `nv` or `nv->name` is NULL; call + `key_find(sched->resmgr, nv->name, &r)`; if not found emit `WARN` and continue; then + `MOVE_PTR(val, nv->val)` and `zarray_put(&worker->resvals, r->idx, val)`. The + existing `sched` pointer (cast from `void* arg`) is already available at the top of + the function. + _Requirements: 2, 6_ + +--- + +### 7. Update Go API server to consume worker resource values + +Update the Go API server frontend to read `resvals_to_sched` (now `xce_name_val_t**`) +from received worker data and expose resource values in the `HostInfo` model. Previously +the Go layer ignored this field entirely. + +- [x] 7.1 In `src/frontend/apiserver/models/hosts.go`, add + `ResourceValues []models.ResourceDistributionInfo` (or + `ResourceValues []ResourceDistributionInfo` if within the same package) to `HostInfo`. + _Requirements: 3_ + +- [x] 7.2 In `src/frontend/apiserver/backend/convert_pure.go`, add a `resvals` field + (`[]models.ResourceDistributionInfo`) to `rawWorkerData`, and update `rawWorkerToModel` + to assign `info.ResourceValues = r.resvals`. + _Requirements: 3_ + +- [x] 7.3 In `src/frontend/apiserver/backend/convert.go`, add + `cNameValToWorkerResval(workerName string, nv *C.xce_name_val_t) *models.ResourceDistributionInfo`: + return nil if `nv` or `nv.val` is nil (with a WARN log); set `ResourceName` from + `nv.name`, `Location` from `workerName`, `IsDynamic: true`; decode the value type + using the same `xce_val_which` switch as `cResvalToDistribution`. + _Requirements: 3_ + +- [x] 7.4 In `src/frontend/apiserver/backend/convert.go`, update `cWorkerToModel` to + iterate `worker.nresvals_to_sched` entries in `worker.resvals_to_sched`, call + `cNameValToWorkerResval` for each, collect non-nil results, and store them in + `rawWorkerData.resvals`. + _Requirements: 3_ + +--- + +### 8. Build, verify, and clean up + +Confirm the full system compiles and passes tests with no regressions. All prior tasks +must be complete. + +- [x] 8.1 Run a full clean build: `nix develop --command bash -c "cd src && ninja -C build"`. + Resolve any remaining compiler errors (expected: none if tasks 1–7 complete). Confirm + `controller.capnp.[ch]` uses `NameValue` codecs for both `Conf4Sched.resvals` and + `Worker.resvals`. + +- [x] 8.2 Run the scheduler and controller test suites: + `nix develop --command bash -c "cd src/frontend && make test"` and any C-level tests + under `src/lib/testsuite/`. Confirm all tests pass. Note: `xcecli/common/output` + failure (`undefined: parseJobTime`) is pre-existing and unrelated to this feature. + +- [x] 8.3 Verify no remaining references to `collect_resval_with_idx` or + `free_resval_with_idx_list` in `src/` (excluding comments or git history): + `grep -r "collect_resval_with_idx\|free_resval_with_idx_list" src/`. Expected: zero + matches. + _Requirements: 5_ + +--- + +## Requirements Coverage + +| Requirement | Tasks | +|------------|-------| +| 1 | 1.1, 1.3, 3.2, 4.1, 5.1 | +| 2 | 1.1, 1.2, 3.3, 4.2, 4.3, 6.1 | +| 3 | 1.1, 1.2, 4.4 | +| 4 | 1.1, 1.3 | +| 5 | 2.1, 2.2, 2.3, 3.4, 4.3, 7.3 | +| 6 | 5.1, 6.1 | diff --git a/.kiro/specs/scheduler-controller-msg-gap/design.md b/.kiro/specs/scheduler-controller-msg-gap/design.md new file mode 100644 index 00000000..bf0f6572 --- /dev/null +++ b/.kiro/specs/scheduler-controller-msg-gap/design.md @@ -0,0 +1,469 @@ +# Design: scheduler-controller-msg-gap + +> Feature: scheduler-controller-msg-gap +> Created: 2026-05-11 +> Status: Design Generated + +## Architecture Pattern & Boundary Map + +**Pattern:** Extension of existing message handler infrastructure. The scheduler uses a two-phase processing model: + +1. **Enqueue phase** (handler.c, runs on IO thread) — lightweight: validate, allocate request struct, enqueue into `pend_updates[0]` +2. **Dequeue phase** (scheduler.c `sched_update()`, runs on main thread) — heavyweight: mutate scheduler state, call plugin hooks, trigger scheduling sessions + +All new handlers follow this pattern. No architectural boundary changes needed. + +**Scope boundary:** Only `src/scheduler/` source files, `src/lib/helper.c`, and `src/include/helper.h` are modified. No controller-side changes. No Cap'n Proto schema changes. + +## Technology Stack & Alignment + +| Aspect | Choice | Rationale | +|--------|--------|-----------| +| Language | C99 | Matches existing codebase convention | +| IPC protocol | Cap'n Proto (existing) | No schema changes — only handler gaps filled | +| Build system | Meson (existing) | No build changes needed | +| Threading model | Same as existing — handler enqueues, main thread dequeues | Preserves thread safety guarantees | +| Logging | Existing `TRACE`/`INFO`/`WARN`/`ERROR` macros | Consistent with codebase | +| Plugin hooks | Follow existing `plugin_*_update()` pattern | Consistency with established plugin API | + +## Components & Interface Contracts + +### Component 1: handler.c — Notify handlers (Req 3, 4, 5, 6) + +Extend `sched_handle_notify()` to process the four currently-stubbed NotifyData events. + +#### `sched_handle_sched_update_job` + +``` +Signature: static int sched_handle_sched_update_job( + xce_scheduler_t* sched, + xce_job_t* job) + +Behavior: + - If job == NULL, return -EINVAL + - Enqueue XCE_SCHED_OP_UPDATE_JOB with job pointer + - On calloc failure, return -ENOMEM + - Return 0 on success +``` + +**Requirement traceability:** 3.1, 3.2, 3.3 + +#### `sched_handle_sched_update_queue` + +``` +Signature: static int sched_handle_sched_update_queue( + xce_scheduler_t* sched, + xce_queue_t* queue) + +Behavior: + - If queue == NULL, return -EINVAL + - Enqueue XCE_SCHED_OP_UPDATE_QUEUE with queue pointer + - On calloc failure, return -ENOMEM + - Return 0 on success +``` + +**Requirement traceability:** 4.1, 4.2, 4.3 + +#### `sched_handle_sched_update_resvals` + +``` +Signature: static int sched_handle_sched_update_resvals( + xce_scheduler_t* sched, + xce_resval_list_t* resvals) + +Behavior: + - If resvals == NULL or count == 0, return 0 + - Enqueue XCE_SCHED_OP_UPDATE_RESVALS with resvals pointer + - On calloc failure, return -ENOMEM + - Return 0 on success +``` + +**Requirement traceability:** 5.1, 5.2, 5.3 + +#### `sched_handle_sched_job_rusage` + +``` +Signature: static int sched_handle_sched_job_rusage( + xce_scheduler_t* sched, + xce_job_rusages_t* rusages) + +Behavior: + - If rusages == NULL, return -EINVAL + - Enqueue XCE_SCHED_OP_JOB_RUSAGE with rusages pointer + - On calloc failure, return -ENOMEM + - Return 0 on success +``` + +**Requirement traceability:** 6.1, 6.2, 6.3 + +#### `sched_handle_notify` switch changes + +The existing switch in `sched_handle_notify()` gains four new cases: + +| NotifyData event | Handler function | Previous behavior | +|---|---|---| +| `NotifyData_data_schedUpdateJob` | `sched_handle_sched_update_job(sched, notify->data.sched_update_job)` | TRACE log only | +| `NotifyData_data_schedUpdateQueue` | `sched_handle_sched_update_queue(sched, notify->data.sched_update_queue)` | Empty `break` | +| `NotifyData_data_schedUpdateResvals` | `sched_handle_sched_update_resvals(sched, notify->data.sched_update_resvals)` | Empty `break` | +| `NotifyData_data_schedJobRusages` | `sched_handle_sched_job_rusage(sched, notify->data.sched_job_rusages)` | Empty `break` | + +### Component 2: handler.c — Dataset handlers (Req 1) + +#### `sched_handle_resources` + +``` +Signature: static int sched_handle_resources( + xce_scheduler_t* sched, + xce_resource_data_t* resdata) + +Behavior: + - If resdata == NULL or resdata->count == 0, return 0 + - Enqueue XCE_SCHED_OP_UPDATE_RESOURCES with resdata pointer + - On calloc failure, return -ENOMEM + - Return 0 on success +``` + +**Requirement traceability:** 1.1, 1.3, 1.4 + +#### `sched_handle_resvals` + +``` +Signature: static int sched_handle_resvals( + xce_scheduler_t* sched, + xce_resval_data_t* resvaldata) + +Behavior: + - If resvaldata == NULL or resvaldata->count == 0, return 0 + - Enqueue XCE_SCHED_OP_UPDATE_RESVALS_FULL with resvaldata pointer + - On calloc failure, return -ENOMEM + - Return 0 on success +``` + +**Requirement traceability:** 1.2, 1.3, 1.4 + +#### `sched_handle_dataset` switch changes + +| Data table | Handler function | Previous behavior | +|---|---|---| +| `Data_data_resources` | `sched_handle_resources(sched, dataset->data.resources)` | Empty `break` | +| `Data_data_resvals` | `sched_handle_resvals(sched, dataset->data.resvals)` | Empty `break` | + +### Component 3: scheduler.h — New op types and update request fields + +Extend `xce_sched_op_t` and `xce_sched_update_request_t`: + +``` +typedef enum { + XCE_SCHED_OP_NONE = 0, + XCE_SCHED_OP_UPDATE_CONFIG, + XCE_SCHED_OP_UPDATE_JOB, + XCE_SCHED_OP_JSTATE_CHANGE, + XCE_SCHED_OP_UPDATE_WORKER, + XCE_SCHED_OP_UPDATE_QUEUE, // NEW + XCE_SCHED_OP_UPDATE_RESOURCES, // NEW + XCE_SCHED_OP_UPDATE_RESVALS_FULL, // NEW — from dataset bulk resvals + XCE_SCHED_OP_UPDATE_RESVALS, // NEW — from notify incremental resvals + XCE_SCHED_OP_JOB_RUSAGE // NEW +} xce_sched_op_t; +``` + +Extend `xce_sched_update_request_t` union: + +``` +struct { + xce_queue_t* queue; // for UPDATE_QUEUE +} queueupdate; +struct { + xce_resource_data_t* resdata; // for UPDATE_RESOURCES +} resourceupdate; +struct { + xce_resval_data_t* resvaldata; // for UPDATE_RESVALS_FULL +} resvalfullupdate; +struct { + xce_resval_list_t* resvals; // for UPDATE_RESVALS (notify) +} resvalupdate; +struct { + xce_job_rusages_t* rusages; // for JOB_RUSAGE +} job_rusage; +``` + +### Component 4: scheduler.c — Dequeue processing in `sched_update()` + +Extend the `switch (req->op)` in `sched_update()` with new cases: + +#### `XCE_SCHED_OP_UPDATE_QUEUE` + +- Find existing queue by name with `sched_get_queue()` +- If found: update mutable fields (status, priority, accept_interval, sched_delay, resreq) +- If not found: return `-ENOENT` — queues are created only via the initial `Conf4Sched` config push, never via notify +- Call `plugin_queue_update()` + +**Requirement traceability:** 4.1, 4.2 + +#### `XCE_SCHED_OP_UPDATE_RESOURCES` + +- Iterate `resdata->resources[]` +- For each: call `makeResource(sched->resmgr, r)` to add or update +- NULL entries skipped (1.3) + +**Requirement traceability:** 1.1 + +#### `XCE_SCHED_OP_UPDATE_RESVALS_FULL` + +- Iterate `resvaldata->vals[]` +- For each: find resource by name in `sched->resmgr`, update value +- Unknown resources skipped with WARN log + +**Requirement traceability:** 1.2 + +#### `XCE_SCHED_OP_UPDATE_RESVALS` + +- Iterate `resvals->vals[]` +- For each: find worker by host name via `sched_get_worker()` +- If found: update worker's `resvals` zarray at the resource index +- Unknown hosts skipped (5.2) + +**Requirement traceability:** 5.1, 5.2, 5.3 + +#### `XCE_SCHED_OP_JOB_RUSAGE` + +- Find job by jobid via `sched_get_job()` +- If not found: return `-ENOENT` (6.2) +- Store rusage data on job object +- Call `plugin_job_rusage_update()` + +**Requirement traceability:** 6.1, 6.2 + +### Component 5: scheduler.c — Job update-in-place (Req 3) + +#### New function: `sched_update_existing_job` + +``` +Signature: static int sched_update_existing_job( + xce_scheduler_t* sched, + xce_job_t* updated) + +Behavior: + - Find existing job by jobid via sched_get_job() + - If not found: return -ENOENT — jobs are created only via + the Data_data_schedjobs dataset push, never via notify + - If found: update mutable fields (state, priority, allocs) + - Call plugin_job_update() + - Return 0 on success, negative on error +``` + +**Requirement traceability:** 3.1, 3.2, 3.3 + +This is wired into the existing `XCE_SCHED_OP_UPDATE_JOB` case — no new op type needed since `sched_on_jobs()` already creates new jobs. The change is to make `sched_update_jobs()` also handle job updates for existing jobids by replacing rather than just adding. + +### Component 6: plugin.h / plugin.c — New rusage hook (Req 6) + +Add to `plugin_hooks_t`: + +``` +typedef int (*plugin_job_rusage_hook_t)(void*, xce_sched_job_t*, xce_job_rusages_t*); +``` + +Add to `plugin_hooks_t` struct: + +``` +plugin_job_rusage_hook_t rusage_hook; +``` + +Add global function: + +``` +int plugin_job_rusage_update(void* sched, xce_sched_job_t* job, xce_job_rusages_t* rusages); +``` + +This follows the exact pattern of `plugin_job_state_change()` — iterate registered plugins, call their hook if non-NULL. + +### Component 7: handler.c — Dead code removal (Req 2) + +Remove from `scheduler_req_handler()`: +- The entire `Request_body_pushmsg` case block (lines 185–198) +- The static functions `sched_handle_jobpush()` and `sched_handle_workerpush()` + +These are unreachable because `vtask_entry()` in `src/lib/xcectx.c` rewrites all `pushmsg` into `datamsg`. + +**Requirement traceability:** 2.1, 2.2 + +### Component 8: helper.c / helper.h — Name-lookup functions (Req 8) + +#### New function: `xce_notify_event_name` + +``` +Signature: char* xce_notify_event_name(int event) + +Behavior: Switch onNotifyData_data_* enum values, return symbolic string. +Fallback: "UNKNOWN(%d)" with numeric substitution. +``` + +Matches pattern of `xce_dataset_table()` — takes int, returns `char*`. + +#### New function: `xce_sched_op_name` + +``` +Signature: char* xce_sched_op_name(xce_sched_op_t op) + +Behavior: Switch on XCE_SCHED_OP_* enum values, return symbolic string. +Fallback: "UNKNOWN(%d)" with numeric substitution. +``` + +Required for Requirement 7's error logging in `sched_update()`. + +#### Changes to `handler.c` logging + +| Current | Changed to | +|---|---| +| `TRACE("%s table = %d", __func__, dataset->table)` | `TRACE("%s table = %s", __func__, xce_dataset_table(dataset->table))` | +| `TRACE("%s...", __func__)` (in stub handlers) | `INFO("%s event = %s jobid = %s", __func__, xce_notify_event_name(notify->event), jobidString)` | + +**Requirement traceability:** 8.1, 8.2, 8.3, 8.4 + +### Component 9: scheduler.c — Error logging in `sched_update()` (Req 7) + +In the `sched_update()` main loop, after the `switch (req->op)` block, when `rc < 0`: + +``` +WARN("%s: op %s failed: %s (%d)", __func__, + xce_sched_op_name(req->op), strerror(-rc), rc); +``` + +**Requirement traceability:** 7.1 + +### Component 10: schedobj.c — Queue update function (Req 4) + +#### New function: `sched_update_existing_queue` + +``` +Signature: int sched_update_existing_queue( + xce_scheduler_t* sched, + xce_queue_t* updated) + +Behavior: + - Find existing queue by name via sched_get_queue() + - If not found: return -ENOENT — queues are created only via + the initial Conf4Sched config push, never via notify + - If found: update mutable fields: + sched_queue->status = updated->status + sched_queue->priority = updated->priority + sched_queue->accept_interval = updated->accept_interval + sched_queue->sched_delay = updated->sched_delay + // resreq is a string field — reallocate and replace + - Call plugin_queue_update() + - Return 0 on success, negative on error +``` + +**Requirement traceability:** 4.1, 4.2 + +### Component 11: `sched_pend_update_killer` updates (scheduler.c) + +Extend the `sched_pend_update_killer` destructor to handle new op types: + +``` +case XCE_SCHED_OP_UPDATE_QUEUE: + // queue pointer belongs to scheduler, no free needed + break; +case XCE_SCHED_OP_UPDATE_RESOURCES: + // resource_data rows belong to scheduler, no free needed + break; +case XCE_SCHED_OP_UPDATE_RESVALS_FULL: + // resval_data rows owned by capnp decode, freed by free_Request + break; +case XCE_SCHED_OP_UPDATE_RESVALS: + // resval_list owned by capnp decode, freed by free_Request + break; +case XCE_SCHED_OP_JOB_RUSAGE: + // rusage struct owned by capnp decode, freed by free_Request + break; +``` + +**Important:** Memory ownership must be traced carefully. For `XCE_SCHED_OP_UPDATE_JOB` and `XCE_SCHED_OP_UPDATE_WORKER`, the existing pattern nullifies `dataset->nrows` and `dataset->rows` after enqueue to transfer ownership. New queue/rusage/resvals handlers should follow the same pattern — the request struct owns the data, and `free_Request()` in `xcectx.c` handles cleanup. + +## Data Flow Diagrams + +### Notify message processing flow + +``` +Controller (sched.c) Scheduler (handler.c) Scheduler (scheduler.c) +───────────────────── ───────────────────── ───────────────────── +xce_notify_req() + │ + ├── schedStartJob ──────────► sched_job_state_change() ──► enq_pend_update(JSTATE_CHANGE) + │ │ + ├── schedExitJob ───────────► sched_job_state_change() ──► │ + │ │ + ├── schedFailJob ───────────► sched_job_state_change() ──► │ + │ │ + ├── schedUpdateJob ────────► sched_handle_sched_update_job() ─► enq_pend_update(UPDATE_JOB) [NEW] + │ │ + ├── schedUpdateQueue ──────► sched_handle_sched_update_queue() ─► enq_pend_update(UPDATE_QUEUE) [NEW] + │ │ + ├── schedUpdateResvals ────► sched_handle_sched_update_resvals() ─► enq_pend_update(UPDATE_RESVALS) [NEW] + │ │ + └── schedJobRusages ───────► sched_handle_sched_job_rusage() ──► enq_pend_update(JOB_RUSAGE) [NEW] + │ + ▼ + sched_update() main loop + ├── UPDATE_JOB ──► sched_update_existing_job() + ├── UPDATE_QUEUE ──► sched_update_existing_queue() + ├── UPDATE_RESVALS ──► update worker resvals + ├── UPDATE_RESOURCES ──► update resmgr [NEW] + ├── UPDATE_RESVALS_FULL ──► update sharedInst [NEW] + └── JOB_RUSAGE ──► store on job + plugin hook [NEW] +``` + +### Dataset message processing flow + +``` +Controller (dataex.c) Scheduler (handler.c) Scheduler (scheduler.c) +──────────────────── ───────────────────── ───────────────────── +xce_push_data_req() + │ + ├── Data_data_schedjobs ──► sched_on_jobs() ──────────► enq_pend_update(UPDATE_JOB) + │ │ + ├── Data_data_schedworkers ► sched_on_workers() ───────► enq_pend_update(UPDATE_WORKER) + │ │ + ├── Data_data_resources ──► sched_handle_resources() ─► enq_pend_update(UPDATE_RESOURCES) [NEW] + │ │ + └── Data_data_resvals ────► sched_handle_resvals() ───► enq_pend_update(UPDATE_RESVALS_FULL) [NEW] + │ + ▼ + sched_update() main loop +``` + +## File Change Summary + +| File | Change Type | Requirements | +|------|-------------|-------------| +| `src/scheduler/src/handler.c` | Modified — add handlers, remove dead code, update logging | 1, 2, 3, 4, 5, 6, 8 | +| `src/scheduler/include/scheduler.h` | Modified — extend enums and structs | 1, 3, 4, 5, 6 | +| `src/scheduler/src/scheduler.c` | Modified — add dequeue cases, error logging, update functions | 1, 3, 4, 5, 6, 7 | +| `src/scheduler/include/plugin.h` | Modified — add rusage hook typedef and global function | 6 | +| `src/scheduler/src/plugin.c` | Modified — add `plugin_job_rusage_update()` implementation | 6 | +| `src/scheduler/src/schedobj.c` | Modified — add `sched_update_existing_queue()` | 4 | +| `src/lib/helper.c` | Modified — add `xce_notify_event_name()`, `xce_sched_op_name()` | 8 | +| `src/include/helper.h` | Modified — add function declarations | 8 | +| `src/scheduler/testsuite/test.c` | Modified — add unit tests for all new handlers | All | + +## Build Order + +1. `src/include/helper.h` — add declarations (no dependencies) +2. `src/lib/helper.c` — add implementations (depends on step 1) +3. `src/scheduler/include/scheduler.h` — extend enums and structs (depends on typemap headers) +4. `src/scheduler/include/plugin.h` — add rusage hook typedef (depends on xce/types.h) +5. `src/scheduler/src/plugin.c` — add `plugin_job_rusage_update()` (depends on step 4) +6. `src/scheduler/src/schedobj.c` — add `sched_update_existing_queue()` (depends on step 3) +7. `src/scheduler/src/scheduler.c` — add dequeue processing, error logging, update functions (depends on steps 3–6) +8. `src/scheduler/src/handler.c` — add handlers, remove dead code, update logging (depends on steps 1–7) +9. `src/scheduler/testsuite/test.c` — add unit tests (depends on all above) + +## Risks & Mitigations + +| Risk | Severity | Mitigation | +|------|----------|-----------| +| Memory ownership of queued data — capnp-decoded structs freed before dequeue processes them | High | Follow existing pattern: for dataset paths, nullify `dataset->nrows`/`dataset->rows` after enqueue to transfer ownership. For notify paths, copy essential data into the request struct rather than holding pointers to capnp memory. | +| `sched_update_existing_job` must handle concurrent modification — handler thread enqueues while main thread processes | Medium | Already mitigated by existing double-buffer swap in `sched_update()`. The handler thread only touches `pend_updates[0]` while the main thread drains `pend_updates[1]`. | +| New `UPDATE_QUEUE` op requires queue name lookup — potential lock contention | Low | `sched_get_queue()` already uses read lock. Queue updates are infrequent. | +| `xce_sched_op_name` must handle future enum additions | Low | Follow `xce_dataset_table` pattern — return `"UNKNOWN(%d)"` for unrecognized values. | \ No newline at end of file diff --git a/.kiro/specs/scheduler-controller-msg-gap/requirements.md b/.kiro/specs/scheduler-controller-msg-gap/requirements.md new file mode 100644 index 00000000..b3096be1 --- /dev/null +++ b/.kiro/specs/scheduler-controller-msg-gap/requirements.md @@ -0,0 +1,110 @@ +# Requirements: scheduler-controller-msg-gap + +> Generated: 2026-05-11T01:34:00-07:00 +> Updated: 2026-05-11T02:00:00-07:00 +> Feature: scheduler-controller-msg-gap +> Status: Requirements Generated + +## Overview + +Analyze the codebase to find gaps in the scheduler's handling of controller messages — missing message types, unhandled edge cases, or incomplete message processing paths. + +## Codebase Analysis Summary + +The scheduler (`src/scheduler/src/handler.c`) receives controller messages through the `scheduler_req_handler` dispatcher, which handles two top-level opcodes: + +- **OPCode_push** — carries ConfigData or Dataset (`datamsg`) messages. Raw `pushmsg` packets are reassembled into `datamsg` by the `vtask_entry()` assembly layer in `src/lib/xcectx.c` (lines 93–106); the handler never sees individual `pushmsg` fragments. +- **OPCode_notify** — carries NotifyData event messages + +The controller (`src/controller/sched.c`) sends six NotifyData event types and two Data table types. The scheduler's handler has **6 gap locations** where the case is present but stubbed out or empty-bodied. The `Request_body_pushmsg` branch in `scheduler_req_handler` (which calls `sched_handle_jobpush`/`sched_handle_workerpush`) is dead code — it can never be reached because `vtask_entry` rewrites all `pushmsg` into `datamsg` before invoking the handler. + +## Requirements + +### Requirement 1: Resource data message handling + +The scheduler SHALL process `Data_data_resources` dataset messages received from the controller. Upon receipt, the system SHALL update the internal resource catalog (resource name, type, flags, ordering type, interval) in the scheduler's resource manager (`sched->resmgr`), reconciling additions, removals, and modifications against the existing set. + +The scheduler SHALL process `Data_data_resvals` dataset messages received from the controller. Upon receipt, the system SHALL update the shared resource value array (`sched->sharedInst`) with the new values indexed by position, and SHALL reset the `sharedInUse` tracking array for the next scheduling session. + +**Acceptance criteria:** +- EARS-UB-1.1: WHEN the scheduler receives a dataset with `table == Data_data_resources`, THEN the system shall add or update each resource in `sched->resmgr` +- EARS-UB-1.2: WHEN the scheduler receives a dataset with `table == Data_data_resvals`, THEN the system shall update `sched->sharedInst` at each value's index position +- EARS-UB-1.3: WHEN either handler encounters a NULL element in the dataset rows, THEN the system shall skip that element and continue processing remaining rows +- EARS-UB-1.4: WHEN either handler encounters a memory allocation failure, THEN the system shall return `-ENOMEM` and free any partially allocated structures + +### Requirement 2: Dead code removal — pushmsg handler branches + +The scheduler SHALL remove the dead `Request_body_pushmsg` branch in `scheduler_req_handler` and the stub functions `sched_handle_jobpush` and `sched_handle_workerpush`. These paths can never execute because the `vtask_entry()` assembly layer in `src/lib/xcectx.c` rewrites all `pushmsg` packets into `datamsg` before invoking the request handler. + +**Acceptance criteria:** +- EARS-UB-2.1: WHEN `scheduler_req_handler` is compiled, THEN the `Request_body_pushmsg` case and its two child branches (job/worker push) shall not exist +- EARS-UB-2.2: WHEN the scheduler is linked, THEN the symbols `sched_handle_jobpush` and `sched_handle_workerpush` shall not be present in the binary + +### Requirement 3: NotifyData event handling for job updates + +The scheduler SHALL process `NotifyData_data_schedUpdateJob` notify messages beyond the current trace-log-only behavior. Upon receipt, the system SHALL locate the job in `sched->jobs` by jobid, update the job's mutable fields (state, priority, queue, resource request) from the notify payload, and invoke `plugin_job_update` to notify plugins of the change. + +**Acceptance criteria:** +- EARS-UB-3.1: WHEN the scheduler receives a notify with `event == NotifyData_data_schedUpdateJob`, THEN the system shall find the existing job by jobid and update its mutable fields +- EARS-UB-3.2: WHEN the jobid in the update notify does not match any existing job (i.e., the job was never pushed via the initial dataset bulk load), THEN the system shall return `-ENOENT` — jobs are created only via the `Data_data_schedjobs` dataset path, never via notify +- EARS-UB-3.3: WHEN the update payload carries job data that is NULL, THEN the system shall return `-EINVAL` + +### Requirement 4: NotifyData event handling for queue updates + +The scheduler SHALL process `NotifyData_data_schedUpdateQueue` notify messages from the controller. Upon receipt, the system SHALL locate the queue in `sched->queues` by name and update its status, priority, accept_interval, sched_delay, and resource request fields from the notify payload. + +**Acceptance criteria:** +- EARS-UB-4.1: WHEN the scheduler receives a notify with `event == NotifyData_data_schedUpdateQueue`, THEN the system shall find the existing queue by name and update its mutable configuration fields +- EARS-UB-4.2: WHEN the queue name in the update notify does not match any existing queue (i.e., the queue was never loaded via the initial config push), THEN the system shall return `-ENOENT` — queues are created only via the `Conf4Sched` config path, never via notify +- EARS-UB-4.3: WHEN the update payload carries queue data that is NULL, THEN the system shall return `-EINVAL` + +### Requirement 5: NotifyData event handling for resource value updates + +The scheduler SHALL process `NotifyData_data_schedUpdateResvals` notify messages from the controller. Upon receipt, the system SHALL iterate the `xce_resval_list_t` payload, and for each resource value, update the per-worker `resvals` array on the named worker, matching by host name. + +**Acceptance criteria:** +- EARS-UB-5.1: WHEN the scheduler receives a notify with `event == NotifyData_data_schedUpdateResvals`, THEN the system shall update each named worker's `resvals` array with the new values +- EARS-UB-5.2: WHEN a named host in the updates does not correspond to any known worker, THEN the system shall skip that update entry and continue processing remaining entries +- EARS-UB-5.3: WHEN the update payload carries zero resource values, THEN the system shall return 0 without modifying state + +### Requirement 6: NotifyData event handling for job resource usage + +The scheduler SHALL process `NotifyData_data_schedJobRusages` notify messages from the controller. Upon receipt, the system SHALL locate the job by jobid, store the resource usage data (`xce_sched_job_rusages_t`) on the job object, and invoke `plugin_job_rusage_update` to notify plugins of the usage change. + +**Acceptance criteria:** +- EARS-UB-6.1: WHEN the scheduler receives a notify with `event == NotifyData_data_schedJobRusages`, THEN the system shall store the usage data on the matching job +- EARS-UB-6.2: WHEN the jobid in the update does not match any existing job, THEN the system shall return `-ENOENT` without modifying state +- EARS-UB-6.3: WHEN the rusage payload is NULL, THEN the system shall return `-EINVAL` + +### Requirement 7: Error handling consistency + +The scheduler SHALL return meaningful error codes from all message handlers that enable the controller to distinguish between transient failures (ENOMEM) and permanent protocol errors (EINVAL, ENOENT). The scheduler's main loop in `sched_update()` SHALL log a WARN-level message when a handler returns a non-zero error code, including the offending opcode and sub-type for operational debugging. + +**Acceptance criteria:** +- EARS-UB-7.1: WHEN a handler returns a non-zero error code, THEN `sched_update()` shall emit a WARN log including the `xce_sched_op_t` value and the error code +- EARS-UB-7.2: WHEN an unknown notify event type is received, THEN the handler shall return `-EINVAL` (already implemented — verify non-regression) +- EARS-UB-7.3: WHEN a push message arrives with an unknown `body_which` tag, THEN the handler shall return `-EINVAL` (already implemented — verify non-regression) + +### Requirement 8: Human-readable message type names in logs + +The scheduler SHALL log message type identifiers using symbolic names rather than numeric values. The existing helpers in `src/lib/helper.c` — `xce_request_op_name()`, `xce_request_msg_name()`, and `xce_dataset_table()` — already provide name lookups for OPCode, Request body tag, and Data table type. A new `xce_notify_event_name(int)` function SHALL be added to cover the NotifyData event type enum. All TRACE, DEBUG, INFO, and WARN messages in `handler.c` that reference these enums SHALL use these name-lookup functions instead of printing numeric values. + +**Acceptance criteria:** +- EARS-UB-8.1: WHEN `scheduler_req_handler` logs a message that includes an opcode, THEN the log shall call `xce_request_op_name()` to show the symbolic name (e.g., `"push"`) rather than the numeric value +- EARS-UB-8.2: WHEN `sched_handle_dataset` logs a message that includes a data table type, THEN the log shall call `xce_dataset_table()` to show the symbolic name (e.g., `"schedjobs"`) rather than the numeric value (currently `TRACE("%s table = %d", ...)`) +- EARS-UB-8.3: WHEN `sched_handle_notify` logs a message that includes a notify event type, THEN the log shall call `xce_notify_event_name()` to show the symbolic name rather than the numeric value +- EARS-UB-8.4: WHEN `xce_notify_event_name()` receives a numeric value that does not correspond to a known NotifyData enum constant, THEN it shall return `"UNKNOWN(%d)"` with the numeric value substituted (matching the pattern used by the existing helpers) +- EARS-UB-8.5: WHEN `sched_update()` emits a WARN log for a handler error (per Requirement 7), THEN the log shall include symbolic names via the appropriate helper function + +## Non-Requirements + +The following areas are intentionally excluded from this specification: +- **New protocol message types** — only existing, already-defined message types in `controller.capnp` are in scope +- **Controller-side changes** — only the scheduler's inbound message handling is in scope +- **Plugin implementation details** — plugin update functions are called but not modified +- **Bulk (Dataset) job/worker push already works** — the `Data_data_schedjobs` and `Data_data_schedworkers` cases in `sched_handle_dataset` already delegate to `sched_on_jobs`/`sched_on_workers` +- **`pushmsg` handler path** — the `Request_body_pushmsg` branch in `scheduler_req_handler` and the functions `sched_handle_jobpush`/`sched_handle_workerpush` are dead code; the `vtask_entry()` assembly layer in `src/lib/xcectx.c` rewrites all `pushmsg` into `datamsg` before the handler is called, so incremental push handling is never needed + +## Next Step + +Run `/kiro:spec-design scheduler-controller-msg-gap` to proceed to design phase. diff --git a/.kiro/specs/scheduler-controller-msg-gap/research.md b/.kiro/specs/scheduler-controller-msg-gap/research.md new file mode 100644 index 00000000..ec34a8ca --- /dev/null +++ b/.kiro/specs/scheduler-controller-msg-gap/research.md @@ -0,0 +1,98 @@ +# Research Log: scheduler-controller-msg-gap + +> Feature: scheduler-controller-msg-gap +> Created: 2026-05-11 + +## Discovery Type + +Extension (light discovery) — extending existing handler code in an established C codebase. + +## Summary + +The scheduler's `handler.c` dispatcher processes controller messages via `scheduler_req_handler`. Six message cases are stubbed out or empty-bodied. The `vtask_entry()` assembly layer in `src/lib/xcectx.c` rewrites all `pushmsg` into `datamsg`, making the `Request_body_pushmsg` branch dead code. Existing plugin hooks (`plugin_job_update`, `plugin_queue_update`, `plugin_worker_update`, `plugin_job_state_change`) exist for notifying plugins of state changes. A `plugin_job_rusage_update` hook does not yet exist and must be added. The `src/lib/helper.c` file already provides `xce_request_op_name()`, `xce_request_msg_name()`, and `xce_dataset_table()` — a new `xce_notify_event_name()` function is needed. + +## Research Log + +### Topic 1: Plugin Hook System + +**Source:** `src/scheduler/include/plugin.h`, `src/scheduler/src/plugin.c` + +**Findings:** +- Hooks follow a `plugin_*_update(void* sched, xce_sched_*_t*)` pattern +- Called after state mutation to notify registered plugins +- Hooks iterate registered plugins and call their function pointers +- `plugin_job_state_change(void*, xce_sched_job_t*, uint32_t)` takes an extra state parameter +- No `plugin_job_rusage_update` hook exists yet — must be added + +**Implication:** New handlers should call the corresponding plugin hook after mutating scheduler state. The rusage handler needs a new hook typedef and global function. + +### Topic 2: Object Conversion Functions + +**Source:** `src/scheduler/src/schedobj.c` + +**Findings:** +- `make_sched_job(void* arg, xce_job_t* j)` — maps jobid, times, state, user/queue/project pointers, resreq, mintasks/maxtasks, allocs, accounts +- `make_sched_queue(void* arg, xce_queue_t* q)` — maps name, attrs, status, priority, accept_interval, sched_delay, resreq +- `make_sched_worker(void* arg, xce_worker_t* w)` — maps name, status, idx, resvals zarray from `w->resvals_to_sched` +- `kill_sched_*` functions free their respective objects + +**Implication:** Job/queue/worker update handlers should use `make_sched_*` for create-or-update semantics. For updates (existing objects), we need update-in-place logic since `make_sched_*` always creates new objects. + +### Topic 3: Resource and Shared Instance Initialization + +**Source:** `src/scheduler/src/scheduler.c` + +**Findings:** +- `sched_init_resources()` — iterates `config->resources[]`, calls `makeResource(sched->resmgr, r)` for each, warns on NULL/duplicate +- `sched_init_sharedInst()` — iterates `config->resvals[]`, uses `MOVE_PTR` to steal val pointers into `sched->sharedInst` zarray +- Both called from `sched_init_config()` during initial config push +- For runtime updates, we need incremental versions that can add new or update existing resources + +**Implication:** Dataset handlers for `Data_data_resources` and `Data_data_resvals` need new update functions that can both create and modify existing entries. + +### Topic 4: Existing Helper Functions + +**Source:** `src/lib/helper.c`, `src/include/helper.h` + +**Findings:** +- `xce_request_op_name(xce_request_t* req)` — returns string for OPCode enum +- `xce_request_msg_name(xce_request_t* req)` — returns string for Request body tag +- `xce_dataset_table(int table)` — returns string for Data table type enum +- All return `"unknown"` for unrecognized values +- `xce_notify_event_name()` does NOT exist yet — must be created +- Signature pattern: `char* func_name(enumerated_type_or_struct_ptr)` returning `char*` + +**Implication:** Create `xce_notify_event_name(int event)` following same pattern. Also consider `char* xce_sched_op_name(xce_sched_op_t op)` for the update request op enum. + +### Topic 5: Cap'n Proto Enum Values (NotifyData) + +**Source:** `src/schema/controller.capnp` + +**Findings:** +All 19 NotifyData event tags: +- `jobPidInfo(0)`, `jobFailInfo(1)`, `jobDispatchFail(2)`, `jobFinishInfo(3)`, `schedStartJob(4)`, `schedExitJob(5)`, `schedFailJob(6)`, `schedUpdateJob(7)`, `schedUpdateResvals(8)`, `stoppedJob(9)`, `resumedJob(10)`, `ckptinfo(11)`, `schedUpdateQueue(12)`, `taskPidInfo(13)`, `taskFailInfo(14)`, `taskFinish(15)`, `postDoneJob(16)`, `schedJobRusages(17)`, `updateJob(18)` + +**Implication:** The `xce_notify_event_name()` function must cover all 19 values. Only the scheduler-relevant ones need handler implementations (schedUpdateJob, schedUpdateQueue, schedUpdateResvals, schedJobRusages are the gap items). + +### Topic 6: Dead Code Analysis + +**Source:** `src/lib/xcectx.c` lines 93-106, `src/scheduler/src/handler.c` lines 172-197 + +**Findings:** +- `vtask_entry()` accumulates `pushmsg` rows into `ctx->dataset`, then rewrites `body_which = Request_body_datamsg` and `body.datamsg = ctx->dataset` +- The `Request_body_pushmsg` case in `scheduler_req_handler` (lines 185-198) calls `sched_handle_jobpush()` and `sched_handle_workerpush()`, both of which are stubs returning 0 +- These functions can never be reached via normal message flow + +**Implication:** Safe to remove the pushmsg case branch and both stub functions. + +## Architecture Decisions + +1. **Handler pattern:** New notify handlers follow the same enqueue-to-pend-updates pattern as `sched_on_job_state_change` — create a `xce_sched_update_request_t`, set the op, enqueue via `enq_pend_update()`, process in `sched_update()` main loop. + +2. **Update-in-place for job/queue/worker:** For NotifyData update events, we need update semantics (find existing, mutate fields) rather than create semantics (make_sched_*). New `sched_update_job`, `sched_update_queue`, `sched_update_worker` functions will handle this. + +3. **New XCE_SCHED_OP enum values:** Need `XCE_SCHED_OP_UPDATE_RESVALS` for resource value updates and `XCE_SCHED_OP_JOB_RUSAGE` for rusage updates. + +4. **Plugin hook for rusage:** Add `plugin_job_rusage_hook_t` typedef and `plugin_job_rusage_update()` global function following existing patterns. + +5. **Helper function:** Add `xce_notify_event_name(int)` and `xce_sched_op_name(xce_sched_op_t)` to `src/lib/helper.c` and `src/include/helper.h`. \ No newline at end of file diff --git a/.kiro/specs/scheduler-controller-msg-gap/spec.json b/.kiro/specs/scheduler-controller-msg-gap/spec.json new file mode 100644 index 00000000..e3cfa6c1 --- /dev/null +++ b/.kiro/specs/scheduler-controller-msg-gap/spec.json @@ -0,0 +1,23 @@ +{ + "feature_name": "scheduler-controller-msg-gap", + "description": "Analyze the codebase to find gaps in the scheduler's handling of controller messages — missing message types, unhandled edge cases, or incomplete message processing paths", + "phase": "tasks-generated", + "created_at": "2026-05-11T01:34:00-07:00", + "updated_at": "2026-05-11T19:30:00-07:00", + "language": "en", + "status": "active", + "approvals": { + "requirements": { + "generated": true, + "approved": true + }, + "design": { + "generated": true, + "approved": true + }, + "tasks": { + "generated": true, + "approved": true + } + } +} \ No newline at end of file diff --git a/.kiro/specs/scheduler-controller-msg-gap/tasks.md b/.kiro/specs/scheduler-controller-msg-gap/tasks.md new file mode 100644 index 00000000..05f1976a --- /dev/null +++ b/.kiro/specs/scheduler-controller-msg-gap/tasks.md @@ -0,0 +1,127 @@ +# Tasks: scheduler-controller-msg-gap + +> Generated: 2026-05-11T19:30:00-07:00 +> Feature: scheduler-controller-msg-gap +> Status: Tasks Generated + +## Overview + +Implementation tasks for filling gaps in the scheduler's handling of controller messages. The work follows the design's build order: shared helpers first, then header definitions, plugin hooks, business logic, handler wiring, and finally tests. + +--- + +## Task 1: Add name-lookup helpers to lib/helper + +**Requirements:** 8 + +Add `xce_notify_event_name()` and `xce_sched_op_name()` to `src/lib/helper.c` and declare them in `src/include/helper.h`, following the exact pattern of the existing `xce_dataset_table()` helper (int argument, `char*` return, `"UNKNOWN(%d)"` fallback). + +- [x] 1.1 Declare `xce_notify_event_name(int event)` and `xce_sched_op_name(xce_sched_op_t op)` in `src/include/helper.h` +- [x] 1.2 Implement `xce_notify_event_name()` in `src/lib/helper.c` — switch on all `NotifyData_data_*` enum values; return symbolic string; fall back to `"UNKNOWN(%d)"` +- [x] 1.3 Implement `xce_sched_op_name()` in `src/lib/helper.c` — switch on all `XCE_SCHED_OP_*` enum values; return symbolic string; fall back to `"UNKNOWN(%d)"` + +--- + +## Task 2: Extend scheduler.h with new op types and request fields + +**Requirements:** 1, 3, 4, 5, 6 + +Add five new `xce_sched_op_t` enum values and five corresponding union members in `xce_sched_update_request_t` to carry the payloads for queue update, resource update, full resval update, incremental resval update, and job rusage. + +- [x] 2.1 Add `XCE_SCHED_OP_UPDATE_QUEUE`, `XCE_SCHED_OP_UPDATE_RESOURCES`, `XCE_SCHED_OP_UPDATE_RESVALS_FULL`, `XCE_SCHED_OP_UPDATE_RESVALS`, and `XCE_SCHED_OP_JOB_RUSAGE` to the `xce_sched_op_t` enum in `src/scheduler/include/scheduler.h` +- [x] 2.2 Add the corresponding named union members (`queueupdate`, `resourceupdate`, `resvalfullupdate`, `resvalupdate`, `job_rusage`) to `xce_sched_update_request_t` + +--- + +## Task 3: Add plugin rusage hook + +**Requirements:** 6 + +Add the `plugin_job_rusage_hook_t` typedef, the `rusage_hook` field in `plugin_hooks_t`, and the `plugin_job_rusage_update()` global function. This follows the identical pattern used by `plugin_job_state_change()`. + +- [x] 3.1 Add `plugin_job_rusage_hook_t` typedef and `rusage_hook` field to `plugin_hooks_t` in `src/scheduler/include/plugin.h` +- [x] 3.2 Implement `plugin_job_rusage_update()` in `src/scheduler/src/plugin.c` — iterate registered plugins and call their `rusage_hook` when non-NULL + +--- + +## Task 4: Add queue update function in schedobj.c + +**Requirements:** 4 + +Implement `sched_update_existing_queue()` in `src/scheduler/src/schedobj.c`. The function locates a queue by name via `sched_get_queue()`, updates its mutable fields (status, priority, accept_interval, sched_delay, resreq string), calls `plugin_queue_update()`, and returns `-ENOENT` if the queue is unknown. + +- [x] 4.1 Implement `sched_update_existing_queue(xce_scheduler_t*, xce_queue_t*)` in `src/scheduler/src/schedobj.c` with correct lookup, field update, string reallocation for resreq, plugin call, and error returns + +--- + +## Task 5: Extend scheduler.c dequeue processing and error logging + +**Requirements:** 1, 3, 4, 5, 6, 7 + +Wire the five new op types into the `switch (req->op)` in `sched_update()`, implement `sched_update_existing_job()`, and add WARN-level error logging after the switch when a handler returns non-zero. + +- [x] 5.1 Add `XCE_SCHED_OP_UPDATE_QUEUE` case — delegate to `sched_update_existing_queue()`, return `-ENOENT` when queue not found +- [x] 5.2 Add `XCE_SCHED_OP_UPDATE_RESOURCES` case — iterate `resdata->resources[]`, call `makeResource(sched->resmgr, r)` for each non-NULL entry +- [x] 5.3 Add `XCE_SCHED_OP_UPDATE_RESVALS_FULL` case — iterate `resvaldata->vals[]`, find resource in `sched->resmgr` by name, update value; log WARN and skip unknown resources +- [x] 5.4 Add `XCE_SCHED_OP_UPDATE_RESVALS` case — iterate `resvals->vals[]`, find worker by host name via `sched_get_worker()`, update worker's `resvals` zarray; silently skip unknown hosts +- [x] 5.5 Add `XCE_SCHED_OP_JOB_RUSAGE` case — find job by jobid, store rusage data, call `plugin_job_rusage_update()`; return `-ENOENT` if job not found +- [x] 5.6 Implement `sched_update_existing_job()` — find job by jobid via `sched_get_job()`, update mutable fields (state, priority, allocs), call `plugin_job_update()`; return `-ENOENT` when jobid is unknown +- [x] 5.7 Add WARN log in `sched_update()` after switch block for non-zero `rc` — include symbolic op name via `xce_sched_op_name()` and `strerror(-rc)` +- [x] 5.8 Extend `sched_pend_update_killer` with no-op cases for all five new op types to suppress unhandled-case compiler warnings + +--- + +## Task 6: Wire notify handlers in handler.c + +**Requirements:** 3, 4, 5, 6, 8 + +Add four static handler functions for the currently-stubbed NotifyData events, wire them into `sched_handle_notify()`, and update the TRACE log calls in `sched_handle_notify()` and `sched_handle_dataset()` to use symbolic name helpers. + +- [x] 6.1 Implement `sched_handle_sched_update_job()` — validate non-NULL, enqueue `XCE_SCHED_OP_UPDATE_JOB`; return `-EINVAL` on NULL, `-ENOMEM` on alloc failure +- [x] 6.2 Implement `sched_handle_sched_update_queue()` — validate non-NULL, enqueue `XCE_SCHED_OP_UPDATE_QUEUE`; return `-EINVAL` on NULL, `-ENOMEM` on alloc failure +- [x] 6.3 Implement `sched_handle_sched_update_resvals()` — skip if NULL or count==0, enqueue `XCE_SCHED_OP_UPDATE_RESVALS`; return `-ENOMEM` on alloc failure +- [x] 6.4 Implement `sched_handle_sched_job_rusage()` — validate non-NULL, enqueue `XCE_SCHED_OP_JOB_RUSAGE`; return `-EINVAL` on NULL, `-ENOMEM` on alloc failure +- [x] 6.5 Wire all four handlers into `sched_handle_notify()` switch — replace stub `break` / TRACE-only cases +- [x] 6.6 Update TRACE log in `sched_handle_dataset()` from `%d` table integer to `%s` via `xce_dataset_table()` (8.2), and update notify event logs to use `xce_notify_event_name()` (8.3) + +--- + +## Task 7: Add dataset handlers in handler.c + +**Requirements:** 1, 8 + +Add `sched_handle_resources()` and `sched_handle_resvals()` static functions and wire them into the `sched_handle_dataset()` switch for the two currently-empty Data table cases. Update the existing opcode log in `scheduler_req_handler` to use `xce_request_op_name()`. + +- [x] 7.1 Implement `sched_handle_resources()` — skip if NULL or count==0, enqueue `XCE_SCHED_OP_UPDATE_RESOURCES`; return `-ENOMEM` on alloc failure +- [x] 7.2 Implement `sched_handle_resvals()` — skip if NULL or count==0, enqueue `XCE_SCHED_OP_UPDATE_RESVALS_FULL`; return `-ENOMEM` on alloc failure +- [x] 7.3 Wire `sched_handle_resources()` and `sched_handle_resvals()` into `sched_handle_dataset()` switch replacing the empty `break` cases +- [x] 7.4 Update opcode log in `scheduler_req_handler` to call `xce_request_op_name()` instead of printing the numeric value (8.1) + +--- + +## Task 8: Remove dead code pushmsg branches + +**Requirements:** 2 + +Delete the `Request_body_pushmsg` case block from `scheduler_req_handler()` and the static functions `sched_handle_jobpush()` and `sched_handle_workerpush()`. These paths are unreachable because `vtask_entry()` rewrites all `pushmsg` into `datamsg` before invoking the handler. + +- [x] 8.1 Remove the `Request_body_pushmsg` case and its two nested branches (`job`/`worker`) from `scheduler_req_handler()` in `src/scheduler/src/handler.c` +- [x] 8.2 Remove the static function definitions `sched_handle_jobpush()` and `sched_handle_workerpush()` from `src/scheduler/src/handler.c` + +--- + +## Task 9: Write unit tests for all new message handlers + +**Requirements:** 1, 2, 3, 4, 5, 6, 7, 8 + +Extend `src/scheduler/testsuite/test.c` with unit tests covering every acceptance criterion. Tests must run without a live server — use stub schedulers, table-driven cases, and the existing testsuite harness. + +- [x] 9.1 Test `xce_notify_event_name()` and `xce_sched_op_name()` — verify known values return correct strings, unknown values return `"UNKNOWN(%d)"` format +- [x] 9.2 Test `sched_handle_resources()` and `sched_handle_resvals()` handler functions — verify enqueue on valid input, skip on NULL/empty, `-ENOMEM` path +- [x] 9.3 Test `sched_handle_sched_update_job()`, `sched_handle_sched_update_queue()`, `sched_handle_sched_update_resvals()`, `sched_handle_sched_job_rusage()` — verify `-EINVAL` on NULL, enqueue on valid input +- [x] 9.4 Test `sched_update_existing_job()` — verify `-ENOENT` for unknown jobid, correct field update and plugin call for known jobid +- [x] 9.5 Test `sched_update_existing_queue()` — verify `-ENOENT` for unknown queue, correct field update and plugin call for known queue; verify `-EINVAL` on NULL payload +- [x] 9.6 Test dequeue path for `XCE_SCHED_OP_UPDATE_RESVALS` — verify unknown host skipped, known host updated +- [x] 9.7 Test dequeue path for `XCE_SCHED_OP_JOB_RUSAGE` — verify `-ENOENT` for unknown jobid, rusage stored and plugin called for known jobid; verify `-EINVAL` on NULL rusage +- [x] 9.8 Verify dead code symbols are absent — compile-time check that `sched_handle_jobpush` / `sched_handle_workerpush` are not defined (2.1, 2.2) +- [ ]* 9.9 Verify WARN log is emitted in `sched_update()` when any handler returns a non-zero error code (optional — may require log capture infrastructure)