fix(frontend): job control no longer returns HTTP 500 (bug-1490)
Two stacked defects in the apiserver job-control link, both of which kept hold/release/cancel from ever reaching the controller: 1. controllers/jobs.go: validateJobControl and foundJob asserted jobinfo.Data["array_idx"].(int64), but StoreToOrderMaps produces array_idx as int (models.JobInfo.ArrayIdx is declared int) -> panic -> Gin Recovery -> HTTP 500 before backend.ControlJob ran. Add normalizeJobInt64 / jobArrayIdx helpers and use them in both functions (job_status already had this fix via normalizeJobStatus; array_idx was missed). 2. backend/clib.go: backend.ControlJob set xce_jobctrl_t.action to the models.JOB_* constant (JOB_STOP=1 / JOB_RESUME=2 / JOB_KILL=3 / JOB_CHKPNT=8), but that field is the capnp JobControl union discriminant (JobControl_u_siginfo=0 / JobControl_u_ckptoption=1). The controller's job_ctrl() selected the wrong union branch (or none, since encode_JobControl drops the union pointer for non-0/1 discriminants) and rejected every control with -EINVAL. Add jobCtrlDiscriminant() and use it. Also: resolveExecUser was defined in the cgo-only convert.go but is exercised by a non-cgo test, so `make test` (CGO_ENABLED=0) failed to build the backend package. Move it to convert_pure.go so it compiles in both modes. Tests: 3 controller regression tests (validateJobControl allowed / not-allowed branches, foundJob array-child branch) + 1 backend jobCtrlDiscriminant test. make test fully green. Cluster E2E verified: hold -> PSUSP, release -> PEND, cancel -> EXIT on a PEND job, no 500, no -EINVAL, all daemons healthy. Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
parent
d3a92f1033
commit
c90e63ec0f
7 changed files with 368 additions and 29 deletions
|
|
@ -394,7 +394,12 @@ func SubmitJob(job *models.JobInfo, arraySize int) error {
|
|||
func ControlJob(jobid uint64, action int, content string) error {
|
||||
var ctrl C.xce_jobctrl_t
|
||||
ctrl.jobid = C.uint64_t(jobid)
|
||||
ctrl.action = C.int(action)
|
||||
// ctrl.action is the capnp JobControl union DISCRIMINANT (0=siginfo,
|
||||
// 1=ckptoption), NOT the models.JOB_* constant. bug-1490: setting it to
|
||||
// JOB_STOP/RESUME/KILL/CHKPNT (1/2/3/8) made the controller's job_ctrl()
|
||||
// select the wrong union branch (or none) and reject every control with
|
||||
// -EINVAL, so hold/release/cancel never reached the job.
|
||||
ctrl.action = C.int(jobCtrlDiscriminant(action))
|
||||
|
||||
// For signal-based actions set signo; for checkpoint set ckpt options.
|
||||
// xce_jobctrl_t.u is a C union — we use a static C helper to set the
|
||||
|
|
|
|||
|
|
@ -249,19 +249,6 @@ func cClusterConfToClusterInfo(conf *C.cluster_config_t) *models.ClusterInfo {
|
|||
|
||||
// modelJobToSubmit converts a Go JobInfo to C xce_job_submit_t heap structs.
|
||||
// The caller must free all returned C memory by calling freeJobSubmit.
|
||||
// resolveExecUser returns the user a job should run as: an explicit exec_user
|
||||
// wins; otherwise the job runs as the submitting user. bug-1500: the submit
|
||||
// conversion previously dropped this entirely, so the worker received a NULL
|
||||
// execuser and ran every job as "nobody". Kept as a plain-Go function so it is
|
||||
// unit-testable without cgo (the Go toolchain rejects `import "C"` in _test.go).
|
||||
func resolveExecUser(job *models.JobInfo) string {
|
||||
execUser := job.ExecUser
|
||||
if execUser == "" {
|
||||
execUser = job.SubmitUser
|
||||
}
|
||||
return execUser
|
||||
}
|
||||
|
||||
func modelJobToSubmit(job *models.JobInfo) (*C.xce_job_submit_t, *C.job_submit_data_t, *C.job_startinfo_t) {
|
||||
si := (*C.job_startinfo_t)(C.calloc(1, C.sizeof_job_startinfo_t))
|
||||
si.cmdline = C.CString(job.Command)
|
||||
|
|
|
|||
|
|
@ -7,6 +7,36 @@ import (
|
|||
"fmt"
|
||||
)
|
||||
|
||||
// resolveExecUser returns the user a job should run as: an explicit exec_user
|
||||
// wins; otherwise the job runs as the submitting user. bug-1500: the submit
|
||||
// conversion previously dropped this entirely, so the worker received a NULL
|
||||
// execuser and ran every job as "nobody". Plain Go so it is unit-testable
|
||||
// without cgo — keep it OUT of the cgo-only convert.go.
|
||||
func resolveExecUser(job *models.JobInfo) string {
|
||||
execUser := job.ExecUser
|
||||
if execUser == "" {
|
||||
execUser = job.SubmitUser
|
||||
}
|
||||
return execUser
|
||||
}
|
||||
|
||||
// jobCtrlDiscriminant maps a models.JOB_* action to the capnp JobControl union
|
||||
// discriminant carried in xce_jobctrl_t.action: JobControl_u_siginfo (0) for
|
||||
// signal-based actions (kill/stop/resume), JobControl_u_ckptoption (1) for
|
||||
// checkpoint. The models.JOB_* constants are a DIFFERENT enum — using them as
|
||||
// the discriminant made the controller's job_ctrl() select the wrong union
|
||||
// branch (or none) and reject every control request with -EINVAL (bug-1490).
|
||||
// Returns -1 for an unknown action (clib.go never calls ControlJob with one).
|
||||
func jobCtrlDiscriminant(action int) int {
|
||||
switch action {
|
||||
case models.JOB_STOP, models.JOB_RESUME, models.JOB_KILL:
|
||||
return 0 // JobControl_u_siginfo
|
||||
case models.JOB_CHKPNT:
|
||||
return 1 // JobControl_u_ckptoption
|
||||
}
|
||||
return -1
|
||||
}
|
||||
|
||||
// JobID bit layout constants — mirrors src/include/config.h.
|
||||
//
|
||||
// |63 62|61 56|55 52|51 20|19 15|14 0|
|
||||
|
|
|
|||
|
|
@ -5,6 +5,33 @@ import (
|
|||
"testing"
|
||||
)
|
||||
|
||||
// TestJobCtrlDiscriminant is a regression test for bug-1490: backend.ControlJob
|
||||
// used to set xce_jobctrl_t.action to the models.JOB_* constant
|
||||
// (JOB_STOP=1/JOB_RESUME=2/JOB_KILL=3/JOB_CHKPNT=8), but that field is the
|
||||
// capnp JobControl union DISCRIMINANT (JobControl_u_siginfo=0,
|
||||
// JobControl_u_ckptoption=1). The controller's job_ctrl() interpreted the wrong
|
||||
// union branch (or none) and rejected every control request with -EINVAL, so
|
||||
// hold/release/cancel never reached the job.
|
||||
func TestJobCtrlDiscriminant(t *testing.T) {
|
||||
cases := []struct {
|
||||
name string
|
||||
action int
|
||||
want int
|
||||
}{
|
||||
{"stop is a signal action", models.JOB_STOP, 0}, // JobControl_u_siginfo
|
||||
{"resume is a signal action", models.JOB_RESUME, 0},
|
||||
{"kill is a signal action", models.JOB_KILL, 0},
|
||||
{"checkpoint is a ckptoption action", models.JOB_CHKPNT, 1}, // JobControl_u_ckptoption
|
||||
}
|
||||
for _, tc := range cases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
if got := jobCtrlDiscriminant(tc.action); got != tc.want {
|
||||
t.Errorf("jobCtrlDiscriminant(%d) = %d, want %d", tc.action, got, tc.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestRawJobToModel_BasicFields(t *testing.T) {
|
||||
r := rawJobData{
|
||||
jobID: 123,
|
||||
|
|
|
|||
|
|
@ -3025,14 +3025,14 @@ func validateJobControl(jobinfo utils.OrderMap, action string,
|
|||
failedJobs = append(failedJobs, map[string]interface{}{
|
||||
"unique_job_id": jobinfo.Data["unique_job_id"].(int64),
|
||||
"job_id": jobinfo.Data["job_id"].(int64),
|
||||
"array_idx": int(jobinfo.Data["array_idx"].(int64)),
|
||||
"array_idx": jobArrayIdx(jobinfo),
|
||||
"msg": fmt.Sprintf("job cannot be <killed> or <signaled>"),
|
||||
})
|
||||
} else {
|
||||
successJobs = append(successJobs, map[string]interface{}{
|
||||
"unique_job_id": jobinfo.Data["unique_job_id"].(int64),
|
||||
"job_id": jobinfo.Data["job_id"].(int64),
|
||||
"array_idx": int(jobinfo.Data["array_idx"].(int64)),
|
||||
"array_idx": jobArrayIdx(jobinfo),
|
||||
"msg": fmt.Sprintf("job is being <%s>", action),
|
||||
})
|
||||
}
|
||||
|
|
@ -3041,7 +3041,7 @@ func validateJobControl(jobinfo utils.OrderMap, action string,
|
|||
successJobs = append(successJobs, map[string]interface{}{
|
||||
"unique_job_id": jobinfo.Data["unique_job_id"].(int64),
|
||||
"job_id": jobinfo.Data["job_id"].(int64),
|
||||
"array_idx": int(jobinfo.Data["array_idx"].(int64)),
|
||||
"array_idx": jobArrayIdx(jobinfo),
|
||||
"msg": fmt.Sprintf("job is being <%s>", action),
|
||||
})
|
||||
} else {
|
||||
|
|
@ -3049,7 +3049,7 @@ func validateJobControl(jobinfo utils.OrderMap, action string,
|
|||
failedJobs = append(failedJobs, map[string]interface{}{
|
||||
"unique_job_id": jobinfo.Data["unique_job_id"].(int64),
|
||||
"job_id": jobinfo.Data["job_id"].(int64),
|
||||
"array_idx": int(jobinfo.Data["array_idx"].(int64)),
|
||||
"array_idx": jobArrayIdx(jobinfo),
|
||||
"msg": fmt.Sprintf("job cannot be <%s>", action),
|
||||
})
|
||||
}
|
||||
|
|
@ -3058,7 +3058,7 @@ func validateJobControl(jobinfo utils.OrderMap, action string,
|
|||
failedJobs = append(failedJobs, map[string]interface{}{
|
||||
"unique_job_id": jobinfo.Data["unique_job_id"].(int64),
|
||||
"job_id": jobinfo.Data["job_id"].(int64),
|
||||
"array_idx": int(jobinfo.Data["array_idx"].(int64)),
|
||||
"array_idx": jobArrayIdx(jobinfo),
|
||||
"msg": fmt.Sprintf("Invalid job_status"),
|
||||
})
|
||||
}
|
||||
|
|
@ -3067,7 +3067,7 @@ func validateJobControl(jobinfo utils.OrderMap, action string,
|
|||
failedJobs = append(failedJobs, map[string]interface{}{
|
||||
"unique_job_id": jobinfo.Data["unique_job_id"].(int64),
|
||||
"job_id": jobinfo.Data["job_id"].(int64),
|
||||
"array_idx": int(jobinfo.Data["array_idx"].(int64)),
|
||||
"array_idx": jobArrayIdx(jobinfo),
|
||||
"msg": fmt.Sprintf("job_status not found for job"),
|
||||
})
|
||||
}
|
||||
|
|
@ -3083,7 +3083,7 @@ func foundJob(jobid int64, array_idx int, jobinfos []utils.OrderMap) ([]utils.Or
|
|||
for _, jobinfo := range jobinfos {
|
||||
if array_idx > 0 {
|
||||
if jobinfo.Data["job_id"].(int64) == jobid &&
|
||||
int(jobinfo.Data["array_idx"].(int64)) == array_idx {
|
||||
jobArrayIdx(jobinfo) == array_idx {
|
||||
jobs = append(jobs, jobinfo)
|
||||
}
|
||||
} else {
|
||||
|
|
@ -3099,26 +3099,48 @@ func foundJob(jobid int64, array_idx int, jobinfos []utils.OrderMap) ([]utils.Or
|
|||
}
|
||||
}
|
||||
|
||||
// normalizeJobStatus converts a job_status value (int, int64, or float64 from
|
||||
// JSON decode) into a plain int. Returns ok=false for unsupported types.
|
||||
func normalizeJobStatus(v any) (int, bool) {
|
||||
// normalizeJobInt64 converts a job field value (int, int32, int64, float64, or
|
||||
// json.Number — whichever representation the store or a JSON decode path
|
||||
// produced) into a plain int64. Returns ok=false for unsupported types.
|
||||
func normalizeJobInt64(v any) (int64, bool) {
|
||||
switch s := v.(type) {
|
||||
case int:
|
||||
return s, true
|
||||
return int64(s), true
|
||||
case int32:
|
||||
return int(s), true
|
||||
return int64(s), true
|
||||
case int64:
|
||||
return int(s), true
|
||||
return s, true
|
||||
case float64:
|
||||
return int(s), true
|
||||
return int64(s), true
|
||||
case json.Number:
|
||||
if n, err := s.Int64(); err == nil {
|
||||
return int(n), true
|
||||
return n, true
|
||||
}
|
||||
}
|
||||
return 0, false
|
||||
}
|
||||
|
||||
// normalizeJobStatus converts a job_status value (int, int64, or float64 from
|
||||
// JSON decode) into a plain int. Returns ok=false for unsupported types.
|
||||
func normalizeJobStatus(v any) (int, bool) {
|
||||
n, ok := normalizeJobInt64(v)
|
||||
return int(n), ok
|
||||
}
|
||||
|
||||
// jobArrayIdx extracts the array_idx field from a job row's Data map, tolerating
|
||||
// any numeric representation the store produces. Defaults to 0 when absent.
|
||||
// Regression for bug-1490: the old code asserted .(int64) on a value the store
|
||||
// produces as int, panicking every control request into an HTTP 500 before it
|
||||
// reached the controller.
|
||||
func jobArrayIdx(row utils.OrderMap) int {
|
||||
if v, ok := row.Data["array_idx"]; ok {
|
||||
if n, ok := normalizeJobInt64(v); ok {
|
||||
return int(n)
|
||||
}
|
||||
}
|
||||
return 0
|
||||
}
|
||||
|
||||
func processJobData(rows []utils.OrderMap, pagetoken string) utils.QueryResult {
|
||||
logger.Debug("Enter processJobData() function", zap.Any("rows", rows))
|
||||
for _, row := range rows {
|
||||
|
|
|
|||
|
|
@ -74,3 +74,75 @@ func TestPreCheckJobRequest_EmptyQueue(t *testing.T) {
|
|||
t.Errorf("empty queue should not trigger reserved-name error: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
// jobRowFromStore builds an OrderMap exactly as store.StoreToOrderMaps
|
||||
// produces it for a JobInfo: job_id/unique_job_id are int64, but array_idx and
|
||||
// job_status are int (models.JobInfo.ArrayIdx/JobStatus are declared int).
|
||||
func jobRowFromStore(jobID int64, arrayIdx int, status int) utils.OrderMap {
|
||||
return utils.OrderMap{
|
||||
Data: map[string]interface{}{
|
||||
"unique_job_id": jobID,
|
||||
"job_id": jobID,
|
||||
"array_idx": arrayIdx,
|
||||
"submit_user": "tester",
|
||||
"queue_name": "normal",
|
||||
"job_status": status,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateJobControl_ArrayIdxInt_Allowed is a regression test for bug-1490:
|
||||
// job control (hold/release/cancel) returned HTTP 500 because validateJobControl
|
||||
// asserted jobinfo.Data["array_idx"].(int64), but the store produces array_idx
|
||||
// as int. That panic was caught by Gin's Recovery middleware → HTTP 500, before
|
||||
// the control request ever reached the controller. An allowed action must now
|
||||
// classify the job as a success without panicking.
|
||||
func TestValidateJobControl_ArrayIdxInt_Allowed(t *testing.T) {
|
||||
resetJobSubmitState()
|
||||
|
||||
jobInfo := jobRowFromStore(2001, 0, models.JOBSTAT["RUN"])
|
||||
|
||||
success, failed := validateJobControl(jobInfo, "stop", "TSTP")
|
||||
if len(success) != 1 {
|
||||
t.Fatalf("expected 1 success job, got %d (failed=%v)", len(success), failed)
|
||||
}
|
||||
if got := success[0]["array_idx"].(int); got != 0 {
|
||||
t.Errorf("array_idx = %d, want 0", got)
|
||||
}
|
||||
if got := success[0]["unique_job_id"].(int64); got != 2001 {
|
||||
t.Errorf("unique_job_id = %d, want 2001", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestValidateJobControl_ArrayIdxInt_NotAllowed covers the "action not allowed"
|
||||
// failure branch of validateJobControl, which asserted array_idx the same way
|
||||
// (bug-1490). Resume on a RUN job is not allowed and must report a failed job
|
||||
// without panicking.
|
||||
func TestValidateJobControl_ArrayIdxInt_NotAllowed(t *testing.T) {
|
||||
resetJobSubmitState()
|
||||
|
||||
jobInfo := jobRowFromStore(2001, 0, models.JOBSTAT["RUN"])
|
||||
|
||||
success, failed := validateJobControl(jobInfo, "resume", "")
|
||||
if len(failed) != 1 {
|
||||
t.Fatalf("expected 1 failed job, got %d (success=%v)", len(failed), success)
|
||||
}
|
||||
if got := failed[0]["array_idx"].(int); got != 0 {
|
||||
t.Errorf("array_idx = %d, want 0", got)
|
||||
}
|
||||
}
|
||||
|
||||
// TestFoundJob_ArrayIdxInt covers foundJob's array-child branch, which also
|
||||
// asserted array_idx as int64 (bug-1490) and panicked for array jobs with an
|
||||
// explicit index range.
|
||||
func TestFoundJob_ArrayIdxInt(t *testing.T) {
|
||||
row := jobRowFromStore(2001, 2, models.JOBSTAT["PEND"])
|
||||
|
||||
jobs, err := foundJob(2001, 2, []utils.OrderMap{row})
|
||||
if err != nil {
|
||||
t.Fatalf("foundJob returned error: %v", err)
|
||||
}
|
||||
if len(jobs) != 1 {
|
||||
t.Fatalf("expected 1 matching job, got %d", len(jobs))
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue