diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index 0364517f..a27db1e0 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -205,7 +205,7 @@ - _Depends: 8.2_ - [ ] 9. Integration and cleanup -- [ ] 9.1 Restrict ordinary ID allocation to single jobs +- [x] 9.1 Restrict ordinary ID allocation to single jobs - Remove the multi-element path from ordinary ID allocation now that the harness and the API server use the array allocator - Migrate the existing ID-allocation test loop that requests sizes above 1 to the array allocator - Done when the old multi-element test loop is gone or migrated, ordinary allocation with size 2 returns invalid-argument in the libxce suite, and the harness and API server builds and suites still pass diff --git a/src/include/idgen.h b/src/include/idgen.h index d10d8efd..ee0ceaf4 100644 --- a/src/include/idgen.h +++ b/src/include/idgen.h @@ -17,9 +17,16 @@ typedef struct { } idgen_ctx_t; extern int idgen_ctx_init(idgen_ctx_t*); -extern int idgen(idgen_ctx_t*, size_t, size_t, size_t, uint64_t*); extern void idgen_ctx_fini(idgen_ctx_t*); +/* Ordinary (non-array) job ID. arraySize must be 1; array jobs are + * allocated with idgen_array(). On success *pidx is an untagged ID and 1 is + * returned. -EINVAL for bad args or arraySize != 1; -EOVERFLOW if the + * sequence field is exhausted for the current second (the context is left + * unchanged). */ +extern int idgen(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, + size_t arraySize, uint64_t* pidx); + /* First element ID of a new n-element array, numbered from 1. * 1 <= n <= MAX_ARRAY_IDX (32767). On success *first = MAKE_JOBID(c, a, ts, * TAG_JOB_ARRAY, (block << 15) | 1), the context is advanced past the whole diff --git a/src/lib/idgen.c b/src/lib/idgen.c index 06724ff4..f1112433 100644 --- a/src/lib/idgen.c +++ b/src/lib/idgen.c @@ -36,34 +36,31 @@ static time_t idgen_tick_locked(idgen_ctx_t* ctx) int idgen(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, size_t arraySize, uint64_t* pidx) { - uint64_t idx; - uint64_t ts, tag, seq; + uint64_t ts, seq; + /* ordinary IDs only: arrays are allocated by idgen_array() */ if ((ctx == NULL) || (clusterId > MAX_CLUSTER_ID) - || (apiserverId > MAX_APISERVER_ID) || (arraySize < 1) - || (arraySize > MAX_ARRAY_IDX + 1) || (pidx == NULL)) { + || (apiserverId > MAX_APISERVER_ID) || (arraySize != 1) + || (pidx == NULL)) { return -EINVAL; } mylock_lock(&(ctx->lock)); - ts = idgen_tick_locked(ctx); + ts = idgen_tick_locked(ctx); + seq = ctx->sequence; - if (arraySize == 1) { - /* non array job */ - tag = 0x0; - seq = ctx->sequence; - ctx->sequence++; - } else { - tag = TAG_JOB_ARRAY; - seq = ROUNDUP(ctx->sequence, N_ARRAY_IDX_BITS); - ctx->sequence = ROUNDUP(seq + 1, N_ARRAY_IDX_BITS); + if (seq > UMAX_BITS(N_SEQUENCE_BITS)) { + /* the sequence field is exhausted for this second (e.g. after + * idgen_array took the last block); the context is unchanged */ + mylock_unlock(&(ctx->lock)); + return -EOVERFLOW; } + + ctx->sequence++; mylock_unlock(&(ctx->lock)); - idx = MAKE_JOBID(clusterId, apiserverId, ts, tag, seq); - - *pidx = idx; - return arraySize; + *pidx = MAKE_JOBID(clusterId, apiserverId, ts, 0x0, seq); + return 1; } int idgen_array(idgen_ctx_t* ctx, size_t clusterId, size_t apiserverId, diff --git a/src/lib/testsuite/test_idgen.c b/src/lib/testsuite/test_idgen.c index 3498215f..d47f9b3b 100644 --- a/src/lib/testsuite/test_idgen.c +++ b/src/lib/testsuite/test_idgen.c @@ -50,6 +50,8 @@ static void test_idgen1(void) idgen_ctx_fini(&ctx); } +/* Multi-element allocation goes through idgen_array (numbered from 1); this + * replaces the old idgen(..., size > 1, ...) loop that asserted index 0. */ static void test_idgen_array(void) { idgen_ctx_t ctx; @@ -65,13 +67,13 @@ static void test_idgen_array(void) for (k = 0; k < 5; k++) { int j; - rc = idgen(&ctx, 0x0, 0x0, i + 1, &idx); - TEST_ASSERT_TRUE(rc >= 0); - if (i >= 1) { - TEST_ASSERT_TRUE((idx & MAX_ARRAY_IDX) == 0); - } + rc = idgen_array(&ctx, 0x0, 0x0, i + 1, &idx); + TEST_ASSERT_EQUAL_INT(i + 1, rc); + TEST_ASSERT_EQUAL_UINT64(TAG_JOB_ARRAY, EXTRACT_ID_TAG(idx)); + TEST_ASSERT_EQUAL_UINT64(1, EXTRACT_ARRAY_IDX(idx)); + TEST_ASSERT_EQUAL_UINT64(i + 1, EXTRACT_ARRAY_IDX(idx + i)); - for (j = 0; j < i; j++) { + for (j = 0; j <= i; j++) { char buf[256]; memset(buf, 0x0, sizeof(buf)); @@ -87,6 +89,33 @@ static void test_idgen_array(void) idgen_ctx_fini(&ctx); } +/* Ordinary allocation is single-job only: any size other than 1 is + * rejected and leaves the context unchanged; size 1 returns 1 and an + * untagged ID (job-array-one-based-index 1.3, 4.2). */ +static void test_idgen_single_only(void) +{ + idgen_ctx_t ctx; + uint64_t idx = 0xdeadbeefULL; + size_t bad[] = { 0, 2, 3, 32767, 32768, SIZE_MAX }; + size_t i; + + idgen_ctx_init(&ctx); + ctx.sequence = 7; + + for (i = 0; i < sizeof(bad) / sizeof(bad[0]); i++) { + TEST_ASSERT_EQUAL_INT(-EINVAL, idgen(&ctx, 0, 0, bad[i], &idx)); + TEST_ASSERT_EQUAL_UINT64(0xdeadbeefULL, idx); + TEST_ASSERT_EQUAL_UINT32(7, ctx.sequence); + } + + TEST_ASSERT_EQUAL_INT(1, idgen(&ctx, 5, 3, 1, &idx)); + TEST_ASSERT_EQUAL_UINT64(0, EXTRACT_ID_TAG(idx)); + TEST_ASSERT_EQUAL_UINT64(5, EXTRACT_ID_CLUSTER(idx)); + TEST_ASSERT_EQUAL_UINT64(3, EXTRACT_ID_APISERVER(idx)); + + idgen_ctx_fini(&ctx); +} + #define ARRAY_BLOCK ((uint32_t)1 << N_ARRAY_IDX_BITS) static void test_idgen_array_invalid(void) @@ -238,6 +267,55 @@ static void test_idgen_array_overflow(void) idgen_ctx_fini(&ctx); } +/* Plain idgen() must not let the sequence spill into the timestamp field: + * once the 20-bit sequence is exhausted (e.g. after idgen_array took block + * 31) it returns -EOVERFLOW and leaves the context unchanged. */ +static void test_idgen_overflow(void) +{ + idgen_ctx_t ctx; + uint64_t idx; + uint32_t full = (uint32_t)1 << N_SEQUENCE_BITS; + + idgen_ctx_init(&ctx); + + for (;;) { + time_t t0 = time(NULL); + int rc_last, rc_over, rc_after_array; + uint64_t last = 0, over = 0xdeadbeefULL, first = 0; + + /* the last sequence value is still usable */ + ctx.timestamp = t0; + ctx.sequence = full - 1; + rc_last = idgen(&ctx, 5, 3, 1, &last); + + /* the next one is not; the context stays as it was */ + rc_over = idgen(&ctx, 5, 3, 1, &over); + + /* idgen_array taking block 31 exhausts the sequence too */ + ctx.sequence = 31 * ARRAY_BLOCK; + rc_after_array = idgen_array(&ctx, 5, 3, 2, &first); + if (rc_after_array == 2) { + rc_after_array = idgen(&ctx, 5, 3, 1, &idx); + } + + if (time(NULL) != t0) { + continue; /* the second changed: the sequence was reset */ + } + + TEST_ASSERT_EQUAL_INT(1, rc_last); + TEST_ASSERT_EQUAL_UINT64(full - 1, EXTRACT_ID_SEQUENCE(last)); + TEST_ASSERT_EQUAL_UINT64((uint64_t)t0, EXTRACT_ID_TIMESTAMP(last)); + TEST_ASSERT_EQUAL_INT(-EOVERFLOW, rc_over); + TEST_ASSERT_EQUAL_UINT64(0xdeadbeefULL, over); + TEST_ASSERT_EQUAL_INT(-EOVERFLOW, rc_after_array); + TEST_ASSERT_EQUAL_UINT32(full, ctx.sequence); + TEST_ASSERT_EQUAL_INT64((int64_t)t0, (int64_t)ctx.timestamp); + break; + } + + idgen_ctx_fini(&ctx); +} + void test_idgen(void) { printf("\n>> idgen test\n\n"); @@ -245,6 +323,8 @@ void test_idgen(void) RUN_TEST(test_idgen_invalid); RUN_TEST(test_idgen1); RUN_TEST(test_idgen_array); + RUN_TEST(test_idgen_single_only); + RUN_TEST(test_idgen_overflow); RUN_TEST(test_idgen_array_invalid); RUN_TEST(test_idgen_array_one_based); RUN_TEST(test_idgen_array_overflow);