diff --git a/src/frontend/apiserver/controllers/jobs.go b/src/frontend/apiserver/controllers/jobs.go index c1e6f8b4..5709166b 100644 --- a/src/frontend/apiserver/controllers/jobs.go +++ b/src/frontend/apiserver/controllers/jobs.go @@ -860,68 +860,36 @@ func (j JobController) RestartJobByID(ctx *gin.Context) { return } - compfilters := make(utils.CompFilterCondition) - filtCdArrays := make([]utils.FilterConditionArray, 0) - subORFilters := make([]utils.FilterCondition, 0) - + // bug-1898: restart exactly the named job. The old job_id CompCondition + // was never evaluated, so the first stored job was copied for any ID. jobid := strings.TrimSpace(jobIDStr) - filters := make([]utils.FilterCondition, 0) - sortOrders := make([]utils.SortOrder, 0) - - filters = append(filters, utils.FilterCondition{ - Field: "job_id", - Operator: "eq", - Value: jobid, - }) - mainFiltCond := utils.FilterCondition{ - Field: filters[0].Field, - Operator: filters[0].Operator, - Value: jobid, + sel, selErr := ParseIdentitySelection(jobid) + if selErr != nil { + returnSelectionError(ctx, jobid, selErr) + return } fields := utils.GetSupportedFields(models.JobInfo{}) - - filtCdArrays = append(filtCdArrays, utils.FilterConditionArray{ - MainFiltCond: mainFiltCond, - SubFiltConds: subORFilters, - }) - - compfilters["job_id"] = filtCdArrays - - var job interface{} - - job = &models.JobInfo{} - - compfiltercd, err := utils.ConvertToNewFilterArray(compfilters, job) - - var limit int = utils.PER_PAGE_LIMIT_MAX - var bookmark string = "" - queryOpts, err := utils.BuildQueryOptions(nil, compfiltercd, sortOrders, limit, bookmark) - if err != nil { - logger.Error(err.Error()) - ReturnError(ctx, common.ERROR_CODE, common.ERROR_QUERY_JOBINFO_MSG+": "+err.Error(), nil) + sortOrders := []utils.SortOrder{{Column: "unique_job_id", Ascending: true}} + rows, ok := queryControlRowsByJobID(ctx, sel, fields, sortOrders) + if !ok { return } - logger.Debug("the job query request:", zap.Any("jquery request arrayjob:", compfiltercd), zap.Any("sortOrders", sortOrders), zap.Any("limit", limit), - zap.String("pagetoken", bookmark), zap.Any("fields", fields)) - - var jqueryReply []utils.OrderMap - - jqueryReply, page_token, err := services.QueryData(services.TableJobInfo, queryOpts, fields) + matches, err := foundJob(int64(sel.Key), sel.Lo, sel.Form != SelBare, rows) if err != nil { - logger.Error("Failed to query job info.", zap.Error(err)) - ReturnError(ctx, common.ERROR_CODE, common.ERROR_QUERY_JOBINFO_MSG+": "+err.Error(), nil) - return - } - // Here, page_token is escaped and will not be recognized by dbapi, - bookmark, _ = url.QueryUnescape(page_token) - if len(jqueryReply) == 0 { logger.Error("No job found for the given job ID", zap.String("jobID", jobIDStr)) ReturnError(ctx, common.ERROR_NOT_FOUND_CODE, common.ERROR_JOB_NOT_FOUND_MSG, nil) return } + if len(matches) > 1 { + err := fmt.Errorf("job <%s> is an array of %d elements; restart one element as [k]", + jobid, len(matches)) + logger.Error(err.Error()) + ReturnError(ctx, common.INVALID_PARAMS_CODE, common.INVALID_PARAMS_MSG+": "+err.Error(), nil) + return + } - oldJob := jqueryReply[0].Data // Get the first job info + oldJob := matches[0].Data logger.Debug("Old job info:", zap.Any("oldJob", oldJob)) if v, ok := oldJob["submit_user"].(string); ok { if v != restarter { @@ -933,7 +901,7 @@ func (j JobController) RestartJobByID(ctx *gin.Context) { } } - jobID, err := backend.GenerateJobID(clusterID, apiserverID, 1) + jobID, err := generateJobID(clusterID, apiserverID, 1) if err != nil { logger.Error("Failed to generate job ID for restart", zap.Error(err)) ReturnError(ctx, common.ERROR_CODE, "failed to generate job ID: "+err.Error(), nil) @@ -1094,7 +1062,7 @@ func (j JobController) RestartJobByID(ctx *gin.Context) { } logger.Info("Generated job ID for restart", zap.Int64("jobID", newJob.JobID)) - err = services.SubmitJob(&newJob, 1) + err = submitJob(&newJob, 1) if err != nil { logger.Error("Failed to submit job for restart process.", zap.Error(err)) ReturnError(ctx, common.ERROR_CODE, common.ERROR_WRITE_DATA_MSG+": "+err.Error(), nil) diff --git a/src/frontend/apiserver/controllers/jobs_handler_test.go b/src/frontend/apiserver/controllers/jobs_handler_test.go index 14b6f529..fdbd0787 100644 --- a/src/frontend/apiserver/controllers/jobs_handler_test.go +++ b/src/frontend/apiserver/controllers/jobs_handler_test.go @@ -204,6 +204,7 @@ func newHandlerFixture(t *testing.T) *handlerFixture { g.GET("/:jobids", jc.GetJobByIDs) g.POST("/create", jc.CreateJob) g.POST("/:jobids/control", jc.ControlJobByID) + g.POST("/:jobids/restart", jc.RestartJobByID) g.POST("/control", jc.ControlJob) f.router = r return f @@ -1429,3 +1430,80 @@ func TestJobHandler_QueryPagination(t *testing.T) { } assertIDs(t, "all pages", sortedIDs(got...), s.all()) } + +// ========== Restart by identity (bug-1898) ========== + +// seedRestartJob seeds a DONE job with a distinctive command. +func (f *handlerFixture) seedRestartJob(unique uint64, command string) { + f.t.Helper() + f.seedJob(models.JobInfo{ + Unique_JobID: int64(unique), + JobID: int64(backend.ExtractArrayBaseID(unique)), + ArrayIdx: backend.ExtractArrayIdx(unique), + JobName: "r", + Command: command, + JobStatus: models.JOBSTAT["DONE"], + }) +} + +func (f *handlerFixture) restart(jobids string) *httptest.ResponseRecorder { + f.t.Helper() + return f.do(http.MethodPost, "/xce/v1/jobs/"+url.PathEscape(jobids)+"/restart", map[string]interface{}{}) +} + +// bug-1898: RestartJobByID copied jqueryReply[0] of an unevaluated job_id +// CompCondition -- any job, not the one named. It must restart exactly the +// named job (ordinary job or [k] element), report an unknown one as +// not found, and refuse a bare array identity (several jobs). +func TestJobHandler_RestartNamedJob(t *testing.T) { + f := newHandlerFixture(t) + arr := handlerTestArrayBase + 30<<15 + ord := handlerTestOrdinaryBase + 31<<15 + // decoys first, so "the first row" is never the target by accident + f.seedRestartJob(handlerTestOrdinaryBase+32<<15, "decoy-a") + f.seedRestartJob(backend.MakeElementID(arr, 1), "elem-1") + f.seedRestartJob(backend.MakeElementID(arr, 2), "elem-2") + f.seedRestartJob(ord, "ordinary") + + cases := []struct { + name, jobids, wantCommand string + }{ + {"ordinary job by decimal id", strconv.FormatUint(ord, 10), "ordinary"}, + {"array element by [k]", strconv.FormatUint(arr, 10) + "[2]", "elem-2"}, + } + for _, c := range cases { + before := len(f.submitted) + w := f.restart(c.jobids) + if w.Code != http.StatusOK && w.Code != http.StatusCreated { + t.Errorf("%s: status %d; body %s", c.name, w.Code, w.Body.String()) + continue + } + if len(f.submitted) != before+1 { + t.Errorf("%s: %d submits, want 1", c.name, len(f.submitted)-before) + continue + } + if got := f.submitted[before].Job.Command; got != c.wantCommand { + t.Errorf("%s: restarted a copy of %q, want %q", c.name, got, c.wantCommand) + } + } + + // unknown job and missing element: not found, nothing submitted + for _, jobids := range []string{ + strconv.FormatUint(handlerTestOrdinaryBase+40<<15, 10), + strconv.FormatUint(arr, 10) + "[9]", + } { + before := len(f.submitted) + w := f.restart(jobids) + if w.Code == http.StatusOK || w.Code == http.StatusCreated || len(f.submitted) != before { + t.Errorf("%s: status %d, %d submits; want not found and none", jobids, w.Code, len(f.submitted)-before) + } + } + + // a bare array identity names several jobs: refused, nothing submitted + before := len(f.submitted) + w := f.restart(strconv.FormatUint(arr, 10)) + if w.Code != http.StatusBadRequest || len(f.submitted) != before { + t.Errorf("bare array identity: status %d, %d submits; want 400 and none; body %s", + w.Code, len(f.submitted)-before, w.Body.String()) + } +}