fix(apiserver): restart copies the named job, not the first stored one (bug-1898)
RestartJobByID looked the job up with a job_id CompCondition, which the store never evaluates, and restarted jqueryReply[0] -- the first stored job -- for any ID, including unknown ones. It now parses the ID with ParseIdentitySelection, fetches the rows by key and matches them with foundJob, as ControlJobByID does: an ordinary job or <array-id>[k] is restarted, an unknown job or element is not found, and a bare array identity (several elements) is refused. Restart goes through the generateJobID/submitJob seams so the handler fixture can test it. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a1526164b5
commit
c73708b81f
GPG key ID:
80533536B0EB651C
2 changed files with 98 additions and 52 deletions
|
|
@ -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 <array-id>[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)
|
||||
|
|
|
|||
|
|
@ -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 <array-id>[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 <array-id>[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())
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue