diff --git a/src/controller/job.c b/src/controller/job.c index 3f7ba584..3dffaf76 100644 --- a/src/controller/job.c +++ b/src/controller/job.c @@ -977,8 +977,10 @@ void merge_job_runusage(xce_job_runusage_t** dst, xce_job_runusage_t* src) if (matched >= 0) { /* Existing host: overwrite scalars only. d's host_name pointer * (possibly xstring-backed) is never touched or freed here. */ - d->host_runusage[matched]->cpu_time = sh->cpu_time; - d->host_runusage[matched]->mem_usage = sh->mem_usage; + d->host_runusage[matched]->cpu_time = sh->cpu_time; + 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 { /* Genuinely new host: append, transferring ownership of sh's * own plain-malloc'd host_name directly -- never xstring. */ diff --git a/src/controller/misc.c b/src/controller/misc.c index c8022b65..c915e474 100644 --- a/src/controller/misc.c +++ b/src/controller/misc.c @@ -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; 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; } -/* bug-1735: derives xce_job_runusage_t's own job-level max_mem/avg_mem/ - * used_mem/cpu_time from the per-host host_runusage[] entries - * update_host_runusage() populates -- replaces update_job_rusages()'s - * (job.c) standing "update other fields of job rusage later" FIXME. +/* bug-1735/bug-1736: derives xce_job_runusage_t's own job-level max_mem/ + * avg_mem/used_mem/cpu_time/peak_cpu_time from the per-host + * host_runusage[] entries update_host_runusage() populates -- replaces + * 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 * 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/ - * avg_cpu_efficiency/peak_cpu_efficiency are intentionally left untouched -- - * bug-1735 was explicitly scope-narrowed to memory + CPU time only; those - * fields are bug-1736 (a design task, no formula exists yet). */ + * the SUM across hosts. peak_cpu_time is the running MAX of that per-report + * cpu_time sum across every report this job has ever received (a job's own + * cpu_time can only trend upward within one report but this is called + * 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) { int i, valid = 0; @@ -190,6 +223,10 @@ void aggregate_job_runusage(xce_job_runusage_t* runusage) runusage->max_mem = mem_max; runusage->avg_mem = mem_sum / valid; 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) diff --git a/src/controller/testsuite/test_host_runusage.c b/src/controller/testsuite/test_host_runusage.c index be3f5a5c..4398028e 100644 --- a/src/controller/testsuite/test_host_runusage.c +++ b/src/controller/testsuite/test_host_runusage.c @@ -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 * 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 * is fixed. * - * This file unit-tests both 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. + * bug-1736 (split from bug-1735, lower priority, display/reporting only): + * two more gaps in the same area. * - * peak_cpu_time/peak_cpu_duration/mem_efficiency/avg_cpu_efficiency/ - * peak_cpu_efficiency are intentionally NOT covered here -- bug-1735 was - * explicitly scope-narrowed to memory + CPU time only; those fields are - * bug-1736 (a design task, not a clean fix -- no formula exists yet). + * 3. xce_host_runusage_t had NO field at all to receive the worker's + * "memory_swap"/"tasks_current" resnames -- a complete dead end, not + * a naming mismatch. Added mem_swap/tasks_current fields plus parsing. + * + * 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 #include @@ -79,6 +90,35 @@ static void free_usages(xce_rusage_t** 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) { int i; @@ -145,6 +185,33 @@ static void test_update_host_runusage_updates_existing_host_in_place(void) 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) */ /* ------------------------------------------------------------------ */ @@ -206,6 +273,37 @@ static void test_aggregate_job_runusage_multi_host_sum_max_avg(void) 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) { 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_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_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); return UNITY_END(); diff --git a/src/include/xce/types.h b/src/include/xce/types.h index b8bd359c..6ef8856b 100644 --- a/src/include/xce/types.h +++ b/src/include/xce/types.h @@ -416,6 +416,8 @@ typedef struct { char* host_name; int cpu_time; double mem_usage; + double mem_swap; + int tasks_current; } xce_host_runusage_t; typedef struct { diff --git a/src/schema/controller.capnp b/src/schema/controller.capnp index e4ac2875..3e6f6776 100644 --- a/src/schema/controller.capnp +++ b/src/schema/controller.capnp @@ -277,9 +277,11 @@ struct JobPIDInfo $C.mapname("job_pidinfo_t"){ # ({jobid, pids[], pgids[], usages[]}); this is the CONTROLLER'S ACCUMULATED # per-job/per-host usage shape, a different struct entirely. struct HostRunusage $C.mapname("xce_host_runusage_t"){ - hostName @0: Text $C.mapname("host_name"); - cpuTime @1: Int32 $C.mapname("cpu_time"); - memUsage @2: Float64 $C.mapname("mem_usage"); + hostName @0: Text $C.mapname("host_name"); + cpuTime @1: Int32 $C.mapname("cpu_time"); + 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"){