From: Tom Clegg Date: Fri, 24 Jun 2022 16:13:52 +0000 (-0400) Subject: Merge branch '18948-update-exit-code-early' X-Git-Tag: 2.5.0~131 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/47e929743cefd13760b9c84e1c148115aa3fc96c?hp=8288bba27e9beff7273aeb65c5200248e52bab02 Merge branch '18948-update-exit-code-early' closes #18948 Arvados-DCO-1.1-Signed-off-by: Tom Clegg --- diff --git a/doc/api/methods/containers.html.textile.liquid b/doc/api/methods/containers.html.textile.liquid index 76e5730c9f..43163c5550 100644 --- a/doc/api/methods/containers.html.textile.liquid +++ b/doc/api/methods/containers.html.textile.liquid @@ -52,8 +52,8 @@ Generally this will contain additional keys that are not present in any correspo |output|string|Portable data hash of the output collection.|Null if the container is not yet finished.| |container_image|string|Portable data hash of a collection containing the docker image used to run the container.|| |progress|number|A number between 0.0 and 1.0 describing the fraction of work done.|| -|priority|integer|Range 0-1000. Indicate scheduling order preference.|Currently assigned by the system as the max() of the priorities of all associated ContainerRequests. See "container request priority":container_requests.html#priority .| -|exit_code|integer|Process exit code.|Null if state!="Complete"| +|priority|integer|Range 0-1000. Indicate scheduling order preference.|Currently assigned by the system as the max() of the priorities of all associated ContainerRequests. See "container request priority":container_requests.html#priority.| +|exit_code|integer|Process exit code.|Null if container process has not exited yet.| |auth_uuid|string|UUID of a token to be passed into the container itself, used to access Keep-backed mounts, etc. Automatically assigned.|Null if state∉{"Locked","Running"} or if @runtime_token@ was provided.| |locked_by_uuid|string|UUID of a token, indicating which dispatch process changed state to Locked. If null, any token can be used to lock. If not null, only the indicated token can modify this container.|Null if state∉{"Locked","Running"}| |runtime_token|string|A v2 token to be passed into the container itself, used to access Keep-backed mounts, etc.|Not returned in API responses. Reset to null when state is "Complete" or "Cancelled".| diff --git a/lib/crunchrun/crunchrun.go b/lib/crunchrun/crunchrun.go index 30871e7349..0e86f604a7 100644 --- a/lib/crunchrun/crunchrun.go +++ b/lib/crunchrun/crunchrun.go @@ -1095,6 +1095,12 @@ func (runner *ContainerRunner) WaitFinish() error { } } runner.CrunchLog.Printf("Container exited with status code %d%s", exitcode, extra) + err = runner.DispatcherArvClient.Update("containers", runner.Container.UUID, arvadosclient.Dict{ + "container": arvadosclient.Dict{"exit_code": exitcode}, + }, nil) + if err != nil { + runner.CrunchLog.Printf("ignoring error updating exit_code: %s", err) + } var returnErr error if err = runner.executorStdin.Close(); err != nil { @@ -1162,10 +1168,9 @@ func (runner *ContainerRunner) updateLogs() { continue } - var updated arvados.Container err = runner.DispatcherArvClient.Update("containers", runner.Container.UUID, arvadosclient.Dict{ "container": arvadosclient.Dict{"log": saved.PortableDataHash}, - }, &updated) + }, nil) if err != nil { runner.CrunchLog.Printf("error updating container log to %s: %s", saved.PortableDataHash, err) continue @@ -1443,13 +1448,13 @@ func (runner *ContainerRunner) UpdateContainerFinal() error { if runner.LogsPDH != nil { update["log"] = *runner.LogsPDH } - if runner.finalState == "Complete" { - if runner.ExitCode != nil { - update["exit_code"] = *runner.ExitCode - } - if runner.OutputPDH != nil { - update["output"] = *runner.OutputPDH - } + if runner.ExitCode != nil { + update["exit_code"] = *runner.ExitCode + } else { + update["exit_code"] = nil + } + if runner.finalState == "Complete" && runner.OutputPDH != nil { + update["output"] = *runner.OutputPDH } return runner.DispatcherArvClient.Update("containers", runner.Container.UUID, arvadosclient.Dict{"container": update}, nil) } diff --git a/lib/crunchrun/crunchrun_test.go b/lib/crunchrun/crunchrun_test.go index 9971757893..76289b951d 100644 --- a/lib/crunchrun/crunchrun_test.go +++ b/lib/crunchrun/crunchrun_test.go @@ -276,7 +276,7 @@ func (client *ArvTestClient) Update(resourceType string, uuid string, parameters if parameters["container"].(arvadosclient.Dict)["state"] == "Running" { client.WasSetRunning = true } - } else if resourceType == "collections" { + } else if resourceType == "collections" && output != nil { mt := parameters["collection"].(arvadosclient.Dict)["manifest_text"].(string) output.(*arvados.Collection).UUID = uuid output.(*arvados.Collection).PortableDataHash = fmt.Sprintf("%x", md5.Sum([]byte(mt))) diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb index 3a04c56046..08f87bbdb1 100644 --- a/services/api/app/models/container.rb +++ b/services/api/app/models/container.rb @@ -478,8 +478,8 @@ class Container < ArvadosModel def validate_change permitted = [:state] - progress_attrs = [:progress, :runtime_status, :log, :output, :output_properties] - final_attrs = [:exit_code, :finished_at] + progress_attrs = [:progress, :runtime_status, :log, :output, :output_properties, :exit_code] + final_attrs = [:finished_at] if self.new_record? permitted.push(:owner_uuid, :command, :container_image, :cwd, diff --git a/services/api/test/unit/container_test.rb b/services/api/test/unit/container_test.rb index ac3e6bea42..bcf99da2e3 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -870,16 +870,14 @@ class ContainerTest < ActiveSupport::TestCase end end - test "Container only set exit code on complete" do + test "can only change exit code while running and at completion" do set_user_from_auth :active c, _ = minimal_new set_user_from_auth :dispatch1 c.lock + check_illegal_updates c, [{exit_code: 1}] c.update_attributes! state: Container::Running - - check_illegal_updates c, [{exit_code: 1}, - {exit_code: 1, state: Container::Cancelled}] - + assert c.update_attributes(exit_code: 1) assert c.update_attributes(exit_code: 1, state: Container::Complete) end @@ -933,7 +931,7 @@ class ContainerTest < ActiveSupport::TestCase end ["auth_uuid", "runtime_token"].each do |tok| - test "#{tok} can set output, progress, runtime_status, state on running container -- but not log" do + test "#{tok} can set output, progress, runtime_status, state, exit_code on running container -- but not log" do if tok == "runtime_token" set_user_from_auth :spectator c, _ = minimal_new(container_image: "9ae44d5792468c58bcf85ce7353c7027+124", @@ -963,6 +961,7 @@ class ContainerTest < ActiveSupport::TestCase assert c.update_attributes(output: collections(:collection_owned_by_active).portable_data_hash) assert c.update_attributes(runtime_status: {'warning' => 'something happened'}) assert c.update_attributes(progress: 0.5) + assert c.update_attributes(exit_code: 0) refute c.update_attributes(log: collections(:real_log_collection).portable_data_hash) c.reload assert c.update_attributes(state: Container::Complete, exit_code: 0)