From 40917a01f0c751961839d6fc0142cab9dbee10b8 Mon Sep 17 00:00:00 2001 From: Rongsong Shen Date: Thu, 1 Oct 2026 12:40:09 +0000 Subject: [PATCH] feat(job-array-one-based-index): control by filter with element selections (task 7.5) Filter-based ControlJob validates a bracketed -J value before fetching (400 with the parser's message; unbracketed names, including commands with spaces, unchanged). FetchJobList evaluates bracketed job_name entries through the selection predicates instead of unevaluated compound conditions, so -J 'foo[1]' acts on element 1 of every array named foo and never on ordinary jobs of that name. The most-recent-job rule applies only without an element number and without all-jobs, and treats the element of a one-element array like an ordinary job (isSingleElementArray, a documented approximation). The old index-section parser is deleted. Completes bug-1898 on the owned paths. Co-Authored-By: Claude Opus 5.5 (1M context) --- .../specs/job-array-one-based-index/tasks.md | 2 +- .../apiserver/controllers/jobmatch.go | 26 +++ .../apiserver/controllers/jobmatch_test.go | 39 ++++ src/frontend/apiserver/controllers/jobs.go | 169 ++++-------------- .../controllers/jobs_handler_test.go | 149 +++++++++++++++ 5 files changed, 251 insertions(+), 134 deletions(-) diff --git a/.kiro/specs/job-array-one-based-index/tasks.md b/.kiro/specs/job-array-one-based-index/tasks.md index 8c659913..4c7cb264 100644 --- a/.kiro/specs/job-array-one-based-index/tasks.md +++ b/.kiro/specs/job-array-one-based-index/tasks.md @@ -171,7 +171,7 @@ - _Requirements: 5.1, 5.3, 5.4, 8.2, 8.3_ - _Depends: 5.4, 7.2_ -- [ ] 7.5 Control jobs by filter with element selections +- [x] 7.5 Control jobs by filter with element selections - Validate a bracketed name filter before fetching (unbracketed names, including command-line names with spaces, unchanged), returning 400 with the specific message - Route the filter-based job fetch's bracketed name handling through the name parser and the selection predicates; with its last use gone, delete the old index-section parser - Apply the most-recent-job rule only when the filter has no element number and all-jobs is not set, acting on the newest row alone when it is ordinary or the only stored element of its array diff --git a/src/frontend/apiserver/controllers/jobmatch.go b/src/frontend/apiserver/controllers/jobmatch.go index 402223f8..78261c76 100644 --- a/src/frontend/apiserver/controllers/jobmatch.go +++ b/src/frontend/apiserver/controllers/jobmatch.go @@ -281,3 +281,29 @@ func rowUint64(row utils.OrderMap, col string) (uint64, bool) { } return 0, false } + +// isSingleElementArray reports whether row is an array element and the only +// stored element of its array: it counts the rows in store.Jobs with row's +// job_id whose unique_job_id carries the array marking (design ApiControl, +// 5.10). The store records no array size and the controller pushes a +// one-element array as a single job, so the stored row count is the only +// signal; a multi-element array whose other elements have all been purged +// from the store is deliberately treated as one-element. +func isSingleElementArray(row utils.OrderMap) bool { + uid, ok := rowUint64(row, "unique_job_id") + if !ok || !backend.IsArrayJob(uid) { + return false + } + jobID, ok := rowUint64(row, "job_id") + if !ok { + return false + } + count := 0 + store.Jobs.Range(func(_ uint64, j models.JobInfo) bool { + if uint64(j.JobID) == jobID && backend.IsArrayJob(uint64(j.Unique_JobID)) { + count++ + } + return count <= 1 + }) + return count == 1 +} diff --git a/src/frontend/apiserver/controllers/jobmatch_test.go b/src/frontend/apiserver/controllers/jobmatch_test.go index 71b569a0..4352877e 100644 --- a/src/frontend/apiserver/controllers/jobmatch_test.go +++ b/src/frontend/apiserver/controllers/jobmatch_test.go @@ -457,3 +457,42 @@ func TestBuildSelectionPredicates_Errors(t *testing.T) { }) } } + +// TestIsSingleElementArray (design ApiControl, 5.10): a row is the only +// stored element of its array when no other stored row with its job_id is an +// array element; ordinary jobs never are. +func TestIsSingleElementArray(t *testing.T) { + orig := store.Jobs + t.Cleanup(func() { store.Jobs = orig }) + store.Jobs = store.NewDataStore[uint64, models.JobInfo]() + + const one = uint64(2)<<40 | uint64(3)<<15 // one-element array + const multi = uint64(2)<<40 | uint64(4)<<15 // two elements + const ord = uint64(1)<<40 | 7 // ordinary job + put := func(uid uint64) utils.OrderMap { + j := models.JobInfo{ + Unique_JobID: int64(uid), + JobID: int64(backend.ExtractArrayBaseID(uid)), + ArrayIdx: backend.ExtractArrayIdx(uid), + JobName: "foo", + } + store.Jobs.Put(uid, j) + return store.StructToOrderMap(j) + } + oneRow := put(backend.MakeElementID(one, 1)) + multiRow := put(backend.MakeElementID(multi, 1)) + put(backend.MakeElementID(multi, 2)) + ordRow := put(ord) + // an ordinary job sharing the one-element array's number does not count + put(one) + + if !isSingleElementArray(oneRow) { + t.Errorf("one-element array element: got false, want true") + } + if isSingleElementArray(multiRow) { + t.Errorf("element of a two-element array: got true, want false") + } + if isSingleElementArray(ordRow) { + t.Errorf("ordinary job: got true, want false") + } +} diff --git a/src/frontend/apiserver/controllers/jobs.go b/src/frontend/apiserver/controllers/jobs.go index c6fcbb13..c1e6f8b4 100644 --- a/src/frontend/apiserver/controllers/jobs.go +++ b/src/frontend/apiserver/controllers/jobs.go @@ -2433,6 +2433,23 @@ func (j JobController) ControlJob(ctx *gin.Context) { filters = getFilters(req, submitter) logger.Debug("final request filters: ", zap.Any("filters", filters)) + // A job name with '[' or ']' is an element selection: validate it before + // fetching (validate-all-first), so a rejection is a 400 with its own + // message. Names without brackets are matched as written, as before + // (unnamed jobs carry their command, with spaces, as their name). + hasElementNumber := strings.ContainsAny(req.Filter.JobName, "[]") + if hasElementNumber { + for _, filter := range filters { + if filter.Field != "job_name" { + continue + } + if _, _, err := BuildSelectionPredicates([]utils.FilterCondition{filter}); err != nil { + returnSelectionError(ctx, filter.Value, err) + return + } + } + } + //1. fetch jobs from db with req.Filter jobList, err := FetchJobList(filters) if err != nil { @@ -2452,9 +2469,14 @@ func (j JobController) ControlJob(ctx *gin.Context) { } var jobInfoList []utils.OrderMap - if !req.Filter.AllJobs { - // will query job order by submittime, will get the last submitted job. - if len(jobList) > 0 && jobList[len(jobList)-1].Data["array_idx"] == 0 { + if !req.Filter.AllJobs && !hasElementNumber { + // "most recent job" rule: rows are sorted by unique_job_id, so the + // last one is the most recent; act on it alone when it is an + // ordinary job (or a pre-upgrade element 0) or the only stored + // element of its array (5.10). A filter with an element number acts + // on exactly the selected elements (5.2). + last := jobList[len(jobList)-1] + if last.Data["array_idx"] == 0 || isSingleElementArray(last) { jobInfoList = jobList[len(jobList)-1:] } else { jobInfoList = jobList @@ -3012,35 +3034,9 @@ func copyRemoteFilesLocally(ctx *gin.Context, files []string, username string) m return result } -// parse [1-3] to start = 1, end=3 -func parseArrayIndexSection(idxsec string) (int, int, error) { - parts := strings.Split(idxsec, "-") - if len(parts) != 2 { - start, err := strconv.Atoi(strings.TrimSpace(parts[0])) - if err != nil { - return 0, 0, err - } - return start, start, nil - } - - start, err := strconv.Atoi(strings.TrimSpace(parts[0])) - if err != nil { - return 0, 0, err - } - - end, err := strconv.Atoi(strings.TrimSpace(parts[1])) - if err != nil { - return 0, 0, err - } - if start >= end { - return 0, 0, fmt.Errorf("start index should be bigger than end index") - } else { - return start, end, nil - } -} - // fetch job list from job_info table according filter condition -// need special handling for job_name=arrayjob[1-3] and exec_hosts cases. +// need special handling for job_name=arrayjob[1-3] (element selections, +// evaluated as row predicates) and exec_hosts cases. func FetchJobList(filters []utils.FilterCondition) ([]utils.OrderMap, error) { logger.Debug("Enter fetchJobList() function", @@ -3053,7 +3049,13 @@ func FetchJobList(filters []utils.FilterCondition) ([]utils.OrderMap, error) { sortOrders := make([]utils.SortOrder, 0) jobLists := make([]utils.OrderMap, 0) - compfilters := make(utils.CompFilterCondition) + // Element selections (job_name values with '[' or ']') become row + // predicates; every other filter stays a plain condition. Parsed before + // the exec_hosts store lookups below (validate-all-first). + filters, preds, err := BuildSelectionPredicates(filters) + if err != nil { + return nil, err + } for i, filter := range filters { if filter.Field == "exec_hosts" { @@ -3088,102 +3090,9 @@ func FetchJobList(filters []utils.FilterCondition) ([]utils.OrderMap, error) { filters[i].Operator = "in" filters[i].Value = "(" + strings.Join(uniqueJobIdStrs, ",") } - - // special handle job_name[index] cases, here index can be [1-3], [1,3,5],[1-3,5-7] situations - if filters[i].Field == "job_name" { - filtCdArrays := make([]utils.FilterConditionArray, 0) - jobNames := extractJobNames(filters[i].Value) - for i := range jobNames { - jobNames[i] = strings.TrimSpace(jobNames[i]) - } - logger.Debug("After extractJobNames jobNames is", zap.Any("jobNames", jobNames)) - - if !(strings.Contains(filters[i].Value, "[") && strings.Contains(filters[i].Value, "]")) { - continue - } - - for _, jobname := range jobNames { - subORFilters := make([]utils.FilterCondition, 0) - jobnameArry, err := parseArrayName(jobname) - logger.Debug("After parseArrayName jobname is ", zap.Any("jobname", jobnameArry)) - - if err != nil { - return nil, errors.New("job name format is invalid") - } - - if err = isValidArrayName(jobnameArry.KeyID); err != nil { - err := errors.New("bad jobname: " + jobnameArry.KeyID) - return nil, err - } - - // in job control filter, operator is "eq" - var operator = "eq" - // treat every section as an OR condition - mainFiltCond := utils.FilterCondition{ - Field: filters[i].Field, - Operator: operator, - Value: jobnameArry.KeyID, - } - if jobnameArry.Indices != nil { - - // array jobid: job_id:in:(101[1-3, 5-6]) - // handle every section in Indices list. - for _, jidxsec := range jobnameArry.Indices { - st, ed, err := parseArrayIndexSection(jidxsec.Index) - if err != nil { - err := errors.New("bad job array index: " + jidxsec.Index) - return nil, err - } - if jidxsec.Step == 1 { - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "between", - Value: "(" + strconv.Itoa(st) + "," + strconv.Itoa(ed) + ")", - }) - } else { // handle step > 1 - idxList := make([]string, 0) - for idx := st; idx <= ed; idx += jidxsec.Step { - idxList = append(idxList, strconv.Itoa(idx)) - } - subORFilters = append(subORFilters, utils.FilterCondition{ - Field: "array_idx", - Operator: "in", - Value: "(" + strings.Join(idxList, ",") + ")", - }) - } - } - } - filtCdArrays = append(filtCdArrays, utils.FilterConditionArray{ - MainFiltCond: mainFiltCond, - SubFiltConds: subORFilters, - }) - } - compfilters["job_name"] = filtCdArrays - } - } - - // remove filter with jobarray[] from filters, for filter in compfilters need special handling. - if len(compfilters) > 0 { - var filtersNew []utils.FilterCondition - for _, filter := range filters { - found := false - for k, _ := range compfilters { - if filter.Field == k { - found = true - break - } - } - if found { - continue - } - filtersNew = append(filtersNew, filter) - } - filters = filtersNew } var filtercds []utils.Condition - var compfiltercd utils.CompCondition - var err error if len(filters) > 0 { filtercds, err = utils.ConvertToNewFilter(filters, &datatype) if err != nil { @@ -3191,13 +3100,6 @@ func FetchJobList(filters []utils.FilterCondition) ([]utils.OrderMap, error) { return nil, err } } - if len(compfilters) > 0 { - compfiltercd, err = utils.ConvertToNewFilterArray(compfilters, &datatype) - if err != nil { - logger.Error(err.Error()) - return nil, err - } - } sortOrders = append(sortOrders, utils.SortOrder{ Column: "unique_job_id", @@ -3206,10 +3108,11 @@ func FetchJobList(filters []utils.FilterCondition) ([]utils.OrderMap, error) { pagetokenStr := "" for { - queryOpts, err := utils.BuildQueryOptions(filtercds, compfiltercd, sortOrders, limit, pagetokenStr) + queryOpts, err := utils.BuildQueryOptions(filtercds, nil, sortOrders, limit, pagetokenStr) if err != nil { return nil, err } + queryOpts.Predicates = preds queryReply, pagetoken, err := services.QueryData(tablename, queryOpts, fields) if err != nil { return nil, err diff --git a/src/frontend/apiserver/controllers/jobs_handler_test.go b/src/frontend/apiserver/controllers/jobs_handler_test.go index f1c83406..5969fa42 100644 --- a/src/frontend/apiserver/controllers/jobs_handler_test.go +++ b/src/frontend/apiserver/controllers/jobs_handler_test.go @@ -1234,3 +1234,152 @@ func TestJobHandler_ControlValidThenInvalid(t *testing.T) { } } } + +// ========== Control by filter with element selections (task 7.5) ========== + +// controlFilterKill posts a kill through the filter-based ControlJob with +// the job-name filter name (the -J value as the client sends it). +func (f *handlerFixture) controlFilterKill(name string) *httptest.ResponseRecorder { + f.t.Helper() + return f.controlByFilter(models.JobControlRequest{ + Filter: models.JobControlFilter{JobName: name}, + Action: "kill", + }) +} + +// TestJobHandler_ControlFilterRejectsBadSelection (5.7, 5.16, 5.5): a +// bracketed -J value the name parser rejects returns 400 with its specific +// message before any job is fetched, and dispatches nothing. +func TestJobHandler_ControlFilterRejectsBadSelection(t *testing.T) { + f := newHandlerFixture(t) + X := handlerTestArrayBase + 40<<15 + elems := f.seedArrayElements(X, "foo", "RUN", 1, 2) + assertSeeded(t, uint64(elems[0].Unique_JobID)) + + cases := []struct{ name, want string }{ + {"foo[40000]", "element numbers must be at most"}, + {"foo[3-1]", "reversed"}, + {"foo[a]", "invalid"}, + {"foo [1]", "invalid"}, + {"foo[1", "invalid"}, + } + for _, c := range cases { + f.dispatched = nil + assert400(t, "-J "+c.name, f.controlFilterKill(c.name), c.want) + if got := f.dispatchedIDs(); len(got) != 0 { + t.Errorf("-J %s: dispatched %v, want none", c.name, got) + } + } +} + +// TestJobHandler_ControlFilterEmptyNameSelectsNothing (design ApiMatch): +// -J '[1]' (control sends eq) matches no row, not the unnamed array. +func TestJobHandler_ControlFilterEmptyNameSelectsNothing(t *testing.T) { + f := newHandlerFixture(t) + Z := handlerTestArrayBase + 41<<15 + elems := f.seedArrayElements(Z, "", "RUN", 1) + assertSeeded(t, uint64(elems[0].Unique_JobID)) + + w := f.controlFilterKill("[1]") + if w.Code != http.StatusOK { + t.Fatalf("status = %d, want 200; body: %s", w.Code, w.Body.String()) + } + if got := f.dispatchedIDs(); len(got) != 0 { + t.Errorf("-J [1] dispatched %v, want none", got) + } +} + +// TestJobHandler_ControlFilterMostRecentOneElementArray (5.10): with no +// element number in the filter, when the newest matching job is the element +// of a one-element array, only that element is acted on. +func TestJobHandler_ControlFilterMostRecentOneElementArray(t *testing.T) { + f := newHandlerFixture(t) + O := handlerTestOrdinaryBase + 50 + f.seedOrdinaryJob(O, "foo", "RUN") + A := handlerTestArrayBase + 42<<15 + a1 := backend.MakeElementID(A, 1) + f.seedArrayElements(A, "foo", "RUN", 1) + assertSeeded(t, O, a1) + + r := decodeControlReply(t, f.controlFilterKill("foo")) + assertExactIDs(t, "-J foo dispatched", f.dispatchedIDs(), []uint64{a1}) + assertExactIDs(t, "-J foo accepted", itemIDs(t, r.Accepted), []uint64{a1}) + + // all-jobs: the rule does not apply, every matching job is acted on + f.dispatched = nil + w := f.controlByFilter(models.JobControlRequest{ + Filter: models.JobControlFilter{JobName: "foo", AllJobs: true}, + Action: "kill", + }) + decodeControlReply(t, w) + assertExactIDs(t, "-J foo -a dispatched", f.dispatchedIDs(), []uint64{O, a1}) +} + +// TestJobHandler_ControlFilterElementSelection (5.2): -J 'foo[1]' acts on +// exactly element 1 of every array named foo -- both one-element arrays and +// element 1 of a larger one -- never on an ordinary job named foo, another +// element, or another name, whatever the newest row is. +func TestJobHandler_ControlFilterElementSelection(t *testing.T) { + f := newHandlerFixture(t) + O := handlerTestOrdinaryBase + 51 + f.seedOrdinaryJob(O, "foo", "RUN") + A := handlerTestArrayBase + 43<<15 + B := handlerTestArrayBase + 44<<15 + C := handlerTestArrayBase + 45<<15 + D := handlerTestArrayBase + 46<<15 + f.seedArrayElements(A, "foo", "RUN", 1) + f.seedArrayElements(B, "foo", "RUN", 1) + f.seedArrayElements(C, "foo", "RUN", 1, 2) + f.seedArrayElements(D, "bar", "RUN", 1) + a1, b1, c1 := backend.MakeElementID(A, 1), backend.MakeElementID(B, 1), backend.MakeElementID(C, 1) + assertSeeded(t, O, a1, b1, c1, backend.MakeElementID(C, 2), backend.MakeElementID(D, 1)) + + r := decodeControlReply(t, f.controlFilterKill("foo[1]")) + assertExactIDs(t, "-J foo[1] dispatched", f.dispatchedIDs(), []uint64{a1, b1, c1}) + assertExactIDs(t, "-J foo[1] accepted", itemIDs(t, r.Accepted), []uint64{a1, b1, c1}) + + // the range form 1-1 selects the same element 1 of every array named foo + f.dispatched = nil + r = decodeControlReply(t, f.controlFilterKill("foo[1-1]")) + assertExactIDs(t, "-J foo[1-1] dispatched", f.dispatchedIDs(), []uint64{a1, b1, c1}) +} + +// TestJobHandler_ControlFilterElementSelectionNewestSingleElement (5.2, +// 5.10): when the most recent job named foo is the element of a one-element +// array, -J 'foo[1]' still acts on element 1 of every array named foo (the +// most-recent rule applies only to filters without an element number), +// while -J 'foo' acts on that most recent element alone. +func TestJobHandler_ControlFilterElementSelectionNewestSingleElement(t *testing.T) { + f := newHandlerFixture(t) + O := handlerTestOrdinaryBase + 53 + f.seedOrdinaryJob(O, "foo", "RUN") + A := handlerTestArrayBase + 47<<15 + B := handlerTestArrayBase + 48<<15 // highest ID: the newest row + f.seedArrayElements(A, "foo", "RUN", 1) + f.seedArrayElements(B, "foo", "RUN", 1) + a1, b1 := backend.MakeElementID(A, 1), backend.MakeElementID(B, 1) + assertSeeded(t, O, a1, b1) + + r := decodeControlReply(t, f.controlFilterKill("foo[1]")) + assertExactIDs(t, "-J foo[1] dispatched", f.dispatchedIDs(), []uint64{a1, b1}) + assertExactIDs(t, "-J foo[1] accepted", itemIDs(t, r.Accepted), []uint64{a1, b1}) + + f.dispatched = nil + r = decodeControlReply(t, f.controlFilterKill("foo")) + assertExactIDs(t, "-J foo dispatched", f.dispatchedIDs(), []uint64{b1}) + assertExactIDs(t, "-J foo accepted", itemIDs(t, r.Accepted), []uint64{b1}) +} + +// TestJobHandler_ControlFilterPlainNameWithSpaces: an unnamed job carries +// its command as its name; kill -J 'sleep 100' keeps working (no brackets, +// no selection parsing). +func TestJobHandler_ControlFilterPlainNameWithSpaces(t *testing.T) { + f := newHandlerFixture(t) + O := handlerTestOrdinaryBase + 52 + f.seedOrdinaryJob(O, "sleep 100", "RUN") + f.seedOrdinaryJob(O+1, "sleep 200", "RUN") + + r := decodeControlReply(t, f.controlFilterKill("sleep 100")) + assertExactIDs(t, "-J 'sleep 100' dispatched", f.dispatchedIDs(), []uint64{O}) + assertExactIDs(t, "-J 'sleep 100' accepted", itemIDs(t, r.Accepted), []uint64{O}) +}