fix(bug-1735): job memory/CPU usage always read 0 in scheduler replay
Two independent gaps in misc.c/job.c, both silently zeroing every real job's resource-usage data: - update_host_runusage() matched worker rusage report resnames "cputime"/"mem", but the real worker's collector (collect_job_rusages(), xcecd_cgroups.c) sends "cpu_time_sec"/ "memory_rss" -- the two never matched, so cpu_time/mem_usage stayed at their calloc()'d zero for every non-simulated job. Aligned both sides, including xce_test's own simulated-worker fixture. - update_job_rusages() had a standing FIXME: xce_job_runusage_t's job-level max_mem/avg_mem/used_mem/cpu_time were never derived from the per-host host_runusage[] entries. Added aggregate_job_runusage() (sum/max/avg across hosts) and wired it in right after the per-host update. Added test_host_runusage.c (standalone unit test, mirrors resval_typeresolve_unit_test's isolation pattern) plus the controller_unit_test stub aggregate_job_runusage() needs since that target excludes the real misc.c. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019QxaSzHRB7pmYFp4cVnwkA
This commit is contained in:
parent
ccef561031
commit
d8a37fa27a
7 changed files with 335 additions and 10 deletions
|
|
@ -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*);
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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'])
|
||||
|
||||
|
|
|
|||
|
|
@ -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;
|
||||
|
|
|
|||
|
|
@ -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]) };
|
||||
|
||||
|
|
|
|||
|
|
@ -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; }
|
||||
|
|
|
|||
240
src/controller/testsuite/test_host_runusage.c
Normal file
240
src/controller/testsuite/test_host_runusage.c
Normal file
|
|
@ -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 <math.h>
|
||||
#include <stdlib.h>
|
||||
#include <string.h>
|
||||
|
||||
#include <unity/unity.h>
|
||||
|
||||
#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();
|
||||
}
|
||||
Loading…
Add table
Add a link
Reference in a new issue