lei.chen/myxce
1
0
Fork 1
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(scheduler): charge unreserved jobs' real memory usage too

Browse source 
A job with no rusage[mem=...] request (reserved=0) was being SKIPPED
entirely during reservation-vs-usage reconciliation, on the mistaken
assumption that "no reservation" meant "nothing needs charging". It
doesn't: this codebase's admission check has no live OS read at
decision time (sched_check_host_satisfy_(), resource.c) -- it compares
the worker's last-reported "mem" value against this scheduler's OWN
tracked resInUse tally, built entirely from summed xce_res_account_t
entries. Skipping an unreserved job left its real, live memory
consumption permanently invisible to that tally -- an actual
over-subscription risk (the scheduler could admit more onto a host
than it can support, believing memory was free that a live process was
genuinely using), not merely a display gap.

Fix: charged = max(reserved, actual) applies to EVERY job, reserved or
not. ensure_host_account_by_resname() (resource.c) now creates a fresh
reserved=0 account entry (resolving residx via key_find() against
core->resmgr, same pattern sched_update_res_accounts() already uses
for a fresh account) instead of skipping when a job has no existing
entry for a resource. Returns NULL (a genuine no-op) only when the
resource isn't registered in the cluster's config at all.

Rewrote resource-accounting.md to correct the mechanism it documented:
"available" is NOT a live OS read (mem vs the similarly-named but
static maxmem was worth naming explicitly too) -- it's worker's
last-reported mem minus this scheduler's own resInUse tally, and that
tally must include every job's real usage regardless of reservation.
Also documented a known, pre-existing property surfaced while
correcting this: since mem's own worker report already reflects real
consumption and the scheduler's charge is a separate layer that
doesn't get cleared on a fresh report, tracking can become
conservative over time (under-utilization risk, never over-
subscription) -- the same direction slots already accepts, and a
separate, larger question than this fix's scope.

Replaced test_reconcile_job_resource_usage_skips_when_no_mem_request
(asserted the now-wrong skip behavior) with two tests: one proving an
unreserved job's real usage is created and charged, one proving a
genuine no-op when "mem" isn't a registered resource at all. Full
scheduler suite: 55/55 passing, zero regressions.

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-31 07:22:16 +00:00
parent 0937c0e16e
commit f4103e0efd
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
3 changed files with 304 additions and 93 deletions
Download patch file Download diff file Expand all files Collapse all files

93
src/scheduler/src/resource.c
View file

@ -605,9 +605,7 @@ int sched_commit_job_alloc(xce_sched_core_t* core, xce_sched_job_t* job)
/*
* find_host_account_by_resname - the xce_res_account_t within `hacct`
* matching `resname` (e.g. "mem"), or NULL if the job never requested that
* resource (see resource-accounting.md: no rusage[] clause for a resource
* means no account entry for it at all, not a zero-valued one).
* matching `resname` (e.g. "mem"), or NULL if no such entry exists yet.
*/
static xce_res_account_t*
find_host_account_by_resname(xce_host_account_t* hacct, const char* resname)
@ -630,6 +628,79 @@ find_host_account_by_resname(xce_host_account_t* hacct, const char* resname)
return NULL;
}
/*
* ensure_host_account_by_resname - find_host_account_by_resname(), or
* (resource-accounting.md correction) CREATE a fresh reserved=0 entry for
* `resname` if the job never requested it via rusage[].
*
* A job with no rusage[mem=...] clause genuinely reserves nothing -- but
* this codebase's admission check (sched_check_host_satisfy_(), above)
* never reads a live OS "how much is actually free" figure: it compares
* the worker's self-reported STATIC total (w->resvals) against this
* scheduler's OWN running tally (w->resInUse), built entirely by summing
* every job's xce_res_account_t.used+reserved. A reservation of 0 does NOT
* mean nothing needs charging -- charged = max(reserved, actual) = actual
* when reserved is 0, same formula, and that actual usage is real memory
* no other job can also use. Skipping it (as an earlier version of this
* function did) let an unreserved job's genuine consumption stay
* permanently invisible to w->resInUse -- the scheduler would believe
* memory was free that a live, unreserved job was actually sitting on, a
* real over-subscription risk, not merely a cosmetic gap.
*
* The new entry's residx is resolved the same way
* sched_update_res_accounts() resolves a fresh account's residx above --
* key_find() against core->resmgr, same lock discipline. Returns NULL
* (nothing to charge, not an error) if `resname` isn't even a registered
* resource in this cluster's config; the caller must treat that as a
* no-op, matching find_host_account_by_resname()'s own contract.
*/
static xce_res_account_t*
ensure_host_account_by_resname(xce_sched_core_t* core,
xce_host_account_t* hacct, const char* resname)
{
xce_res_account_t* entry;
xce_res_account_t** grown;
xce_resource_t* r = NULL;
int rc = 0;
entry = find_host_account_by_resname(hacct, resname);
if (entry != NULL) {
return entry;
}
lock(core->resmgr, RTTI_LOCK_READ);
rc = key_find(core->resmgr, (void*)resname, (void**)&r);
unlock(core->resmgr);
if ((rc < 0) || (r == NULL)) {
return NULL;
}
entry = (xce_res_account_t*)calloc(1, sizeof(*entry));
if (entry == NULL) {
return NULL;
}
entry->resname = strdup(resname);
entry->residx = r->idx;
entry->used = 0.0;
entry->reserved = 0.0;
entry->allocated = 0.0;
grown = (xce_res_account_t**)realloc(
hacct->accounts, (size_t)(hacct->naccount + 1) * sizeof(*grown));
if (grown == NULL) {
FREEUP(entry->resname);
FREEUP(entry);
return NULL;
}
hacct->accounts = grown;
hacct->accounts[hacct->naccount] = entry;
hacct->naccount++;
return entry;
}
/*
* sched_reconcile_host_resource_usage - resource-accounting.md's
* reservation-vs-actual-usage policy for one REAL (gauge-style, e.g. "mem")
@ -710,11 +781,14 @@ int sched_reconcile_host_resource_usage(xce_sched_core_t* core,
* controller side, mapped explicitly here rather than unified across the
* codebase (out of this fix's scope too).
*
* A no-op (0, nothing charged) if the job never requested rusage[mem=...]
* at all (find_host_account_by_resname() then returns NULL, per that
* function's own doc comment) -- resource-accounting.md's "no rusage
* means reserved=0" case, which needs no reconciliation since there was
* never anything held back.
* A job that never requested rusage[mem=...] gets reserved=0, NOT skipped
* entirely: ensure_host_account_by_resname() creates the missing account
* entry on first use rather than treating "no reservation" as "nothing to
* charge" -- see that function's own doc comment for why this codebase's
* admission check specifically requires it (no live OS "available" read
* anywhere in the path; every job's real usage must be reflected in
* w->resInUse, reserved or not). The only genuine no-op is when "mem"
* isn't even a registered resource in this cluster's config at all.
*/
#define SCHED_RUSAGE_MEM_RESNAME "memory_rss"
#define SCHED_MEM_RESOURCE_NAME "mem"
@ -741,7 +815,8 @@ int sched_reconcile_job_resource_usage(xce_sched_core_t* core,
return 0;
}
mem_acct = find_host_account_by_resname(hacct, SCHED_MEM_RESOURCE_NAME);
mem_acct = ensure_host_account_by_resname(core, hacct,
SCHED_MEM_RESOURCE_NAME);
if (mem_acct == NULL) {
return 0;
}

74
src/scheduler/testsuite/test_resource_accounting.c
View file

@ -65,6 +65,7 @@
#include <unity/unity.h>
#include "resobj.h"
#include "scheduler.h"
#include "sim.h"
@ -623,20 +624,44 @@ static void test_reconcile_job_resource_usage_skips_multihost_jobs(void)
TEST_ASSERT_EQUAL_INT(0, rc);
}
/*
* test_reconcile_job_resource_usage_charges_unreserved_job_full_usage -
* resource-accounting.md's correction: a job with NO rusage[mem=...] at
* all is reserved=0, but that must NOT mean "nothing to charge". This
* codebase's admission check has no live OS read at decision time (see
* the steering doc) -- w->resInUse is the ONLY place a job's real memory
* footprint is ever reflected, so an unreserved job's actual usage must
* still be charged in full (max(0, actual) == actual), or the scheduler's
* own capacity tracking silently under-counts real consumption and can
* over-admit new jobs onto memory a live, unreserved job is genuinely
* sitting on.
*/
static void
test_reconcile_job_resource_usage_skips_when_no_mem_request(void)
test_reconcile_job_resource_usage_charges_unreserved_job_full_usage(void)
{
xce_scheduler_t sched;
xce_sched_worker_t* w;
xce_job_account_t* jacct;
xce_job_rusages_t* rusages = make_mem_rusages(99.0f);
xce_job_rusages_t* rusages = make_mem_rusages(5.0f);
xce_resource_t mem_res = { 0 };
xce_resource_t* registered;
int rc;
memset(&sched, 0, sizeof(sched));
setup_core_with_worker(&(sched.core), WORKER_NAME);
w = sched_get_worker(&(sched.core), WORKER_NAME);
TEST_ASSERT_NOT_NULL(w);
/* A single-host account with NO "mem" entry at all -- resource-
* accounting.md's "no rusage[mem=...] means reserved=0, nothing to
* reconcile" case. */
/* "mem" must be a real, registered resource for ensure_host_account_
* by_resname()'s key_find() lookup to resolve a residx -- mirrors how
* a real cluster config would already have it registered long before
* any job ever runs. */
mem_res.name = strdup(SCHED_MEM_RESOURCE_NAME);
registered = makeResource(sched.core.resmgr, &mem_res);
TEST_ASSERT_NOT_NULL(registered);
/* A single-host account with NO "mem" entry at all -- the job never
* requested rusage[mem=...]. */
jacct = (xce_job_account_t*)calloc(1, sizeof(*jacct));
jacct->nhosts = 1;
jacct->accounts = (xce_host_account_t**)calloc(1, sizeof(*jacct->accounts));
@ -646,6 +671,42 @@ test_reconcile_job_resource_usage_skips_when_no_mem_request(void)
rc = sched_reconcile_job_resource_usage(&(sched.core), jacct, rusages);
TEST_ASSERT_EQUAL_INT(0, rc);
/* An account entry now exists (created on first use), reserved=0,
* used == the full actual usage, and the pool reflects it. */
TEST_ASSERT_EQUAL_INT(1, jacct->accounts[0]->naccount);
TEST_ASSERT_EQUAL_STRING(SCHED_MEM_RESOURCE_NAME,
jacct->accounts[0]->accounts[0]->resname);
TEST_ASSERT_EQUAL_FLOAT(0.0,
(float)jacct->accounts[0]->accounts[0]->reserved);
TEST_ASSERT_EQUAL_FLOAT(5.0, (float)jacct->accounts[0]->accounts[0]->used);
TEST_ASSERT_EQUAL_FLOAT(
5.0, get_worker_resinuse(w, jacct->accounts[0]->accounts[0]->residx));
}
static void
test_reconcile_job_resource_usage_noop_when_mem_not_a_registered_resource(void)
{
xce_scheduler_t sched;
xce_job_account_t* jacct;
xce_job_rusages_t* rusages = make_mem_rusages(99.0f);
int rc;
memset(&sched, 0, sizeof(sched));
setup_core_with_worker(&(sched.core), WORKER_NAME);
/* Deliberately do NOT register "mem" into sched.core.resmgr -- a
* misconfigured cluster with no such resource defined at all. */
jacct = (xce_job_account_t*)calloc(1, sizeof(*jacct));
jacct->nhosts = 1;
jacct->accounts = (xce_host_account_t**)calloc(1, sizeof(*jacct->accounts));
jacct->accounts[0] = (xce_host_account_t*)calloc(1, sizeof(xce_host_account_t));
jacct->accounts[0]->host = strdup(WORKER_NAME);
jacct->accounts[0]->naccount = 0;
rc = sched_reconcile_job_resource_usage(&(sched.core), jacct, rusages);
TEST_ASSERT_EQUAL_INT(0, rc);
TEST_ASSERT_EQUAL_INT(0, jacct->accounts[0]->naccount);
}
static void test_reconcile_job_resource_usage_null_args_return_einval(void)
@ -673,7 +734,8 @@ int main(void)
RUN_TEST(test_reconcile_host_resource_usage_null_args_return_einval);
RUN_TEST(test_reconcile_job_resource_usage_maps_memory_rss_to_mem_account);
RUN_TEST(test_reconcile_job_resource_usage_skips_multihost_jobs);
RUN_TEST(test_reconcile_job_resource_usage_skips_when_no_mem_request);
RUN_TEST(test_reconcile_job_resource_usage_charges_unreserved_job_full_usage);
RUN_TEST(test_reconcile_job_resource_usage_noop_when_mem_not_a_registered_resource);
RUN_TEST(test_reconcile_job_resource_usage_null_args_return_einval);
return UNITY_END();
}