diff --git a/src/controller/include/cluster.h b/src/controller/include/cluster.h index c55323ac..d439012c 100644 --- a/src/controller/include/cluster.h +++ b/src/controller/include/cluster.h @@ -198,6 +198,7 @@ extern int job_ctrl(cluster_t*, xce_jobctrl_t*); extern int update_job_rusages(cluster_t*, endpoint_t*, xce_request_t*); extern int update_host_runusage(xce_job_runusage_t**, char*, int, xce_rusage_t**); +extern void aggregate_job_runusage(xce_job_runusage_t*); extern int update_pids(job_pidinfo_t**, int, int*); extern int update_pgids(job_pidinfo_t**, int, int*); extern int get_ep_role(endpoint_t*); diff --git a/src/controller/job.c b/src/controller/job.c index 30177b45..3f7ba584 100644 --- a/src/controller/job.c +++ b/src/controller/job.c @@ -3193,13 +3193,14 @@ int update_job_rusages(cluster_t* c, endpoint_t* ep, xce_request_t* request) update_host_runusage(&(j->job.run_usage), host, rusages->count, rusages->usages); + /* bug-1735: derive job-level max_mem/avg_mem/used_mem/cpu_time from the + * per-host entries update_host_runusage() just updated -- replaces the + * standing "update other fields of job rusage later" FIXME. */ + aggregate_job_runusage(j->job.run_usage); + update_pids(&(j->job.pidinfo), rusages->npids, rusages->pids); update_pgids(&(j->job.pidinfo), rusages->npgids, rusages->pgids); - - /* FIXME - * update other fields of job rusage later - */ /* TEMP diagnostic (bug-1731 job-finish-notify-loss investigation). * DEBUG3-gated -- see bug-1731's own "re-add before next occurrence" * recommendation; the DEBUG3 log-retention bump (bug-1731 support, diff --git a/src/controller/meson.build b/src/controller/meson.build index 089ecd55..71dc00b6 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -355,3 +355,27 @@ test('resval typeresolve test', resval_typeresolve_unit_test, timeout: 60, env: ['ASAN_OPTIONS=detect_leaks=0']) +# bug-1735: misc.c's update_host_runusage()/aggregate_job_runusage() are +# small, dependency-light functions (calloc/realloc/strcmp only, no RTTI +# locking, no networking) -- standalone binary linking the REAL misc.c +# directly, mirroring resval_typeresolve_unit_test's isolation rationale +# above. get_ep_worker()/get_ep_role() (same file, RTTI-backed) are unused +# by these tests but still link fine via controller_unit_test_deps' existing +# librtti_dep. +host_runusage_unit_test_src = [ + 'testsuite/test_host_runusage.c', + 'misc.c', +] + +host_runusage_unit_test = executable('host_runusage_unit_test', + host_runusage_unit_test_src, + include_directories: [incdir], + dependencies: controller_unit_test_deps, + c_args: ['-DXCE_TESTING'], + install: false) + +test('host runusage test', host_runusage_unit_test, + suite: 'controller', + timeout: 60, + env: ['ASAN_OPTIONS=detect_leaks=0']) + diff --git a/src/controller/misc.c b/src/controller/misc.c index 82ff8633..c8022b65 100644 --- a/src/controller/misc.c +++ b/src/controller/misc.c @@ -126,12 +126,16 @@ int update_host_runusage(xce_job_runusage_t** runusage, char* host, int count, } for (i = 0; i < count; i++) { - if (strcmp(usages[i]->resname, "cputime") == 0) { + /* bug-1735: these must match the REAL worker's rusage collector + * (collect_job_rusages(), xcecd_cgroups.c res_names[]) -- "cputime"/ + * "mem" never matched anything that worker actually sends, so + * cpu_time/mem_usage stayed at their calloc()'d zero forever. */ + if (strcmp(usages[i]->resname, "cpu_time_sec") == 0) { rusages->host_runusage[j]->cpu_time = usages[i]->value; continue; } - if (strcmp(usages[i]->resname, "mem") == 0) { + if (strcmp(usages[i]->resname, "memory_rss") == 0) { rusages->host_runusage[j]->mem_usage = usages[i]->value; continue; } @@ -140,6 +144,54 @@ int update_host_runusage(xce_job_runusage_t** runusage, char* host, int count, 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. + * 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). */ +void aggregate_job_runusage(xce_job_runusage_t* runusage) +{ + int i, valid = 0; + double mem_sum = 0.0; + double mem_max = 0.0; + int cpu_sum = 0; + + if ((runusage == NULL) || (runusage->host_runusage == NULL)) { + return; + } + + for (i = 0; i < runusage->num_hosts; i++) { + xce_host_runusage_t* h = runusage->host_runusage[i]; + + if (h == NULL) { + continue; + } + + mem_sum += h->mem_usage; + cpu_sum += h->cpu_time; + + if ((valid == 0) || (h->mem_usage > mem_max)) { + mem_max = h->mem_usage; + } + + valid++; + } + + if (valid == 0) { + return; + } + + runusage->used_mem = mem_sum; + runusage->max_mem = mem_max; + runusage->avg_mem = mem_sum / valid; + runusage->cpu_time = cpu_sum; +} + int update_pids(job_pidinfo_t** pidinfo, int npids, int* pids) { job_pidinfo_t* info; diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index ce20b631..3cd90c3d 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -4284,9 +4284,12 @@ static void check_run_jobs() int npids = 1; int npgids = 0; int nusage = 2; + /* bug-1735: match the real worker's collect_job_rusages() + * resnames (xcecd_cgroups.c), not the old never-matching + * "cputime"/"mem" strings. */ xce_rusage_t data[] = { - { "cputime", 100.0 }, - { "mem", 200.0 } + { "cpu_time_sec", 100.0 }, + { "memory_rss", 200.0 } }; xce_rusage_t* usages[] = { &(data[0]), &(data[1]) }; @@ -4348,9 +4351,12 @@ static void check_run_tasks() int npids = 1; int npgids = 0; int nusage = 2; + /* bug-1735: match the real worker's collect_job_rusages() + * resnames (xcecd_cgroups.c), not the old never-matching + * "cputime"/"mem" strings. */ xce_rusage_t data[] = { - { "cputime", 100.0 }, - { "mem", 200.0 } + { "cpu_time_sec", 100.0 }, + { "memory_rss", 200.0 } }; xce_rusage_t* usages[] = { &(data[0]), &(data[1]) }; diff --git a/src/controller/testsuite/test_commit_barrier_reply.c b/src/controller/testsuite/test_commit_barrier_reply.c index e8f7d8b7..48fe83d0 100644 --- a/src/controller/testsuite/test_commit_barrier_reply.c +++ b/src/controller/testsuite/test_commit_barrier_reply.c @@ -197,6 +197,7 @@ int update_pids(job_pidinfo_t **i, int n, int *p) { (void)i; (void)n; (void)p; return 0; } int update_pgids(job_pidinfo_t **i, int n, int *p) { (void)i; (void)n; (void)p; return 0; } +void aggregate_job_runusage(xce_job_runusage_t *r) { (void)r; } int on_scheduler_join(cluster_t *c) { (void)c; return 0; } int add_apiserver(cluster_t *c, char *n, endpoint_t *e) { (void)c; (void)n; (void)e; return 0; } diff --git a/src/controller/testsuite/test_host_runusage.c b/src/controller/testsuite/test_host_runusage.c new file mode 100644 index 00000000..be3f5a5c --- /dev/null +++ b/src/controller/testsuite/test_host_runusage.c @@ -0,0 +1,240 @@ +/* test_host_runusage.c -- bug-1735 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 + * independent, confirmed root causes in misc.c/job.c: + * + * 1. update_host_runusage() (misc.c) only ever matched resnames + * "cputime"/"mem" -- but the REAL worker's rusage collector + * (collect_job_rusages(), xcecd_cgroups.c) sends "cpu_time_sec"/ + * "memory_rss" (see that file's res_names[] array). The two never + * matched, so xce_host_runusage_t.cpu_time/mem_usage stayed at their + * calloc()'d zero forever, for every real (non-simulated) job. + * + * 2. update_job_rusages() (job.c) had a standing FIXME ("update other + * fields of job rusage later") -- xce_job_runusage_t's own job-level + * max_mem/avg_mem/used_mem/cpu_time were never derived from the + * per-host host_runusage[] entries update_host_runusage() populates, + * 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. + * + * 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). + */ +#include +#include +#include + +#include + +#include "cluster.h" + +/* ------------------------------------------------------------------ */ +/* Helpers */ +/* ------------------------------------------------------------------ */ + +/* This project's shared unity_dep is built with double-precision support + * excluded (TEST_ASSERT_EQUAL_DOUBLE compiles to an unconditional "Unity + * Double Precision Disabled" failure) -- no other test file in this + * codebase uses it either. Plain epsilon comparison in ordinary C avoids + * Unity's own double formatting/compare path entirely. */ +#define ASSERT_DOUBLE_EQ_MSG(expected, actual, msg) \ + TEST_ASSERT_TRUE_MESSAGE(fabs((expected) - (actual)) < 1e-6, msg) +#define ASSERT_DOUBLE_EQ(expected, actual) \ + ASSERT_DOUBLE_EQ_MSG(expected, actual, #actual " != " #expected) + +static xce_rusage_t** make_usages(const char* name1, float val1, + const char* name2, float val2) +{ + xce_rusage_t** usages = (xce_rusage_t**)calloc(2, sizeof(xce_rusage_t*)); + + usages[0] = (xce_rusage_t*)calloc(1, sizeof(xce_rusage_t)); + usages[0]->resname = strdup(name1); + usages[0]->value = val1; + + usages[1] = (xce_rusage_t*)calloc(1, sizeof(xce_rusage_t)); + usages[1]->resname = strdup(name2); + usages[1]->value = val2; + + return usages; +} + +static void free_usages(xce_rusage_t** usages) +{ + free(usages[0]->resname); + free(usages[0]); + free(usages[1]->resname); + free(usages[1]); + free(usages); +} + +static void free_runusage(xce_job_runusage_t* r) +{ + int i; + + if (r == NULL) { + return; + } + + for (i = 0; i < r->num_hosts; i++) { + free(r->host_runusage[i]->host_name); + free(r->host_runusage[i]); + } + free(r->host_runusage); + free(r); +} + +/* ------------------------------------------------------------------ */ +/* Tests -- update_host_runusage() resname alignment (root cause 1) */ +/* ------------------------------------------------------------------ */ + +static void +test_update_host_runusage_matches_real_worker_resnames(void) +{ + xce_job_runusage_t* runusage = NULL; + xce_rusage_t** usages + = make_usages("cpu_time_sec", 42.0f, "memory_rss", 128.5f); + + int rc = update_host_runusage(&runusage, strdup("host-a"), 2, usages); + + TEST_ASSERT_EQUAL_INT(0, rc); + TEST_ASSERT_NOT_NULL(runusage); + TEST_ASSERT_EQUAL_INT(1, runusage->num_hosts); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 42, runusage->host_runusage[0]->cpu_time, + "cpu_time must be set from the worker's real 'cpu_time_sec' resname " + "(bug-1735 root cause 1) -- NOT the old, never-matching 'cputime'"); + ASSERT_DOUBLE_EQ_MSG( + 128.5, runusage->host_runusage[0]->mem_usage, + "mem_usage must be set from the worker's real 'memory_rss' resname " + "(bug-1735 root cause 1) -- NOT the old, never-matching 'mem'"); + + free_usages(usages); + free_runusage(runusage); +} + +static void test_update_host_runusage_updates_existing_host_in_place(void) +{ + xce_job_runusage_t* runusage = NULL; + xce_rusage_t** first = make_usages("cpu_time_sec", 10.0f, "memory_rss", 50.0f); + xce_rusage_t** second = make_usages("cpu_time_sec", 20.0f, "memory_rss", 75.0f); + + update_host_runusage(&runusage, strdup("host-a"), 2, first); + update_host_runusage(&runusage, strdup("host-a"), 2, second); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, runusage->num_hosts, + "a second report for the SAME host must update in place, not " + "append a duplicate host entry"); + TEST_ASSERT_EQUAL_INT(20, runusage->host_runusage[0]->cpu_time); + ASSERT_DOUBLE_EQ(75.0, runusage->host_runusage[0]->mem_usage); + + free_usages(first); + free_usages(second); + free_runusage(runusage); +} + +/* ------------------------------------------------------------------ */ +/* Tests -- aggregate_job_runusage() (root cause 2) */ +/* ------------------------------------------------------------------ */ + +static xce_job_runusage_t* make_runusage_with_hosts(int n, const int* cpu, + const double* mem) +{ + xce_job_runusage_t* r = (xce_job_runusage_t*)calloc(1, sizeof(*r)); + int i; + + r->num_hosts = n; + r->host_runusage + = (xce_host_runusage_t**)calloc(n, sizeof(xce_host_runusage_t*)); + + for (i = 0; i < n; i++) { + r->host_runusage[i] + = (xce_host_runusage_t*)calloc(1, sizeof(xce_host_runusage_t)); + r->host_runusage[i]->host_name = strdup("h"); + r->host_runusage[i]->cpu_time = cpu[i]; + r->host_runusage[i]->mem_usage = mem[i]; + } + + return r; +} + +static void test_aggregate_job_runusage_single_host(void) +{ + int cpu[] = { 30 }; + double mem[] = { 256.0 }; + xce_job_runusage_t* r = make_runusage_with_hosts(1, cpu, mem); + + aggregate_job_runusage(r); + + ASSERT_DOUBLE_EQ(256.0, r->used_mem); + ASSERT_DOUBLE_EQ(256.0, r->max_mem); + ASSERT_DOUBLE_EQ(256.0, r->avg_mem); + TEST_ASSERT_EQUAL_INT(30, r->cpu_time); + + free_runusage(r); +} + +static void test_aggregate_job_runusage_multi_host_sum_max_avg(void) +{ + int cpu[] = { 10, 20, 30 }; + double mem[] = { 100.0, 300.0, 200.0 }; + xce_job_runusage_t* r = make_runusage_with_hosts(3, cpu, mem); + + aggregate_job_runusage(r); + + ASSERT_DOUBLE_EQ_MSG( + 600.0, r->used_mem, + "used_mem is the SUM of every host's mem_usage (total footprint of " + "a multi-host job), not any single host's value"); + ASSERT_DOUBLE_EQ_MSG(300.0, r->max_mem, "max_mem is the MAX across hosts"); + ASSERT_DOUBLE_EQ_MSG(200.0, r->avg_mem, "avg_mem is the AVERAGE across hosts"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 60, r->cpu_time, "job-level cpu_time is the SUM across hosts"); + + free_runusage(r); +} + +static void test_aggregate_job_runusage_null_and_empty_are_safe(void) +{ + xce_job_runusage_t empty = { 0 }; + + aggregate_job_runusage(NULL); /* must not crash */ + aggregate_job_runusage(&empty); /* num_hosts == 0, must not crash */ + + ASSERT_DOUBLE_EQ(0.0, empty.used_mem); +} + +/* ------------------------------------------------------------------ */ +/* Standalone Unity main (this binary is its own test executable -- see */ +/* meson.build's host_runusage_unit_test) */ +/* ------------------------------------------------------------------ */ + +void setUp(void) { return; } +void tearDown(void) { return; } + +int main(void) +{ + printf("\n>> bug-1735: host/job rusage resname + aggregation fix\n\n"); + + UNITY_BEGIN(); + + RUN_TEST(test_update_host_runusage_matches_real_worker_resnames); + RUN_TEST(test_update_host_runusage_updates_existing_host_in_place); + 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_null_and_empty_are_safe); + + return UNITY_END(); +}