diff --git a/.gitignore b/.gitignore index 3396de60..7209775f 100644 --- a/.gitignore +++ b/.gitignore @@ -68,3 +68,7 @@ src/subprojects/.wraplock # --- de-nix build: packaging output (src/tools/xce-package.sh) --- src/dist/ + +# --- bug-1971 evidence archive: local test-failure snapshots (83M of logs), +# produced by src/tools/test-failure-evidence-archive.sh -- never checked in --- +src/testlogs.archive/ diff --git a/admin-plane-gaps.md b/admin-plane-gaps.md index 6ac12495..bebd110d 100644 --- a/admin-plane-gaps.md +++ b/admin-plane-gaps.md @@ -197,5 +197,29 @@ - 显示面终验：push priority 45→46→47（无 apiserver 重启）后 `xce queues list` 即时显示 47；作业 `xce jobs submit -q short` → DONE 全流程过，四 daemon 健康。 - 运维注意：build.dbg 产物是 system-interpreter + nix-lib 混链（裸跑段错误）；部署须用 `patchelf --set-interpreter /nix/.../glibc-2.42-47/lib/ld-linux-x86-64.so.2 --set-rpath ` 后再 `--help` 预检。 -### 断点 ②：YAML flag/resourcemap 接线（待做） +### 断点 ②：YAML flag/resourcemap 接线（已实施；单测全绿 + t1 E2E 全通过） + +**四处修复**（admin-plane-gaps 1.10）： + +1. **schema**（src/schema/controller.capnp）：`Resource.distributions @8` / `hostBased @9` 取消注释（此前被注释掉——resourcemap 的目标字段根本不在 wire 上）。ResourceDistribution 结构本就活跃、编解码器早已生成，schema 改动后 encode/decode/free_Resource 自动带上 distributions（`decode_ResourceDistribution_list` 分配的正是 `res_distribution_t*`，与 C 结构零胶水）。 + +2. **resobj 深拷贝**（src/lib/resobj.c）：`resobj_init` 的 `#if 0 /* FIXME */` 存根替换为真深拷贝——distributions 逐项深拷（value 按 which 拷贝、svalue strdup、hosts strdup），hbased_defined 深拷；`resobj_destroy` 对应释放。**必须深拷不能 MOVE_PTR**：源是 myconf->cluster_config（持续持有并在下次激活时 free），MOVE_PTR 会把配置树偷空。新增 `resval_deep_copy`/`resval_free` 静态助手（与 update_val_res/resval_obj_killer 同一所有权约定）。调度器 makeResource/resourceApplyDef 路径同文件同修复（推送上来的定义也能带 distributions）。 + +3. **控制器 reconcile**（src/controller/cluster.c + config_repl.c）：新增 `reconcile_config_resources(c)`——`cluster_build_resources`（boot 建条目、激活重跑 EEXIST 容忍）+ `seed_sharedinst_from_config`（对 SHARED|GLOBAL 且 distributions[0] 带 value 的资源，deep-copy 该 value 进 `c->sharedinst[live->idx]`，zarray_put 换值语义=幂等激活直接热更新容量）。**seed 用 resmgr 的运行时 idx（key_add 分配），不能用 config 自带的 idx 字段（YAML 来源恒 0）**。调用点三处：boot hook（xce_config_init）、config_activate_committed 的 -ENOENT fallback 路径与 hash-verified 路径（均在 `xcesched_push_configs` 之前，保证下一次 conf4sched push 即携带 seed）。调度器零改动——core_apply_config_resvals/sched_init_sharedInst 本就把 conf4sched.resvals 送进 core->sharedInst，calc_wanted_global_resvals 以 XCE_RESF_GLOBAL 门控 job wants。 + - **YAML 映射规则**（mapping.py）：只有 `location: [all]` + `value` 形态置 `SHARED|GLOBAL`（0x08|0x04，即 license 池场景）；其余形态（host 子集/多段/per-host）忠实映射进 distributions 但不置位——host 子集静态共享池需要 elastic-shared-resources 调度器工作，不在本次范围。 + +4. **Python 工具链**（xceadmin）：mapping.py 新增 `RESOURCE_FLAG_SHARED/_GLOBAL/_SHARED_GLOBAL` 常量 + `_build_resource_value`（YAML 值按类型选 union 成员：bool→bvalue、number→fvalue、string→svalue，经 `.u` 组访问）+ `_build_resource` 映射 resourcemap→distributions；build.py 移除 `_UNMAPPED_BUT_VALID_ITEM_KEYS["resources"]` 的 resourcemap 条目、`_format_resource` 增加 `shared=true` 与 `distributions=[hosts]=value` 渲染（`(reported)` 表示动态无 value 形态）。schema 单点编辑即可——meson-build.sh 构建时把 src/schema/controller.capnp 同步进 xcecommon/schema_files/。 + +**单测**（全绿）： +- Python：`test_mapping.py` 新增 `test_map_list_section_resourcemap_maps_distributions_and_all_flag`（四形态逐一断言）+ `test_map_list_section_resourcemap_real_repo_configs`（真 configs/resources.yaml 全量分布数核对）；`test_config_build.py` 的过时 `test_build_warns_not_fails_on_schema_valid_unmapped_resourcemap_field` 替换为 `test_build_maps_resourcemap_to_distributions`；full golden `expected.bin` 再生（5312→6696 字节，distributions 上 wire 的直接证据；minimal fixture 无 resources.yaml 不受影响）。373 用例全绿。 +- C：`test_config_resource_reconcile.c` 两个用例——注册+seed+深拷非别名断言、SHARED-only/无 value/非 shared 三类不 seed 断言、幂等重跑+容量热更新（100→250）断言；controller 套件 1347 tests 全绿，ASAN 套件 6/6 OK。 + +**语义注意**：`resourceApplyDef`（调度器）只比较 valtype/ordtype/flags/interval——已注册资源定义变更（含 distributions）报 RES_APPLY_CHANGED 但不 apply（重启拾取）；而控制器侧 sharedinst seed 走 value 换入，容量热更新。分布结构变更（新增 host 段）目前不热更新到已注册调度器对象，与既有"定义变更需重启"语义一致。 + +**E2E 结果（2026-10-23 t1，14:20–14:28）**：修复后 controller 部署重启 → boot 双 reconcile 正常（14:20:30 pre-election 激活建 4 队列 + phase2 xce_config_init 二次 reconcile，旧代码在此中毒）→ scheduler 重连，conf4sched push 完整应用 → 作业流全通过：恢复的 12:41 卡死 PEND 作业走到 DONE；rusage[staticres4=1] 作业 PEND→RUN→DONE；30 作业饱和压测（24 槽全占 + 6 PEND，槽释放后全部收敛 DONE）；live config push（gen 12 激活，4 队列 "updated by configuration activation" + re-push 到已连接 scheduler）**不再崩溃**——这正是 12:41:13 旧代码 segfault 的同形场景。全部 4 daemon 健康，t1/t2/t3 OK。 + +**修复中发现并修掉的深层缺陷（queue 载荷所有权）**：bp2 最初只修了 sched.c 的 double-free；t1 上 12:41:13 的 encode_Threshold_list segfault 根因更深——**queue 的 capnp 载荷（users/hosts/thresholds/windows）在创建路径（qobj.c queue_init）被 MOVE_PTR 消耗掉定义**，而 reconcile_config_queues 对同一 cluster_config 对象会跑多次（boot：pre-election 激活一次 + phase2 build_queues 又一次），第二次 refresh 读到已掏空的定义：thresholds 是 count>0/array=NULL 的毒对（MOVE_PTR 置空指针但不落 count），generate 的 encode_Threshold_list 无 NULL 卫兵，下次队列/worker push 即崩。修复＝创建与刷新两条路径都改**深拷贝**（定义归 cluster_config 持有并在激活换装时 free；镜像 resobj_init distributions 深拷与 worker.c apply_join_state 的 dup 模式）：qobj.c queue_init 深拷 users/hosts/thresholds/windows_config；queue.c queue_refresh_def 深拷同四项（定义里 NULL/0 从此真表示"配置无此列表"，刷新清空运行时队列是正确语义）；新增 `dup_group_config`（src/lib/utils.c，生成代码无此 dup）。UT：test_reconcile_thresholds_survive_second_pass 驱动三连 reconcile + 真 encode_QueueConf→wire→decode 回读断言（1348/1348 绿；全套件扫描 sched 99 个/worker/harness/xcelib/xceadmin 375 全绿，sched_repl_trace_baseline_test 一个既有失败经 stash 验证与本改动无关）。 + +**运维观察（未修，记录）**：CLI `config push` 单节点 ackedCount==0 误报拒绝（push_config_to_standbys 只数 standby 传输层 ACK，controller 端实际成功——以 controller 日志为准）；scheduler 对已注册资源定义变更报 WARN 不 apply（重启拾取——本次 scheduler 重启后 staticres4/res1 正常注册，作业可调度）；饱和时 PEND reason 显示 `-`（PEND-reason 发布链路对 NO_RESOURCE 场景仍未出端到端值，P1 遗留）；t1 controller 无 core dump（ulimit 0）——12:41 segfault 现场只能靠 objdump 模式匹配定位。 + ### 断点 ③：done 作业 live GC（待做） diff --git a/src/controller/cluster.c b/src/controller/cluster.c index bc096d4e..a2810a23 100644 --- a/src/controller/cluster.c +++ b/src/controller/cluster.c @@ -1552,6 +1552,140 @@ int cluster_build_resources(cluster_t* c) return 0; } +static void resval_obj_killer(void* arg); + +/* admin-plane-gaps 1.10 (YAML resourcemap wiring): seed c->sharedinst[idx] + * for every config-declared STATIC SHARED|GLOBAL resource whose + * distributions carry a value (the YAML `location: [all]` + `value:` shape + * -- the license-pool scenario). The scheduler's two-phase shared-pool + * machinery (core_apply_config_resvals -> core->sharedInst, gated on + * XCE_RESF_GLOBAL in calc_wanted_global_resvals) consumes exactly these + * resvals from every conf4sched push, so seeding them here makes a + * YAML-declared shared pool schedulable WITHOUT any worker reporting it -- + * jobs asking rusage[res=1] are admitted up to the declared capacity, and + * get PREASON_NO_RESOURCE once it is exhausted. + * + * Pattern mirrors update_val_res (worker.c): the array owns the xce_val_t + * (calloc'd here, string values privately strdup'd), and zarray_put's + * killer (resval_obj_killer) frees a replaced slot -- so re-running after + * a config activation that changed the value simply swaps in the new one. + * The distribution value is DEEP-COPIED out of myconf->cluster_config, + * which keeps owning its decoded tree. + * + * Per-host-value distributions (the other resourcemap shapes) are + * intentionally NOT seeded: host-subset static pooling needs the planned + * elastic-shared-resources scheduler work (admin-plane-gaps 1.10); today + * they stay definition-only, exactly as before this change. */ +static void seed_sharedinst_from_config(cluster_t* c) +{ + int i; + + if ((c == NULL) || (c->myconf == NULL) + || (c->myconf->cluster_config == NULL)) { + return; + } + + for (i = 0; i < c->myconf->cluster_config->nres; i++) { + xce_resource_t* r = c->myconf->cluster_config->resources[i]; + const res_distribution_t* d; + xce_resource_t* live; + xce_val_t* v; + + if ((r == NULL) || (r->name == NULL)) { + continue; + } + if ((r->flags & XCE_RESF_SHARED) == 0 + || (r->flags & XCE_RESF_GLOBAL) == 0) { + continue; + } + if (r->ndistributions < 1 || r->distributions == NULL + || r->distributions[0] == NULL || r->distributions[0]->value == NULL) { + continue; + } + d = r->distributions[0]; + + /* Seed at the RUNTIME idx: resmgr idx values are key_add-assigned + * (makeResource), not config-declared -- the decoded config's own + * idx field is unset (0) for YAML-sourced definitions. */ + lock(c->resmgr, RTTI_LOCK_READ); + key_find(c->resmgr, (void*)r->name, (void**)&live); + unlock(c->resmgr); + if (live == NULL) { + WARN("seed_sharedinst: resource %s not found in resmgr -- " + "shared pool stays unseeded until the next config activation", + r->name); + continue; + } + + v = (xce_val_t*)calloc(1, sizeof(*v)); + if (v == NULL) { + WARN("seed_sharedinst: resource %s: out of memory -- shared " + "pool stays unseeded until the next config activation", + r->name); + continue; + } + if (d->value->which == Value_u_svalue) { + v->which = Value_u_svalue; + v->u.svalue = strdup(d->value->u.svalue != NULL + ? d->value->u.svalue : ""); + if (v->u.svalue == NULL) { + FREEUP(v); + WARN("seed_sharedinst: resource %s: out of memory -- shared " + "pool stays unseeded until the next config activation", + r->name); + continue; + } + } else { + memcpy(v, d->value, sizeof(*v)); + } + + if (zarray_put(&(c->sharedinst), live->idx, v) < 0) { + resval_obj_killer(v); + WARN("seed_sharedinst: resource %s: zarray_put idx %d failed -- " + "shared pool stays unseeded until the next config activation", + r->name, live->idx); + continue; + } + INFO("seeded shared resource %s (idx %d) from configuration", + r->name, live->idx); + } +} + +/* admin-plane-gaps 1.10: materialize the active config's resource + * definitions into the runtime resmgr and seed static shared pools from + * them. Twin of reconcile_config_queues (bug-1469): makeResource tolerates + * EEXIST on re-run (definitions are registered at boot; a re-run after a + * config activation keeps the existing object -- resourceApplyDef's + * change-detection contract applies to the push path), and the sharedinst + * seeding is a value swap via zarray_put, so the whole pass is idempotent. + * Call at boot (xce_config_init) and on both config_activate_committed + * activation paths, BEFORE xcesched_push_configs so the scheduler's very + * next conf4sched push carries the seeded resvals. */ +int reconcile_config_resources(cluster_t* c) +{ + int rc; + + if ((c == NULL) || (c->resmgr == NULL) || (c->myconf == NULL)) { + return 0; + } + + rc = cluster_build_resources(c); + if (rc < 0 && rc != -EEXIST) { + /* EEXIST = a re-run (config activation after boot / a previous + * activation): definitions are already registered, keep them -- + * the seeding below still applies. Any other error (ENOMEM) is + * reported so the caller logs it, but the seeding still runs for + * the definitions that DID register. */ + WARN("%s: cluster_build_resources rc=%d -- keeping previous " + "definitions", __func__, rc); + return rc; + } + + seed_sharedinst_from_config(c); + + return 0; +} + int cluster_load_jobs(cluster_t* c) { load_jobs(c); @@ -1590,7 +1724,7 @@ static int xce_config_init(int type, int situation, void* data, va_list ap) if (rc < 0) { return rc; } - rc = cluster_build_resources(c); + rc = reconcile_config_resources(c); if (rc < 0) { return rc; } diff --git a/src/controller/config_repl.c b/src/controller/config_repl.c index de5b0685..63291d7a 100644 --- a/src/controller/config_repl.c +++ b/src/controller/config_repl.c @@ -337,6 +337,9 @@ int config_activate_committed(cluster_t *c) "load_primary_config fallback rc=%d", lrc); if (lrc == 0) { reconcile_config_queues(c, 1); /* bug-1469 */ + /* admin-plane-gaps 1.10: materialize resource definitions + + * seed static shared pools (see the hash-verified path below). */ + (void)reconcile_config_resources(c); /* admin-plane-gaps 1.1: same confvar refresh + live-scheduler * re-push as the hash-verified activation path below (this * fallback genuinely activates locally-held bytes on recovery, @@ -399,6 +402,15 @@ int config_activate_committed(cluster_t *c) * a restart -- build_queues() only ran at boot). */ reconcile_config_queues(c, 1); + /* admin-plane-gaps 1.10 (YAML resourcemap wiring): register any NEW + * resource definitions the activated config added and re-seed + * c->sharedinst for static SHARED|GLOBAL resources (the YAML + * `location: [all]` + `value:` shape) -- without this, a YAML-declared + * shared pool only existed on the scheduler after a controller + * restart. Runs BEFORE xcesched_push_configs so the very next + * conf4sched push carries the seeded resvals to the live scheduler. */ + (void)reconcile_config_resources(c); + /* admin-plane-gaps 1.1 (config reachability): materialize the rest of * the activated configuration into live runtime state and re-push it to * the connected scheduler, so a running scheduler picks the change up diff --git a/src/controller/include/cluster.h b/src/controller/include/cluster.h index dcd83cbc..9eeb6641 100644 --- a/src/controller/include/cluster.h +++ b/src/controller/include/cluster.h @@ -114,6 +114,15 @@ extern int build_queues(void*); * that edits a queue hot-applies on the controller too. Returns the * number created (refreshed queues are reported via TRACE, not the return). */ extern int reconcile_config_queues(cluster_t*, int notify); +/* admin-plane-gaps 1.10: register every resource definition in the active + * config with the runtime resmgr (create-or-keep, EEXIST tolerated) and + * seed c->sharedinst for static SHARED|GLOBAL resources whose + * distributions carry a value (the YAML `location: [all]` + `value:` + * shape -- the license-pool scenario). Idempotent: safe to call from the + * boot hook and from both config_activate_committed activation paths. + * Callers wanting the seeds to reach a live scheduler must run (or have + * run) xcesched_push_configs afterward. */ +extern int reconcile_config_resources(cluster_t*); /* admin-plane-gaps 1.1: (re)build/refresh the confvar entries the * scheduler/worker config pushes export. Idempotent -- creates on first * run (boot), refreshes in place on later runs (config activation). */ diff --git a/src/controller/meson.build b/src/controller/meson.build index 0be6fe6a..de90a38a 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -159,6 +159,7 @@ controller_unit_test_src = [ 'testsuite/test_phase2_cold_boot.c', 'testsuite/test_persist_encryption.c', 'testsuite/test_config_queue_reconcile.c', + 'testsuite/test_config_resource_reconcile.c', 'testsuite/test_wal_job_rusage_fact.c', 'testsuite/test_wal_job_ckpt_fact.c', 'testsuite/test_wal_job_preason_fact.c', diff --git a/src/controller/queue.c b/src/controller/queue.c index 7efed268..7ed52961 100644 --- a/src/controller/queue.c +++ b/src/controller/queue.c @@ -5,6 +5,7 @@ #include "cluster.h" #include "persist.h" #include "qobj.h" +#include "utils.h" /* dup_group_config/dup_resource_thresholds/dup_timewindow_config */ #include "xce/cluster_def.h" #include "xce/types.h" #include "xstring.h" @@ -52,25 +53,31 @@ int build_queues(void* c) * scheduler's bug-1886 in-place re-apply in sched_init_queues()/ * core_upsert_queue()). Called under `q` RTTI_LOCK_WRITE. * - * Ownership notes (mirrors qobj.c's queue_init, which built these fields - * originally): + * Ownership notes (mirrors qobj.c's queue_init, which since the + * admin-plane-gaps bp2 fix deep-copies these same payloads at creation): * - resreq/pre_exec/post_exec are interned xstrings -- dereference the * old, makeString the new (never free()). * - users/hosts/thresholds/windows are decoded capnp payloads owned by - * the queue -- free with the generated free_* helpers, replace with - * the NEW definition's payloads via MOVE_PTR (the config's - * cluster_config owns the old copies and is retired by - * load_primary_config after activation; the runtime queue must not - * alias into it). + * the queue -- free the old copies with the generated free_* helpers + * and install DEEP COPIES of the definition's payloads. Never + * MOVE_PTR out of the definition: the config's cluster_config keeps + * owning (and later frees) its copies, and reconcile_config_queues + * runs MORE THAN ONCE over the SAME config object (boot fires it from + * the pre-election config_activate_committed AND from phase2's + * build_queues), so a MOVE left the definition drained and a second + * pass either cleared the live queue's payloads or adopted a poisoned + * (count>0, array=NULL) thresholds pair that segfaulted + * encode_Threshold_list on the next queue/worker push (t1 controller + * 2026-10-23 12:41:13). Mirrors worker.c apply_join_state's + * proven-safe dup pattern. * - status/priority/accept_interval/sched_delay/attrs are scalars -- * plain copies (config is the source of truth; a re-activation that * changed an admin state bit in the config applies it, and later WAL * re-applies of queue_ctrl facts still win on replay order). * - * Returns 0, or -ENOMEM when an interned-string allocation failed (the - * queue keeps its previous definition's strings in that case -- scalars - * may already have been copied, which is benign: the next activation - * re-applies). */ + * Returns 0, or -ENOMEM when an allocation failed (the queue keeps its + * previous definition's payloads in that case -- scalars may already have + * been copied, which is benign: the next activation re-applies). */ static int queue_refresh_def(queue_t* q, xce_queue_t* qdef) { char* ns; @@ -93,29 +100,61 @@ static int queue_refresh_def(queue_t* q, xce_queue_t* qdef) if (q->queue.users != NULL) { free_GroupConfig_ptr(&(q->queue.users)); } - MOVE_PTR(q->queue.users, qdef->users); + if (qdef->users != NULL) { + q->queue.users = dup_group_config(qdef->users); + if (q->queue.users == NULL) { + return -ENOMEM; + } + } if (q->queue.hosts != NULL) { free_GroupConfig_ptr(&(q->queue.hosts)); } - MOVE_PTR(q->queue.hosts, qdef->hosts); + if (qdef->hosts != NULL) { + q->queue.hosts = dup_group_config(qdef->hosts); + if (q->queue.hosts == NULL) { + return -ENOMEM; + } + } if (q->queue.nthresholds > 0 && q->queue.thresholds != NULL) { free_Threshold_list(q->queue.nthresholds, q->queue.thresholds); - q->queue.thresholds = NULL; + q->queue.thresholds = NULL; + q->queue.nthresholds = 0; } - q->queue.nthresholds = qdef->nthresholds; - MOVE_PTR(q->queue.thresholds, qdef->thresholds); + if (qdef->thresholds != NULL && qdef->nthresholds > 0) { + q->queue.thresholds = dup_resource_thresholds(qdef->thresholds, + qdef->nthresholds); + if (q->queue.thresholds == NULL) { + return -ENOMEM; + } + q->queue.nthresholds = qdef->nthresholds; + } + /* else: the definition carries no thresholds (or a poisoned + * (count>0, NULL) shape, which we refuse to adopt) -- the runtime + * queue ends at (0, NULL), which encodes as an empty list. */ if (q->queue.runwindow_config != NULL) { free_TimeWindow_ptr(&(q->queue.runwindow_config)); } - MOVE_PTR(q->queue.runwindow_config, qdef->runwindow_config); + if (qdef->runwindow_config != NULL) { + q->queue.runwindow_config + = dup_timewindow_config(qdef->runwindow_config); + if (q->queue.runwindow_config == NULL) { + return -ENOMEM; + } + } if (q->queue.dispatchwindow_config != NULL) { free_TimeWindow_ptr(&(q->queue.dispatchwindow_config)); } - MOVE_PTR(q->queue.dispatchwindow_config, qdef->dispatchwindow_config); + if (qdef->dispatchwindow_config != NULL) { + q->queue.dispatchwindow_config + = dup_timewindow_config(qdef->dispatchwindow_config); + if (q->queue.dispatchwindow_config == NULL) { + return -ENOMEM; + } + } ns = makeString(qdef->pre_exec); if (ns == NULL && qdef->pre_exec != NULL) { diff --git a/src/controller/sched.c b/src/controller/sched.c index 33f6e1eb..c976e641 100644 --- a/src/controller/sched.c +++ b/src/controller/sched.c @@ -212,6 +212,20 @@ int xcesched_push_configs(cluster_t* c) free_name_val_list(&(configdata.data.conf4sched->resvals), &(configdata.data.conf4sched->nresvals)); + /* free_name_val_list() frees the resval ENTRIES *and the backing + * array* (utils.c FREEUP(resvals)) -- the array xreslist.data still + * aliases -- and NULLs only its own argument. Detach the list's + * ownership so the xlist_fini() below does not free the same array a + * second time (the ownership class bug-1506 fixed in sched_worker_ + * change()). Latent since inception: free_name_val_list() early- + * returns WITHOUT freeing the array when count<=0, so every push + * before admin-plane-gaps 1.10's sharedinst seeding ran with + * nresvals==0 and never armed it -- the first seeded push aborted the + * controller with glibc "double free or corruption (out)". */ + xreslist.data = NULL; + xreslist.count = 0; + xreslist.caps = 0; + FREEUP(configdata.data.conf4sched); unlock_collect_queues(&qlist); diff --git a/src/controller/testsuite/controller_test.c b/src/controller/testsuite/controller_test.c index 7ba1f11b..1072edd9 100644 --- a/src/controller/testsuite/controller_test.c +++ b/src/controller/testsuite/controller_test.c @@ -90,6 +90,7 @@ extern void test_wal_job_fail_fact(void); extern void test_phase2_cold_boot(void); extern void test_persist_encryption(void); extern void test_config_queue_reconcile(void); +extern void test_config_resource_reconcile(void); extern void test_wal_job_rusage_fact(void); extern void test_wal_job_ckpt_fact(void); extern void test_wal_job_preason_fact(void); @@ -304,6 +305,7 @@ int main(int argc, char* argv[]) TEST(test_phase2_cold_boot); TEST(test_persist_encryption); TEST(test_config_queue_reconcile); + TEST(test_config_resource_reconcile); TEST(test_wal_job_rusage_fact); TEST(test_wal_job_ckpt_fact); TEST(test_wal_job_preason_fact); diff --git a/src/controller/testsuite/test_config_queue_reconcile.c b/src/controller/testsuite/test_config_queue_reconcile.c index 0f836333..f54ce57a 100644 --- a/src/controller/testsuite/test_config_queue_reconcile.c +++ b/src/controller/testsuite/test_config_queue_reconcile.c @@ -11,15 +11,18 @@ #include #include +#include #include #include #include "clusterconfig.h" #include "cluster.h" +#include "controller.capnp.h" #include "hook.h" #include "myconf.h" #include "qobj.h" +#include "utils.h" #include "xce/cluster_def.h" #include "xstring.h" @@ -180,9 +183,175 @@ static void test_reconcile_refreshes_existing_queue_in_place(void) dereference(c.qmgr); } +/* 2026-10-23 t1 controller segfault (encode_Threshold_list): the create path + * (qobj.c queue_init) MOVED the config definition's payloads out (users/ + * hosts/thresholds/windows -- MOVE_PTR nulls the src pointers, and for + * thresholds left src->nthresholds stale), and queue_refresh_def MOVED them + * out again on refresh. reconcile_config_queues runs MORE THAN ONCE over + * the SAME cluster_config object (boot: pre-election config_activate_committed + * AND phase2's build_queues), so the second pass "refreshed" the live queue + * from the drained definition: thresholds became the poisoned (count>0, + * array=NULL) pair, the generated encode_Threshold_list has no NULL guard, + * and the next queue/worker push dereferenced the NULL array base -- the + * controller died. The fix makes BOTH paths deep-copy (the definition's + * cluster_config keeps owning its payloads and frees them at swap; mirrors + * resobj_init's distributions deep-copy and worker.c apply_join_state's dup + * pattern), so a definition is never consumed, (NULL/0) in it genuinely + * means "no list in this config", and reconcile is idempotent. This test + * drives the poison scenario end to end: a queue carrying real thresholds + * and membership must survive a triple reconcile with values intact, the + * definition must stay untouched, and the encodable invariant + * (count>0 <=> array!=NULL) must hold throughout. */ +static void test_reconcile_thresholds_survive_second_pass(void) +{ + kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING, + .nslots = 16, .delta = 8 }; + cluster_t c; + myconf_t conf; + cluster_config_t cfg; + xce_queue_t qa; + xce_res_threshold_t thr; + xce_res_threshold_t* thr_arr[1]; + xce_queue_t* queues[1]; + queue_t* q; + + if (makeString((char*)"cqr-probe") == NULL) + xstring_table_init(64); + + memset(&c, 0, sizeof(c)); + memset(&conf, 0, sizeof(conf)); + memset(&cfg, 0, sizeof(cfg)); + TEST_ASSERT_EQUAL_INT(0, xce_hook_init(&c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + c.qmgr = makeInstance(&xkvmgr_type, &str_params); + TEST_ASSERT_NOT_NULL(c.qmgr); + + /* A queue definition that actually carries a sched_stop threshold (the + * "night" queue in src/configs/queues.yaml is the real-world shape). + * The definition is created once and reconciled twice more, exactly + * like a config object that survives boot's build_queues AND the + * pre-election config_activate_committed. */ + memset(&qa, 0, sizeof(qa)); + qa.queue = (char*)"cqr-thr"; + qa.nthresholds = 1; + memset(&thr, 0, sizeof(thr)); + thr.name = (char*)"tmp"; + thr.sched = 2000; + thr.stop = 1000; + thr_arr[0] = &thr; + qa.thresholds = thr_arr; + queues[0] = &qa; + cfg.nqueues = 1; + cfg.queues = queues; + conf.cluster_config = &cfg; + c.myconf = &conf; + + /* First pass (boot's build_queues): queue created, thresholds + * deep-copied in; the definition keeps ownership of its array. */ + TEST_ASSERT_EQUAL_INT(0, build_queues(&c)); + q = cqr_find(&c, "cqr-thr"); + TEST_ASSERT_NOT_NULL(q); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, q->queue.nthresholds, + "first reconcile must adopt the thresholds count"); + TEST_ASSERT_NOT_NULL_MESSAGE(q->queue.thresholds, + "first reconcile must adopt the thresholds array"); + TEST_ASSERT_EQUAL_STRING("tmp", q->queue.thresholds[0]->name); + /* (UNITY_INCLUDE_DOUBLE is off in this build -- compare as bits.) */ + TEST_ASSERT_EQUAL_MEMORY(&thr.sched, &q->queue.thresholds[0]->sched, + sizeof(double)); + TEST_ASSERT_EQUAL_MEMORY(&thr.stop, &q->queue.thresholds[0]->stop, + sizeof(double)); + + /* The create path must NOT have consumed the definition (the old + * MOVE_PTR drained it to (0, dangling-array) -- the poison seed). */ + TEST_ASSERT_EQUAL_INT_MESSAGE(1, qa.nthresholds, + "create must deep-copy, not MOVE the definition's thresholds out"); + TEST_ASSERT_EQUAL_PTR_MESSAGE(thr_arr, qa.thresholds, + "create must not steal the definition's thresholds array"); + + /* Second pass over the SAME config object (boot phase2): the refresh + * re-copies from the intact definition -- the live queue must stay + * (1, non-NULL). The old code left it (count>0, array=NULL), and the + * next encode_Threshold_list segfaulted on the NULL base. */ + TEST_ASSERT_EQUAL_INT(0, reconcile_config_queues(&c, 1)); + TEST_ASSERT_EQUAL_PTR_MESSAGE(q, cqr_find(&c, "cqr-thr"), + "refresh must stay in place"); + TEST_ASSERT_EQUAL_INT_MESSAGE(1, q->queue.nthresholds, + "second reconcile must keep the thresholds count"); + TEST_ASSERT_NOT_NULL_MESSAGE(q->queue.thresholds, + "2026-10-23 t1: the double reconcile used to leave (count>0, NULL), " + "segfaulting the next encode_Threshold_list"); + TEST_ASSERT_EQUAL_STRING("tmp", q->queue.thresholds[0]->name); + TEST_ASSERT_EQUAL_MEMORY(&thr.sched, &q->queue.thresholds[0]->sched, + sizeof(double)); + + /* The definition must STILL be intact after the refresh pass too + * (refresh deep-copies as well -- never MOVEs out of the config). */ + TEST_ASSERT_EQUAL_INT(1, qa.nthresholds); + TEST_ASSERT_EQUAL_PTR(thr_arr, qa.thresholds); + + /* Third pass still consistent (idempotent). */ + TEST_ASSERT_EQUAL_INT(0, reconcile_config_queues(&c, 1)); + TEST_ASSERT_EQUAL_INT(1, q->queue.nthresholds); + TEST_ASSERT_NOT_NULL(q->queue.thresholds); + TEST_ASSERT_EQUAL_STRING("tmp", q->queue.thresholds[0]->name); + + /* Sanity: the encodable invariant the crash violated. */ + TEST_ASSERT_TRUE((q->queue.nthresholds > 0) == + (q->queue.thresholds != NULL)); + + /* End to end: encode the LIVE queue exactly like the conf4sched push + * does (xcesched_push_configs -> encode_QueueConf -> encode_Threshold_ + * list). The generated list encoder has no NULL guard, so before the + * fix the (count>0, NULL) shape the double reconcile produced crashed + * right here. Round-trip the encoded queue back and check the + * threshold survived the wire. */ + { + struct capn cap; + QueueConf_ptr qp; + int64_t sz; + uint8_t* buf; + struct capn dcap; + QueueConf_ptr dp; + struct QueueConf wire; + xce_queue_t back; + + memset(&back, 0, sizeof(back)); + capn_init_malloc(&cap); + encode_QueueConf_ptr(capn_root(&cap).seg, &qp, &q->queue); + capn_setp(capn_root(&cap), 0, qp.p); + sz = capn_size(&cap); + TEST_ASSERT_TRUE(sz > 0); + buf = (uint8_t*)malloc((size_t)sz); + TEST_ASSERT_NOT_NULL(buf); + /* capn_write_mem returns the bytes written (positive), not 0. */ + TEST_ASSERT_TRUE(capn_write_mem(&cap, buf, (size_t)sz, 0) > 0); + capn_free(&cap); + + TEST_ASSERT_EQUAL_INT(0, capn_init_mem(&dcap, buf, (size_t)sz, 0)); + dp.p = capn_getp(capn_root(&dcap), 0, 1); + read_QueueConf(&wire, dp); + decode_QueueConf(&back, &wire); + TEST_ASSERT_EQUAL_STRING("cqr-thr", back.queue); + TEST_ASSERT_EQUAL_INT(1, back.nthresholds); + TEST_ASSERT_NOT_NULL(back.thresholds); + TEST_ASSERT_EQUAL_STRING("tmp", back.thresholds[0]->name); + TEST_ASSERT_EQUAL_MEMORY(&thr.sched, &back.thresholds[0]->sched, + sizeof(double)); + TEST_ASSERT_EQUAL_MEMORY(&thr.stop, &back.thresholds[0]->stop, + sizeof(double)); + free_QueueConf(&back); + capn_free(&dcap); + free(buf); + } + + xce_hook_fini(&c.hooksys); + dereference(c.qmgr); +} + void test_config_queue_reconcile(void) { UnitySetTestFile(__FILE__); RUN_TEST(test_reconcile_creates_queue_added_by_config); RUN_TEST(test_reconcile_refreshes_existing_queue_in_place); + RUN_TEST(test_reconcile_thresholds_survive_second_pass); } diff --git a/src/controller/testsuite/test_config_resource_reconcile.c b/src/controller/testsuite/test_config_resource_reconcile.c new file mode 100644 index 00000000..46ef051d --- /dev/null +++ b/src/controller/testsuite/test_config_resource_reconcile.c @@ -0,0 +1,250 @@ +/* test_config_resource_reconcile.c -- admin-plane-gaps 1.10: a resource + * definition added by a live config upgrade used to need a controller + * restart (cluster_build_resources() ran only at boot), and a YAML-declared + * STATIC shared pool (resourcemap `location: [all]` + `value:`, the + * license-pool shape) never reached c->sharedinst at all -- the scheduler's + * two-phase shared-pool machinery consumes exactly those resvals from every + * conf4sched push. reconcile_config_resources() registers the config's + * resource definitions (create-or-keep, EEXIST tolerated) and seeds + * c->sharedinst for SHARED|GLOBAL resources whose distributions carry a + * value; the whole pass is idempotent (a re-run swaps the seeded value via + * zarray_put's killer, mirroring update_val_res's ownership contract). */ +#include + +#include +#include + +#include + +#include "clusterconfig.h" +#include "cluster.h" +#include "hook.h" +#include "myconf.h" +#include "resobj.h" +#include "xce/cluster_def.h" +#include "xstring.h" + +/* Same ownership contract as cluster.c's resval_obj_killer (static there -- + * test-local twin): c->sharedinst owns each xce_val_t and a string value's + * private strdup. */ +static void crr_val_killer(void* arg) +{ + xce_val_t* v = (xce_val_t*)arg; + + if (v == NULL) { + return; + } + if (v->which == Value_u_svalue) { + FREEUP(v->u.svalue); + } + free(v); +} + +static xce_resource_t* crr_find(cluster_t* c, const char* name) +{ + xce_resource_t* r = NULL; + lock(c->resmgr, RTTI_LOCK_READ); + key_find(c->resmgr, (void*)name, (void**)&r); + unlock(c->resmgr); + return r; +} + +static xce_val_t* crr_shared_val(cluster_t* c, int idx) +{ + if (idx >= c->sharedinst.caps) { + return NULL; + } + return (xce_val_t*)zarray_get(&c->sharedinst, idx); +} + +/* A config with: + * res0 "slots" -- built-in hostbased (no resourcemap): registered, never seeded + * res1 "lic1" -- SHARED|GLOBAL, distribution value 100: seeded + * res2 "hostres"-- SHARED but NOT GLOBAL (per-host shape): NOT seeded + * res3 "dyn1" -- SHARED|GLOBAL, distribution WITHOUT a value: NOT seeded */ +static void crr_build_config(cluster_config_t* cfg, xce_resource_t** resources, + res_distribution_t** dists, xce_val_t** vals) +{ + static xce_resource_t r0, r1, r2, r3; + static res_distribution_t d1, d2, d3; + static xce_val_t v1, v3; + + memset(&r0, 0, sizeof(r0)); + r0.name = (char*)"slots"; + r0.flags = XCE_RESF_BUILTIN; + + memset(&r1, 0, sizeof(r1)); + r1.name = (char*)"lic1"; + r1.flags = XCE_RESF_SHARED | XCE_RESF_GLOBAL; + memset(&d1, 0, sizeof(d1)); + memset(&v1, 0, sizeof(v1)); + v1.which = Value_u_fvalue; + v1.u.fvalue = 100.0; + d1.value = &v1; + r1.ndistributions = 1; + r1.distributions = (res_distribution_t**)&dists[0]; + dists[0] = &d1; + vals[1] = &v1; + + memset(&r2, 0, sizeof(r2)); + r2.name = (char*)"hostres"; + r2.flags = XCE_RESF_SHARED; + memset(&d2, 0, sizeof(d2)); + memset(&v3, 0, sizeof(v3)); + v3.which = Value_u_fvalue; + v3.u.fvalue = 5.0; + d2.value = &v3; + r2.ndistributions = 1; + r2.distributions = (res_distribution_t**)&dists[1]; + dists[1] = &d2; + vals[2] = &v3; + + memset(&r3, 0, sizeof(r3)); + r3.name = (char*)"dyn1"; + r3.flags = XCE_RESF_SHARED | XCE_RESF_GLOBAL; + memset(&d3, 0, sizeof(d3)); /* value == NULL: worker-reported shape */ + r3.ndistributions = 1; + r3.distributions = (res_distribution_t**)&dists[2]; + dists[2] = &d3; + vals[3] = NULL; + + resources[0] = &r0; + resources[1] = &r1; + resources[2] = &r2; + resources[3] = &r3; + cfg->nres = 4; + cfg->resources = resources; +} + +static void test_reconcile_resources_registers_and_seeds_shared_pools(void) +{ + kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING, + .nslots = 16, .delta = 8 }; + cluster_t c; + myconf_t conf; + cluster_config_t cfg; + xce_resource_t* resources[4]; + res_distribution_t* dists[3]; + xce_val_t* vals[4]; + xce_resource_t* r; + + if (makeString((char*)"crr-probe") == NULL) + xstring_table_init(64); + + memset(&c, 0, sizeof(c)); + memset(&conf, 0, sizeof(conf)); + TEST_ASSERT_EQUAL_INT(0, xce_hook_init(&c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + c.resmgr = makeInstance(&xkvmgr_type, &str_params); + TEST_ASSERT_NOT_NULL(c.resmgr); + zarray_init(&c.sharedinst, 16, + (zarray_item_killer_t)crr_val_killer); + + crr_build_config(&cfg, resources, dists, vals); + conf.cluster_config = &cfg; + c.myconf = &conf; + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, reconcile_config_resources(&c), + "a full pass over a fresh config must succeed"); + + /* every definition registered with its own deep-copied runtime object */ + r = crr_find(&c, "lic1"); + TEST_ASSERT_NOT_NULL(r); + TEST_ASSERT_EQUAL_INT(1, r->ndistributions); + TEST_ASSERT_NOT_NULL(r->distributions[0]->value); + /* deep-copy: the runtime object must NOT alias the config's value */ + TEST_ASSERT_NOT_EQUAL_MESSAGE(vals[1], r->distributions[0]->value, + "the resmgr object owns a private copy of the config value"); + TEST_ASSERT_EQUAL_FLOAT(100.0, r->distributions[0]->value->u.fvalue); + TEST_ASSERT_NOT_NULL(crr_find(&c, "slots")); + TEST_ASSERT_NOT_NULL(crr_find(&c, "hostres")); + TEST_ASSERT_NOT_NULL(crr_find(&c, "dyn1")); + + /* SHARED|GLOBAL + valued distribution -> seeded into sharedinst */ + xce_val_t* v = crr_shared_val(&c, r->idx); + TEST_ASSERT_NOT_NULL_MESSAGE(v, "the static shared pool must be seeded " + "from its config distribution value"); + TEST_ASSERT_EQUAL_FLOAT(100.0, v->u.fvalue); + + /* SHARED-only (no GLOBAL) is NOT seeded -- host-subset pooling needs + * the planned elastic-shared-resources scheduler work. */ + xce_resource_t* rh = crr_find(&c, "hostres"); + TEST_ASSERT_NULL_MESSAGE(crr_shared_val(&c, rh->idx), + "a SHARED-but-not-GLOBAL resource must not seed the shared pool"); + + /* SHARED|GLOBAL but no distribution value (dynamic shape) is NOT + * seeded -- workers report those values at runtime. */ + xce_resource_t* rd = crr_find(&c, "dyn1"); + TEST_ASSERT_NULL_MESSAGE(crr_shared_val(&c, rd->idx), + "a value-less distribution must not seed the shared pool"); + + /* built-in, not shared at all: not seeded */ + xce_resource_t* rb = crr_find(&c, "slots"); + TEST_ASSERT_NULL(crr_shared_val(&c, rb->idx)); + + xce_hook_fini(&c.hooksys); + dereference(c.resmgr); + zarray_fini(&c.sharedinst); +} + +/* Idempotency + hot-apply: a second activation pass (1) keeps the existing + * runtime objects (makeResource tolerates EEXIST), (2) swaps the seeded + * value in place when the config changed it, and (3) leaves other slots + * untouched. */ +static void test_reconcile_resources_is_idempotent_and_hot_applies(void) +{ + kvmgr_params_t str_params = { .key_type = KVMGR_KEY_STRING, + .nslots = 16, .delta = 8 }; + cluster_t c; + myconf_t conf; + cluster_config_t cfg; + xce_resource_t* resources[4]; + res_distribution_t* dists[3]; + xce_val_t* vals[4]; + xce_resource_t* r; + xce_val_t* v; + + if (makeString((char*)"crr-probe") == NULL) + xstring_table_init(64); + + memset(&c, 0, sizeof(c)); + memset(&conf, 0, sizeof(conf)); + TEST_ASSERT_EQUAL_INT(0, xce_hook_init(&c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + c.resmgr = makeInstance(&xkvmgr_type, &str_params); + TEST_ASSERT_NOT_NULL(c.resmgr); + zarray_init(&c.sharedinst, 16, + (zarray_item_killer_t)crr_val_killer); + + crr_build_config(&cfg, resources, dists, vals); + conf.cluster_config = &cfg; + c.myconf = &conf; + + TEST_ASSERT_EQUAL_INT(0, reconcile_config_resources(&c)); + r = crr_find(&c, "lic1"); + TEST_ASSERT_NOT_NULL(r); + v = crr_shared_val(&c, r->idx); + TEST_ASSERT_NOT_NULL(v); + TEST_ASSERT_EQUAL_FLOAT(100.0, v->u.fvalue); + + /* The admin edits the shared pool's capacity in the activated config. */ + vals[1]->u.fvalue = 250.0; + + TEST_ASSERT_EQUAL_INT_MESSAGE(0, reconcile_config_resources(&c), + "a re-run over the same config (idempotent activation) must succeed"); + TEST_ASSERT_EQUAL_PTR_MESSAGE(r, crr_find(&c, "lic1"), + "an existing runtime resource must be kept, not re-created"); + v = crr_shared_val(&c, r->idx); + TEST_ASSERT_NOT_NULL(v); + TEST_ASSERT_EQUAL_FLOAT_MESSAGE(250.0, v->u.fvalue, + "the edited capacity must hot-apply into the shared pool"); + + xce_hook_fini(&c.hooksys); + dereference(c.resmgr); + zarray_fini(&c.sharedinst); +} + +void test_config_resource_reconcile(void) +{ + UnitySetTestFile(__FILE__); + RUN_TEST(test_reconcile_resources_registers_and_seeds_shared_pools); + RUN_TEST(test_reconcile_resources_is_idempotent_and_hot_applies); +} diff --git a/src/controller/testsuite/test_job_array_sched_push.c b/src/controller/testsuite/test_job_array_sched_push.c index 851b9190..efe91070 100644 --- a/src/controller/testsuite/test_job_array_sched_push.c +++ b/src/controller/testsuite/test_job_array_sched_push.c @@ -45,11 +45,16 @@ #include "cluster.h" #include "controller.capnp.h" #include "hook.h" +#include "kvmgr.h" +#include "myconf.h" #include "netio.h" #include "request.h" +#include "resobj.h" #include "xce/cluster_def.h" #include "xce/types.h" #include "xlist.h" +#include "xstring.h" +#include "zarray.h" extern int on_scheduler_join(cluster_t*); @@ -269,6 +274,318 @@ static void test_single_element_job_new_is_unaffected(void) printf("test_single_element_job_new_is_unaffected: PASS\n"); } +/* ------------------------------------------------------------------ */ +/* admin-plane-gaps 1.10/2: xcesched_push_configs() resval ownership */ +/* regression (the seeded sharedinst push) */ +/* */ +/* admin-plane-gaps breakpoint 2 (YAML resourcemap wiring) made the */ +/* controller SEED c->sharedinst for static SHARED|GLOBAL resources */ +/* (config_activate_committed -> reconcile_config_resources), so */ +/* xcesched_push_configs() started pushing with nresvals > 0 for the */ +/* first time ever. That immediately armed a LATENT double free: the */ +/* function freed the pushed resvals with free_name_val_list() -- which */ +/* frees the ENTRIES *and the backing array* (utils.c's FREEUP(resvals))*/ +/* -- then xlist_fini(&xreslist) freed the SAME array a second time */ +/* (the array xreslist.data aliases), aborting the controller with */ +/* glibc "double free or corruption (out)". Every pre-1.10 push ran */ +/* with nresvals == 0 and never armed it, because free_name_val_list() */ +/* early-returns WITHOUT freeing the array when count <= 0. */ +/* */ +/* The fix detaches xreslist (data/count/caps zeroed) after the */ +/* free_name_val_list() call and before the xlist_fini() block -- the */ +/* same ownership class bug-1506 fixed in sched_worker_change() (whose */ +/* own regression test lives in test_sched_worker_change.c, the other */ +/* standalone binary linking real sched.c). */ +/* */ +/* These tests need the REAL xcesched_push_configs(), which only this */ +/* standalone binary can link (controller_unit_test's xcesched_push_ */ +/* configs is test_config_repl.c's no-op capturing fake). Under ASAN a */ +/* regression of the double free aborts INSIDE xcesched_push_configs() */ +/* -- before any assertion below even runs. */ +/* ------------------------------------------------------------------ */ + +/* Same killer cluster.c's resval_obj_killer uses for c->sharedinst: + * the array owns each xce_val_t, and a string value owns a private + * strdup()ed copy. */ +static void jsp_sharedinst_killer(void* p) +{ + xce_val_t* v = (xce_val_t*)p; + + if (v == NULL) { + return; + } + if (v->which == Value_u_svalue) { + FREEUP(v->u.svalue); + } + free(v); +} + +/* Stack fixtures for the one cluster_t this pair of tests drives. The + * resources/queues/confvars collects run against a real resmgr (kvmgr) + * but NULL configmgr/qmgr (both are RTTI-API NULL-safe; the real + * controller has them non-NULL but this fixture has no configs/queues + * to contribute). */ +static cluster_t g_jsp_cfg_c; +static endpoint_t g_jsp_cfg_ep; +static myconf_t g_jsp_myconf; +static cluster_config_t g_jsp_ccfg; +static var g_jsp_resmgr; +static xce_resource_t* g_jsp_lic_res; +static zarray_t g_jsp_sharedinst; +static int g_jsp_cfg_seeded_state; /* 0 = fresh, 1 = seeded */ + +/* Registers one SHARED|GLOBAL resource ("staticres4", the E2E license-pool + * name) in a fresh kvmgr resmgr -- mirroring cluster_build_resources()/ + * reconcile_config_resources()'s own makeResource() call -- and returns the + * runtime object (idx assigned by key_add). rdef carries a distributions[0] + * with value = fvalue 100.0 exactly like the seeded YAML resourcemap shape + * (`location: [all]` + `value: 100`): resobj_init() DEEP-COPIES + * distributions into the runtime object, so the fixture's own rdef/dvalue + * can live on the caller's stack. */ +static xce_resource_t* jsp_cfg_setup(const char* resname) +{ + kvmgr_params_t resmgr_params = { + .key_type = KVMGR_KEY_STRING, + .nslots = 64, + .delta = 16, + .datakiller = NULL + }; + xce_val_t* dvalue; + res_distribution_t* dist; + xce_resource_t rdef; + xce_resource_t* r; + + if (!g_jsp_cfg_seeded_state) { + memset(&g_jsp_cfg_c, 0, sizeof(g_jsp_cfg_c)); + memset(&g_jsp_myconf, 0, sizeof(g_jsp_myconf)); + memset(&g_jsp_ccfg, 0, sizeof(g_jsp_ccfg)); + TEST_ASSERT_EQUAL_INT( + 0, xce_hook_init(&g_jsp_cfg_c.hooksys, XCE_CONTROLLER_HOOK_MAX)); + g_jsp_cfg_seeded_state = 1; + } + + /* xcesched_push_configs()'s guard: both must be non-NULL or the push + * is skipped entirely. */ + g_jsp_myconf.cluster_config = &g_jsp_ccfg; + g_jsp_ccfg.clustername = (char*)"jsp-cluster"; + g_jsp_cfg_c.myconf = &g_jsp_myconf; + + g_jsp_resmgr = makeInstance(&xkvmgr_type, &resmgr_params); + TEST_ASSERT_NOT_NULL(g_jsp_resmgr); + g_jsp_cfg_c.resmgr = g_jsp_resmgr; + + /* The config-owned resource definition (what myconf->cluster_config-> + * resources would point at after an activation). SHARED|GLOBAL with a + * static value -- the YAML resourcemap mapping's exact output shape. */ + dvalue = (xce_val_t*)calloc(1, sizeof(*dvalue)); + TEST_ASSERT_NOT_NULL(dvalue); + dvalue->which = Value_u_fvalue; + dvalue->u.fvalue = 100.0; + + dist = (res_distribution_t*)calloc(1, sizeof(*dist)); + TEST_ASSERT_NOT_NULL(dist); + dist->value = dvalue; + dist->nhosts = 0; /* encode_ResourceDistribution strlen()s hosts[i]; + the [all] shape has no per-host legs */ + dist->hosts = NULL; + + memset(&rdef, 0, sizeof(rdef)); + rdef.name = (char*)resname; + rdef.valtype = XCE_VAL_NUMERIC; + rdef.flags = XCE_RESF_SHARED | XCE_RESF_GLOBAL; + rdef.idx = -1; + rdef.ndistributions = 1; + rdef.distributions = &dist; + r = makeResource(g_jsp_resmgr, &rdef); + TEST_ASSERT_NOT_NULL(r); + g_jsp_lic_res = r; + + /* c->sharedinst seeding (reconcile_config_resources -> + * seed_sharedinst_from_config): one heap xce_val_t at the resource's + * RUNTIME idx, owned by the array via the killer above. */ + TEST_ASSERT_EQUAL_INT(0, zarray_init(&g_jsp_sharedinst, 16, + jsp_sharedinst_killer)); + g_jsp_cfg_c.sharedinst = g_jsp_sharedinst; + + memset(&g_jsp_cfg_ep, 0, sizeof(g_jsp_cfg_ep)); + g_jsp_cfg_ep.fd = -1; + + g_jsp_cfg_c.sched.ep = &g_jsp_cfg_ep; + g_jsp_cfg_c.running = 1; + g_jsp_cfg_c.myhost = &g_jsp_cfg_c.myhost_info; + g_jsp_cfg_c.myhost_info.name = (char*)"ctrl-jsp-cfg"; + g_jsp_cfg_c.myhost_info.key = (char*)""; + + return r; +} + +static void jsp_cfg_teardown(void) +{ + jsp_drain_ep(&g_jsp_cfg_ep); + g_jsp_cfg_c.sched.ep = NULL; + + zarray_fini(&g_jsp_cfg_c.sharedinst); + memset(&g_jsp_sharedinst, 0, sizeof(g_jsp_sharedinst)); + g_jsp_cfg_c.sharedinst = g_jsp_sharedinst; + + g_jsp_cfg_c.resmgr = NULL; + g_jsp_cfg_c.myconf = NULL; + g_jsp_lic_res = NULL; + if (g_jsp_resmgr != NULL) { + dereference(g_jsp_resmgr); + g_jsp_resmgr = NULL; + } +} + +/* Raw-capnp-decode the queued config push and return the deep-copied + * Conf4Sched payload in *out (caller frees via free_ConfigData_ptr + + * free(), then capn_free). Asserts the envelope shape. Returns 0, or + * -1 if the queue did not hold exactly one configpushmsg packet. */ +static int jsp_decode_config_push(endpoint_t* ep, xce_configdata_t** out) +{ + data_item_t* it; + struct capn c; + struct Request rq; + Request_ptr rp; + int found = 0; + int rc = -1; + + *out = NULL; + while ((it = endpoint_deq(ep)) != NULL) { + io_packet_t* pkt = ITPTR_TO_PACKET(it); + + if (found == 0) { + capn_init_mem(&c, (const uint8_t*)pkt->data, pkt->sz, 0); + rp.p = capn_getp(capn_root(&c), 0, 1); + read_Request(&rq, rp); + if (rq.body_which == Request_body_configpushmsg) { + decode_ConfigData_ptr(out, rq.body.configpushmsg); + found = 1; + } + capn_free(&c); + } + free_packet(pkt); + free(pkt); + } + + if (found && *out != NULL + && (*out)->data_which == ConfigData_data_schedconfig + && (*out)->data.conf4sched != NULL) { + rc = 0; + } + + return rc; +} + +/* Test 3 (regression): a SEEDED sharedinst produces one configpushmsg with + * the seeded resval (name + fvalue 100.0) -- and, under ASAN, completing at + * all proves the post-push free sequence no longer double-frees the resval + * backing array. Pre-fix this aborts inside xcesched_push_configs(). */ +static void test_seeded_sharedinst_push_carries_resvals_without_double_free( + void) +{ + xce_resource_t* r; + xce_val_t* v; + xce_configdata_t* cd = NULL; + xce_conf4sched_t* cf; + int rc; + + r = jsp_cfg_setup("staticres4"); + TEST_ASSERT_TRUE(r->idx >= 0); + + v = (xce_val_t*)calloc(1, sizeof(*v)); + TEST_ASSERT_NOT_NULL(v); + v->which = Value_u_fvalue; + v->u.fvalue = 100.0; + TEST_ASSERT_TRUE(zarray_put(&g_jsp_cfg_c.sharedinst, r->idx, v) >= 0); + + /* Caller contract: c->sched.lock is held (same as the config_repl.c + * activation seam and on_scheduler_join's push branch). */ + mylock_lock(&(g_jsp_cfg_c.sched.lock)); + rc = xcesched_push_configs(&g_jsp_cfg_c); + mylock_unlock(&(g_jsp_cfg_c.sched.lock)); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, "the seeded config push must succeed"); + + rc = jsp_decode_config_push(&g_jsp_cfg_ep, &cd); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, + "exactly one configpushmsg carrying a non-NULL schedconfig must be " + "queued"); + + cf = cd->data.conf4sched; + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, cf->nresvals, + "the push must carry exactly the one seeded shared resval"); + TEST_ASSERT_NOT_NULL(cf->resvals); + TEST_ASSERT_EQUAL_STRING_MESSAGE( + "staticres4", cf->resvals[0]->name, + "the resval's name must be the shared resource's name"); + TEST_ASSERT_NOT_NULL(cf->resvals[0]->val); + TEST_ASSERT_EQUAL_INT_MESSAGE( + Value_u_fvalue, cf->resvals[0]->val->which, + "the seeded value must survive the wire encode/decode as an fvalue"); + /* This Unity build excludes double-precision assertions (a + * TEST_ASSERT_EQUAL_DOUBLE here fails with "Unity Double Precision + * Disabled"), so compare against an exact-equality expectation the + * literal way -- fvalue is copied by value through both encoders. */ + TEST_ASSERT_TRUE_MESSAGE( + cf->resvals[0]->val->u.fvalue == 100.0, + "the seeded static pool value (the YAML `value: 100` shape) must " + "reach the scheduler-side payload intact"); + TEST_ASSERT_EQUAL_STRING_MESSAGE( + "jsp-cluster", cf->clustername, + "the push must still carry the clustername from the active config"); + + free_ConfigData_ptr(&cd); + free(cd); + + jsp_cfg_teardown(); + printf("test_seeded_sharedinst_push_carries_resvals_without_double_free:" + " PASS\n"); +} + +/* Test 4 (latency documentation): an UNSEEDED sharedinst still pushes, with + * nresvals == 0. This is the shape EVERY push had before breakpoint 2's + * seeding existed -- and why the double free stayed latent for years: + * free_name_val_list() early-returns WITHOUT freeing the array when + * count <= 0, so xreslist.data was never actually freed twice. */ +static void test_unseeded_push_still_sends_empty_resvals(void) +{ + xce_resource_t* r; + xce_configdata_t* cd = NULL; + xce_conf4sched_t* cf; + int rc; + + r = jsp_cfg_setup("staticres4"); + TEST_ASSERT_TRUE(r->idx >= 0); + (void)r; + + mylock_lock(&(g_jsp_cfg_c.sched.lock)); + rc = xcesched_push_configs(&g_jsp_cfg_c); + mylock_unlock(&(g_jsp_cfg_c.sched.lock)); + TEST_ASSERT_EQUAL_INT(0, rc); + + rc = jsp_decode_config_push(&g_jsp_cfg_ep, &cd); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, rc, + "an unseeded cluster must still push configs to the scheduler"); + + cf = cd->data.conf4sched; + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, cf->nresvals, + "no sharedinst seeds means no resvals on the push -- this is the " + "shape every pre-admin-plane-gaps-1.10 push had, which is why the " + "xreslist double free stayed latent (free_name_val_list() does not " + "free the backing array when count <= 0)"); + + free_ConfigData_ptr(&cd); + free(cd); + + jsp_cfg_teardown(); + printf("test_unseeded_push_still_sends_empty_resvals: PASS\n"); +} + /* ------------------------------------------------------------------ */ /* Standalone Unity main (this binary is its own test executable — see */ /* meson.build's sched_push_unit_test) */ @@ -282,10 +599,21 @@ int main(void) printf("\n>> job-array-single-wal-record task 5.1: compact scheduler " "push\n\n"); + /* makeResource() (resobj.c) interns resource names via makeString(), + * which needs the global xstring table initialized before first use -- + * same requirement test_sched_worker_change.c documents at its own + * main(). This is a standalone binary (no other test file shares + * process init with it), so it is done exactly once here. The two + * compact-push tests above never needed it (no makeResource call); + * the admin-plane-gaps resval-ownership tests below do. */ + xstring_table_init(139); + UNITY_BEGIN(); RUN_TEST(test_multi_element_job_new_produces_one_compact_jobarray_push); RUN_TEST(test_single_element_job_new_is_unaffected); + RUN_TEST(test_seeded_sharedinst_push_carries_resvals_without_double_free); + RUN_TEST(test_unseeded_push_still_sends_empty_resvals); return UNITY_END(); } diff --git a/src/include/utils.h b/src/include/utils.h index d81ee63b..1d5b7fb1 100644 --- a/src/include/utils.h +++ b/src/include/utils.h @@ -211,6 +211,7 @@ extern int xce_timewindow_is_open(xce_timewindow_config_t*, time_t); extern void free_resource_thresholds(xce_res_threshold_t**, int); extern xce_res_threshold_t** dup_resource_thresholds(xce_res_threshold_t**, int); +extern xce_group_config_t* dup_group_config(const xce_group_config_t*); extern void free_name_val_list(xce_name_val_t***, int*); extern void free_strlist(xce_strlist_t*); extern int fmt_jobid_ex(char*, size_t, uint64_t, char*, char*); diff --git a/src/lib/qobj.c b/src/lib/qobj.c index 8009e1c2..702891a1 100644 --- a/src/lib/qobj.c +++ b/src/lib/qobj.c @@ -39,16 +39,56 @@ static int queue_init(meta_obj_t* obj, va_list ap) dst->queue.accept_interval = src->accept_interval; dst->queue.sched_delay = src->sched_delay; dst->queue.resreq = makeString(src->resreq); - MOVE_PTR(dst->queue.users, src->users); - MOVE_PTR(dst->queue.hosts, src->hosts); - dst->queue.nthresholds = src->nthresholds; - MOVE_PTR(dst->queue.thresholds, src->thresholds); - src->nthresholds = 0; + if (src->users != NULL) { + dst->queue.users = dup_group_config(src->users); + if (dst->queue.users == NULL) { + return -ENOMEM; + } + } + if (src->hosts != NULL) { + dst->queue.hosts = dup_group_config(src->hosts); + if (dst->queue.hosts == NULL) { + return -ENOMEM; + } + } + /* admin-plane-gaps bp2 (2026-10-23): DEEP-COPY the config-owned + * payloads -- do NOT MOVE_PTR them out of the definition. The + * definition lives in myconf->cluster_config, which keeps owning (and + * later frees) its copies, and reconcile_config_queues runs MORE THAN + * ONCE over the SAME config object (boot fires it from the pre-election + * config_activate_committed AND from phase2's build_queues), so a + * create-path MOVE left the definition drained and the second pass's + * refresh saw a poisoned (count>0, array=NULL) thresholds pair that + * segfaulted encode_Threshold_list on the next queue/worker push (t1 + * controller 12:41:13). Deep-copying also keeps (NULL/0) in a + * definition meaning "this config really has no list", which + * queue_refresh_def relies on. Mirrors resobj_init's distributions + * deep-copy; queue_destroy frees the private copies. */ + if (src->nthresholds > 0 && src->thresholds != NULL) { + dst->queue.thresholds = dup_resource_thresholds(src->thresholds, + src->nthresholds); + if (dst->queue.thresholds == NULL) { + return -ENOMEM; + } + dst->queue.nthresholds = src->nthresholds; + } MOVE_PTR(dst->queue.runwindow, src->runwindow); MOVE_PTR(dst->queue.dispatchwindow, src->dispatchwindow); - MOVE_PTR(dst->queue.runwindow_config, src->runwindow_config); - MOVE_PTR(dst->queue.dispatchwindow_config, src->dispatchwindow_config); + if (src->runwindow_config != NULL) { + dst->queue.runwindow_config = dup_timewindow_config( + src->runwindow_config); + if (dst->queue.runwindow_config == NULL) { + return -ENOMEM; + } + } + if (src->dispatchwindow_config != NULL) { + dst->queue.dispatchwindow_config = dup_timewindow_config( + src->dispatchwindow_config); + if (dst->queue.dispatchwindow_config == NULL) { + return -ENOMEM; + } + } dst->queue.pending_ctrl = src->pending_ctrl; dst->queue.pre_exec = makeString(src->pre_exec); dst->queue.post_exec = makeString(src->post_exec); diff --git a/src/lib/resobj.c b/src/lib/resobj.c index 8a6b27c6..bc1be771 100644 --- a/src/lib/resobj.c +++ b/src/lib/resobj.c @@ -21,11 +21,112 @@ static size_t resobj_datasize(va_list ap) return sz; } +/* Deep-copy one xce_val_t. A string value owns a private strdup()ed copy + * (same ownership rule as the resvals stored in c->sharedinst / + * w->host.resvals, see update_val_res in controller/worker.c); scalar + * values are copied by value. The caller's `src` (typically a decoded + * config-distribution value owned by myconf->cluster_config) is only read, + * never aliased. */ +static xce_val_t* resval_deep_copy(const xce_val_t* src) +{ + xce_val_t* v = (xce_val_t*)calloc(1, sizeof(*v)); + + if (v == NULL) { + return NULL; + } + + if (src->which == Value_u_svalue) { + v->which = Value_u_svalue; + v->u.svalue = strdup(src->u.svalue != NULL ? src->u.svalue : ""); + if (v->u.svalue == NULL) { + free(v); + return NULL; + } + } else { + memcpy(v, src, sizeof(*v)); + } + + return v; +} + +static void resval_free(xce_val_t* v) +{ + if (v == NULL) { + return; + } + if (v->which == Value_u_svalue) { + FREEUP(v->u.svalue); + } + free(v); +} + +/* admin-plane-gaps 1.10 (YAML resourcemap wiring): a resource definition + * carries its per-host/shared instance declarations in `distributions` + * (+ `hbased_defined`, the per-host fallback). makeResource_(lib/resobj.c) + * builds the runtime xce_resource_t from a config-owned definition, so the + * copy must DEEP-COPY these members: the previous stubs left them NULL, + * which severed distributions on the only path that survives into the + * runtime resmgr (and later into the wire push to the scheduler). The + * #if 0 MOVE_PTR alternative would STEAL the pointers out of + * myconf->cluster_config, which keeps owning (and later frees) them -- + * deep-copy is the only correct form here. resobj_destroy frees the + * private copies. Returns 0 on success, -ENOMEM on allocation failure. */ +static int resobj_copy_distributions(xce_resource_t* dst, + const xce_resource_t* src) +{ + if (src->ndistributions == 0) { + return 0; + } + + dst->distributions = (res_distribution_t**)calloc( + src->ndistributions, sizeof(res_distribution_t*)); + if (dst->distributions == NULL) { + return -ENOMEM; + } + + for (int i = 0; i < src->ndistributions; i++) { + const res_distribution_t* sd = src->distributions[i]; + res_distribution_t* dd + = (res_distribution_t*)calloc(1, sizeof(*dd)); + + if (dd == NULL) { + return -ENOMEM; + } + dst->distributions[i] = dd; + dst->ndistributions++; + + if (sd->value != NULL) { + dd->value = resval_deep_copy(sd->value); + if (dd->value == NULL) { + return -ENOMEM; + } + } + + if (sd->nhosts > 0) { + dd->hosts = (char**)calloc(sd->nhosts, sizeof(char*)); + if (dd->hosts == NULL) { + return -ENOMEM; + } + for (int j = 0; j < sd->nhosts; j++) { + dd->hosts[j] = strdup(sd->hosts[j] != NULL ? sd->hosts[j] + : ""); + if (dd->hosts[j] == NULL) { + return -ENOMEM; + } + dd->nhosts++; + } + } + } + + return 0; +} + static int resobj_init(meta_obj_t* obj, va_list ap) { xce_resource_t* dst = RTTI_OBJ_TO_DATA(xce_resource_t, obj); resobj_private_t* priv = (resobj_private_t*)RTTI_OBJ_TO_PRIV(obj); xce_resource_t* src = va_arg(ap, xce_resource_t*); + int rc; if (src == NULL) { return -EINVAL; @@ -36,21 +137,24 @@ static int resobj_init(meta_obj_t* obj, va_list ap) dst->name = makeString(src->name); dst->desc = makeString(src->desc); -#if 0 - if (src->distributions != NULL) { - MOVE_PTR(dst->distributions, src->distributions); - } -#else - /* FIXME: */ - dst->distributions = NULL; -#endif - -#if 0 - MOVE_PTR(dst->hbased_defined, src->hbased_defined); -#else - /* FIXME: */ + /* ndistributions points at dst's own (to-be-filled) array now; zero the + * deep members so resobj_destroy can free whatever resobj_copy_ + * distributions managed to build on the -ENOMEM rollback path. */ + dst->ndistributions = 0; + dst->distributions = NULL; dst->hbased_defined = NULL; -#endif + + rc = resobj_copy_distributions(dst, src); + if (rc < 0) { + return rc; + } + + if (src->hbased_defined != NULL) { + dst->hbased_defined = resval_deep_copy(src->hbased_defined); + if (dst->hbased_defined == NULL) { + return -ENOMEM; + } + } priv->refcount = 1; @@ -65,6 +169,25 @@ static void resobj_destroy(meta_obj_t* obj) dereference(r->name); dereference(r->desc); + for (int i = 0; i < r->ndistributions; i++) { + res_distribution_t* d = r->distributions[i]; + + if (d == NULL) { + continue; + } + resval_free(d->value); + for (int j = 0; j < d->nhosts; j++) { + FREEUP(d->hosts[j]); + } + FREEUP(d->hosts); + free(d); + } + FREEUP(r->distributions); + r->ndistributions = 0; + + resval_free(r->hbased_defined); + r->hbased_defined = NULL; + memset(priv, 0x0, sizeof(*priv)); return; diff --git a/src/lib/utils.c b/src/lib/utils.c index 446ef3d7..a736ba25 100644 --- a/src/lib/utils.c +++ b/src/lib/utils.c @@ -524,6 +524,87 @@ int copy_strlist(xce_strlist_t* dst, xce_strlist_t src) } return 0; } + +/* Deep-copy one xce_group_config_t (queue users/hosts membership). The + * generated capnp code has no dup for it, and the controller's runtime + * queue objects need their own copy: the config's cluster_config keeps + * owning the original and frees it on the next activation swap + * (admin-plane-gaps bp2, 2026-10-23). The caller frees with the generated + * free_GroupConfig_ptr(). Returns NULL (+errno) when src==NULL or on + * allocation failure. */ +xce_group_config_t* dup_group_config(const xce_group_config_t* src) +{ + xce_group_config_t* dst; + int i; + + if (src == NULL) { + errno = EINVAL; + return NULL; + } + + dst = (xce_group_config_t*)calloc(1, sizeof(*dst)); + if (dst == NULL) { + errno = ENOMEM; + return NULL; + } + + if (src->name != NULL) { + dst->name = strdup(src->name); + if (dst->name == NULL) { + free_GroupConfig(dst); + errno = ENOMEM; + return NULL; + } + } + + if (src->members.count > 0) { + dst->members.data + = (char**)calloc(src->members.count, sizeof(char*)); + if (dst->members.data == NULL) { + free_GroupConfig(dst); + errno = ENOMEM; + return NULL; + } + for (i = 0; i < src->members.count; i++) { + if (src->members.data[i] == NULL) { + continue; + } + dst->members.data[i] = strdup(src->members.data[i]); + if (dst->members.data[i] == NULL) { + dst->members.count = i + 1; + free_GroupConfig(dst); + errno = ENOMEM; + return NULL; + } + } + dst->members.count = src->members.count; + } + + if (src->ex_members.count > 0) { + dst->ex_members.data + = (char**)calloc(src->ex_members.count, sizeof(char*)); + if (dst->ex_members.data == NULL) { + free_GroupConfig(dst); + errno = ENOMEM; + return NULL; + } + for (i = 0; i < src->ex_members.count; i++) { + if (src->ex_members.data[i] == NULL) { + continue; + } + dst->ex_members.data[i] = strdup(src->ex_members.data[i]); + if (dst->ex_members.data[i] == NULL) { + dst->ex_members.count = i + 1; + free_GroupConfig(dst); + errno = ENOMEM; + return NULL; + } + } + dst->ex_members.count = src->ex_members.count; + } + + return dst; +} void free_name_val_list(xce_name_val_t*** vals, int *count_p) { xce_name_val_t** resvals = *vals; diff --git a/src/schema/controller.capnp b/src/schema/controller.capnp index f03a2de1..446562b7 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -210,8 +210,8 @@ struct Resource $C.mapname("xce_resource_t"){ flags @5: Int32; interval @6: Int32; lastReport @7: UInt64 $C.mapname("last_report"); -# distributions @8: List(ResourceDistribution) $C.maplistcount("ndistributions"); -# hostBased @9: Value $C.mapname("hbased_defined"); + distributions @8: List(ResourceDistribution) $C.maplistcount("ndistributions"); + hostBased @9: Value $C.mapname("hbased_defined"); } struct Threshold $C.mapname("xce_res_threshold_t"){ diff --git a/src/xceadmin/src/xceadmin/build.py b/src/xceadmin/src/xceadmin/build.py index 44f47458..edf33494 100644 --- a/src/xceadmin/src/xceadmin/build.py +++ b/src/xceadmin/src/xceadmin/build.py @@ -36,11 +36,10 @@ from xcecommon.schema import load_schema #: allows. Keyed by SECTION_MAP's ``capnp_field`` (not ``yaml_file``, #: since ``hosts.yaml`` feeds two fields with two different item shapes). _UNMAPPED_BUT_VALID_ITEM_KEYS: dict[str, frozenset[str]] = { - # type/order/consumable/release ARE mapped (mapping.py's - # _RESOURCE_VALTYPE/_RESOURCE_ORDTYPE/flag constants) -- only - # resourcemap has no destination (controller.capnp:208-209 has its - # target fields commented out). - "resources": frozenset({"resourcemap"}), + # resources: every schema-valid key is mapped now -- type/order/ + # consumable/release via mapping.py's valtype/ordtype/flag constants, + # resourcemap -> distributions (admin-plane-gaps 1.10; the [all]+value + # shape also sets the SHARED|GLOBAL flag bits) -- no entry needed. "queues": frozenset( { "backfill", @@ -187,11 +186,29 @@ def _format_bytes_human(n: float) -> str: return f"{value:.2f} PB" +def _format_value(v) -> str: + """Render a capnp ``Value`` union by its set member (the same + numeric/boolean/string discrimination the mapper used to build it). + The mapper only ever writes fvalue/bvalue/svalue, so an integer-member + ``which`` on a dumped config means the value was never set (the + dynamic, worker-reported shapes) -- rendered as such rather than as a + (confusing) 0.""" + which = str(v.u.which) + if which == "svalue": + return repr(v.u.svalue) + if which == "bvalue": + return str(bool(v.u.bvalue)) + if which == "fvalue": + return str(v.u.fvalue) + return "(reported)" + + def _format_resource(r) -> str: valtype = _RESOURCE_VALTYPE_NAMES.get(r.valtype, r.valtype) ordtype = _RESOURCE_ORDTYPE_NAMES.get(r.ordtype, r.ordtype) consumable = bool(r.flags & mapping.RESOURCE_FLAG_CONSUMABLE) release = bool(r.flags & mapping.RESOURCE_FLAG_RELEASE) + shared = bool(r.flags & mapping.RESOURCE_FLAG_SHARED) parts = [f"name={r.name}", f"type={valtype}", f"order={ordtype}"] if r.desc: parts.append(f"desc={r.desc!r}") @@ -199,6 +216,15 @@ def _format_resource(r) -> str: parts.append(f"interval={r.interval}") parts.append(f"consumable={consumable}") parts.append(f"release={release}") + if shared: + parts.append("shared=true") + if len(r.distributions) > 0: + rendered = [] + for d in r.distributions: + hosts = ",".join(str(h) for h in d.hosts) or "(none)" + value = _format_value(d.value) + rendered.append(f"[{hosts}]={value}") + parts.append("distributions=" + " ".join(rendered)) return " " + " ".join(parts) diff --git a/src/xceadmin/src/xcecommon/mapping.py b/src/xceadmin/src/xcecommon/mapping.py index 9a53cc2f..007efa96 100644 --- a/src/xceadmin/src/xcecommon/mapping.py +++ b/src/xceadmin/src/xcecommon/mapping.py @@ -76,11 +76,22 @@ reason -- never a silent drop. ``lastReport`` remain unset: ``idx`` has no YAML source at all (controller/runtime-assigned), and ``lastReport`` is a runtime timestamp field (epoch seconds of last report) -- excluded by - Requirement 9.2's "no timestamp" rule. ``resourcemap`` has no - destination: the capnp fields it would map to (``distributions``, - ``hostBased``) are commented out of the ``Resource`` struct - (``controller.capnp:208-209``) -- mapping it needs a schema change - first, out of this module's scope. + Requirement 9.2's "no timestamp" rule. + + ``resourcemap`` maps to the ``Resource.distributions`` list + (``List(ResourceDistribution)``, one entry per resourcemap item, + order preserved): ``location`` -> ``hosts``, ``value`` -> ``value`` + (numeric -> ``fvalue``, boolean -> ``bvalue``, string -> ``svalue``). + A resourcemap item whose location list is exactly ``[all]`` (the + cluster-wide static shared pool -- the license-pool shape) additionally + sets the ``XCE_RESF_SHARED|XCE_RESF_GLOBAL`` flag bits so the + controller seeds it into the scheduler's shared-instance pool + (admin-plane-gaps 1.10); all other shapes map definition-only -- + host-subset static pooling needs the planned elastic-shared-resources + scheduler work. A resourcemap item without a ``value`` (the dynamic + reported shapes, ``[default]``/``[hostA, hostB]``) maps the + distribution's hosts but leaves its value unset -- workers report + those values at runtime. ``QueueConf`` (``queues.yaml``) ``attrs`` (raw ``UInt32`` bitfield), ``status``, ``acceptIntervals``, @@ -199,6 +210,18 @@ RESOURCE_ORDTYPE: dict[str, int] = {"increase": 0, "decrease": 1, "na": 2} RESOURCE_FLAG_CONSUMABLE = 0x80 RESOURCE_FLAG_RELEASE = 0x20 +# Set together on a resourcemap item whose location is exactly ["all"] +# (cluster-wide static shared pool): XCE_RESF_SHARED | XCE_RESF_GLOBAL in +# include/xce/types.h. The controller's config activation seeds such a +# resource's distribution value into c->sharedinst, and the scheduler's +# two-phase scheduling gates shared-pool admission on XCE_RESF_GLOBAL +# (calc_wanted_global_resvals) -- the YAML route to the license-pool +# scenario (admin-plane-gaps 1.10). +RESOURCE_FLAG_SHARED = 0x08 +RESOURCE_FLAG_GLOBAL = 0x04 +RESOURCE_FLAG_SHARED_GLOBAL = RESOURCE_FLAG_SHARED | RESOURCE_FLAG_GLOBAL + + def new_cluster_config_builder() -> "capnp._DynamicStructBuilder": """Convenience constructor: ``load_schema().ClusterConfig.new_message()``. @@ -355,6 +378,22 @@ def _build_named_group(item: dict, target: "capnp._DynamicStructBuilder") -> Non _build_membership_group(item, target) +def _build_resource_value(value: int | float | bool | str, target: "capnp._DynamicStructBuilder") -> None: + """Write a YAML resourcemap ``value`` into a ``Value`` struct builder, + picking the union member by YAML type (bool before int -- bool is an + int subclass in Python; numeric -> fvalue, boolean -> bvalue, string -> + svalue), mirroring mark_host_res's YAML-value decoding in + controller/worker.c. The union is accessed through the ``u`` group + (``Value.u.fvalue`` etc.) -- the members are not direct struct fields. + """ + if isinstance(value, bool): + target.u.bvalue = value + elif isinstance(value, (int, float)): + target.u.fvalue = float(value) + else: + target.u.svalue = str(value) + + def _build_resource(item: dict, target: "capnp._DynamicStructBuilder") -> None: """Populate one ``Resource`` list item from a ``resources.yaml`` entry. See module docstring, ``Resource``, for what is intentionally left @@ -374,6 +413,22 @@ def _build_resource(item: dict, target: "capnp._DynamicStructBuilder") -> None: flags |= RESOURCE_FLAG_CONSUMABLE if item.get("release"): flags |= RESOURCE_FLAG_RELEASE + resourcemap = item.get("resourcemap") or [] + if resourcemap: + distributions = target.init("distributions", len(resourcemap)) + for dist, entry in zip(distributions, resourcemap): + locations = entry.get("location") or [] + if locations: + hosts = dist.init("hosts", len(locations)) + for i, host in enumerate(locations): + hosts[i] = host + if "value" in entry: + _build_resource_value(entry["value"], dist.value) + if locations == ["all"]: + # Cluster-wide static shared pool: the one shape the + # scheduler can admit jobs against today (see module + # docstring, Resource). + flags |= RESOURCE_FLAG_SHARED_GLOBAL target.flags = flags diff --git a/src/xceadmin/tests/fixtures/full/expected.bin b/src/xceadmin/tests/fixtures/full/expected.bin index f5c671e5..4d0e67af 100644 Binary files a/src/xceadmin/tests/fixtures/full/expected.bin and b/src/xceadmin/tests/fixtures/full/expected.bin differ diff --git a/src/xceadmin/tests/test_config_build.py b/src/xceadmin/tests/test_config_build.py index 3e279d29..b127f532 100644 --- a/src/xceadmin/tests/test_config_build.py +++ b/src/xceadmin/tests/test_config_build.py @@ -198,19 +198,42 @@ def test_build_maps_resource_type_order_consumable_release(tmp_path, capsys): assert "'release'" not in stderr -def test_build_warns_not_fails_on_schema_valid_unmapped_resourcemap_field(tmp_path, capsys): - """resourcemap has no destination -- controller.capnp's distributions/ - hostBased fields are commented out (needs a schema change first). +def test_build_maps_resourcemap_to_distributions(tmp_path, capsys): + """resourcemap maps to Resource.distributions (one entry per item, + order preserved; location -> hosts, value -> Value union member by + YAML type) -- no longer an "unmapped field" warning. Only the + location==["all"] + value shape (cluster-wide static shared pool) + additionally sets XCE_RESF_SHARED|XCE_RESF_GLOBAL (admin-plane-gaps + 1.10); other shapes map definition-only. """ _write_yaml( tmp_path / "resources.yaml", { "resources": [ { - "name": "gpus", + "name": "licenses", "type": "numeric", - "resourcemap": [{"location": ["host1"], "value": 1}], - } + "consumable": True, + "resourcemap": [{"location": ["all"], "value": 100}], + }, + { + "name": "hostpool", + "type": "numeric", + "resourcemap": [ + {"location": ["host1", "host2"], "value": 100}, + {"location": ["default"]}, # dynamic, reported + ], + }, + { + "name": "flagres", + "type": "boolean", + "resourcemap": [{"location": ["host1"], "value": True}], + }, + { + "name": "strres", + "type": "string", + "resourcemap": [{"location": ["host1"], "value": "v1"}], + }, ] }, ) @@ -219,12 +242,34 @@ def test_build_warns_not_fails_on_schema_valid_unmapped_resourcemap_field(tmp_pa mod = load_schema() with mod.ClusterConfig.from_bytes(data) as reader: - assert reader.resources[0].name == "gpus" + resources = {r.name: r for r in reader.resources} + + licenses = resources["licenses"] + assert len(licenses.distributions) == 1 + assert [str(h) for h in licenses.distributions[0].hosts] == ["all"] + assert licenses.distributions[0].value.u.which == "fvalue" + assert licenses.distributions[0].value.u.fvalue == 100.0 + assert licenses.flags == 0x80 | 0x08 | 0x04 # CONSUMABLE|SHARED|GLOBAL + + hostpool = resources["hostpool"] + assert len(hostpool.distributions) == 2 + assert [str(h) for h in hostpool.distributions[0].hosts] == ["host1", "host2"] + assert hostpool.distributions[0].value.u.fvalue == 100.0 + # dynamic shape: hosts mapped, value left unset (capnp union default) + assert [str(h) for h in hostpool.distributions[1].hosts] == ["default"] + assert hostpool.distributions[1].value.u.which == "ivalue32" + assert hostpool.flags == 0 # no SHARED|GLOBAL without location [all] + + flag = resources["flagres"] + assert flag.distributions[0].value.u.which == "bvalue" + assert bool(flag.distributions[0].value.u.bvalue) is True + + sres = resources["strres"] + assert sres.distributions[0].value.u.which == "svalue" + assert str(sres.distributions[0].value.u.svalue) == "v1" stderr = capsys.readouterr().err - assert "resources.yaml" in stderr - assert "'resourcemap'" in stderr - assert "gpus" in stderr + assert "'resourcemap'" not in stderr def test_build_warns_on_schema_valid_unmapped_queue_fields(tmp_path, capsys): diff --git a/src/xceadmin/tests/test_mapping.py b/src/xceadmin/tests/test_mapping.py index 95aa5b10..88ceb67c 100644 --- a/src/xceadmin/tests/test_mapping.py +++ b/src/xceadmin/tests/test_mapping.py @@ -182,6 +182,96 @@ def test_map_list_section_noop_on_empty_or_missing_items(cluster_config): assert len(cluster_config.resources) == 0 +def test_map_list_section_resourcemap_maps_distributions_and_all_flag(cluster_config): + """resourcemap -> distributions (admin-plane-gaps 1.10): one + ResourceDistribution per resourcemap item in YAML order (location -> + hosts, value -> the Value union member picked by YAML type). Only the + location==["all"] + value shape sets XCE_RESF_SHARED|XCE_RESF_GLOBAL + (the cluster-wide static shared pool / license-pool shape); other + shapes map definition-only. + """ + items = [ + { + "name": "licenses", + "type": "numeric", + "consumable": True, + "resourcemap": [{"location": ["all"], "value": 100}], + }, + { + "name": "hostpool", + "type": "numeric", + "resourcemap": [ + {"value": 100, "location": ["host1", "host2"]}, + {"location": ["host5", "host6"]}, # dynamic, worker-reported + ], + }, + { + "name": "bigmem", + "type": "boolean", + "resourcemap": [{"value": True, "location": ["host1"]}], + }, + { + "name": "strres2", + "type": "string", + "resourcemap": [{"value": "v1", "location": ["host1"]}], + }, + {"name": "plain", "type": "numeric"}, # no resourcemap at all + ] + mapping.map_list_section(_spec_for("resources"), items, cluster_config) + + r0, r1, r2, r3, r4 = cluster_config.resources + + # [all]+value: SHARED|GLOBAL OR'd on top of CONSUMABLE + assert r0.flags == 0x80 | 0x08 | 0x04 # CONSUMABLE | SHARED | GLOBAL + assert len(r0.distributions) == 1 + assert [str(h) for h in r0.distributions[0].hosts] == ["all"] + assert r0.distributions[0].value.u.which == "fvalue" + assert r0.distributions[0].value.u.fvalue == 100.0 + + # host-subset static pool + dynamic shape: distributions mapped + # faithfully, flags untouched (no SHARED|GLOBAL without location [all]) + assert r1.flags == 0 + assert len(r1.distributions) == 2 + assert [str(h) for h in r1.distributions[0].hosts] == ["host1", "host2"] + assert r1.distributions[0].value.u.fvalue == 100.0 + assert [str(h) for h in r1.distributions[1].hosts] == ["host5", "host6"] + assert r1.distributions[1].value.u.which == "ivalue32" # capnp union default + + # boolean/string values pick their union member + assert r2.distributions[0].value.u.which == "bvalue" + assert bool(r2.distributions[0].value.u.bvalue) is True + assert r3.distributions[0].value.u.which == "svalue" + assert str(r3.distributions[0].value.u.svalue) == "v1" + + # no resourcemap -> no distributions, no extra flags + assert len(r4.distributions) == 0 + assert r4.flags == 0 + + +def test_map_list_section_resourcemap_real_repo_configs(cluster_config): + """The mapper handles the real src/configs/resources.yaml: every + resourcemap entry lands as a distribution, and the two [all]+value + resources (staticres4, res1) carry SHARED|GLOBAL. + """ + data = _load_real_yaml("resources.yaml") + mapping.map_list_section(_spec_for("resources"), data["resources"], cluster_config) + + by_name = {r.name: r for r in cluster_config.resources} + for r in by_name.values(): + expected_maps = [item for item in data["resources"] if item["name"] == r.name][0].get("resourcemap") or [] + assert len(r.distributions) == len(expected_maps) + + for name in ("staticres4", "res1"): + assert by_name[name].flags & 0x08 | 0x04 == 0x08 | 0x04 + assert len(by_name[name].distributions) == 1 + assert [str(h) for h in by_name[name].distributions[0].hosts] == ["all"] + assert by_name[name].distributions[0].value.u.fvalue == 100.0 + + # per-host/per-default shapes stay definition-only + for name in ("staticres1", "staticres2", "staticres3", "host_quota"): + assert by_name[name].flags & (0x08 | 0x04) == 0 + + # --------------------------------------------------------------------------- # map_list_section: users.yaml/hosts.yaml -> ugroups/hgroups (named groups) # ---------------------------------------------------------------------------