fix(scheduler): percent-limit capacity reads maxmem/maxswap, not mem/swap (bug-1834)
update_limit_res_value() computed a percent-based limit's capacity from the resource's own live, fluctuating value (e.g. "mem"), letting the enforced cap drift with unrelated memory pressure instead of staying pinned to a stable fraction of total capacity. instantiate_limit() now resolves a static capacity-source resource once at instantiation time (mem->maxmem, swap->maxswap, table-driven), stored on a new xce_vres_t.percent_res field; a missing counterpart falls back to today's behavior rather than failing instantiation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01RLSNJhNoULvVcUhXEFafQC
This commit is contained in:
parent
7982f28557
commit
ea76f4771f
8 changed files with 368 additions and 15 deletions
|
|
@ -826,6 +826,15 @@ typedef struct {
|
|||
typedef struct {
|
||||
xce_resource_t* vres;
|
||||
xce_resource_t* res;
|
||||
/* bug-1834: resolved capacity-source resource for a percent-based
|
||||
* limit on a dynamic/consumable resource (e.g. "mem" -> "maxmem",
|
||||
* "swap" -> "maxswap") -- NULL when `res` itself already IS the right
|
||||
* source (percent==0, or no static counterpart is registered/known).
|
||||
* update_limit_res_value() (plugins/limit.c) reads this in preference
|
||||
* to `res` so a percent limit's capacity is pinned to the resource's
|
||||
* static total instead of drifting with its live, fluctuating value
|
||||
* (see .kiro/steering/resource-accounting.md's "mem is not maxmem"). */
|
||||
xce_resource_t* percent_res;
|
||||
xce_limit_t* limit;
|
||||
} xce_vres_t;
|
||||
|
||||
|
|
|
|||
|
|
@ -9,6 +9,8 @@
|
|||
#include "xcelimit.h"
|
||||
#include "xstring.h"
|
||||
|
||||
#include "test_resmgr_fixture.h"
|
||||
|
||||
static void test_build_consumer1(void)
|
||||
{
|
||||
xce_consumer_t* c = NULL;
|
||||
|
|
@ -647,6 +649,239 @@ static void test_fmt_limit_wrapper_matches_fmt_limit_units_raw_mode(void)
|
|||
destroy(umgr);
|
||||
}
|
||||
|
||||
/*
|
||||
* bug-1834: a percent-based limit's capacity must be pinned to the
|
||||
* resource's static total, not its live/fluctuating value -- instantiate_
|
||||
* limit() (xcelimit.c) resolves this ONCE at instantiation time into
|
||||
* vres->percent_res, which update_limit_res_value() (scheduler/plugins/
|
||||
* limit.c) then prefers over vres->res for the percent multiplication.
|
||||
* These four tests exercise the resolution itself (the only part of the
|
||||
* fix testable at this level -- update_limit_res_value() is scheduler-
|
||||
* private and its existing white-box test harness stubs get_resvals()/
|
||||
* put_resvals() as no-ops, see test_repl_limit_table.c).
|
||||
*
|
||||
* add_vres() (xcelimit.c) key_add()s the limit's own new "limit=<name>"
|
||||
* vres into resmgr with no datakiller, so each test frees that one entry
|
||||
* by hand before tearing down the shared make_test_resmgr() fixture --
|
||||
* mirroring free_test_resmgr()'s own manual-free pattern for its 8
|
||||
* pre-registered resources.
|
||||
*/
|
||||
static void free_instantiated_vres(var resmgr, xce_vres_t* vres)
|
||||
{
|
||||
if (vres == NULL) {
|
||||
return;
|
||||
}
|
||||
|
||||
if (vres->vres != NULL) {
|
||||
key_del(resmgr, (void*)vres->vres->name);
|
||||
FREEUP(vres->vres->name);
|
||||
FREEUP(vres->vres);
|
||||
}
|
||||
|
||||
FREEUP(vres);
|
||||
}
|
||||
|
||||
static void
|
||||
test_instantiate_limit_percent_mem_limit_resolves_percent_res_to_maxmem(
|
||||
void)
|
||||
{
|
||||
var umgr;
|
||||
var qmgr;
|
||||
var hmgr;
|
||||
var pmgr;
|
||||
var resmgr;
|
||||
xce_limit_t* limit;
|
||||
xce_vres_t* vres = NULL;
|
||||
int rc;
|
||||
xce_limit_config_t limit_config
|
||||
= { .name = "pct_mem_limit",
|
||||
.val
|
||||
= &(xce_limit_val_t) { .resname = "mem", .percent = 1,
|
||||
.val = 0.2 } };
|
||||
|
||||
groupmgr_init(&umgr, 4, 4);
|
||||
groupmgr_init(&qmgr, 4, 4);
|
||||
groupmgr_init(&hmgr, 4, 4);
|
||||
groupmgr_init(&pmgr, 4, 4);
|
||||
resmgr = make_test_resmgr();
|
||||
TEST_ASSERT_NOT_NULL(resmgr);
|
||||
|
||||
rc = build_limit(&limit, umgr, qmgr, hmgr, pmgr, &limit_config);
|
||||
TEST_ASSERT_TRUE(rc == 1);
|
||||
|
||||
rc = instantiate_limit(&vres, resmgr, limit, "", "", "");
|
||||
TEST_ASSERT_TRUE(rc >= 0);
|
||||
TEST_ASSERT_NOT_NULL(vres);
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
vres->percent_res,
|
||||
"bug-1834: a percent-based 'mem' limit must resolve a static "
|
||||
"capacity-source resource ('maxmem'), not leave capacity pinned "
|
||||
"to mem's own live, fluctuating value");
|
||||
TEST_ASSERT_EQUAL_STRING("maxmem", vres->percent_res->name);
|
||||
TEST_ASSERT_EQUAL_INT(TEST_RES_MAXMEM_IDX, vres->percent_res->idx);
|
||||
|
||||
free_instantiated_vres(resmgr, vres);
|
||||
kill_limit(limit);
|
||||
free_test_resmgr(resmgr);
|
||||
destroy(pmgr);
|
||||
destroy(hmgr);
|
||||
destroy(qmgr);
|
||||
destroy(umgr);
|
||||
|
||||
printf("test_instantiate_limit_percent_mem_limit_resolves_percent_res_"
|
||||
"to_maxmem: PASS\n");
|
||||
}
|
||||
|
||||
static void
|
||||
test_instantiate_limit_percent_swap_limit_resolves_percent_res_to_maxswap(
|
||||
void)
|
||||
{
|
||||
var umgr;
|
||||
var qmgr;
|
||||
var hmgr;
|
||||
var pmgr;
|
||||
var resmgr;
|
||||
xce_limit_t* limit;
|
||||
xce_vres_t* vres = NULL;
|
||||
int rc;
|
||||
xce_limit_config_t limit_config
|
||||
= { .name = "pct_swap_limit",
|
||||
.val
|
||||
= &(xce_limit_val_t) { .resname = "swap", .percent = 1,
|
||||
.val = 0.5 } };
|
||||
|
||||
groupmgr_init(&umgr, 4, 4);
|
||||
groupmgr_init(&qmgr, 4, 4);
|
||||
groupmgr_init(&hmgr, 4, 4);
|
||||
groupmgr_init(&pmgr, 4, 4);
|
||||
resmgr = make_test_resmgr();
|
||||
TEST_ASSERT_NOT_NULL(resmgr);
|
||||
|
||||
rc = build_limit(&limit, umgr, qmgr, hmgr, pmgr, &limit_config);
|
||||
TEST_ASSERT_TRUE(rc == 1);
|
||||
|
||||
rc = instantiate_limit(&vres, resmgr, limit, "", "", "");
|
||||
TEST_ASSERT_TRUE(rc >= 0);
|
||||
TEST_ASSERT_NOT_NULL(vres);
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
vres->percent_res,
|
||||
"bug-1834: a percent-based 'swap' limit must resolve a static "
|
||||
"capacity-source resource ('maxswap'), same pairing as mem/maxmem");
|
||||
TEST_ASSERT_EQUAL_STRING("maxswap", vres->percent_res->name);
|
||||
TEST_ASSERT_EQUAL_INT(TEST_RES_MAXSWAP_IDX, vres->percent_res->idx);
|
||||
|
||||
free_instantiated_vres(resmgr, vres);
|
||||
kill_limit(limit);
|
||||
free_test_resmgr(resmgr);
|
||||
destroy(pmgr);
|
||||
destroy(hmgr);
|
||||
destroy(qmgr);
|
||||
destroy(umgr);
|
||||
|
||||
printf("test_instantiate_limit_percent_swap_limit_resolves_percent_res_"
|
||||
"to_maxswap: PASS\n");
|
||||
}
|
||||
|
||||
static void
|
||||
test_instantiate_limit_absolute_value_mem_limit_leaves_percent_res_null(
|
||||
void)
|
||||
{
|
||||
var umgr;
|
||||
var qmgr;
|
||||
var hmgr;
|
||||
var pmgr;
|
||||
var resmgr;
|
||||
xce_limit_t* limit;
|
||||
xce_vres_t* vres = NULL;
|
||||
int rc;
|
||||
xce_limit_config_t limit_config
|
||||
= { .name = "abs_mem_limit",
|
||||
.val
|
||||
= &(xce_limit_val_t) { .resname = "mem", .percent = 0,
|
||||
.val = 10.0 } };
|
||||
|
||||
groupmgr_init(&umgr, 4, 4);
|
||||
groupmgr_init(&qmgr, 4, 4);
|
||||
groupmgr_init(&hmgr, 4, 4);
|
||||
groupmgr_init(&pmgr, 4, 4);
|
||||
resmgr = make_test_resmgr();
|
||||
TEST_ASSERT_NOT_NULL(resmgr);
|
||||
|
||||
rc = build_limit(&limit, umgr, qmgr, hmgr, pmgr, &limit_config);
|
||||
TEST_ASSERT_TRUE(rc == 1);
|
||||
|
||||
rc = instantiate_limit(&vres, resmgr, limit, "", "", "");
|
||||
TEST_ASSERT_TRUE(rc >= 0);
|
||||
TEST_ASSERT_NOT_NULL(vres);
|
||||
TEST_ASSERT_NULL_MESSAGE(
|
||||
vres->percent_res,
|
||||
"an absolute-value (percent=0) limit never reads a live resource "
|
||||
"value for its capacity at all (update_limit_res_value() uses "
|
||||
"limit->val.val directly) -- no substitution is needed or made");
|
||||
|
||||
free_instantiated_vres(resmgr, vres);
|
||||
kill_limit(limit);
|
||||
free_test_resmgr(resmgr);
|
||||
destroy(pmgr);
|
||||
destroy(hmgr);
|
||||
destroy(qmgr);
|
||||
destroy(umgr);
|
||||
|
||||
printf("test_instantiate_limit_absolute_value_mem_limit_leaves_percent_"
|
||||
"res_null: PASS\n");
|
||||
}
|
||||
|
||||
static void
|
||||
test_instantiate_limit_percent_limit_without_known_static_counterpart_leaves_percent_res_null(
|
||||
void)
|
||||
{
|
||||
var umgr;
|
||||
var qmgr;
|
||||
var hmgr;
|
||||
var pmgr;
|
||||
var resmgr;
|
||||
xce_limit_t* limit;
|
||||
xce_vres_t* vres = NULL;
|
||||
int rc;
|
||||
xce_limit_config_t limit_config
|
||||
= { .name = "pct_ncpus_limit",
|
||||
.val
|
||||
= &(xce_limit_val_t) { .resname = "ncpus", .percent = 1,
|
||||
.val = 0.5 } };
|
||||
|
||||
groupmgr_init(&umgr, 4, 4);
|
||||
groupmgr_init(&qmgr, 4, 4);
|
||||
groupmgr_init(&hmgr, 4, 4);
|
||||
groupmgr_init(&pmgr, 4, 4);
|
||||
resmgr = make_test_resmgr();
|
||||
TEST_ASSERT_NOT_NULL(resmgr);
|
||||
|
||||
rc = build_limit(&limit, umgr, qmgr, hmgr, pmgr, &limit_config);
|
||||
TEST_ASSERT_TRUE(rc == 1);
|
||||
|
||||
rc = instantiate_limit(&vres, resmgr, limit, "", "", "");
|
||||
TEST_ASSERT_TRUE(rc >= 0);
|
||||
TEST_ASSERT_NOT_NULL(vres);
|
||||
TEST_ASSERT_NULL_MESSAGE(
|
||||
vres->percent_res,
|
||||
"bug-1834's lookup table only knows mem->maxmem and swap->maxswap "
|
||||
"-- a percent-based limit on any other resource must gracefully "
|
||||
"fall back to `res` (today's behavior), not fail instantiation");
|
||||
TEST_ASSERT_NOT_NULL_MESSAGE(
|
||||
vres->res, "the limit's own resname must still resolve normally");
|
||||
|
||||
free_instantiated_vres(resmgr, vres);
|
||||
kill_limit(limit);
|
||||
free_test_resmgr(resmgr);
|
||||
destroy(pmgr);
|
||||
destroy(hmgr);
|
||||
destroy(qmgr);
|
||||
destroy(umgr);
|
||||
|
||||
printf("test_instantiate_limit_percent_limit_without_known_static_"
|
||||
"counterpart_leaves_percent_res_null: PASS\n");
|
||||
}
|
||||
|
||||
void test_limit(void)
|
||||
{
|
||||
printf("\n>> limit test\n\n");
|
||||
|
|
@ -667,6 +902,14 @@ void test_limit(void)
|
|||
test_fmt_limit_units_non_byte_valued_resource_ignores_units_mode);
|
||||
RUN_TEST(test_fmt_limit_units_swp_resource_applies_mb_to_bytes_conversion);
|
||||
RUN_TEST(test_fmt_limit_wrapper_matches_fmt_limit_units_raw_mode);
|
||||
RUN_TEST(
|
||||
test_instantiate_limit_percent_mem_limit_resolves_percent_res_to_maxmem);
|
||||
RUN_TEST(
|
||||
test_instantiate_limit_percent_swap_limit_resolves_percent_res_to_maxswap);
|
||||
RUN_TEST(
|
||||
test_instantiate_limit_absolute_value_mem_limit_leaves_percent_res_null);
|
||||
RUN_TEST(
|
||||
test_instantiate_limit_percent_limit_without_known_static_counterpart_leaves_percent_res_null);
|
||||
|
||||
xstring_table_fini();
|
||||
}
|
||||
|
|
|
|||
|
|
@ -29,11 +29,14 @@ var make_test_resmgr(void)
|
|||
var resmgr = makeInstance(&kvtab_type, ¶ms);
|
||||
if (resmgr == NULL) return NULL;
|
||||
|
||||
add_resource(resmgr, "type", TEST_RES_TYPE_IDX, XCE_VAL_STRING);
|
||||
add_resource(resmgr, "mem", TEST_RES_MEM_IDX, XCE_VAL_NUMERIC);
|
||||
add_resource(resmgr, "gpu", TEST_RES_GPU_IDX, XCE_VAL_BOOLEAN);
|
||||
add_resource(resmgr, "ncpus", TEST_RES_NCPUS_IDX, XCE_VAL_NUMERIC);
|
||||
add_resource(resmgr, "arch", TEST_RES_ARCH_IDX, XCE_VAL_STRING);
|
||||
add_resource(resmgr, "type", TEST_RES_TYPE_IDX, XCE_VAL_STRING);
|
||||
add_resource(resmgr, "mem", TEST_RES_MEM_IDX, XCE_VAL_NUMERIC);
|
||||
add_resource(resmgr, "gpu", TEST_RES_GPU_IDX, XCE_VAL_BOOLEAN);
|
||||
add_resource(resmgr, "ncpus", TEST_RES_NCPUS_IDX, XCE_VAL_NUMERIC);
|
||||
add_resource(resmgr, "arch", TEST_RES_ARCH_IDX, XCE_VAL_STRING);
|
||||
add_resource(resmgr, "maxmem", TEST_RES_MAXMEM_IDX, XCE_VAL_NUMERIC);
|
||||
add_resource(resmgr, "swap", TEST_RES_SWAP_IDX, XCE_VAL_NUMERIC);
|
||||
add_resource(resmgr, "maxswap", TEST_RES_MAXSWAP_IDX, XCE_VAL_NUMERIC);
|
||||
|
||||
return resmgr;
|
||||
}
|
||||
|
|
@ -44,7 +47,8 @@ void free_test_resmgr(var resmgr)
|
|||
|
||||
/* Free each xce_resource_t we allocated (datakiller=NULL means kvtab
|
||||
* won't free them, so we must do it here). */
|
||||
const char* names[] = { "type", "mem", "gpu", "ncpus", "arch", NULL };
|
||||
const char* names[] = { "type", "mem", "gpu", "ncpus",
|
||||
"arch", "maxmem", "swap", "maxswap", NULL };
|
||||
for (int i = 0; names[i] != NULL; i++) {
|
||||
xce_resource_t* r = NULL;
|
||||
int rc = key_find(resmgr, (void*)names[i], (void**)&r);
|
||||
|
|
|
|||
|
|
@ -4,13 +4,16 @@
|
|||
#include <rtti.h>
|
||||
|
||||
/* Test resource indices — must match make_test_resmgr() registration order */
|
||||
#define TEST_RES_TYPE_IDX 0 /* "type" — RES_TYPE_STRING */
|
||||
#define TEST_RES_MEM_IDX 1 /* "mem" — RES_TYPE_NUMERIC */
|
||||
#define TEST_RES_GPU_IDX 2 /* "gpu" — RES_TYPE_BOOL */
|
||||
#define TEST_RES_NCPUS_IDX 3 /* "ncpus" — RES_TYPE_NUMERIC */
|
||||
#define TEST_RES_ARCH_IDX 4 /* "arch" — RES_TYPE_STRING */
|
||||
#define TEST_RES_TYPE_IDX 0 /* "type" — RES_TYPE_STRING */
|
||||
#define TEST_RES_MEM_IDX 1 /* "mem" — RES_TYPE_NUMERIC */
|
||||
#define TEST_RES_GPU_IDX 2 /* "gpu" — RES_TYPE_BOOL */
|
||||
#define TEST_RES_NCPUS_IDX 3 /* "ncpus" — RES_TYPE_NUMERIC */
|
||||
#define TEST_RES_ARCH_IDX 4 /* "arch" — RES_TYPE_STRING */
|
||||
#define TEST_RES_MAXMEM_IDX 5 /* "maxmem" — RES_TYPE_NUMERIC (bug-1834) */
|
||||
#define TEST_RES_SWAP_IDX 6 /* "swap" — RES_TYPE_NUMERIC (bug-1834) */
|
||||
#define TEST_RES_MAXSWAP_IDX 7 /* "maxswap" — RES_TYPE_NUMERIC (bug-1834) */
|
||||
|
||||
/* Create a test resmgr with 5 pre-registered resources.
|
||||
/* Create a test resmgr with 8 pre-registered resources.
|
||||
* Returns a var (kvtab) that can be passed to parse_select / parse_resreq_primary.
|
||||
* Caller must call free_test_resmgr() when done. */
|
||||
extern var make_test_resmgr(void);
|
||||
|
|
|
|||
|
|
@ -472,6 +472,41 @@ int is_host_based_limit(var resmgr, xce_limit_t* limit)
|
|||
return c->per_consumer;
|
||||
}
|
||||
|
||||
/* bug-1834: a percent-based limit's capacity must come from a resource's
|
||||
* static total, not its live/fluctuating value -- "mem"/"swap" are both
|
||||
* XCE_RESF_DYNAMIC load indices (resource-accounting.md), so their own
|
||||
* live value is the wrong base for a stable percentage-of-capacity cap.
|
||||
* "maxmem"/"maxswap" are the static, self-reported-once counterparts
|
||||
* (resources.yaml) that should be read instead. Table-driven so a future
|
||||
* dynamic/static pair (e.g. tmp/maxtmp) is a one-line addition, not a new
|
||||
* branch. */
|
||||
static const struct {
|
||||
const char* resname;
|
||||
const char* percent_resname;
|
||||
} percent_capacity_resource_map[] = {
|
||||
{ "mem", "maxmem" },
|
||||
{ "swap", "maxswap" },
|
||||
};
|
||||
|
||||
static const char* percent_capacity_resname(const char* resname)
|
||||
{
|
||||
size_t i;
|
||||
|
||||
if (resname == NULL) {
|
||||
return NULL;
|
||||
}
|
||||
|
||||
for (i = 0; i < sizeof(percent_capacity_resource_map)
|
||||
/ sizeof(percent_capacity_resource_map[0]);
|
||||
i++) {
|
||||
if (strcmp(resname, percent_capacity_resource_map[i].resname) == 0) {
|
||||
return percent_capacity_resource_map[i].percent_resname;
|
||||
}
|
||||
}
|
||||
|
||||
return NULL;
|
||||
}
|
||||
|
||||
int add_vres(xce_resource_t** pvres, var resmgr, char* name, int global)
|
||||
{
|
||||
xce_resource_t* vres = NULL;
|
||||
|
|
@ -578,6 +613,23 @@ int instantiate_limit(xce_vres_t** pvres, var resmgr, xce_limit_t* limit,
|
|||
return -ENOENT;
|
||||
}
|
||||
|
||||
/* bug-1834: for a percent-based limit, resolve the static
|
||||
* capacity-source resource ONCE here rather than re-deriving it on
|
||||
* every update_limit_res_value() call. A missing/unregistered
|
||||
* counterpart is not a hard failure -- vres->percent_res simply stays
|
||||
* NULL and the caller falls back to `res` (this limit's own resname),
|
||||
* preserving today's behavior for any resource with no known static
|
||||
* pair. */
|
||||
if (limit->val.percent != 0) {
|
||||
const char* percent_resname = percent_capacity_resname(limit->val.resname);
|
||||
|
||||
if (percent_resname != NULL) {
|
||||
lock(resmgr, RTTI_LOCK_READ);
|
||||
key_find(resmgr, (void*)percent_resname, (void**)&(vres->percent_res));
|
||||
unlock(resmgr);
|
||||
}
|
||||
}
|
||||
|
||||
return rc;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -141,8 +141,9 @@ typedef struct {
|
|||
|
||||
static int update_limit_res_value(zarray_t* array, xce_vres_t* vres)
|
||||
{
|
||||
float val = 0.0;
|
||||
int rc = 0;
|
||||
float val = 0.0;
|
||||
int rc = 0;
|
||||
xce_resource_t* capacity_res;
|
||||
|
||||
if ((array == NULL) || (vres == NULL)) {
|
||||
return -EINVAL;
|
||||
|
|
@ -151,7 +152,16 @@ static int update_limit_res_value(zarray_t* array, xce_vres_t* vres)
|
|||
if (vres->limit->val.percent == 0) {
|
||||
val = vres->limit->val.val;
|
||||
} else {
|
||||
rc = get_resvals(array, 1, &(vres->res->idx), &val);
|
||||
/* bug-1834: prefer the resolved static capacity-source resource
|
||||
* (e.g. "maxmem" for a "mem" limit) over `res` (the limit's own,
|
||||
* possibly-dynamic resname) so this limit's capacity is a stable
|
||||
* fraction of total capacity, not a fraction of whatever the
|
||||
* resource's live value happens to be right now. `percent_res` is
|
||||
* NULL (falls back to `res`, today's behavior) whenever
|
||||
* instantiate_limit() found no known static counterpart. */
|
||||
capacity_res = (vres->percent_res != NULL) ? vres->percent_res : vres->res;
|
||||
|
||||
rc = get_resvals(array, 1, &(capacity_res->idx), &val);
|
||||
if (rc < 0) {
|
||||
return rc;
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue