435 lines
16 KiB
Markdown
435 lines
16 KiB
Markdown
# 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.
|