From: Tom Clegg Date: Thu, 23 Jun 2022 18:26:28 +0000 (-0400) Subject: 18948: Update container record exit_code as soon as it is known. X-Git-Tag: 2.5.0~131^2~1 X-Git-Url: https://git.arvados.org/arvados.git/commitdiff_plain/1f0da8d97cfb16e478d5dfed896e86112eb82120?ds=sidebyside 18948: Update container record exit_code as soon as it is known. 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..685bd15e65 100644 --- a/services/api/test/unit/container_test.rb +++ b/services/api/test/unit/container_test.rb @@ -933,7 +933,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 +963,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)