fix(bug-1736): capture memory_swap/tasks_current, compute peak_cpu_time

The worker's rusage collector already measures and sends
"memory_swap"/"tasks_current" per job, but xce_host_runusage_t had no
field to receive them at all -- a complete dead end, not a naming
mismatch like bug-1735's cpu_time/mem_usage. Added mem_swap/
tasks_current fields (types.h), matching capnp fields on HostRunusage
(controller.capnp, new ordinals @3/@4) for WAL field-completeness, and
wired update_host_runusage() (misc.c) to parse them. Also fixed
merge_job_runusage()'s (job.c) existing-host merge branch, which only
copied cpu_time/mem_usage per host -- the two new fields would
otherwise have been silently dropped on WAL replay/catchup for a host
already seen.

Also computed peak_cpu_time as the running max of aggregate_job_
runusage()'s per-call cpu_time sum across every report a job has ever
received, guarding against a later report reducing it.

Deliberately NOT computed: peak_cpu_duration and the three efficiency
fields (mem_efficiency/avg_cpu_efficiency/peak_cpu_efficiency) -- these
need a "used vs. requested" ratio against a job's resource request (a
resreq expression tree, not a plain core/slot count) and/or an
unspecified duration semantic. A documented scope decision, not an
oversight -- see aggregate_job_runusage()'s doc comment.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA
This commit is contained in:
Rongsong Shen 2026-08-30 15:32:02 +00:00
parent dfcd345ded
commit 5c2c6664e5
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
5 changed files with 169 additions and 26 deletions

View file

@ -977,8 +977,10 @@ void merge_job_runusage(xce_job_runusage_t** dst, xce_job_runusage_t* src)
if (matched >= 0) { if (matched >= 0) {
/* Existing host: overwrite scalars only. d's host_name pointer /* Existing host: overwrite scalars only. d's host_name pointer
* (possibly xstring-backed) is never touched or freed here. */ * (possibly xstring-backed) is never touched or freed here. */
d->host_runusage[matched]->cpu_time = sh->cpu_time; d->host_runusage[matched]->cpu_time = sh->cpu_time;
d->host_runusage[matched]->mem_usage = sh->mem_usage; d->host_runusage[matched]->mem_usage = sh->mem_usage;
d->host_runusage[matched]->mem_swap = sh->mem_swap;
d->host_runusage[matched]->tasks_current = sh->tasks_current;
} else { } else {
/* Genuinely new host: append, transferring ownership of sh's /* Genuinely new host: append, transferring ownership of sh's
* own plain-malloc'd host_name directly -- never xstring. */ * own plain-malloc'd host_name directly -- never xstring. */

View file

@ -139,21 +139,54 @@ int update_host_runusage(xce_job_runusage_t** runusage, char* host, int count,
rusages->host_runusage[j]->mem_usage = usages[i]->value; rusages->host_runusage[j]->mem_usage = usages[i]->value;
continue; continue;
} }
/* bug-1736: the worker already sends these two (collect_job_rusages(),
* xcecd_cgroups.c res_names[]) but xce_host_runusage_t had no field
* to receive them at all -- a complete dead end, not a naming
* mismatch like bug-1735's cpu_time/mem_usage. */
if (strcmp(usages[i]->resname, "memory_swap") == 0) {
rusages->host_runusage[j]->mem_swap = usages[i]->value;
continue;
}
if (strcmp(usages[i]->resname, "tasks_current") == 0) {
rusages->host_runusage[j]->tasks_current = (int)usages[i]->value;
continue;
}
} }
return 0; return 0;
} }
/* bug-1735: derives xce_job_runusage_t's own job-level max_mem/avg_mem/ /* bug-1735/bug-1736: derives xce_job_runusage_t's own job-level max_mem/
* used_mem/cpu_time from the per-host host_runusage[] entries * avg_mem/used_mem/cpu_time/peak_cpu_time from the per-host
* update_host_runusage() populates -- replaces update_job_rusages()'s * host_runusage[] entries update_host_runusage() populates -- replaces
* (job.c) standing "update other fields of job rusage later" FIXME. * update_job_rusages()'s (job.c) standing "update other fields of job
* rusage later" FIXME.
*
* used_mem is the SUM across hosts (a multi-host job's total memory * used_mem is the SUM across hosts (a multi-host job's total memory
* footprint); max_mem/avg_mem are the max/average across hosts; cpu_time is * footprint); max_mem/avg_mem are the max/average across hosts; cpu_time is
* the SUM across hosts. peak_cpu_time/peak_cpu_duration/mem_efficiency/ * the SUM across hosts. peak_cpu_time is the running MAX of that per-report
* avg_cpu_efficiency/peak_cpu_efficiency are intentionally left untouched -- * cpu_time sum across every report this job has ever received (a job's own
* bug-1735 was explicitly scope-narrowed to memory + CPU time only; those * cpu_time can only trend upward within one report but this is called
* fields are bug-1736 (a design task, no formula exists yet). */ * repeatedly over the job's life, and a merge/replay/WAL-catchup path could
* in principle deliver reports out of order -- tracking the max explicitly,
* not just trusting "latest == largest", is what makes it a genuine peak
* rather than a duplicate of cpu_time).
*
* bug-1736's OTHER two fields, peak_cpu_duration and the three efficiency
* fields (mem_efficiency/avg_cpu_efficiency/peak_cpu_efficiency), are
* DELIBERATELY left untouched here -- a documented scope decision, not an
* oversight (steering persistence.md's field-completeness rule explicitly
* allows this when documented). Unlike peak_cpu_time, these have no
* unambiguous definition to implement without inventing a formula: the
* efficiency fields need a "used vs. requested" ratio against a job's
* resource request, which is a resreq expression tree (parse_resreq()),
* not a plain core/slot count -- deriving a well-defined denominator from
* that is a real design task, not a two-line fix. peak_cpu_duration's own
* semantics (wall-clock elapsed to the peak? duration the peak was
* sustained?) were never specified anywhere. Both need a product decision
* before a formula can be implemented correctly. */
void aggregate_job_runusage(xce_job_runusage_t* runusage) void aggregate_job_runusage(xce_job_runusage_t* runusage)
{ {
int i, valid = 0; int i, valid = 0;
@ -190,6 +223,10 @@ void aggregate_job_runusage(xce_job_runusage_t* runusage)
runusage->max_mem = mem_max; runusage->max_mem = mem_max;
runusage->avg_mem = mem_sum / valid; runusage->avg_mem = mem_sum / valid;
runusage->cpu_time = cpu_sum; runusage->cpu_time = cpu_sum;
if (cpu_sum > runusage->peak_cpu_time) {
runusage->peak_cpu_time = cpu_sum;
}
} }
int update_pids(job_pidinfo_t** pidinfo, int npids, int* pids) int update_pids(job_pidinfo_t** pidinfo, int npids, int* pids)

View file

@ -1,4 +1,4 @@
/* test_host_runusage.c -- bug-1735 regression suite /* test_host_runusage.c -- bug-1735/bug-1736 regression suite
* *
* bug-1735: job memory usage always reads 0 in the scheduler's simulation * bug-1735: job memory usage always reads 0 in the scheduler's simulation
* record file, despite the worker daemon genuinely measuring it. Two * record file, despite the worker daemon genuinely measuring it. Two
@ -18,19 +18,30 @@
* so a multi-host job's aggregate usage stayed 0 even once (1) above * so a multi-host job's aggregate usage stayed 0 even once (1) above
* is fixed. * is fixed.
* *
* This file unit-tests both fixes directly against the REAL misc.c (linked * bug-1736 (split from bug-1735, lower priority, display/reporting only):
* standalone, mirroring resval_typeresolve_unit_test's/sched_worker_change_ * two more gaps in the same area.
* unit_test's isolation rationale -- misc.c's update_host_runusage()/
* aggregate_job_runusage() are calloc/realloc/strcmp-only, no RTTI locking,
* no networking, so they link and run with zero fake/stub scaffolding).
* get_ep_worker()/get_ep_role() (same file) are untouched by this fix and
* unused by these tests, but still link fine via controller_unit_test_deps'
* existing librtti_dep.
* *
* peak_cpu_time/peak_cpu_duration/mem_efficiency/avg_cpu_efficiency/ * 3. xce_host_runusage_t had NO field at all to receive the worker's
* peak_cpu_efficiency are intentionally NOT covered here -- bug-1735 was * "memory_swap"/"tasks_current" resnames -- a complete dead end, not
* explicitly scope-narrowed to memory + CPU time only; those fields are * a naming mismatch. Added mem_swap/tasks_current fields plus parsing.
* bug-1736 (a design task, not a clean fix -- no formula exists yet). *
* 4. xce_job_runusage_t.peak_cpu_time was never computed. Added as the
* running MAX of aggregate_job_runusage()'s per-call cpu_time sum
* across every call for a given job (not just the latest report).
*
* peak_cpu_duration/mem_efficiency/avg_cpu_efficiency/peak_cpu_efficiency
* are DELIBERATELY still not covered -- see aggregate_job_runusage()'s own
* doc comment (misc.c) for why: no unambiguous formula/definition exists
* for these without a product decision, unlike peak_cpu_time.
*
* This file unit-tests all four fixes directly against the REAL misc.c
* (linked standalone, mirroring resval_typeresolve_unit_test's/
* sched_worker_change_unit_test's isolation rationale -- misc.c's
* update_host_runusage()/aggregate_job_runusage() are calloc/realloc/
* strcmp-only, no RTTI locking, no networking, so they link and run with
* zero fake/stub scaffolding). get_ep_worker()/get_ep_role() (same file)
* are untouched by this fix and unused by these tests, but still link
* fine via controller_unit_test_deps' existing librtti_dep.
*/ */
#include <math.h> #include <math.h>
#include <stdlib.h> #include <stdlib.h>
@ -79,6 +90,35 @@ static void free_usages(xce_rusage_t** usages)
free(usages); free(usages);
} }
static xce_rusage_t** make_usages4(const char* n1, float v1, const char* n2,
float v2, const char* n3, float v3,
const char* n4, float v4)
{
const char* names[4] = { n1, n2, n3, n4 };
float values[4] = { v1, v2, v3, v4 };
xce_rusage_t** usages = (xce_rusage_t**)calloc(4, sizeof(xce_rusage_t*));
int i;
for (i = 0; i < 4; i++) {
usages[i] = (xce_rusage_t*)calloc(1, sizeof(xce_rusage_t));
usages[i]->resname = strdup(names[i]);
usages[i]->value = values[i];
}
return usages;
}
static void free_usages4(xce_rusage_t** usages)
{
int i;
for (i = 0; i < 4; i++) {
free(usages[i]->resname);
free(usages[i]);
}
free(usages);
}
static void free_runusage(xce_job_runusage_t* r) static void free_runusage(xce_job_runusage_t* r)
{ {
int i; int i;
@ -145,6 +185,33 @@ static void test_update_host_runusage_updates_existing_host_in_place(void)
free_runusage(runusage); free_runusage(runusage);
} }
/* ------------------------------------------------------------------ */
/* Tests -- bug-1736: memory_swap/tasks_current (root cause 3) */
/* ------------------------------------------------------------------ */
static void test_update_host_runusage_parses_swap_and_task_count(void)
{
xce_job_runusage_t* runusage = NULL;
xce_rusage_t** usages = make_usages4(
"cpu_time_sec", 5.0f, "memory_rss", 64.0f, "memory_swap", 12.5f,
"tasks_current", 3.0f);
int rc = update_host_runusage(&runusage, strdup("host-a"), 4, usages);
TEST_ASSERT_EQUAL_INT(0, rc);
ASSERT_DOUBLE_EQ_MSG(
12.5, runusage->host_runusage[0]->mem_swap,
"mem_swap must be set from the worker's 'memory_swap' resname "
"(bug-1736) -- previously had no field to land in at all");
TEST_ASSERT_EQUAL_INT_MESSAGE(
3, runusage->host_runusage[0]->tasks_current,
"tasks_current must be set from the worker's 'tasks_current' "
"resname (bug-1736) -- previously had no field to land in at all");
free_usages4(usages);
free_runusage(runusage);
}
/* ------------------------------------------------------------------ */ /* ------------------------------------------------------------------ */
/* Tests -- aggregate_job_runusage() (root cause 2) */ /* Tests -- aggregate_job_runusage() (root cause 2) */
/* ------------------------------------------------------------------ */ /* ------------------------------------------------------------------ */
@ -206,6 +273,37 @@ static void test_aggregate_job_runusage_multi_host_sum_max_avg(void)
free_runusage(r); free_runusage(r);
} }
static void test_aggregate_job_runusage_peak_cpu_time_is_running_max(void)
{
int cpu1[] = { 50 };
double mem1[] = { 100.0 };
xce_job_runusage_t* r = make_runusage_with_hosts(1, cpu1, mem1);
aggregate_job_runusage(r);
TEST_ASSERT_EQUAL_INT_MESSAGE(50, r->peak_cpu_time,
"peak_cpu_time tracks the first report");
/* Second report: a LOWER cpu_time (e.g. a merge/replay delivered an
* older report after a newer one) -- peak must NOT regress. */
r->host_runusage[0]->cpu_time = 20;
aggregate_job_runusage(r);
TEST_ASSERT_EQUAL_INT_MESSAGE(
50, r->peak_cpu_time,
"peak_cpu_time (bug-1736) is the running MAX across every report "
"this job has ever received, not just the latest one -- a lower "
"subsequent report must not regress it");
TEST_ASSERT_EQUAL_INT_MESSAGE(
20, r->cpu_time, "cpu_time itself IS the latest report, unlike peak");
/* Third report: a NEW high -- peak must advance. */
r->host_runusage[0]->cpu_time = 90;
aggregate_job_runusage(r);
TEST_ASSERT_EQUAL_INT_MESSAGE(90, r->peak_cpu_time,
"peak_cpu_time advances on a new high");
free_runusage(r);
}
static void test_aggregate_job_runusage_null_and_empty_are_safe(void) static void test_aggregate_job_runusage_null_and_empty_are_safe(void)
{ {
xce_job_runusage_t empty = { 0 }; xce_job_runusage_t empty = { 0 };
@ -232,8 +330,10 @@ int main(void)
RUN_TEST(test_update_host_runusage_matches_real_worker_resnames); RUN_TEST(test_update_host_runusage_matches_real_worker_resnames);
RUN_TEST(test_update_host_runusage_updates_existing_host_in_place); RUN_TEST(test_update_host_runusage_updates_existing_host_in_place);
RUN_TEST(test_update_host_runusage_parses_swap_and_task_count);
RUN_TEST(test_aggregate_job_runusage_single_host); RUN_TEST(test_aggregate_job_runusage_single_host);
RUN_TEST(test_aggregate_job_runusage_multi_host_sum_max_avg); RUN_TEST(test_aggregate_job_runusage_multi_host_sum_max_avg);
RUN_TEST(test_aggregate_job_runusage_peak_cpu_time_is_running_max);
RUN_TEST(test_aggregate_job_runusage_null_and_empty_are_safe); RUN_TEST(test_aggregate_job_runusage_null_and_empty_are_safe);
return UNITY_END(); return UNITY_END();

View file

@ -416,6 +416,8 @@ typedef struct {
char* host_name; char* host_name;
int cpu_time; int cpu_time;
double mem_usage; double mem_usage;
double mem_swap;
int tasks_current;
} xce_host_runusage_t; } xce_host_runusage_t;
typedef struct { typedef struct {

View file

@ -277,9 +277,11 @@ struct JobPIDInfo $C.mapname("job_pidinfo_t"){
# ({jobid, pids[], pgids[], usages[]}); this is the CONTROLLER'S ACCUMULATED # ({jobid, pids[], pgids[], usages[]}); this is the CONTROLLER'S ACCUMULATED
# per-job/per-host usage shape, a different struct entirely. # per-job/per-host usage shape, a different struct entirely.
struct HostRunusage $C.mapname("xce_host_runusage_t"){ struct HostRunusage $C.mapname("xce_host_runusage_t"){
hostName @0: Text $C.mapname("host_name"); hostName @0: Text $C.mapname("host_name");
cpuTime @1: Int32 $C.mapname("cpu_time"); cpuTime @1: Int32 $C.mapname("cpu_time");
memUsage @2: Float64 $C.mapname("mem_usage"); memUsage @2: Float64 $C.mapname("mem_usage");
memSwap @3: Float64 $C.mapname("mem_swap");
tasksCurrent @4: Int32 $C.mapname("tasks_current");
} }
struct JobRunusage $C.mapname("xce_job_runusage_t"){ struct JobRunusage $C.mapname("xce_job_runusage_t"){