diff --git a/src/include/preason.h b/src/include/preason.h index 042c4ab0..82d36eed 100644 --- a/src/include/preason.h +++ b/src/include/preason.h @@ -12,6 +12,45 @@ extern void erase_job_preason(xce_job_preason_t*); extern int add_job_preason(xce_job_preason_t*, int, char*, char*); extern xce_pendreason_t* make_pendreason(uint64_t); +/* + * De-duplicating pend-reason builder (bug-1927). + * + * add_job_preason() always appends, which is right for callers that build a + * reason list entry by entry (tests, the harness setpend command). A caller + * that re-evaluates the same hosts many times while building one job's + * reasons -- the scheduler's allocator, once per placement plan -- uses + * add_job_preason_once() instead, so each distinct (host, code) host reason + * and each distinct (host, resource, code) resource reason appears once. + * Entries keep the order of their first add. A NULL resource-reason host is + * stored as "" (as add_job_preason() does) and is the same key as "". + * + * xce_job_preason_t is the wire-mapped record (controller.capnp + * JobPendReason), so the lookup index lives beside it, owned by the + * builder: zero-initialize it, pass the same index with the same reasons + * for every add, and release it with erase_preason_index(). The index only + * points at list positions and checks the entry there on every hit, so an + * erase of the lists (a main reason, erase_job_preason()) or plain + * add_job_preason() calls in between leave it correct, never stale: an + * erased entry is simply added again. Expected O(1) per add. + * + * host == NULL && res == NULL sets the main reason exactly like + * add_job_preason() (erasing both lists) and returns its result. + * + * Returns the list's new entry count (> 0) when an entry was appended, 0 + * when an identical entry is already present (or a main reason was set), + * -EINVAL for a NULL reasons or index, or -ENOMEM (nothing appended). + */ +typedef struct { + int nslots; /* power of two; 0 until the first add */ + int nused; + uint32_t* hashes; + int* refs; /* 0 empty, else 1 + (list position << 1 | is_res) */ +} xce_preason_index_t; + +extern int add_job_preason_once(xce_job_preason_t*, xce_preason_index_t*, int, + char*, char*); +extern void erase_preason_index(xce_preason_index_t*); + /* * Pending-reason equality: "are these two pending-reason records identical". * diff --git a/src/lib/preason.c b/src/lib/preason.c index 13088a63..783d496d 100644 --- a/src/lib/preason.c +++ b/src/lib/preason.c @@ -162,6 +162,183 @@ int add_job_preason(xce_job_preason_t* reasons, int code, char* host, char* res) return rc; } +/* ------------------------------------------------------------------------ + * De-duplicating builder (bug-1927). Contract in preason.h. An open- + * addressing table (linear probing, load <= 1/2) of list positions keyed by + * the entry's hash; a hit is confirmed against the list entry itself, so + * the table never has to follow erases of the lists. + * ---------------------------------------------------------------------- */ + +#define PREASON_INDEX_MIN_SLOTS 64 + +static uint32_t preason_fnv1a(uint32_t h, const void* data, size_t n) +{ + const unsigned char* p = (const unsigned char*)data; + size_t i; + + for (i = 0; i < n; i++) { + h ^= p[i]; + h *= 16777619u; + } + + return h; +} + +/* host is never NULL here ("" for a host-less resource reason) */ +static uint32_t preason_key_hash(int is_res, int code, const char* host, + const char* res) +{ + uint32_t h = 2166136261u; + + h = preason_fnv1a(h, &is_res, sizeof(is_res)); + h = preason_fnv1a(h, &code, sizeof(code)); + h = preason_fnv1a(h, host, strlen(host) + 1); + if (is_res) { + h = preason_fnv1a(h, res, strlen(res) + 1); + } + + return h; +} + +/* Does `ref` name a list entry, still present, equal to the key? */ +static int preason_ref_matches(const xce_job_preason_t* r, int ref, int is_res, + int code, const char* host, const char* res) +{ + int pos = (ref - 1) >> 1; + + if (((ref - 1) & 1) != is_res) { + return 0; + } + + if (is_res) { + const xce_res_reason_t* e; + + if (pos >= r->nres_reasons) { + return 0; + } + e = r->res_based_reasons[pos]; + return (e != NULL) && (e->reason == code) && str_eq(e->host, host) + && str_eq(e->resource, res); + } else { + const xce_host_reason_t* e; + + if (pos >= r->nhost_reasons) { + return 0; + } + e = r->host_based_reasons[pos]; + return (e != NULL) && (e->reason == code) && str_eq(e->host, host); + } +} + +/* Doubles the table (first size PREASON_INDEX_MIN_SLOTS), rehashing every + * slot from its stored hash. 0 or -ENOMEM with the table unchanged. */ +static int preason_index_grow(xce_preason_index_t* idx) +{ + uint32_t* hashes; + int* refs; + int n; + int i; + + if (idx->nslots > (1 << 29)) { + return -ENOMEM; + } + + n = (idx->nslots == 0) ? PREASON_INDEX_MIN_SLOTS : idx->nslots * 2; + hashes = (uint32_t*)calloc((size_t)n, sizeof(*hashes)); + refs = (int*)calloc((size_t)n, sizeof(*refs)); + if ((hashes == NULL) || (refs == NULL)) { + FREEUP(hashes); + FREEUP(refs); + return -ENOMEM; + } + + for (i = 0; i < idx->nslots; i++) { + int j; + + if (idx->refs[i] == 0) { + continue; + } + for (j = (int)(idx->hashes[i] & (uint32_t)(n - 1)); refs[j] != 0; + j = (j + 1) & (n - 1)) { + } + hashes[j] = idx->hashes[i]; + refs[j] = idx->refs[i]; + } + + FREEUP(idx->hashes); + FREEUP(idx->refs); + idx->hashes = hashes; + idx->refs = refs; + idx->nslots = n; + + return 0; +} + +int add_job_preason_once(xce_job_preason_t* reasons, xce_preason_index_t* idx, + int code, char* host, char* res) +{ + const char* key_host; + uint32_t hash; + int is_res; + int mask; + int j; + int rc; + + if ((reasons == NULL) || (idx == NULL)) { + return -EINVAL; + } + + if ((host == NULL) && (res == NULL)) { + rc = add_job_preason(reasons, code, NULL, NULL); + return (rc < 0) ? rc : 0; + } + + if ((2 * (idx->nused + 1)) > idx->nslots) { + rc = preason_index_grow(idx); + if (rc < 0) { + return rc; + } + } + + is_res = (res != NULL); + key_host = (host != NULL) ? host : ""; + hash = preason_key_hash(is_res, code, key_host, res); + mask = idx->nslots - 1; + + for (j = (int)(hash & (uint32_t)mask); idx->refs[j] != 0; + j = (j + 1) & mask) { + if ((idx->hashes[j] == hash) + && preason_ref_matches(reasons, idx->refs[j], is_res, code, + key_host, res)) { + return 0; + } + } + + rc = add_job_preason(reasons, code, host, res); + if (rc <= 0) { + return (rc < 0) ? rc : -EINVAL; + } + + /* rc is the list's new count: the entry sits at rc - 1 */ + idx->hashes[j] = hash; + idx->refs[j] = 1 + (((rc - 1) << 1) | is_res); + idx->nused++; + + return rc; +} + +void erase_preason_index(xce_preason_index_t* idx) +{ + if (idx == NULL) { + return; + } + + FREEUP(idx->hashes); + FREEUP(idx->refs); + idx->nslots = 0; + idx->nused = 0; +} + xce_pendreason_t *make_pendreason(uint64_t jobid) { xce_pendreason_t *reason = NULL; xce_job_preason_t *preason = NULL; diff --git a/src/lib/testsuite/test_preason.c b/src/lib/testsuite/test_preason.c index 3ad6a035..ebcd1a04 100644 --- a/src/lib/testsuite/test_preason.c +++ b/src/lib/testsuite/test_preason.c @@ -1,3 +1,4 @@ +#include #include #include #include @@ -518,6 +519,179 @@ static void test_preason_eq_does_not_modify_arguments(void) erase_job_preason(&b); } + +/* ------------------------------------------------------------------------ + * bug-1927: add_job_preason_once() keeps each (host, resource, code) and + * each (host, code) once, in first-add order. + * ---------------------------------------------------------------------- */ + +static void assert_res_entry(const xce_job_preason_t* r, int i, int code, + const char* host, const char* res) +{ + TEST_ASSERT_TRUE(i < r->nres_reasons); + TEST_ASSERT_NOT_NULL(r->res_based_reasons[i]); + TEST_ASSERT_EQUAL_INT(code, r->res_based_reasons[i]->reason); + TEST_ASSERT_EQUAL_STRING(host, r->res_based_reasons[i]->host); + TEST_ASSERT_EQUAL_STRING(res, r->res_based_reasons[i]->resource); +} + +static void test_once_identical_res_reason_kept_once(void) +{ + xce_job_preason_t r; + xce_preason_index_t idx; + + memset(&r, 0, sizeof(r)); + memset(&idx, 0, sizeof(idx)); + + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(1, r.nres_reasons); + TEST_ASSERT_EQUAL_INT(0, r.nhost_reasons); + assert_res_entry(&r, 0, 2, "h1", "slots"); + + erase_preason_index(&idx); + erase_job_preason(&r); +} + +static void test_once_identical_host_reason_kept_once(void) +{ + xce_job_preason_t r; + xce_preason_index_t idx; + + memset(&r, 0, sizeof(r)); + memset(&idx, 0, sizeof(idx)); + + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 3, "h1", NULL)); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 3, "h1", NULL)); + TEST_ASSERT_EQUAL_INT(1, r.nhost_reasons); + TEST_ASSERT_EQUAL_INT(0, r.nres_reasons); + TEST_ASSERT_EQUAL_INT(3, r.host_based_reasons[0]->reason); + TEST_ASSERT_EQUAL_STRING("h1", r.host_based_reasons[0]->host); + + erase_preason_index(&idx); + erase_job_preason(&r); +} + +/* Entries differing in any one of code, host or resource are distinct; a + * host reason and a resource reason on the same host are distinct; a + * host-less (shared) resource reason is kept once too. First-add order. */ +static void test_once_distinct_entries_kept_in_first_add_order(void) +{ + xce_job_preason_t r; + xce_preason_index_t idx; + + memset(&r, 0, sizeof(r)); + memset(&idx, 0, sizeof(idx)); + + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(2, add_job_preason_once(&r, &idx, 2, "h1", "mem")); + TEST_ASSERT_EQUAL_INT(3, add_job_preason_once(&r, &idx, 2, "h2", "slots")); + TEST_ASSERT_EQUAL_INT(4, add_job_preason_once(&r, &idx, 1, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(5, add_job_preason_once(&r, &idx, 2, NULL, "lic")); + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 2, "h1", NULL)); + TEST_ASSERT_EQUAL_INT(2, add_job_preason_once(&r, &idx, 3, "h1", NULL)); + /* every one again, in another order: nothing appended */ + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 3, "h1", NULL)); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, NULL, "lic")); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, "h2", "slots")); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 1, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, "h1", "mem")); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, "h1", NULL)); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + + TEST_ASSERT_EQUAL_INT(5, r.nres_reasons); + assert_res_entry(&r, 0, 2, "h1", "slots"); + assert_res_entry(&r, 1, 2, "h1", "mem"); + assert_res_entry(&r, 2, 2, "h2", "slots"); + assert_res_entry(&r, 3, 1, "h1", "slots"); + assert_res_entry(&r, 4, 2, "", "lic"); /* host-less: stored as "" */ + TEST_ASSERT_EQUAL_INT(2, r.nhost_reasons); + TEST_ASSERT_EQUAL_INT(2, r.host_based_reasons[0]->reason); + TEST_ASSERT_EQUAL_INT(3, r.host_based_reasons[1]->reason); + + erase_preason_index(&idx); + erase_job_preason(&r); +} + +/* A main reason erases the lists; the index must not then claim the + * erased entries are still present. */ +static void test_once_after_main_reason_erase_adds_again(void) +{ + xce_job_preason_t r; + xce_preason_index_t idx; + + memset(&r, 0, sizeof(r)); + memset(&idx, 0, sizeof(idx)); + + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 3, "h2", NULL)); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 1, NULL, NULL)); + TEST_ASSERT_EQUAL_INT(1, r.main_reason); + TEST_ASSERT_EQUAL_INT(0, r.nres_reasons); + TEST_ASSERT_EQUAL_INT(0, r.nhost_reasons); + + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 3, "h2", NULL)); + TEST_ASSERT_EQUAL_INT(1, add_job_preason_once(&r, &idx, 2, "h9", "mem")); + TEST_ASSERT_EQUAL_INT(2, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(0, add_job_preason_once(&r, &idx, 2, "h1", "slots")); + TEST_ASSERT_EQUAL_INT(2, r.nres_reasons); + assert_res_entry(&r, 0, 2, "h9", "mem"); + assert_res_entry(&r, 1, 2, "h1", "slots"); + TEST_ASSERT_EQUAL_INT(1, r.nhost_reasons); + + erase_preason_index(&idx); + erase_job_preason(&r); +} + +/* The bounds-test shape at scale: 64 passes over 2000 hosts keep 2000 + * entries (the index grows past its first size). */ +static void test_once_many_passes_keep_one_entry_per_host(void) +{ + xce_job_preason_t r; + xce_preason_index_t idx; + char host[16]; + int pass; + int i; + + memset(&r, 0, sizeof(r)); + memset(&idx, 0, sizeof(idx)); + + for (pass = 0; pass < 64; pass++) { + for (i = 0; i < 2000; i++) { + snprintf(host, sizeof(host), "bh%04d", i); + TEST_ASSERT_EQUAL_INT((pass == 0) ? (i + 1) : 0, + add_job_preason_once(&r, &idx, 2, host, + "slots")); + } + } + TEST_ASSERT_EQUAL_INT(2000, r.nres_reasons); + for (i = 0; i < 2000; i++) { + snprintf(host, sizeof(host), "bh%04d", i); + assert_res_entry(&r, i, 2, host, "slots"); + } + + erase_preason_index(&idx); + erase_job_preason(&r); +} + +static void test_once_bad_arguments(void) +{ + xce_job_preason_t r; + xce_preason_index_t idx; + + memset(&r, 0, sizeof(r)); + memset(&idx, 0, sizeof(idx)); + + TEST_ASSERT_EQUAL_INT(-EINVAL, add_job_preason_once(NULL, &idx, 2, "h1", + "slots")); + TEST_ASSERT_EQUAL_INT(-EINVAL, add_job_preason_once(&r, NULL, 2, "h1", + "slots")); + TEST_ASSERT_EQUAL_INT(0, r.nres_reasons); + erase_preason_index(NULL); + erase_preason_index(&idx); /* never used: nothing to free */ +} + void test_preason(void) { printf("\n>> preason test\n\n"); @@ -542,4 +716,10 @@ void test_preason(void) RUN_TEST(test_preason_eq_null_list_entries); RUN_TEST(test_preason_eq_null_string_fields); RUN_TEST(test_preason_eq_does_not_modify_arguments); + RUN_TEST(test_once_identical_res_reason_kept_once); + RUN_TEST(test_once_identical_host_reason_kept_once); + RUN_TEST(test_once_distinct_entries_kept_in_first_add_order); + RUN_TEST(test_once_after_main_reason_erase_adds_again); + RUN_TEST(test_once_many_passes_keep_one_entry_per_host); + RUN_TEST(test_once_bad_arguments); } diff --git a/src/scheduler/include/sched/types.h b/src/scheduler/include/sched/types.h index 919c00ae..e4897050 100644 --- a/src/scheduler/include/sched/types.h +++ b/src/scheduler/include/sched/types.h @@ -7,6 +7,7 @@ #include #include "kvmgr.h" +#include "preason.h" #include "resreq.h" #include "typemap.h" #include "xce/types.h" @@ -196,6 +197,11 @@ typedef struct { xce_sched_alloc_req_t alloc_req; xce_job_alloc_t* alloc; xce_pendreason_t* reason; + /* bug-1927: lookup index for reason's lists, so a host re-evaluated by + * every plan of the attempt records each (host, resource, code) once + * (add_job_preason_once(), preason.h). Zeroed by sched_ctx_init(), + * released by sched_ctx_fini(); it never owns reason itself. */ + xce_preason_index_t reason_index; /* Req 9.5: defaults to XCE_ALLOC_NORMAL via sched_ctx_init()'s existing * memset(ctx, 0, sizeof(*ctx)) -- XCE_ALLOC_NORMAL is the enum's zero * value, so no explicit assignment is needed. */ diff --git a/src/scheduler/src/alloc_runner.c b/src/scheduler/src/alloc_runner.c index da013dad..632ebedf 100644 --- a/src/scheduler/src/alloc_runner.c +++ b/src/scheduler/src/alloc_runner.c @@ -62,8 +62,9 @@ static int sched_alloc_ensure_reason_(xce_sched_ctx_t* ctx) * the same COMPOUND/ALTER alternative (Req 2.3). * * Unmet: adds one host-less NO_RESOURCE reason per entry of the unmet list - * the check returns, and returns 0 (Req 2.2). vget() returns 0 on success, - * so only a negative result skips an entry (bug-1893). + * the check returns, unless the attempt already recorded it (bug-1927), + * and returns 0 (Req 2.2). vget() returns 0 on success, so only a negative + * result skips an entry (bug-1893). * * Any failure returns its negative error code (Req 2.4). */ static int sched_alloc_prepare_shared_(xce_sched_core_t* core, @@ -96,8 +97,10 @@ static int sched_alloc_prepare_shared_(xce_sched_core_t* core, continue; } - add_job_preason(ctx->reason->reasons, PREASON_NO_RESOURCE, NULL, - r->name); + /* bug-1927: a COMPOUND's later primary short on the same + * pool does not repeat the entry */ + add_job_preason_once(ctx->reason->reasons, &(ctx->reason_index), + PREASON_NO_RESOURCE, NULL, r->name); } return 0; diff --git a/src/scheduler/src/resource.c b/src/scheduler/src/resource.c index 20bcf817..8b3519c8 100644 --- a/src/scheduler/src/resource.c +++ b/src/scheduler/src/resource.c @@ -556,8 +556,9 @@ int sched_try_alloc_on_host_(xce_sched_worker_t* w, xce_sched_core_t* core, return -errno; } - add_job_preason(ctx->reason->reasons, PREASON_MATCH_FAIL, w->name, - NULL); + /* bug-1927: once per (host, code), not once per plan */ + add_job_preason_once(ctx->reason->reasons, &(ctx->reason_index), + PREASON_MATCH_FAIL, w->name, NULL); return 0; } @@ -598,8 +599,11 @@ int sched_try_alloc_on_host_(xce_sched_worker_t* w, xce_sched_core_t* core, continue; } - add_job_preason(ctx->reason->reasons, PREASON_NO_RESOURCE, - w->name, r->name); + /* bug-1927: every plan re-checks this host; record each + * (host, resource, code) once */ + add_job_preason_once(ctx->reason->reasons, + &(ctx->reason_index), + PREASON_NO_RESOURCE, w->name, r->name); } return 0; } diff --git a/src/scheduler/src/schedobj.c b/src/scheduler/src/schedobj.c index 681ffa1a..4ae0c94e 100644 --- a/src/scheduler/src/schedobj.c +++ b/src/scheduler/src/schedobj.c @@ -542,6 +542,7 @@ void sched_ctx_fini(xce_sched_ctx_t* ctx) xvec_idval_fini(&(ctx->alloc_req.shared)); xvec_idval_fini(&(ctx->alloc_req.host)); xvec_idval_fini(&(ctx->unmeet)); + erase_preason_index(&(ctx->reason_index)); } void sched_ctx_set_primary(xce_sched_ctx_t* ctx, xce_primary_resreq_t* primary) diff --git a/src/scheduler/testsuite/test_alloc_equivalence_parjob.c b/src/scheduler/testsuite/test_alloc_equivalence_parjob.c index 834041d0..f8a64014 100644 --- a/src/scheduler/testsuite/test_alloc_equivalence_parjob.c +++ b/src/scheduler/testsuite/test_alloc_equivalence_parjob.c @@ -586,16 +586,19 @@ static void eq_assert_reasons(eq_fixture_t* f, int n, const char** pairs) } } -/* 1, 3, 4: host reasons (Req 3.6/3.7): exactly one per short resource of - * every selection-matching unclaimed short host of each bucket search. */ +/* 1, 3, 4: host reasons (Req 3.6/3.7): every bucket search records a + * reason for each short resource of every selection-matching unclaimed + * short host, and the set holds each (host, resource) once, in first-check + * order (bug-1927): a later plan re-checking a host adds nothing new. + * 3: plan [1,1] re-finds A and B short on mem; 4: [2,2] and [2,1,1] + * re-find B and C short on slots. */ static void test_parjob_host_reasons_every_short_host(void) { static const char* two[] = { HOST_A, "slots", HOST_B, "slots" }; - static const char* hosts[] - = { HOST_A, "mem", HOST_A, "slots", HOST_B, "mem", - HOST_B, "slots", HOST_A, "mem", HOST_B, "mem" }; - static const char* back[] = { HOST_A, "slots", HOST_B, "slots", HOST_C, - "slots", HOST_B, "slots", HOST_C, "slots" }; + static const char* hosts[] = { HOST_A, "mem", HOST_A, "slots", + HOST_B, "mem", HOST_B, "slots" }; + static const char* back[] + = { HOST_A, "slots", HOST_B, "slots", HOST_C, "slots" }; eq_fixture_t f; eq_primary_t p; xce_sched_worker_t* w[3]; @@ -605,11 +608,11 @@ static void test_parjob_host_reasons_every_short_host(void) eq_finish(&f, &p); TEST_ASSERT_EQUAL_INT(0, eq_run(&f, &p, &EQ_SHORT_HOSTS, w)); - eq_assert_reasons(&f, 6, hosts); + eq_assert_reasons(&f, 4, hosts); eq_finish(&f, &p); TEST_ASSERT_EQUAL_INT(1, eq_run(&f, &p, &EQ_ROLLBACK, w)); - eq_assert_reasons(&f, 5, back); + eq_assert_reasons(&f, 3, back); eq_finish(&f, &p); } diff --git a/src/scheduler/testsuite/test_alloc_runner_search.c b/src/scheduler/testsuite/test_alloc_runner_search.c index 911da29a..e2270327 100644 --- a/src/scheduler/testsuite/test_alloc_runner_search.c +++ b/src/scheduler/testsuite/test_alloc_runner_search.c @@ -602,6 +602,39 @@ static void test_every_short_host_gets_reason(void) sr_fixture_fini(&f); } +/* bug-1927: two plans that each check the same three short hosts record + * each (host, slots, NO_RESOURCE) reason once, in first-check order. Every + * plan re-evaluates the hosts, which used to append the identical entry + * once per plan (6 entries here, plans x hosts in the bounds test). */ +static void test_short_host_reason_recorded_once_across_plans(void) +{ + sr_fixture_t f; + xce_sched_worker_t *a, *b, *c; + + sr_fixture_init(&f); + a = sr_add_host(&f, "sr-a", 1.0f); + b = sr_add_host(&f, "sr-b", 1.0f); + c = sr_add_host(&f, "sr-c", 1.0f); + sr_ctx_begin(&f, 3); + script_plan(1, 3, 0, 0); + script_plan(1, 2, 0, 0); + + TEST_ASSERT_EQUAL_INT(0, sr_run(&f)); + TEST_ASSERT_EQUAL_INT(6, g_nlog); /* both plans checked every host */ + sr_assert_log(0, a, 3); + sr_assert_log(1, b, 3); + sr_assert_log(3, a, 2); + sr_assert_log(5, c, 2); + TEST_ASSERT_NOT_NULL(f.ctx.reason); + TEST_ASSERT_EQUAL_INT(0, f.ctx.reason->reasons->main_reason); + TEST_ASSERT_EQUAL_INT(0, f.ctx.reason->reasons->nhost_reasons); + TEST_ASSERT_EQUAL_INT(3, f.ctx.reason->reasons->nres_reasons); + sr_assert_slots_reason(&f, 0, "sr-a"); + sr_assert_slots_reason(&f, 1, "sr-b"); + sr_assert_slots_reason(&f, 2, "sr-c"); + sr_fixture_fini(&f); +} + /* ---------------------------------------------------------------------- */ /* Malformed plans and the claims buffer (Req 3.8, 5.7) */ /* ---------------------------------------------------------------------- */ @@ -761,6 +794,7 @@ int main(void) RUN_TEST(test_failing_bucket_releases_earlier_claims); RUN_TEST(test_rollback_then_later_plan_with_bucket_counts); RUN_TEST(test_every_short_host_gets_reason); + RUN_TEST(test_short_host_reason_recorded_once_across_plans); RUN_TEST(test_malformed_plans_skipped); RUN_TEST(test_claims_buffer_grows_to_larger_plan); RUN_TEST(test_claims_buffer_growth_failure); diff --git a/src/scheduler/testsuite/test_trace_bounds.c b/src/scheduler/testsuite/test_trace_bounds.c index 15fb6122..e9dc3460 100644 --- a/src/scheduler/testsuite/test_trace_bounds.c +++ b/src/scheduler/testsuite/test_trace_bounds.c @@ -15,6 +15,9 @@ * count every other line as omitted (stored + omitted == the unbounded * record's lines) and keep every decision-wide counter identical to the * unbounded run. Its rendering shows the truncation marker and the totals. + * The pend reason set holds one (host, slots, NO_RESOURCE) entry per worker + * although every plan re-evaluated every host (bug-1927: it held one per + * host per plan, ~93k entries). * * Store bound (9.5): 10,000 traced decisions put into the REPL trace * store: first the real bounded record from the large trace, then records @@ -213,6 +216,50 @@ static void start_sched(void) g_sched->core.trace_ctl = &g_ctl; } +static int cmp_res_reason(const void* pa, const void* pb) +{ + const xce_res_reason_t* a = *(const xce_res_reason_t* const*)pa; + const xce_res_reason_t* b = *(const xce_res_reason_t* const*)pb; + int c = strcmp(a->host, b->host); + + if (c == 0) { + c = strcmp(a->resource, b->resource); + } + return (c != 0) ? c : (a->reason - b->reason); +} + +/* bug-1927: the pend reason set holds each (host, resource, code) once: + * here one (host, slots, NO_RESOURCE) per worker, although each of the 64 + * plans re-evaluated every host. Sorted copy, adjacent entries compared. */ +static void assert_reasons_distinct(const xce_pendreason_t* reason) +{ + const xce_job_preason_t* r; + xce_res_reason_t** sorted; + int dups = 0; + int i; + + TEST_ASSERT_NOT_NULL(reason); + r = reason->reasons; + TEST_ASSERT_NOT_NULL(r); + TEST_ASSERT_EQUAL_INT(0, r->nhost_reasons); + sorted = (xce_res_reason_t**)calloc((size_t)r->nres_reasons + 1, + sizeof(*sorted)); + TEST_ASSERT_NOT_NULL(sorted); + memcpy(sorted, r->res_based_reasons, + (size_t)r->nres_reasons * sizeof(*sorted)); + qsort(sorted, (size_t)r->nres_reasons, sizeof(*sorted), cmp_res_reason); + for (i = 1; i < r->nres_reasons; i++) { + dups += (cmp_res_reason(&sorted[i - 1], &sorted[i]) == 0); + } + free(sorted); + if (getenv("XCE_TRACE_BOUNDS_DUMP") != NULL) { + fprintf(stderr, "pend reasons: %d resource entries, %d duplicates\n", + r->nres_reasons, dups); + } + TEST_ASSERT_EQUAL_INT_MESSAGE(0, dups, "duplicate pend reason entries"); + TEST_ASSERT_EQUAL_INT(NWORKERS, r->nres_reasons); +} + /* Feeds the large job to the allocator thread with line bound `bound` and * returns the record its decision carries (a pend). Frees the decision. */ static xce_sched_trace_rec_t* feed_large(int bound) @@ -235,6 +282,7 @@ static xce_sched_trace_rec_t* feed_large(int bound) TEST_ASSERT_NOT_NULL_MESSAGE(decision, "the allocator pushed nothing"); TEST_ASSERT_EQUAL_INT_MESSAGE(XCE_DECISION_PEND, decision->op, "the large job must pend"); + assert_reasons_distinct(decision->u.reason); rec = decision->trace; decision->trace = NULL; free(decision); @@ -344,6 +392,11 @@ static void test_large_parallel_job_trace_stays_within_default_bound(void) "discarded 0\n")); TEST_ASSERT_NOT_NULL(strstr(text, "\n totals: host evaluations: ")); TEST_ASSERT_NULL(strstr(full_text, "trace lines omitted")); + /* bug-1927: one reason per host in the pend outcome */ + TEST_ASSERT_NOT_NULL( + strstr(text, "\n host bh0000: resource slots: NO_RESOURCE\n")); + TEST_ASSERT_NOT_NULL(strstr( + text, "\n omitted hosts' pend reasons: NO_RESOURCE x950\n")); TEST_ASSERT_TRUE(count_lines(text) < 2 * XCE_TRACE_LINE_BOUND_DEFAULT); TEST_ASSERT_TRUE(count_lines(full_text) >= total_lines); free(text);