diff --git a/README.md b/README.md index d62c078..cdba588 100644 --- a/README.md +++ b/README.md @@ -25,14 +25,31 @@ The CLI uses an existing bearer token, so it never puts a client secret on a com ```bash export FIRECREST_URL=https://firecrest.example FIRECREST_SYSTEM=my-system FIRECREST_TOKEN=… -firecrest-go submit job.sh /home/me +firecrest-go systems +firecrest-go submit -name myjob -out myjob.out -err myjob.err job.sh /home/me firecrest-go status 12345 -firecrest-go download /home/me/output.txt ./output.txt +firecrest-go download /home/me/myjob.out ./output.txt ``` +Pass `-out`/`-err` when you intend to read a job's output back. Without them the +scheduler picks the default `slurm-.out`, and whether that file really appears +where the API reports it is up to the site's Slurm configuration — see the caveat below. + ## Verification -Tests use `httptest`, require no running FirecREST stack, and are checked with `go vet`, `staticcheck`, and `go test -race`. No real Alps credentials or systems are exercised by this repository. +Unit tests use `httptest`, require no running FirecREST stack, and are checked with +`go vet`, `staticcheck`, and `go test -race`. + +Beyond that, this client has been **run end to end against a real FirecREST v2 server** +(`f7t-appversion: 2.6.0`) on the project's local demo stack, on 2026-09-15: OAuth2 +client-credentials token, `systems`, `submit`, `status` polled to `COMPLETED`, and +`download` of the job's stdout all succeeded. What that run found is recorded in +`docs/verification-v2.md`. + +**No real Alps system and no real CSCS credentials have been exercised.** The demo +stack's scheduler is a shell stand-in, not Slurm, and its tokens are not representative +of Keycloak's short-lived ones — so timing, token refresh under load, and real +scheduler states remain unverified. ## License diff --git a/cmd/firecrest-go/main.go b/cmd/firecrest-go/main.go index 77f1b8e..e0eda3e 100644 --- a/cmd/firecrest-go/main.go +++ b/cmd/firecrest-go/main.go @@ -19,6 +19,9 @@ func main() { baseURL := fs.String("url", os.Getenv("FIRECREST_URL"), "FirecREST base URL") system := fs.String("system", os.Getenv("FIRECREST_SYSTEM"), "FirecREST system name") token := fs.String("token", os.Getenv("FIRECREST_TOKEN"), "Bearer token") + name := fs.String("name", "", "submit: job name") + stdout := fs.String("out", "", "submit: standard output path, relative to the working directory") + stderr := fs.String("err", "", "submit: standard error path, relative to the working directory") fs.Parse(os.Args[2:]) if *baseURL == "" || *system == "" || *token == "" { fmt.Fprintln(os.Stderr, "-url, -system and -token (or matching environment variables) are required") @@ -37,7 +40,13 @@ func main() { if err != nil { fatal(err) } - id, err := client.Submit(context.Background(), firecrest.SubmitRequest{Job: firecrest.JobDescription{Script: string(script), WorkingDirectory: fs.Arg(1)}}) + id, err := client.Submit(context.Background(), firecrest.SubmitRequest{Job: firecrest.JobDescription{ + Script: string(script), + WorkingDirectory: fs.Arg(1), + Name: *name, + StandardOutput: *stdout, + StandardError: *stderr, + }}) if err != nil { fatal(err) } @@ -46,11 +55,29 @@ func main() { if fs.NArg() != 1 { usage() } + job, err := client.Job(context.Background(), fs.Arg(0)) + if err != nil { + fatal(err) + } status, err := client.JobStatus(context.Background(), fs.Arg(0)) if err != nil { fatal(err) } - fmt.Printf("job=%s stdout=%s stderr=%s\n", status.JobID, status.StandardOutput, status.StandardError) + exit := "?" + if job.Status.ExitCode != nil { + exit = fmt.Sprint(*job.Status.ExitCode) + } + fmt.Printf("job=%s name=%s state=%s exit=%s terminal=%t stdout=%s stderr=%s\n", + job.JobID, job.Name, job.Status.State, exit, job.Status.Terminal(), + status.StandardOutput, status.StandardError) + case "systems": + systems, err := client.Systems(context.Background()) + if err != nil { + fatal(err) + } + for _, s := range systems { + fmt.Println(s.Name) + } case "download": if fs.NArg() != 2 { usage() @@ -78,6 +105,6 @@ func main() { } func fatal(err error) { fmt.Fprintln(os.Stderr, "error:", err); os.Exit(1) } func usage() { - fmt.Fprintln(os.Stderr, "usage: firecrest-go [flags] args") + fmt.Fprintln(os.Stderr, "usage: firecrest-go [flags] args") os.Exit(2) } diff --git a/docs/verification-v2.md b/docs/verification-v2.md new file mode 100644 index 0000000..f704982 --- /dev/null +++ b/docs/verification-v2.md @@ -0,0 +1,163 @@ +# End-to-end verification against FirecREST v2 (VDLP-50) + +Date: 2026-09-15. Server: the project's local demo stack, `f7t-appversion: 2.6.0`, +one system `fakecluster`. Issuer: the demo launcher on `:8025` (it acts as the OIDC +issuer; there is no separate Keycloak in this stack). + +This client was written against the v2 **specification** in VDLP-49 and had never +been run against a server. This document records what that first contact found. +It is a record of observations, not a changelog. + +## Result + +All four commands work. Three defects were found on the way, one in this client and +two in the demo stack. Details below. + +| Command | Outcome | +| --- | --- | +| token, OAuth2 client credentials | works | +| `systems` | works — `fakecluster` | +| `submit` | works — HTTP 201, `{"jobId":"14"}` | +| `status` | works, but only after adding the state call (see 2) | +| `download` | works on an explicit output path; fails on the default one (see 4) | + +## 1. The submit body used snake_case; the schema is camelCase + +The client sent `working_directory`, `standard_output`, `standard_error`. The +published `JobDescriptionModel` names these `workingDirectory`, `standardOutput`, +`standardError`. + +FirecREST 2.6.0 accepts **both**. Submitting the same job twice, once each way, +produced two jobs whose scheduler-side metadata was identical: + +``` +snake_case -> 201 {"jobId":"10"} meta: 10|snake-test|/home/demo/tmp|/home/demo/tmp/snake.out +camelCase -> 201 {"jobId":"11"} meta: 11|camel-test|/home/demo/tmp|/home/demo/tmp/camel.out +``` + +So this was never going to surface as a failed request — the fields were being +honoured. It matters anyway: acceptance of the snake_case form comes from Pydantic +models that populate by field name as well as by alias, which is an implementation +detail of this server version, not a promise in the contract. A future release that +sets `populate_by_name=False`, or a different v2 implementation, would silently drop +`working_directory` and run every job in the user's home directory instead of the +requested one — a wrong-directory job, not an error. + +**Changed**: the struct tags now carry the documented camelCase names. The unit test +that asserted the snake_case wire format was asserting the wrong thing and was +updated with it. + +## 2. `/metadata` carries no job state, so `status` could not answer "is it done?" + +`GET /compute/{system}/jobs/{id}/metadata` returns exactly: + +```json +{"jobs":[{"jobId":"12","script":"…","standardInput":"/dev/null", + "standardOutput":"/home/demo/vdlp50-rt.out","standardError":"…"}]} +``` + +`jobId`, `script`, and the three stream paths. No state, no exit code. The issue +asked to poll "until a terminal state", and with only this call there is nothing to +poll — the response is identical for a queued job and a finished one. + +State lives on the sibling endpoint `GET /compute/{system}/jobs/{id}`: + +```json +{"jobs":[{"jobId":"12","name":"vdlp50","status":{"state":"COMPLETED", + "stateReason":"None","exitCode":0,"interruptSignal":0}, …}]} +``` + +**Added**: `Client.Job` reading that endpoint, a `JobState.Terminal()` predicate over +Slurm's terminal states, and `Client.Systems` for `/status/systems`. `status` now +reports state, exit code and terminality alongside the paths. `Terminal()` treats an +unrecognised state as non-terminal, so an unknown state makes a caller poll again +rather than report a wrong outcome. + +Note for whoever writes the polling loop: `Terminal()` is the state-based stop +condition, and it is the one to use. `exitCode` is not a substitute — the field is +nullable in the schema, and this run only ever observed jobs that had already +finished, so its value before terminality is unverified here. + +## 3. Stack defect: `standardOutput` is concatenated onto the working directory + +Submitting an **absolute** `standardOutput` produces a doubled path in the metadata +response: + +``` +request : workingDirectory=/home/demo/tmp standardOutput=/home/demo/tmp/snake.out +metadata: "standardOutput": "/home/demo/tmp//home/demo/tmp/snake.out" +``` + +The file is written to the right place; only the reported path is wrong. The +join is unconditional instead of respecting an already-absolute path. Not patched — +it is the stack's defect, and per the issue's constraints it is documented, not +worked around. Practical consequence: **pass output paths relative to the working +directory**, which is what the CLI's `-out`/`-err` now do. + +## 4. Stack defect: the default output path reported does not exist + +Submitting without `standardOutput`, `status` reports `/home/demo/slurm-13.out`, and +downloading it fails: + +``` +$ firecrest-go download /home/demo/slurm-13.out /tmp/final.out +error: FirecREST API error: status=404 detail="{\"errorType\":\"error\", + \"message\":\"Remote process failed with exit status:1 and error message: + base64: /home/demo/slurm-13.out: No such file or directory\", …}" +``` + +The client is reporting faithfully what the server told it. The two halves of the +`fakecluster` stand-in disagree about where default output goes: + +``` +sbatch : [ -n "$OUT" ] || OUT_ABS="$JOBDIR/stdout" # /var/spool/fakeslurm/13/stdout +scontrol: [ -n "$out" ] || out="slurm-${jid}.out" # reports /home/demo/slurm-13.out +``` + +`sbatch` writes the job's stdout into its spool directory; `scontrol`, which is what +FirecREST reads the metadata from, reports the conventional Slurm name. The file at +the reported path is never created. This is a defect of the demo stand-in and would +not occur against real Slurm, where the default output genuinely lands in the working +directory — so it should not be read as a v2 API defect. + +With an explicit output path the whole chain works: + +``` +$ firecrest-go submit -name vdlp50 -out vdlp50.out -err vdlp50.err job.sh /home/demo +14 +$ firecrest-go status 14 +job=14 name=vdlp50 state=COMPLETED exit=0 terminal=true stdout=/home/demo/vdlp50.out stderr=/home/demo/vdlp50.err +$ firecrest-go download /home/demo/vdlp50.out /tmp/final.out +OK (23 bytes): VDLP-50 final run\ndone +``` + +## 5. Confirmed correct, no change needed + +- **Authentication.** `clientcredentials.Config` against the launcher's `/token` + endpoint yields a usable token; `/status/systems` returns 200 with it and 401 + without. The issue flagged the issuer as a likely first discovery — it is not one, + the flow works as written. +- **`ops/download` is a byte stream**, `content-type: application/octet-stream`, not + a JSON envelope. The client's raw-body handling is right. It is *not* `ops/view`, + so the documented `/ops/view` size-default defect does not apply to this client. +- **`ops/upload`** with multipart field `file` and the directory in `?path=` + returns 204, matching the client's implementation. +- **The `max_ops_file_size` limit is real and enforced**: downloading a 2 MiB file + against the configured 1 MiB limit returns `413 {"message":"Command output exceeded + buffer limit."}`. `Download` surfaces it as an `APIError` with the status intact. + No streaming/chunked path exists in this client — large files need the async + `filesystem/{system}/transfer/*` endpoints, still out of scope. +- **`jobId` is returned as a JSON string** (`{"jobId":"14"}`), not the number the + original unit test assumed. The client already accepted both; a test now pins both. + +## Not verified + +No real Alps system, no real CSCS credentials. `fakecluster`'s scheduler is a shell +script that runs the job inline and synchronously — there is no queue, so no job was +ever observed in `PENDING` or `RUNNING`, and the terminal-state logic has been +exercised only against `COMPLETED`. Demo tokens are long-lived; refresh behaviour +under a realistic ~300 s TTL is untested. + +One further gap, unrelated to this client: `GET /status/{system}/userinfo` fails on +this stack with `sacctmgr: command not found` — the stand-in does not provide that +binary. The endpoint is not used here. diff --git a/firecrest.go b/firecrest.go index c7589c5..38fa680 100644 --- a/firecrest.go +++ b/firecrest.go @@ -43,14 +43,20 @@ type SubmitRequest struct { } // JobDescription is deliberately limited to portable submission fields. +// +// Field names follow FirecREST v2's published schema (JobDescriptionModel), +// which is camelCase. FirecREST 2.6.0 also accepts the snake_case spellings +// this client used previously, because its Pydantic models populate by field +// name as well as by alias — but that is an implementation courtesy, not part +// of the contract, so the documented names are what we send. type JobDescription struct { Script string `json:"script"` - WorkingDirectory string `json:"working_directory"` + WorkingDirectory string `json:"workingDirectory"` Name string `json:"name,omitempty"` Account string `json:"account,omitempty"` Partition string `json:"partition,omitempty"` - StandardOutput string `json:"standard_output,omitempty"` - StandardError string `json:"standard_error,omitempty"` + StandardOutput string `json:"standardOutput,omitempty"` + StandardError string `json:"standardError,omitempty"` Environment map[string]string `json:"env,omitempty"` } @@ -83,6 +89,9 @@ type JobMetadata struct { } // JobStatus returns metadata for a job, including standard output and error paths. +// +// Note that /metadata carries no scheduler state: to learn whether a job is +// still running, use Job, which reads GET /compute/{system}/jobs/{id}. func (c *Client) JobStatus(ctx context.Context, jobID string) (JobMetadata, error) { var response struct { Jobs []JobMetadata `json:"jobs"` @@ -96,6 +105,88 @@ func (c *Client) JobStatus(ctx context.Context, jobID string) (JobMetadata, erro return response.Jobs[0], nil } +// JobState is the scheduler state of a job, as reported by sacct. +type JobState struct { + State string `json:"state"` + StateReason string `json:"stateReason"` + ExitCode *int `json:"exitCode"` + InterruptSignal *int `json:"interruptSignal"` +} + +// Terminal reports whether the job has stopped changing state, so a caller can +// end a polling loop. Slurm's terminal states are enumerated explicitly: an +// unknown state is treated as non-terminal, which makes a caller poll again +// rather than report a wrong outcome. +func (s JobState) Terminal() bool { + switch s.State { + case "COMPLETED", "FAILED", "CANCELLED", "TIMEOUT", "NODE_FAIL", + "PREEMPTED", "BOOT_FAIL", "DEADLINE", "OUT_OF_MEMORY", "REVOKED": + return true + default: + return false + } +} + +// Job describes a job's scheduler state and placement. +type Job struct { + JobID string `json:"jobId"` + Name string `json:"name"` + Status JobState `json:"status"` + Cluster string `json:"cluster"` + Partition string `json:"partition"` + Nodes string `json:"nodes"` + User string `json:"user"` + Account string `json:"account"` + WorkingDirectory string `json:"workingDirectory"` +} + +// JobState fetches a job's scheduler state via GET /compute/{system}/jobs/{id}. +// This is the call to poll; JobStatus reports only paths and the script. +func (c *Client) Job(ctx context.Context, jobID string) (Job, error) { + var response struct { + Jobs []Job `json:"jobs"` + } + if err := c.doJSON(ctx, http.MethodGet, "compute", path.Join("jobs", jobID), nil, &response); err != nil { + return Job{}, err + } + if len(response.Jobs) != 1 { + return Job{}, fmt.Errorf("decode job: expected one job, got %d", len(response.Jobs)) + } + return response.Jobs[0], nil +} + +// System is one HPC system exposed by this FirecREST installation. +type System struct { + Name string `json:"name"` +} + +// Systems lists the systems this installation serves. Unlike every other call +// it is not scoped to c.system, so it is the cheapest way to check that a +// configured system name actually exists before using it. +func (c *Client) Systems(ctx context.Context) ([]System, error) { + var response struct { + Systems []System `json:"systems"` + } + u := *c.baseURL + u.Path = strings.TrimRight(u.Path, "/") + "/status/systems" + req, err := http.NewRequestWithContext(ctx, http.MethodGet, u.String(), nil) + if err != nil { + return nil, err + } + resp, err := c.http.Do(req) + if err != nil { + return nil, fmt.Errorf("FirecREST request: %w", err) + } + defer resp.Body.Close() + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + return nil, newAPIError(resp) + } + if err := json.NewDecoder(resp.Body).Decode(&response); err != nil { + return nil, fmt.Errorf("decode response: %w", err) + } + return response.Systems, nil +} + // Download fetches a small file through the synchronous filesystem ops endpoint. // The caller owns closing the returned body. func (c *Client) Download(ctx context.Context, remotePath string) (io.ReadCloser, error) { diff --git a/firecrest_test.go b/firecrest_test.go index 0306099..7180327 100644 --- a/firecrest_test.go +++ b/firecrest_test.go @@ -36,7 +36,7 @@ func TestSubmitUsesV2PathAndJSON(t *testing.T) { t.Errorf("content type = %q", got) } body, _ := io.ReadAll(r.Body) - if !strings.Contains(string(body), `"working_directory":"/work"`) { + if !strings.Contains(string(body), `"workingDirectory":"/work"`) { t.Errorf("body = %s", body) } w.WriteHeader(http.StatusCreated) @@ -126,3 +126,106 @@ func TestAPIErrorIsQueryable(t *testing.T) { t.Fatalf("expected APIError, got %#v", err) } } + +// FirecREST 2.6.0 returns jobId as a JSON string; the OpenAPI schema types it +// as a nullable string. The numeric form is kept working because the original +// client was written against it and it costs one branch to accept both. +func TestSubmitAcceptsStringAndNumericJobID(t *testing.T) { + for _, tc := range []struct{ name, body, want string }{ + {"string, as FirecREST 2.6.0 replies", `{"jobId":"42"}`, "42"}, + {"number", `{"jobId":42}`, "42"}, + } { + t.Run(tc.name, func(t *testing.T) { + client := testClient(t, func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusCreated) + _, _ = w.Write([]byte(tc.body)) + }) + id, err := client.Submit(context.Background(), SubmitRequest{Job: JobDescription{Script: "echo hi", WorkingDirectory: "/work"}}) + if err != nil { + t.Fatal(err) + } + if id != tc.want { + t.Fatalf("job ID = %q, want %q", id, tc.want) + } + }) + } +} + +func TestJobReadsSchedulerState(t *testing.T) { + client := testClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/compute/cluster-a/jobs/42" { + t.Errorf("path = %s", r.URL.Path) + } + _, _ = w.Write([]byte(`{"jobs":[{"jobId":"42","name":"j","cluster":"cluster-a","partition":"debug", + "nodes":"node0","user":"demo","workingDirectory":"/home/demo", + "status":{"state":"COMPLETED","stateReason":"None","exitCode":0,"interruptSignal":0}}]}`)) + }) + job, err := client.Job(context.Background(), "42") + if err != nil { + t.Fatal(err) + } + if job.Status.State != "COMPLETED" || !job.Status.Terminal() { + t.Fatalf("job = %#v", job) + } + if job.Status.ExitCode == nil || *job.Status.ExitCode != 0 { + t.Fatalf("exit code = %v", job.Status.ExitCode) + } + if job.WorkingDirectory != "/home/demo" || job.Partition != "debug" { + t.Fatalf("job = %#v", job) + } +} + +// A running job must not be mistaken for a finished one, and an unrecognised +// state must keep a caller polling rather than report a wrong outcome. +func TestJobStateTerminal(t *testing.T) { + for state, want := range map[string]bool{ + "COMPLETED": true, "FAILED": true, "CANCELLED": true, "TIMEOUT": true, + "OUT_OF_MEMORY": true, "NODE_FAIL": true, + "PENDING": false, "RUNNING": false, "CONFIGURING": false, + "COMPLETING": false, "SUSPENDED": false, "": false, "SOMETHING_NEW": false, + } { + if got := (JobState{State: state}).Terminal(); got != want { + t.Errorf("Terminal(%q) = %t, want %t", state, got, want) + } + } +} + +// Systems is the one call that is not scoped to the client's system, so it must +// not acquire the /{system}/ path segment every other call carries. +func TestSystemsIsNotSystemScoped(t *testing.T) { + client := testClient(t, func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/status/systems" { + t.Errorf("path = %s", r.URL.Path) + } + _, _ = w.Write([]byte(`{"systems":[{"name":"fakecluster"}]}`)) + }) + systems, err := client.Systems(context.Background()) + if err != nil { + t.Fatal(err) + } + if len(systems) != 1 || systems[0].Name != "fakecluster" { + t.Fatalf("systems = %#v", systems) + } +} + +// FirecREST's error envelope is JSON, not the plain text httptest usually +// returns; APIError must surface its message rather than swallow it. +func TestAPIErrorCarriesFirecRESTEnvelope(t *testing.T) { + const envelope = `{"errorType":"error","message":"Access token is invalid.","causedBy":null,"data":null,"user":null}` + client := testClient(t, func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte(envelope)) + }) + _, err := client.Job(context.Background(), "42") + if !errors.Is(err, ErrUnauthorized) { + t.Fatalf("expected ErrUnauthorized, got %v", err) + } + var apiErr *APIError + if !errors.As(err, &apiErr) { + t.Fatalf("expected APIError, got %#v", err) + } + if !strings.Contains(apiErr.Detail, "Access token is invalid.") { + t.Fatalf("detail = %q", apiErr.Detail) + } +}